tsk-dtwu7x [OPEN] Cluster app: device and node revoke/blocking (Jay - #2238
tsk-dtwu7x [OPEN] Cluster app: device and node revoke/blocking (Jay#2238jaylfc wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd device blocking/unblocking and prevent blocked devices from re-pairing
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
BOUNCE - code-level review against card tsk-dtwu7x. The backend core is good: block=revoked+blocked with re-pair refusal, and the guarded-ALTER _post_init is exactly the right migration pattern for this repo (PRAGMA check, mirrors decision_store). But the PR covers roughly a third of the card and none of its mandated tests, so it cannot merge as-is. MISSING vs the card (its own text, not my invention):
Code findings on what IS here: The store/route work here is worth keeping; the remainder goes to a fold card so the missing scope is not silently dropped. |
|
nemotron-ultra-kilo review VERDICT: Code is mostly correct with solid security intent, but has a logic bug in list_for_user query and missing tests.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
Code Review by Qodo
1. Blocked repaire bypass
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| revoked INTEGER NOT NULL DEFAULT 0, | ||
| blocked INTEGER NOT NULL DEFAULT 0 |
There was a problem hiding this comment.
1. blocked added to schema 📜 Skill insight ≡ Correctness
The devices table SCHEMA now includes the post-ship blocked column even though the code comments state it was added after initial release and is also retrofitted via _post_init. This violates the rule that migration-added columns must not appear in SCHEMA, to avoid schema/migration drift and ensure consistent initialization behavior.
Agent Prompt
## Issue description
`blocked` is described as a post-ship column and is added via guarded `ALTER TABLE` in `_post_init`, but it is also present in `SCHEMA`. Per compliance, `SCHEMA` must represent the first-open DDL and must not reference columns introduced later.
## Issue Context
Keeping migration-added columns out of `SCHEMA` avoids drift between fresh DB creation and upgraded DBs, and keeps initialization logic consistent.
## Fix Focus Areas
- tinyagentos/device_store.py[27-58]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if body.push_token and await store.find_blocked_by_push_token(user.user_id, body.push_token) is not None: | ||
| return JSONResponse( | ||
| {"error": "device is blocked; unblock it before re-pairing"}, | ||
| status_code=403, | ||
| ) |
There was a problem hiding this comment.
3. Blocked repaire bypass 🐞 Bug ⛨ Security
The new blocked-device guard in register_device is skipped when push_token is empty, but the request model allows empty push_token values. A blocked device can therefore re-register with an empty push_token and receive a fresh scoped_token, defeating the block control.
Agent Prompt
## Issue description
The new blocked-device re-pair prevention only runs when `body.push_token` is truthy. Because `RegisterIn.push_token` defaults to `""` (and the push-token update route can also set `""`), a blocked device can bypass the block check by registering with an empty push token and still obtain a new scoped token.
## Issue Context
Blocking is explicitly intended to prevent a stolen/attacker-held device from silently re-pairing under a new scoped token. That guarantee currently only holds when a non-empty push token is provided.
## Fix Focus Areas
- tinyagentos/routes/devices.py[21-65]
- tinyagentos/device_store.py[156-168]
- tinyagentos/routes/devices.py[34-36]
## Proposed fix
1. Make `push_token` required and non-empty for `/api/devices/register` (e.g., `Field(min_length=1, max_length=...)` and/or a validator that rejects blank/whitespace).
2. Disallow setting `push_token` to an empty string in `PushTokenIn` (same `min_length=1` / validator), so a device cannot clear its identifying token to evade a future block.
3. (If empty push tokens must be supported for some clients) introduce an alternate durable device identity for blocking, because the current mechanism cannot enforce “no re-pair” without a stable identifier.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-orB review VERDICT: The diff implements a device blocking feature with proper security intent, but has a potential bypass in registration and lacks test coverage in the diff.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
1 participant
Autonomous build of board card tsk-dtwu7x.
Files:
tinyagentos/device_store.py | 69 +++++++++++++++++++++++++++++++++++++++----
tinyagentos/routes/devices.py | 53 ++++++++++++++++++++++++++++++++-
2 files changed, 116 insertions(+), 6 deletions(-)
Summary by Gitar
devices.pyblockeddatabase column and persistence methods indevice_store.pyThis will update automatically on new commits.