fix(auth): stop accumulating authorized-client rows per app relaunch - #7978
fix(auth): stop accumulating authorized-client rows per app relaunch#7978ImBIOS wants to merge 3 commits into
Conversation
…tored agent selections opencode <=1.18 writes ESC ]0;<cwd>: ready BEL to stdout for every non-help command even when stdout is a pipe (agent list, models --verbose, debug skill). T3's ChildProcessSpawner captures that stdout via collectStreamAsString and the parsers stored a polluted agent id like "\x1b]0;imbios: ready\x07build" in model_selection_json. Later sendTurn used that polluted id and opencode rejected it with "Agent not found: \"\x1b]0;imbios: ready\x07build\"" which was surfaced as session.error UnknownError + a generic SessionPrompt UnknownError wrapper (the stack the user pasted). Fix: - packages/shared/src/stripTerminalEscapes.ts: shared OSC/CSI sanitizer - apps/server/src/provider/opencodeRuntime.ts: strip before parseModels/Agent/Skills and via parse* entry points; keeps skills from silently degrading to [] when polluted - apps/server/src/provider/Layers/OpenCodeProvider.ts: sanitize inventory agent names/variants and --version parsing; build clean capability option ids - apps/server/src/provider/Layers/OpenCodeAdapter.ts & textGeneration/OpenCodeTextGeneration.ts: sanitize stored getModelSelectionStringOptionValue values before promptAsync - packages/shared/src/model.ts: sanitize persisted option values and model slugs on read (repairs 3 polluted threads without DB migration) - tests: add OSC/ANSI regression cases for both parsers Polluted threads still read as clean via model.ts sanitizer; no migration needed but DB can be cleaned with stripTerminalEscapes. Fixes the reported UnknownError at SessionPrompt.createUserMessage and the earlier "Agent not found" session.error.
Every bearer bootstrap exchange minted a brand-new auth session row and sessions live for 30 days, so the Settings authorized-clients list filled with duplicate "T3 Code Desktop" entries from repeated desktop launches, window reloads, and dev restarts — all from a single machine. Clients now present a stable per-install client_instance_id on token exchange (desktop persists one in its state dir, web/desktop renderer in localStorage, mobile in secure storage). When an exchange carries an instance id, the server reuses the client's existing compatible session: it extends the expiry and re-signs a token against it instead of creating a new row. Incompatible sessions (e.g. widened scopes) are revoked before the replacement is issued; DPoP exchanges are exempt since their key thumbprint is not persisted. Issuance also prunes expired and revoked session rows so the table no longer grows unbounded.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect service conventions review of the changed service code. One finding: a reused error class whose message no longer matches the failure it now reports. The rest of the touched services (namespace imports, Context.Service + make/layer shape, environment-based dependency acquisition in DesktopLocalEnvironmentAuth, AuthSessionRepository additions, SessionStore.issue error mapping) follow the conventions.
Posted via Macroscope — Effect Service Conventions
| if (existing?.trim()) return existing; | ||
| const instanceId = yield* Effect.tryPromise({ | ||
| try: () => import("../lib/uuid").then(({ uuidv4 }) => uuidv4()), | ||
| catch: (cause) => new MobileDeviceIdGenerationError({ cause }), |
There was a problem hiding this comment.
MobileDeviceIdGenerationError renders "Failed to generate the mobile agent-awareness device id.", which is inaccurate for a client-instance-id failure, and the error carries no attribute distinguishing the two cases. Consider either a distinct error class for this failure, or adding a structural discriminator (e.g. key) and deriving the message from it.
| catch: (cause) => new MobileDeviceIdGenerationError({ cause }), | |
| catch: (cause) => new MobileClientInstanceIdGenerationError({ cause }), |
Posted via Macroscope — Effect Service Conventions
| // DPoP credentials are excluded because their key thumbprint is not | ||
| // persisted, so a reused session could not be safely re-signed. | ||
| if (client.instanceId !== undefined && input?.proofKeyThumbprint === undefined) { | ||
| const candidates = yield* authSessions |
There was a problem hiding this comment.
🟡 Medium auth/SessionStore.ts:593
Concurrent bootstrap exchanges for the same new instanceId create multiple active sessions, so duplicate authorized-client rows still accumulate. The listActiveForIdentity check at line 593 is not atomic with the later create; serialize this lookup-and-create sequence or enforce uniqueness with a database upsert/constraint.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/auth/SessionStore.ts around line 593:
Concurrent bootstrap exchanges for the same new `instanceId` create multiple active sessions, so duplicate authorized-client rows still accumulate. The `listActiveForIdentity` check at line 593 is not atomic with the later `create`; serialize this lookup-and-create sequence or enforce uniqueness with a database upsert/constraint.
| `, | ||
| }); | ||
|
|
||
| const updateSessionExpirationRow = SqlSchema.void({ |
There was a problem hiding this comment.
🟠 High persistence/AuthSessions.ts:308
When a session is revoked or pruned after listActiveForIdentity selects it, updateSessionExpirationRow still reports success without updating a row, so SessionStore.issue signs and returns a token for an invalid session. Use an update operation that returns or counts affected rows and fail when none matched, or make the selection and update atomic.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/persistence/AuthSessions.ts around line 308:
When a session is revoked or pruned after `listActiveForIdentity` selects it, `updateSessionExpirationRow` still reports success without updating a row, so `SessionStore.issue` signs and returns a token for an invalid session. Use an update operation that returns or counts affected rows and fail when none matched, or make the selection and update atomic.
| const stored = input.read(CLIENT_INSTANCE_ID_STORAGE_KEY); | ||
| if (typeof stored === "string" && stored.trim() !== "") { | ||
| return stored; | ||
| } | ||
| const created = input.createId(); | ||
| input.write(CLIENT_INSTANCE_ID_STORAGE_KEY, created); | ||
| return created; |
There was a problem hiding this comment.
🟠 High authorization/clientInstanceId.ts:13
readOrCreateClientInstanceId lets storage exceptions escape, so blocked or full localStorage makes clientMetadata() throw during capabilitiesLayer construction and prevents the client from connecting. Catch failures from both input.read and input.write, then return the generated ID as an in-memory fallback.
- const stored = input.read(CLIENT_INSTANCE_ID_STORAGE_KEY);
+ let stored: string | null | undefined;
+ try {
+ stored = input.read(CLIENT_INSTANCE_ID_STORAGE_KEY);
+ } catch {
+ stored = undefined;
+ }
if (typeof stored === "string" && stored.trim() !== "") {
return stored;
}
const created = input.createId();
- input.write(CLIENT_INSTANCE_ID_STORAGE_KEY, created);
+ try {
+ input.write(CLIENT_INSTANCE_ID_STORAGE_KEY, created);
+ } catch {
+ // Keep the generated ID in memory when persistence is unavailable.
+ }
return created;Also found in 1 other location(s)
apps/web/src/connection/platform.ts:136
clientInstanceIdaccessesglobalThis.localStoragewithout exception handling. ThelocalStoragegetter can throwSecurityErrorwhen browser policy blocks persistence (for example when cookies/storage are disabled), andsetItemcan also fail; because this now runs whilecapabilitiesLayeris constructed, such users lose the entire connection platform instead of merely operating without a persisted instance ID. Optional chaining does not catch an exception thrown by the property getter.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/authorization/clientInstanceId.ts around lines 13-19:
`readOrCreateClientInstanceId` lets storage exceptions escape, so blocked or full `localStorage` makes `clientMetadata()` throw during `capabilitiesLayer` construction and prevents the client from connecting. Catch failures from both `input.read` and `input.write`, then return the generated ID as an in-memory fallback.
Also found in 1 other location(s):
- apps/web/src/connection/platform.ts:136 -- `clientInstanceId` accesses `globalThis.localStorage` without exception handling. The `localStorage` getter can throw `SecurityError` when browser policy blocks persistence (for example when cookies/storage are disabled), and `setItem` can also fail; because this now runs while `capabilitiesLayer` is constructed, such users lose the entire connection platform instead of merely operating without a persisted instance ID. Optional chaining does not catch an exception thrown by the property getter.
| kind: "session", | ||
| sid: reusable.sessionId, | ||
| sub: reusable.subject, | ||
| scopes: reusable.scopes, |
There was a problem hiding this comment.
🟠 High auth/SessionStore.ts:618
The reuse path issues a token with reusable.scopes, so a client that previously had broad scopes keeps those permissions after requesting a reduced scope set. Because scopes.every(...) accepts stored supersets, the token's authorization exceeds the current request; use the requested scopes for the reused claims (or only reuse on exact scope equality).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/auth/SessionStore.ts around line 618:
The reuse path issues a token with `reusable.scopes`, so a client that previously had broad scopes keeps those permissions after requesting a reduced scope set. Because `scopes.every(...)` accepts stored supersets, the token's authorization exceeds the current request; use the requested `scopes` for the reused claims (or only reuse on exact scope equality).
| * through `shell: true` spawns. | ||
| */ | ||
| const OSC_RE = /\x1b\].*?(?:\x07|\x1b\\)/g; | ||
| const CSI_RE = /\x1b\[[0-9;?]*[ -/]*[@-~]/g; |
There was a problem hiding this comment.
🟠 High src/stripTerminalEscapes.ts:17
CSI_RE leaves valid colon-delimited SGR sequences such as \x1b[38:2::255:0:0m in the output, so those escapes can pollute parsed identifiers or break JSON/version parsing. Match the full CSI parameter-byte range, including :, instead of restricting it to digits, ;, and ?.
| const CSI_RE = /\x1b\[[0-9;?]*[ -/]*[@-~]/g; | |
| const CSI_RE = /\x1b\[[0-?]*[ -/]*[@-~]/g; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/shared/src/stripTerminalEscapes.ts around line 17:
`CSI_RE` leaves valid colon-delimited SGR sequences such as `\x1b[38:2::255:0:0m` in the output, so those escapes can pollute parsed identifiers or break JSON/version parsing. Match the full CSI parameter-byte range, including `:`, instead of restricting it to digits, `;`, and `?`.
| (cause) => new DesktopLocalEnvironmentAuthSessionBootstrapError({ cause }), | ||
| ), | ||
| ); | ||
| if (Option.isNone(stored)) { |
There was a problem hiding this comment.
🟡 Medium backend/DesktopLocalEnvironmentAuth.ts:77
A whitespace-only client-instance-id file causes a new instanceId to be generated on every desktop launch, so each bearer bootstrap creates a distinct authorized-client session. The persistence branch checks only Option.isNone(stored), so it skips writing the replacement for present-but-empty content; persist whenever the stored value is missing or blank.
| if (Option.isNone(stored)) { | |
| if (Option.isNone(stored) || stored.value.trim() === "") { |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/backend/DesktopLocalEnvironmentAuth.ts around line 77:
A whitespace-only `client-instance-id` file causes a new `instanceId` to be generated on every desktop launch, so each bearer bootstrap creates a distinct authorized-client session. The persistence branch checks only `Option.isNone(stored)`, so it skips writing the replacement for present-but-empty content; persist whenever the stored value is missing or blank.
| credential: connect.credential, | ||
| dpopProof: bootstrapDpop, | ||
| clientMetadata: authClientMetadata(), | ||
| clientMetadata: authClientMetadata({ instanceId: yield* loadClientInstanceId() }), |
There was a problem hiding this comment.
🟠 High cloud/linkEnvironment.ts:567
Cloud connect and refresh now fail before exchangeRemoteDpopAccessToken whenever secure storage cannot load the client instance ID, even though instanceId is optional. Handle that storage failure by omitting the metadata (or using an in-memory ID) so a transient or corrupt secure-storage read does not block authentication.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/cloud/linkEnvironment.ts around line 567:
Cloud connect and refresh now fail before `exchangeRemoteDpopAccessToken` whenever secure storage cannot load the client instance ID, even though `instanceId` is optional. Handle that storage failure by omitting the metadata (or using an in-memory ID) so a transient or corrupt secure-storage read does not block authentication.
| } satisfies IssuedSession; | ||
| } | ||
| for (const stale of candidates) { | ||
| yield* authSessions |
There was a problem hiding this comment.
🟠 High auth/SessionStore.ts:663
When requested scopes widen and replacement issuance fails in crypto.randomUUIDv4, claims encoding, or authSessions.create, every existing session for the instance has already been revoked, so the exchange returns an error while leaving the client with no usable session. The loop at 663 runs before replacement creation; create the replacement and revoke stale rows atomically, or defer revocation until after successful creation.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/auth/SessionStore.ts around line 663:
When requested scopes widen and replacement issuance fails in `crypto.randomUUIDv4`, claims encoding, or `authSessions.create`, every existing session for the instance has already been revoked, so the exchange returns an error while leaving the client with no usable session. The loop at `663` runs before replacement creation; create the replacement and revoke stale rows atomically, or defer revocation until after successful creation.
1 participant
Fixes #7977
Every bearer bootstrap exchange minted a brand-new auth_sessions row (30-day TTL), so Settings → Authorized clients fills with duplicate "T3 Code Desktop" entries per relaunch.
This patch adds optional client_instance_id to /oauth/token and reuses the existing compatible session (same subject+method+instance_id) instead of inserting a new row. Also prunes expired/revoked rows.
Managed via fh as fix-auth-stop-accumulating-authorized-client-sessi-p5zy3k6c in ImBIOS/.forkhub.
Verify:
Closes #7977
Note
Stop accumulating authorized-client rows per app relaunch via client
instanceIdclient_instance_idand send it during the DPoP bearer token exchange. The server stores it onauth_sessions(migration 41 adds the column) and threads it throughAuthClientMetadataandVerifiedSession.SessionStore.make.issuereuses an existing active session for the same subject/method/instanceIdwhen its scopes cover the requested scopes — extending expiration and issuing a new token for the samesessionId. When requested scopes widen, stale instance sessions are revoked and a fresh session is issued.prune(now)runs after each creation to delete expired or revoked sessions.stripTerminalEscapes/sanitizeTerminalValuemodule is applied to opencode CLI output parsers (parseModelsCliOutput,parseAgentListCliOutput,parseSkillsCliOutput, version probe) and model-selection values so agent/variant/model ids no longer include OSC or ANSI escape sequences.client_instance_id; existing persisted sessions lack the column (nullable, safe). Desktop persists the id underenvironment.stateDir; if the write fails it logs a warning and proceeds with an in-memory id.📊 Macroscope summarized d8458d1. 24 files reviewed, 11 issues evaluated, 3 issues filtered, 8 comments posted
🗂️ Filtered Issues
apps/web/src/connection/platform.ts — 0 comments posted, 1 evaluated, 1 filtered
clientInstanceIdaccessesglobalThis.localStoragewithout exception handling. ThelocalStoragegetter can throwSecurityErrorwhen browser policy blocks persistence (for example when cookies/storage are disabled), andsetItemcan also fail; because this now runs whilecapabilitiesLayeris constructed, such users lose the entire connection platform instead of merely operating without a persisted instance ID. Optional chaining does not catch an exception thrown by the property getter. [ Cross-file consolidated ]docs/internals/environment-auth.md — 0 comments posted, 2 evaluated, 2 filtered
client_instance_idis operational input:SessionStore.issueuses it to select a session for reuse and to revoke prior sessions when scopes are incompatible. Clients relying on this contract may treat the value as disposable UI metadata even though changing or colliding it alters authentication-session lifecycle. [ Out of scope (post-validation triage) ]client_instance_idexchanges.SessionStore.issuereturns immediately after extending and re-signing a reusable session, whileauthSessions.prune({ now })is only called on the new-session path. Thus repeated compatible relaunch exchanges can continue indefinitely without performing the documented pruning. [ Out of scope ]