Repository navigation
Use stable user IDs across identity boundaries - #1072
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR replaces externally exposed numeric user identifiers with validated stable user IDs across authentication cookies, package-app handoffs, admin routes and APIs, feature flags, loader data, audit reasons, and related tests. ChangesStable user identity migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Worker
participant D1
participant PackageApp
Browser->>Worker: Send v2 cookie with stableUserId
Worker->>D1: Resolve users.stable_user_id
D1-->>Worker: Return account and internal database id
Worker-->>Browser: Return authenticated response
PackageApp->>Worker: Consume v2 handoff token
Worker->>D1: Resolve package-app owner by stableUserId
D1-->>PackageApp: Return validated owner
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
9 tasks
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fc9cbc6. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
Bot
force-pushed
the
cursor/track-identity-stable-id-e721
branch
from
July 30, 2026 19:49
fc9cbc6 to
d248457
Compare
Contributor
|
🔎 Preview deployed: https://kody-pr-1072.kody-a99.workers.dev Worker: Mocks:
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/admin/user-usage-data.ts (1)
85-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep downstream usage joins keyed by
users.id.The stable ID is correctly validated at this boundary, but
loadUserMonthRollupsandreadEntitlementConsumptionreceiveusageUserIdderived from the stable ID rather thanrow.id. Their internaluser_idqueries will no longer match the existing numeric joins, producing empty or incorrect usage data. Return the stable ID publicly, but passrow.idto internal usage and entitlement reads.Proposed direction
- const usageUserId = resolveUserStableId(row) + const usageStableUserId = resolveUserStableId(row) + const internalUserId = row.id loadUserMonthRollups({ - userId: usageUserId, + userId: internalUserId, }) readEntitlementConsumption({ - usageUserId, + usageUserId: internalUserId, }) - stableUserId: usageUserId, + stableUserId: usageStableUserId,As per coding guidelines, “every read and write path must be scoped by
userId”; use the resolved numeric key for internal reads after validating the stable-ID boundary.🤖 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 `@packages/worker/src/admin/user-usage-data.ts` around lines 85 - 130, Keep the public response keyed by the resolved stable ID, but update the internal calls to loadUserMonthRollups and readEntitlementConsumption to pass row.id as their userId/usageUserId value. Leave stableUserId validation and the returned stableUserId unchanged so downstream database queries continue matching users.id.Source: Coding guidelines
🧹 Nitpick comments (4)
packages/worker/src/app/handlers/admin-feature-flags.ts (1)
377-382: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable branch.
At this point
hasUsernameis true, which meansusername !== null, so this guard can never fire. Dropping it removes a misleading error path.🤖 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 `@packages/worker/src/app/handlers/admin-feature-flags.ts` around lines 377 - 382, Remove the unreachable `if (!username)` guard and its invalid response from the surrounding feature-flag handler, since `hasUsername` already guarantees a non-null username. Preserve the remaining username-based processing and error paths unchanged.packages/worker/src/app/handlers/admin-users.node.test.ts (1)
46-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
stableUserIdfixture helper. Both test files define the identical hex-padding helper whilepackages/worker/src/test-support/stable-user-id.tsalready exists for stable-id test utilities.
packages/worker/src/app/handlers/admin-users.node.test.ts#L46-L49: move the helper intopackages/worker/src/test-support/stable-user-id.tsand import it.packages/worker/src/app/handlers/admin-feature-flags.node.test.ts#L48-L53: import the shared helper instead of redefining it.🤖 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 `@packages/worker/src/app/handlers/admin-users.node.test.ts` around lines 46 - 49, The stableUserId helper is duplicated across both admin handler test files. Move or consolidate the implementation into packages/worker/src/test-support/stable-user-id.ts, export it there, and update stableUserId usage in packages/worker/src/app/handlers/admin-users.node.test.ts (lines 46-49) and packages/worker/src/app/handlers/admin-feature-flags.node.test.ts (lines 48-53) to import the shared helper and remove their local definitions.packages/worker/src/admin/users-data.ts (1)
250-269: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThree queries where two suffice.
loadAdminUserByTargetalready resolves the row; re-fetching it vialoadAdminUserRowByStableUserIdjust to obtainidadds a round trip per plan update (and a TOCTOU window). Consider having the target resolution return the row id, or resolving the row once and reusing it.🤖 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 `@packages/worker/src/admin/users-data.ts` around lines 250 - 269, The updateAdminUserPlan flow performs redundant target lookups before updating the plan. Refactor target resolution so it retrieves and retains the matched row’s id and stableUserId in one query, then use those values for the UPDATE and final result lookup, removing the separate loadAdminUserRowByStableUserId call while preserving the null behavior when no target matches.packages/worker/src/mcp/capabilities/admin/admin-user-get.ts (1)
12-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "exactly one selector" schema across two admin capabilities. Both files define an identical
{ stableUserId?, email?, username? }object with a.refine()requiring exactly one field. Extracting a shared schema/refine factory intoadmin-shared.ts(alongsidestableUserIdSchema) would remove the duplication and keep future selector-validation changes in one place.
packages/worker/src/mcp/capabilities/admin/admin-user-get.ts#L12-L24: replace the inline object+refine with a sharedadminUserTargetSelectorSchema(or similar) imported fromadmin-shared.ts.packages/worker/src/mcp/capabilities/admin/admin-user-usage.ts#L64-L76: same replacement; merge with its existingusage-specific output fields.🤖 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 `@packages/worker/src/mcp/capabilities/admin/admin-user-get.ts` around lines 12 - 24, Extract the shared exactly-one-user-selector schema into admin-shared.ts alongside stableUserIdSchema, preserving the stableUserId, email, username fields and validation message. In packages/worker/src/mcp/capabilities/admin/admin-user-get.ts lines 12-24, import and use the shared selector schema; in packages/worker/src/mcp/capabilities/admin/admin-user-usage.ts lines 64-76, replace the duplicated selector fields/refinement while retaining and merging its usage-specific output fields.
🤖 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 `@packages/worker/src/admin/users-data.ts`:
- Around line 196-240: Update loadAdminUserByTarget to reject an explicitly
supplied stableUserId when isStableUserId returns false, rather than falling
through to the email or username selectors. Preserve the existing stable-ID,
email, username, and not-found lookup behavior for valid or absent stableUserId
values, using the function’s established error mechanism for the invalid-input
result.
In `@packages/worker/src/app/handlers/admin-users.ts`:
- Around line 520-526: Update readStableUserIdField in
packages/worker/src/app/handlers/admin-users.ts (lines 520-526) and the
corresponding stable-user-ID boundary readers in
packages/worker/src/app/handlers/admin-feature-flags.ts (lines 426-429),
packages/worker/src/admin/users-data.ts (lines 95-99), and
packages/worker/src/admin/users-data.ts (lines 206-210) to call
normalizeStableUserId before validation and return the canonical value. Ensure
the normalized ID is used for database bindings and self-account comparisons so
valid non-canonical input resolves correctly.
In `@packages/worker/src/mcp/capabilities/admin/feature-flag-shared.ts`:
- Around line 84-97: Update the username resolution branch in the input lookup
to normalize the supplied username with trimming and lowercasing before querying
users.username, matching the normalization used during admin user creation.
Preserve the empty-input validation, lookup, and returned dbUserId/stableUserId
behavior.
---
Outside diff comments:
In `@packages/worker/src/admin/user-usage-data.ts`:
- Around line 85-130: Keep the public response keyed by the resolved stable ID,
but update the internal calls to loadUserMonthRollups and
readEntitlementConsumption to pass row.id as their userId/usageUserId value.
Leave stableUserId validation and the returned stableUserId unchanged so
downstream database queries continue matching users.id.
---
Nitpick comments:
In `@packages/worker/src/admin/users-data.ts`:
- Around line 250-269: The updateAdminUserPlan flow performs redundant target
lookups before updating the plan. Refactor target resolution so it retrieves and
retains the matched row’s id and stableUserId in one query, then use those
values for the UPDATE and final result lookup, removing the separate
loadAdminUserRowByStableUserId call while preserving the null behavior when no
target matches.
In `@packages/worker/src/app/handlers/admin-feature-flags.ts`:
- Around line 377-382: Remove the unreachable `if (!username)` guard and its
invalid response from the surrounding feature-flag handler, since `hasUsername`
already guarantees a non-null username. Preserve the remaining username-based
processing and error paths unchanged.
In `@packages/worker/src/app/handlers/admin-users.node.test.ts`:
- Around line 46-49: The stableUserId helper is duplicated across both admin
handler test files. Move or consolidate the implementation into
packages/worker/src/test-support/stable-user-id.ts, export it there, and update
stableUserId usage in packages/worker/src/app/handlers/admin-users.node.test.ts
(lines 46-49) and
packages/worker/src/app/handlers/admin-feature-flags.node.test.ts (lines 48-53)
to import the shared helper and remove their local definitions.
In `@packages/worker/src/mcp/capabilities/admin/admin-user-get.ts`:
- Around line 12-24: Extract the shared exactly-one-user-selector schema into
admin-shared.ts alongside stableUserIdSchema, preserving the stableUserId,
email, username fields and validation message. In
packages/worker/src/mcp/capabilities/admin/admin-user-get.ts lines 12-24, import
and use the shared selector schema; in
packages/worker/src/mcp/capabilities/admin/admin-user-usage.ts lines 64-76,
replace the duplicated selector fields/refinement while retaining and merging
its usage-specific output fields.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db031b6d-7c68-4d9e-8836-9c4b6b8c8e37
📒 Files selected for processing (67)
docs/contributing/adding-capabilities.mddocs/contributing/architecture/authentication.mde2e/admin-rbac.spec.tspackages/worker/client/routes/admin-feature-flags.tsxpackages/worker/client/routes/admin-invites.tsxpackages/worker/client/routes/admin-users.tsxpackages/worker/src/admin/user-usage-data.node.test.tspackages/worker/src/admin/user-usage-data.tspackages/worker/src/admin/users-data.tspackages/worker/src/app/admin-invites-data.tspackages/worker/src/app/auth-redirect.node.test.tspackages/worker/src/app/auth-session.node.test.tspackages/worker/src/app/auth-session.tspackages/worker/src/app/authenticated-user.node.test.tspackages/worker/src/app/email-change.tspackages/worker/src/app/handlers/account-billing.tspackages/worker/src/app/handlers/account-email-change.node.test.tspackages/worker/src/app/handlers/account-profile.node.test.tspackages/worker/src/app/handlers/account-resend-verification.node.test.tspackages/worker/src/app/handlers/account.node.test.tspackages/worker/src/app/handlers/admin-feature-flags.node.test.tspackages/worker/src/app/handlers/admin-feature-flags.tspackages/worker/src/app/handlers/admin-invites.tspackages/worker/src/app/handlers/admin-users.node.test.tspackages/worker/src/app/handlers/admin-users.tspackages/worker/src/app/handlers/auth-handler.node.test.tspackages/worker/src/app/handlers/auth-page.node.test.tspackages/worker/src/app/handlers/auth-provider.node.test.tspackages/worker/src/app/handlers/auth-provider.tspackages/worker/src/app/handlers/auth.tspackages/worker/src/app/handlers/passkeys.node.test.tspackages/worker/src/app/handlers/pending-verification.node.test.tspackages/worker/src/app/handlers/session-handler.node.test.tspackages/worker/src/app/handlers/two-factor.node.test.tspackages/worker/src/app/handlers/verify-email-change.tspackages/worker/src/app/handlers/verify.tspackages/worker/src/app/handlers/webauthn.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/package-app-handoff.node.test.tspackages/worker/src/app/package-app-handoff.tspackages/worker/src/app/package-app-origin.tspackages/worker/src/app/package-app-origin.workers.test.tspackages/worker/src/app/package-app-owner.tspackages/worker/src/app/package-app-session.tspackages/worker/src/app/permissions-server.node.test.tspackages/worker/src/app/request-auth-cache.tspackages/worker/src/app/routes.tspackages/worker/src/app/ssr-render.node.test.tspackages/worker/src/app/verify-session.tspackages/worker/src/app/webauthn.tspackages/worker/src/feature-flags/service.node.test.tspackages/worker/src/feature-flags/service.tspackages/worker/src/feature-flags/types.tspackages/worker/src/identity/admin-user-creation.tspackages/worker/src/mcp/capabilities/admin/admin-capabilities.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-feature-flag-capabilities.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-feature-flag-override.tspackages/worker/src/mcp/capabilities/admin/admin-platform-account-create.tspackages/worker/src/mcp/capabilities/admin/admin-shared.tspackages/worker/src/mcp/capabilities/admin/admin-user-create.tspackages/worker/src/mcp/capabilities/admin/admin-user-get.tspackages/worker/src/mcp/capabilities/admin/admin-user-update.tspackages/worker/src/mcp/capabilities/admin/admin-user-usage.tspackages/worker/src/mcp/capabilities/admin/feature-flag-shared.tspackages/worker/src/oauth-handlers.tspackages/worker/src/oauth-handlers.workers.test.tspackages/worker/src/user-id.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #1069
Summary
stable_user_id; old signed payloads fail closed and require re-login.users.idonly after boundary validation, preserving integer PK/FK joins without schema changes.Validation and deploy
npm run validateand PR CI passed, including all 19 Playwright tests.usage_rollups.user_idand entitlement-owned tables are intentionally TEXT stable IDs, not numericusers.idjoins.f1079665.3b84c504, which contains the merge./healthreports3b84c504; a fresh password login completed end-to-end and loaded the authenticated/accountpage.Boundary grep evidence
These searches return no matches:
Remaining numeric
userIduses in changed admin handlers are internal D1/RBAC/feature-flag join keys after stable-ID resolution.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@21f2068e· Head:b94eaefbClassification: extends — changes identity contracts at session, admin, OAuth, and feature-flag boundaries without adding a primitive or changing the database schema.
Primitives touched
app-sessionsapp-uimcp-oauthrbacfeature-flagsSystem map
Stable identity crosses every credential and admin boundary, then resolves once to the unchanged numeric D1 join key.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
users.idremains an internal D1 PK/FK only; no migration is introduced.Conductor report
Status: done
Verified: local validation, PR CI, main validation, production deploy/health, and a fresh production login all pass. No migrations or sibling-owned files changed.
Scope spill: production login verification created disposable account
cursor-identity-login-2031. Cleanup was attempted twice throughPOST /account/delete, but the account remains because the fail-closed deletion safety checks returned 503; no further retries were made.Summary by CodeRabbit
New Features
Security
stableUserIdcookies/tokens.Documentation