harden(account): packaged builds ignore development Clerk config#851
Conversation
A developer project that sets CLERK_ISSUER/CLERK_JWKS_URL/ CLERK_OAUTH_CLIENT_ID (or ADE_ACCOUNT_DIRECTORY_URL) to ADE's development Clerk instance leaked into the packaged production app's brain, which then authenticated against dev Clerk and got a 401 "invalid token" from the production account directory. Packaged builds now set ADE_RUNTIME_PACKAGED=1 (bootstrap.ts via the existing source-checkout check; desktop main.ts via app.isPackaged). When set, the account resolvers discard any Clerk override that resolves to a development instance (*.clerk.accounts.dev, incl. trailing-dot aliases) or the development account directory — atomically snapping issuer, JWKS, client, and directory back to the built-in production defaults (never a hybrid) and logging one warning. The machine publisher's directory override is guarded the same way. Source-checkout/dev builds are unchanged; ADE_ALLOW_DEVELOPMENT_CLERK=1 is a deliberate escape hatch. Non-development custom issuer overrides are still honored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Independent review findings:
- Packaged detection keyed off the module filename (bootstrap.cjs), but
bootstrap is bundled into main.cjs/cli.cjs, so dev builds running built
bundles misidentified as packaged and would auth against production
Clerk. Extract isSourceCheckoutRuntimeModule to runtimePackaging.ts and
make it directory-based (matches an apps/{ade-cli,desktop}/{src,dist}/
path segment) — packaged resources have no such segment.
- Route accountMachineDirectoryService's raw ADE_ACCOUNT_DIRECTORY_URL env
fallback through the dev-directory guard (was a latent bypass).
- Match the development account directory by host, so a dev-host URL with
an extra path/subdomain is also caught.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes Account-auth hardening: packaged builds ignore development Clerk overrides ( |
📝 WalkthroughWalkthroughThe change detects packaged runtimes and centralizes development Clerk and account-directory checks. Packaged CLI and desktop flows now use production account services, while non-packaged overrides remain supported. Tests cover runtime detection, configuration precedence, URL trust, authentication, publishing, and machine lifecycle behavior. ChangesPackaged account configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/main/services/account/accountBridge.ts (1)
105-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicated fallback resolution.
You can eliminate the duplicated multi-line call to
resolveOfficialAccountDirectoryBaseUrlby inverting theshouldIgnoreDevelopmentAccountDirectoryUrlcondition and falling through to the default return at the bottom of the function.♻️ Proposed refactor
- if (machineOverride?.trim()) { - if (shouldIgnoreDevelopmentAccountDirectoryUrl(machineOverride, process.env)) { - warnDevelopmentClerkIgnored(); - return resolveOfficialAccountDirectoryBaseUrl({ - env: process.env, - projectRoots: projectRoot ? [projectRoot] : [], - }); - } - return parseTrustedAccountDirectoryBaseUrl(machineOverride); - } + if (machineOverride?.trim()) { + if (!shouldIgnoreDevelopmentAccountDirectoryUrl(machineOverride, process.env)) { + return parseTrustedAccountDirectoryBaseUrl(machineOverride); + } + warnDevelopmentClerkIgnored(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/account/accountBridge.ts` around lines 105 - 118, Refactor the account directory URL selection around shouldIgnoreDevelopmentAccountDirectoryUrl so the ignored-development case does not return an inline resolveOfficialAccountDirectoryBaseUrl call. Invert the condition to return parseTrustedAccountDirectoryBaseUrl only when the override is allowed, then let the existing default return handle ignored or missing overrides without duplicating fallback resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/bootstrap.ts`:
- Around line 314-319: Make packaged-runtime detection authoritative: in
apps/ade-cli/src/bootstrap.ts, update the logic around
isSourceCheckoutRuntimeModule so ADE_RUNTIME_PACKAGED is set to "1" whenever the
module is classified as packaged, regardless of its existing environment value;
in apps/desktop/src/main/main.ts, set it to "1" whenever app.isPackaged is true.
Preserve ADE_ALLOW_DEVELOPMENT_CLERK=1 as the sole documented override.
In `@apps/ade-cli/src/services/account/sharedAccountAuthService.ts`:
- Around line 79-89: Update the configuration validation around
isClerkDevelopmentIssuer so development OAuth client IDs are rejected
independently of the issuer or JWKS host. When config.clientId is the
development client ID, replace the entire issuer/clientId tuple with the default
ADE values; otherwise preserve the existing configuration and handling for
non-development settings.
- Around line 79-108: Update both enforcement functions in
apps/ade-cli/src/services/account/sharedAccountAuthService.ts (lines 79-108) to
treat DEVELOPMENT_ADE_CLERK_OAUTH_CLIENT_ID as an independent development
configuration, alongside development issuer/JWKS checks. Add packaged OAuth
coverage in apps/ade-cli/src/services/account/sharedAccountAuthService.test.ts
(lines 188-218) using a custom issuer with the development client ID, and
packaged attestation coverage in lines 290-327 using custom issuer/JWKS values
with the development client ID; both cases must enforce packaged defaults.
---
Nitpick comments:
In `@apps/desktop/src/main/services/account/accountBridge.ts`:
- Around line 105-118: Refactor the account directory URL selection around
shouldIgnoreDevelopmentAccountDirectoryUrl so the ignored-development case does
not return an inline resolveOfficialAccountDirectoryBaseUrl call. Invert the
condition to return parseTrustedAccountDirectoryBaseUrl only when the override
is allowed, then let the existing default return handle ignored or missing
overrides without duplicating fallback resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f2409ce-5c6d-4adc-ac87-624c80b2a457
⛔ Files ignored due to path filters (2)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**
📒 Files selected for processing (13)
apps/ade-cli/src/bootstrap.test.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/runtimePackaging.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.test.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/account/sharedAccountAuthService.test.tsapps/ade-cli/src/services/account/sharedAccountAuthService.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/account/accountBridge.trust.test.tsapps/desktop/src/main/services/account/accountBridge.tsapps/desktop/src/shared/accountDirectory.ts
The packaged-build enforcement discarded dev issuers and dev JWKS hosts but not the development OAuth client id, so a custom issuer paired with DEVELOPMENT_ADE_CLERK_OAUTH_CLIENT_ID survived. Treat the dev client id as an independent development-instance marker in both enforcement functions, replacing the whole tuple with production defaults. Tests cover a dev client id behind a custom issuer/JWKS for OAuth and attestation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Pushed |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3b37039d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
isLoginConfigured() read the raw CLERK_* values, so in a packaged build a stale partial development secret (issuer or client id only) reported configured=false and disabled the Account-page sign-in — even though the hardened resolver would fall back to production and log in fine. Derive it from the same resolveAccountOAuthConfig() the login flow uses (which applies the packaged development-Clerk-ignore policy), and drop the now dead readProjectSecret helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Pushed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b124efac68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The config resolvers were hardened, but a stored account.session.v1 created against dev Clerk bypassed them: getAccessToken() returned the stored dev access token directly and refreshed against the stored dev oauthConfig, so a packaged build with a pre-existing dev session kept hitting the production directory with a dev token (401) until manual sign-out. In packaged mode (and equally for ADE_ACCOUNT_TOKEN credentials), reject any session whose stored oauthConfig issuer/client is a development instance or whose access-token `iss` claim is a dev host, before token return, refresh, userinfo, or directory use. A persisted dev session is compare-and-deleted from the shared credential store (only when the stored value still matches what we read) so status flips to signed-out without erasing a newer peer-written session, and the user re-signs-in against production. ADE_ALLOW_DEVELOPMENT_CLERK=1 still keeps dev working; source checkouts are unchanged. Swept the whole account surface (getAccessToken, refresh, status, userinfo, env-token, publisher, directory) for equivalent bypasses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Pushed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/account/accountAuthService.ts`:
- Around line 793-805: The invalidateStoredSessionIfCurrent function must not
use the non-atomic getSync/deleteSync fallback. Require updateSync/CAS for
conditional invalidation; when updateSync is unavailable, reject the session
without deleting any credential. Add coverage for a credential store lacking
updateSync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55828851-0c67-4c69-9a4a-ee8b11706069
⛔ Files ignored due to path filters (2)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**
📒 Files selected for processing (11)
apps/ade-cli/src/services/account/accountAuthService.test.tsapps/ade-cli/src/services/account/accountAuthService.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.test.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/account/sharedAccountAuthService.test.tsapps/ade-cli/src/services/account/sharedAccountAuthService.tsapps/desktop/src/main/services/account/accountBridge.trust.test.tsapps/desktop/src/main/services/account/accountBridge.tsapps/desktop/src/shared/accountDirectory.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/ade-cli/src/services/account/accountMachineDirectoryService.test.ts
- apps/ade-cli/src/services/account/accountMachineDirectoryService.ts
- apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts
- apps/ade-cli/src/services/account/sharedAccountAuthService.test.ts
- apps/desktop/src/shared/accountDirectory.ts
- apps/desktop/src/main/services/account/accountBridge.trust.test.ts
- apps/ade-cli/src/services/account/sharedAccountAuthService.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48ddd8ee55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review findings on the stored-session rejection: - The updateSync-less fallback in invalidateStoredSessionIfCurrent did a non-atomic getSync-then-deleteSync, which could delete a production credential a peer wrote between the two calls. Drop the non-atomic delete: use atomic compare-and-delete when available, otherwise leave the value in place and reject the development session on every read. - A rejected development ADE_ACCOUNT_TOKEN reported source: "env-token" with signedIn: false, so downstream treated it as an active env credential and blocked production re-auth. Report a fully signed-out status (source: null) instead. Adds coverage for a store without atomic compare-and-delete (rejected, not clobbered). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Pushed
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba272fc812
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two final review findings on the packaged dev-Clerk policy: - startLogin()/startDeviceLogin()/createToken() blocked whenever a raw ADE_ACCOUNT_TOKEN was present, so after a rejected development token correctly reported signed-out, Sign In threw "already providing" instead of starting production login. Route every login-gating path through readAcceptedEnvCredential(), which treats a rejected development credential as absent, so production login proceeds. - readSession() returned null right after invalidating a development session, ignoring the promised retry: when the atomic compare-and-delete saw a peer had already replaced it with a production session, that record stayed but the read still reported signed-out. Re-read once and return the peer-written production session in the same call; only a still-rejectable or absent session yields signed-out. Retry at most once. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Pushed
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c061a826
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Why
A developer's ADE project that sets
CLERK_ISSUER/CLERK_JWKS_URL/CLERK_OAUTH_CLIENT_ID(orADE_ACCOUNT_DIRECTORY_URL) to ADE's development Clerk instance (*.clerk.accounts.dev) leaked into the packaged production desktop app's brain. The app then authenticated against dev Clerk, got a dev token, and the production account directory returnedHTTP 401: invalid token— the Mac couldn't publish to the account directory ("Your Macs" empty). Normal users never hit this (no overrides → production defaults); it's a developer-machine footgun.What
Packaged builds set
ADE_RUNTIME_PACKAGED=1(ade-clibootstrap.tsvia a directory-based source-checkout check; desktopmain.tsviaapp.isPackaged). When set, the account resolvers discard any Clerk override that resolves to a development instance (*.clerk.accounts.dev, incl. trailing-dot aliases, or the dev account-directory host) and atomically snap issuer + JWKS + client + directory back to the built-in production defaults — never a hybrid — logging one warning. The machine publisher's directory override and the directory service's raw-env fallback are guarded the same way.ADE_ALLOW_DEVELOPMENT_CLERK=1is a deliberate escape hatch.Tests
New/extended coverage in
sharedAccountAuthService.test.ts,accountBridge.trust.test.ts,accountMachinePublisherService.test.ts,accountMachineDirectoryService.test.ts,bootstrap.test.ts: packaged+dev→prod (atomic), not-packaged+dev→dev, escape hatch, custom-prod-issuer honored, dev-JWKS-behind-prod-issuer→prod, dev-directory-override→prod, and packaged-detection classification for dev bundles vsADE.appresources.Independently reviewed (found + fixed a fail-closed packaged-detection regression before push). Desktop/ade-cli only — no iOS surface.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Greptile Summary
This PR hardens packaged account configuration so development Clerk and account-directory overrides do not leak into production desktop or CLI builds. The main changes are:
ADE_RUNTIME_PACKAGEDat startup.ADE_ACCOUNT_TOKENpaths reject development Clerk material unlessADE_ALLOW_DEVELOPMENT_CLERK=1is set.Confidence Score: 5/5
Safe to merge with low risk.
The changed paths are focused on packaged account configuration and include tests for fallback, escape hatch, custom issuer, session invalidation, and directory routing behavior. No current functional or security bugs were identified.
No files require special attention.
What T-Rex did
Important Files Changed
ADE_RUNTIME_PACKAGEDfor non-source-checkout CLI runtimes using the shared packaging classifier.ADE_RUNTIME_PACKAGEDbefore account services initialize.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Runtime as Packaged CLI/Desktop participant Env as Env/Project Secrets participant Resolver as Account Config Resolvers participant Auth as Account Auth Service participant Directory as Account Directory Runtime->>Env: "set ADE_RUNTIME_PACKAGED=1" Env->>Resolver: "provide CLERK_* / ADE_ACCOUNT_DIRECTORY_URL" Resolver->>Resolver: detect development Clerk or dev directory alt development config and no escape hatch Resolver-->>Auth: production issuer, JWKS, client, directory Auth->>Auth: reject stored/env development session material Auth->>Directory: use production directory with production token else "source checkout or ADE_ALLOW_DEVELOPMENT_CLERK=1" Resolver-->>Auth: honor configured development/custom values Auth->>Directory: use configured trusted directory end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Runtime as Packaged CLI/Desktop participant Env as Env/Project Secrets participant Resolver as Account Config Resolvers participant Auth as Account Auth Service participant Directory as Account Directory Runtime->>Env: "set ADE_RUNTIME_PACKAGED=1" Env->>Resolver: "provide CLERK_* / ADE_ACCOUNT_DIRECTORY_URL" Resolver->>Resolver: detect development Clerk or dev directory alt development config and no escape hatch Resolver-->>Auth: production issuer, JWKS, client, directory Auth->>Auth: reject stored/env development session material Auth->>Directory: use production directory with production token else "source checkout or ADE_ALLOW_DEVELOPMENT_CLERK=1" Resolver-->>Auth: honor configured development/custom values Auth->>Directory: use configured trusted directory endReviews (6): Last reviewed commit: "harden(account): unblock re-auth and hon..." | Re-trigger Greptile