Repository navigation
Retire machine credentials after device access changes - #2521
Conversation
lilyshen0722
left a comment
There was a problem hiding this comment.
Two reproduced revocation gaps remain.
-
A successful machine removal can be followed by a usable runtime bearer from an already-running mint.
backend/services/machineService.ts:208–210finishes deletion and its second credential scan, but the separate runtime issuer is not fenced by registration's post-issue check. In the actual/api/agent-binding/runtime-tokenhandler, pause before the daemon-parent query atbackend/routes/agentBinding.ts:388–390, complete machine DELETE with 200, then resume. The query returns no active daemon;parentId: daemonCredential?._id || nullat line 396 lets the actual issuer return 201 with an active, machine-bound, parentless credential. Its next runtime-auth call succeeds (204), although the machine is absent and the daemon itself now receives 401. This is not a fabricated orphan row: the existing route and issuer minted it. Restoring only the runtime machine-existence check removed in this head makes the same probe refuse with 401. Fence the runtime mint/removal composition or enforce a durable machine veto on runtime authentication; a second removal scan plus registration recheck does not cover this issuer. Add this deferred actual-mint regression. -
A cleanup failure leaves a device displayed as revoked while its daemon and runtime bearer remain usable, and refresh removes the UI retry.
backend/services/deviceAuthorizationService.ts:192–200commitsdeviceTokens.$.revokedAtbefore machine retirement. Injecting one rejection from the firstAgentCredential.revokeCascadeproduces DELETE 500, a persisted revoked timestamp, and GET/api/auth/devicesreporting that timestamp, while the existing daemon still returns 200 and its runtime bearer returns 204. The UI initially shows an error, but reload/Refresh clears that error andfrontend/src/v2/components/V2DevicesPanel.tsx:58renders “Revoked” instead of the Revoke button. A second DELETE through the API successfully recovers and makes both bearers return 401; the normal UI no longer offers that retry after refresh. Keep authorization fail-closed once revocation is persisted, and make incomplete cleanup durably recoverable with truthful UI state. Moving the timestamp after cleanup alone would reopen the registration race.
Evidence: all four changed author suites pass on this head (35 tests). The combined exact-archive run, including my two service probe files, has 37 passed / 2 failed across six suites, using Node 22, MongoMemoryServer, actual Express/auth/issuer/removal code, --detectOpenHandles, and no force exit. The two failures are the cases above. Foreign-owner refusals preserve live access, and removal revokes an actual spawn child of a standalone seat while preserving the agent identity and installation. The earlier ownership-match control passed while the runtime machine check was present. My initial fixture assumed null instead of an absent revocation field and reused the timestamp prefix of ObjectIds for two usernames; both fixture errors were corrected before the quoted run.
Mutation/control evidence: deleting only the route's device-binding argument makes the author device-revoke test fail because its machine survives; restoring the deleted runtime machine check makes the deferred-mint refusal pass. Every temporary production change was restored, and all PR files plus the runtime auth/mint files were byte-compared against this commit. Full 12-file diff (+375/−20) and the two intervening corrections read; diff check clean; review rubric/checklist unchanged. Merge base: 7065201.
These are local tests with dummy credentials and one deliberate database-operation failure/deferred query. They do not measure a deployed daemon, operator device revocation, browser rendering, or a production database. The conservative removal of pre-binding machines is documented in the PR body and is not a finding here.
Written by Rhea, a Commonly agent — pod.
GATE: HOLD
lilyshen0722
left a comment
There was a problem hiding this comment.
Both findings in review 5631692815 are closed. The original cleanup-failure probe now gets daemon 401 and runtime 401 immediately after the durable device revoke, before cleanup retry. The deferred runtime mint across successful machine deletion now returns 401 without a bearer; a separate pause after the parent lookup also returns 401 and leaves no active machine-bound runtime credential. Foreign-owner refusals preserve access, and machine removal revokes a standalone seat's actual spawn child while preserving the agent identity and installation.
Validation: 112/112 tests pass across 14 suites on the exact head (57.727s, exit 0), including my three independent probe files and the corrected installations fixture. These are Node 22 Jest runs with --detectOpenHandles, no force exit, actual Express/auth/issuer/removal composition and MongoMemoryServer. My four additional rate-limit probes exercise the real route registrations and limiter implementation with a reduced three-request budget and stubbed authentication: rotating credentials reach auth three times, then receive 429 without another auth call for runtime installations, ensemble responses, GitHub issues and task reads. This is not a measurement of production traffic or the 3000-request budget.
Independent mutation evidence carries from unchanged production code: separately removing the daemon live-authorization refusal, runtime live-authorization refusal, or post-issue mint refusal makes the corresponding boundary probe fail (200, 204, or 201 instead of 401). Omitting the new installations pre-auth limiter makes its flood probe fail with 401 instead of 429; the other three families remain passing. The earlier device-binding omission also fails its machine-retirement assertion. All temporary mutations were restored and the final archive byte-verified against this head.
Full 21-file scope (+525/-65) reviewed, including the 20-route rate-limit baseline reduction and final three-line test-mock correction; prior review evidence carried only where production bytes are unchanged. Merge base 7065201; diff check clean; rubric/checklist unchanged through current main 92045e0. Exact-head CI is green, including CodeQL, all test tiers, aggregate Test & Coverage and metadata guards.
Nonblocking: failed cleanup can leave inert rows until API retry, and the refreshed Devices UI does not offer that cleanup retry. Its displayed revocation is now enforced by authentication. UI behavior was source-reviewed only; this gate does not certify a browser walk, deployed daemon, operator credentials or production database. The documented conservative removal of legacy unbound machines remains intentional.
Written by Rhea, a Commonly agent — pod.
GATE: PASS
|
Second gate asked: |
lilyshen0722
left a comment
There was a problem hiding this comment.
#2521 TASK-279 @ 356b387 — HOLD
The revocation path has Rhea's exact-head security PASS and green CI. This head also breaks the shipped “On my computer” install path:
-
backend/routes/registry/install.ts:292-293removeswrapperfrom the self-serve fallback.frontend/src/v2/components/V2AgentBYO.tsx:507-513still sendsruntimeType: 'wrapper'. For a new agent name,getByNamehas no row, so the install deterministically returns 404 “Agent not found in registry” before placement. The base head admitted wrapper here; this PR deletes the three wrapper service cases fromself-serve-install.test.js, so green CI no longer covers that contract. Restore the wrapper fallback and its service coverage. -
The same UI change removes
instanceId: 'default'from the install request, but the following placement request still sendsinstanceId: 'default'(V2AgentBYO.tsx:529-530). With a persona,displayNameis the persona name (:519), soderiveInstanceIdin the install route derives its slug instead ofdefault. Once the 404 is fixed, persona-based machine installs will target a different instance at placement. Keep the two calls on one instance identity and cover that path.
These are source-level cross-layer traces at the exact head, not a browser or deployed-daemon result. git diff --check is clean and exact-head CI is green. The revocation findings Rhea previously raised are closed in her separate security gate; this HOLD is for the install regression introduced in this diff.
Written by Sable, a Commonly agent — pod.
GATE: HOLD
Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
356b387 to
e078838
Compare
lilyshen0722
left a comment
There was a problem hiding this comment.
Delta gate from review 5631985072. Both original revocation holds remain closed. The seven commits retain the reviewed patches: six replay identically, and the remaining commit differs only in import context. Twenty of the 21 changed files are byte-identical to the cleared tree. The sole file difference is main's existing refusal logger imported and mounted on POST /api/agent-binding/request; its composition was read and tested.
The full PR remains backend-only, 21 files (+525/-65), based on bfa1f51. #2516's wrapper self-install fallback and the install/placement instanceId: 'default' values remain present, unchanged by this PR. The install route carries main's additional refusal logging; the fallback logic is retained. Diff check is clean, the final archive matches the head byte for byte, and the rubric/checklist are unchanged through current main 25836c3.
Exact-head validation: 126/126 tests pass across 15 suites, with --detectOpenHandles, no force exit (193.275s, exit 0). This reruns the prior 14 suites and adds the real-Mongo self-serve install suite, including creating a wrapper identity and accepting its machine placement. My cleanup-failure, both deferred-mint races, ownership/cascade controls and four pre-auth limiter probes pass. The limiter probes use a reduced three-request budget and stubbed auth. The independently failing guard-removal mutations from the prior gate carry on unchanged production logic; they were not rerun for this rebase. All exact-head CI checks are green or skipped, including CodeQL, backend shards, Service Tests, E2E, and aggregate Test & Coverage.
Prior limitations and the nonblocking cleanup-retry advisory carry: this is local Node 22/Express/MongoMemoryServer evidence, not a live device/daemon or browser walk. Failed cleanup leaves authorization denied even if inert rows await API retry.
Written by Rhea, a Commonly agent — pod.
GATE: PASS
|
Re-gate asked: |
|
Gate asked: |
lilyshen0722
left a comment
There was a problem hiding this comment.
#2521 TASK-279 at e078838: PASS
The rebase clears my prior HOLD (review 5632049748). The current PR merge diff contains no backend/routes/registry/install.ts, frontend/src/v2/components/V2AgentBYO.tsx, or self-serve install test changes. At this exact head, the install route still accepts wrapper for self-serve registration, and the BYO install and subsequent machine placement both send instanceId: 'default'. Thus #2516's shipped wrapper and instance contract remains intact.
The device-revocation change is backend-only in the current PR diff. Rhea's independent security review at this commit ends GATE: PASS and includes the real-Mongo wrapper install/placement case. Exact-head CI is green, including service tests, backend checks, E2E, CodeQL, and the stale-base guard. I found no remaining code defect from my prior review.
Credit: the branch was rebased without carrying the unrelated install/UI edits, preserving the existing self-serve flow while keeping the reviewed revocation controls.
Written by Sable, a Commonly agent, for pod 6a692a1be833c668acdb84cf.
GATE: PASS
@coderabbitai ignore
Summary
Evidence
DELETE /api/auth/devices/:id; previously it only changedUser.deviceTokens.revokedAt. Daemon calls use a separateAgentCredentialbearer.Operational note
A device revoke also removes older machines with no recorded device binding. That fail-closed fallback may require those machines to be registered again.