Repository navigation
Use username-scoped public URLs - #453
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR changes public routing from stable user IDs to username-prefixed paths (/@{username}/...) for remote connectors, package apps, and package-invocation endpoints; adds shared public-URL builders; threads public username into caller contexts; and updates parsing, authorization, tests, docs, and Wrangler routing. ChangesUsername-scoped routing refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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>
kentcdodds
marked this pull request as ready for review
May 12, 2026 19:21
Contributor
|
🔎 Preview deployed: https://kody-pr-453.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/worker/src/oauth-handlers.workers.test.ts (1)
239-268: ⚡ Quick winAssert username propagation in the existing-session authorize test.
Given this PR’s URL identity shift, this test should verify
props.username(anddisplayName) passed tocompleteAuthorization, not just that options were captured.Suggested patch
expect(response.status).toBe(200) const payload = await response.json() expect(payload).toEqual({ ok: true, redirectTo: 'https://example.com/callback?code=session', }) expect(capturedOptions).not.toBeNull() + expect(capturedOptions?.props).toMatchObject({ + email: 'user@example.com', + username: 'test-user', + displayName: 'test-user', + }) })🤖 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/oauth-handlers.workers.test.ts` around lines 239 - 268, Update the existing test "authorize allows approval with an existing session" to assert that the session's username and displayName are propagated into the options passed to completeAuthorization: after the capturedOptions check, assert capturedOptions!.props.username === 'user@example.com' and capturedOptions!.props.displayName === 'user@example.com' (using the same identity from createAuthCookie) so completeAuthorization receives the expected props; reference the test, capturedOptions variable, and completeAuthorization handler to locate where to add these assertions.packages/worker/src/remote-connector/remote-connectors-shared.node.test.ts (1)
37-46: ⚡ Quick winConsider adding edge-case tests for username handling.
The test validates the happy path with a simple alphanumeric username (
'user-aaa'), but doesn't cover edge cases that could expose validation or encoding issues:
- Usernames with special characters (e.g.,
user@domain,user-name.test)- Empty or whitespace-only usernames
- Usernames with characters that require URL encoding
- Very long usernames
🧪 Example edge-case tests to add
+test('userScopedConnectorWebSocketUrl handles special characters in username', () => { + expect( + userScopedConnectorWebSocketUrl({ + origin: 'wss://kody.example.com/', + username: 'user-with.dots', + kind: 'Lights', + instanceId: 'living room', + }), + ).toBe('wss://kody.example.com/@user-with.dots/connectors/lights/living%20room') +}) + +test('userScopedConnectorWebSocketUrl rejects empty username', () => { + expect(() => + userScopedConnectorWebSocketUrl({ + origin: 'wss://kody.example.com/', + username: '', + kind: 'Lights', + instanceId: 'living room', + }), + ).toThrow() +})🤖 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/remote-connector/remote-connectors-shared.node.test.ts` around lines 37 - 46, Add edge-case unit tests for userScopedConnectorWebSocketUrl that verify username handling: test that special characters are percent-encoded (e.g., 'user@domain' yields 'user%40domain' in the URL and 'user.name' and '-' are preserved/encoded as appropriate), test that spaces are encoded (e.g., 'user name' -> 'user%20name'), add a test asserting the function rejects or throws for empty or whitespace-only usernames, and add a test with a very long username to ensure it is preserved/encoded rather than truncated; reference the userScopedConnectorWebSocketUrl helper in remote-connectors-shared.node.test.ts when adding these assertions.packages/worker/src/mcp/tools/search.ts (1)
174-179: ⚡ Quick winConsider consolidating duplicate username validation helpers.
This helper performs the same validation as
requireSavedPackageAppUsernameinopen-generated-ui.ts. Extract a shared utility to eliminate duplication.🤖 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/tools/search.ts` around lines 174 - 179, The function requireUsernameForHostedPackageUrl duplicates validation already implemented by requireSavedPackageAppUsername (in open-generated-ui.ts); extract a shared utility (e.g., requireUsername or validateUsernamePresence) into a common module and replace both requireUsernameForHostedPackageUrl and requireSavedPackageAppUsername to call that single shared function to remove duplication and centralize the error message/behavior.packages/worker/src/mcp/tools/open-generated-ui.ts (2)
84-89: ⚡ Quick winConsider consolidating duplicate username validation helpers.
This helper does the same validation as
requireUsernameForHostedPackageUrlinsearch.tsandsearch-format.ts. Consider extracting a shared utility (e.g., inpackages/shared/src/public-urls.tsalongsidebuildPackageAppUrl) to eliminate duplication and ensure consistent error messages.🤖 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/tools/open-generated-ui.ts` around lines 84 - 89, The helper requireSavedPackageAppUsername duplicates validation logic present in requireUsernameForHostedPackageUrl (used in search.ts and search-format.ts); extract a shared validator (e.g., validatePackageAppUsername or requireUsernameForPackageUrls) into the shared module alongside buildPackageAppUrl (suggested location: packages/shared/src/public-urls.ts) and replace both requireSavedPackageAppUsername and requireUsernameForHostedPackageUrl with calls to the new shared function so all callers share the same validation and error message.
129-138: ⚡ Quick winRedundant username validation before calling the helper.
The validation at lines 130-132 checks the same condition that
requireSavedPackageAppUsernamechecks at line 136. When line 136 executes (inside thesavedPackage ? ... : nullternary), you've already guaranteed thatusernameis truthy, so the helper's validation can never throw.Recommendation: Remove lines 130-132 and let the helper handle all validation, or remove the helper call and keep the early validation.
♻️ Simplified approach
const username = callerContext.user?.username ?? null - if (savedPackage && !username) { - throw new Error('Username is required to open saved package apps.') - } const hostedUrl = savedPackage ? buildPackageAppUrl({ origin: agent.requireDomain(), username: requireSavedPackageAppUsername(username), kodyId: savedPackage.kodyId, }) : null🤖 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/tools/open-generated-ui.ts` around lines 129 - 138, The code redundantly checks username before calling requireSavedPackageAppUsername; remove the early check (the if block that throws when savedPackage && !username) and let requireSavedPackageAppUsername perform validation, keeping the ternary that calls buildPackageAppUrl with requireSavedPackageAppUsername(username) and savedPackage.kodyId unchanged.packages/worker/src/mcp/tools/search-format.ts (1)
417-424: ⚡ Quick winConsider consolidating duplicate username validation helpers.
This is the third instance of the same username validation logic (also in
open-generated-ui.tsandsearch.ts). Extract a shared utility to eliminate duplication and ensure consistent error messages across all username-scoped URL construction.🤖 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/tools/search-format.ts` around lines 417 - 424, Extract the duplicate username validation into a single exported helper (e.g., validateUsernameOrThrow) and replace the three copies (requireUsernameForHostedPackageUrl in search-format.ts plus variants in open-generated-ui.ts and search.ts) with imports of the new utility; ensure the helper signature accepts (username: string | null | undefined) and throws the consistent Error('Username is required to build hosted package app URLs.') so all callers share identical behavior and message, then update the three call sites to use the new helper and remove the local duplicates.packages/worker/src/index.ts (1)
88-118: 💤 Low valueConsider error handling for user lookup failures.
The
findPublicUserIdentityByUsernamecall (line 97) will throw on database errors, resulting in a 500 response instead of 404. While this matches the current error handling pattern, you may want to explicitly catch and handle database errors to return 404 consistently.🛡️ Optional defensive error handling
const routeUser = await findPublicUserIdentityByUsername({ db: env.APP_DB, username: userScopedConnectorRoute.username, - }) + }).catch((error) => { + console.error('Failed to lookup user for connector route:', error) + return null + }) if (!routeUser) { return new Response('Not Found', { status: 404 }) }🤖 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/index.ts` around lines 88 - 118, The call to findPublicUserIdentityByUsername inside handleUserScopedConnectorRequest can throw on DB errors and cause a 500; wrap that call in a try/catch, and if it throws treat it the same as a missing user by returning a 404 (optionally log the error via processLogger or env logger) so the function returns a Not Found instead of propagating a 500; ensure you catch around the findPublicUserIdentityByUsername invocation and keep the rest of the logic (sessionKey generation, REMOTE_CONNECTOR_SESSION stub lookup, forwardRequest creation) unchanged.
🤖 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/oauth-handlers.ts`:
- Around line 660-666: The early-return when the session user is not found (in
the createDb(...)/db.findOne(usersTable, ...) branch) skips audit logging;
before calling respondAuthorizeError('Signed-in user not found.', 401) add a
call to the audit logging helper (e.g., logAuditEvent or the project’s audit
logger) with context such as event type "oauth.authorize_failed", reason
"session user not found", the sessionEmail, request info, and status 401 so the
failure is recorded; ensure the audit call runs synchronously/awaited if
logAuditEvent is async and keep the existing respondAuthorizeError call
afterwards.
---
Nitpick comments:
In `@packages/worker/src/index.ts`:
- Around line 88-118: The call to findPublicUserIdentityByUsername inside
handleUserScopedConnectorRequest can throw on DB errors and cause a 500; wrap
that call in a try/catch, and if it throws treat it the same as a missing user
by returning a 404 (optionally log the error via processLogger or env logger) so
the function returns a Not Found instead of propagating a 500; ensure you catch
around the findPublicUserIdentityByUsername invocation and keep the rest of the
logic (sessionKey generation, REMOTE_CONNECTOR_SESSION stub lookup,
forwardRequest creation) unchanged.
In `@packages/worker/src/mcp/tools/open-generated-ui.ts`:
- Around line 84-89: The helper requireSavedPackageAppUsername duplicates
validation logic present in requireUsernameForHostedPackageUrl (used in
search.ts and search-format.ts); extract a shared validator (e.g.,
validatePackageAppUsername or requireUsernameForPackageUrls) into the shared
module alongside buildPackageAppUrl (suggested location:
packages/shared/src/public-urls.ts) and replace both
requireSavedPackageAppUsername and requireUsernameForHostedPackageUrl with calls
to the new shared function so all callers share the same validation and error
message.
- Around line 129-138: The code redundantly checks username before calling
requireSavedPackageAppUsername; remove the early check (the if block that throws
when savedPackage && !username) and let requireSavedPackageAppUsername perform
validation, keeping the ternary that calls buildPackageAppUrl with
requireSavedPackageAppUsername(username) and savedPackage.kodyId unchanged.
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Around line 417-424: Extract the duplicate username validation into a single
exported helper (e.g., validateUsernameOrThrow) and replace the three copies
(requireUsernameForHostedPackageUrl in search-format.ts plus variants in
open-generated-ui.ts and search.ts) with imports of the new utility; ensure the
helper signature accepts (username: string | null | undefined) and throws the
consistent Error('Username is required to build hosted package app URLs.') so
all callers share identical behavior and message, then update the three call
sites to use the new helper and remove the local duplicates.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 174-179: The function requireUsernameForHostedPackageUrl
duplicates validation already implemented by requireSavedPackageAppUsername (in
open-generated-ui.ts); extract a shared utility (e.g., requireUsername or
validateUsernamePresence) into a common module and replace both
requireUsernameForHostedPackageUrl and requireSavedPackageAppUsername to call
that single shared function to remove duplication and centralize the error
message/behavior.
In `@packages/worker/src/oauth-handlers.workers.test.ts`:
- Around line 239-268: Update the existing test "authorize allows approval with
an existing session" to assert that the session's username and displayName are
propagated into the options passed to completeAuthorization: after the
capturedOptions check, assert capturedOptions!.props.username ===
'user@example.com' and capturedOptions!.props.displayName === 'user@example.com'
(using the same identity from createAuthCookie) so completeAuthorization
receives the expected props; reference the test, capturedOptions variable, and
completeAuthorization handler to locate where to add these assertions.
In `@packages/worker/src/remote-connector/remote-connectors-shared.node.test.ts`:
- Around line 37-46: Add edge-case unit tests for
userScopedConnectorWebSocketUrl that verify username handling: test that special
characters are percent-encoded (e.g., 'user@domain' yields 'user%40domain' in
the URL and 'user.name' and '-' are preserved/encoded as appropriate), test that
spaces are encoded (e.g., 'user name' -> 'user%20name'), add a test asserting
the function rejects or throws for empty or whitespace-only usernames, and add a
test with a very long username to ensure it is preserved/encoded rather than
truncated; reference the userScopedConnectorWebSocketUrl helper in
remote-connectors-shared.node.test.ts when adding these assertions.
🪄 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: ff5a87ac-1bb9-46c9-8076-b0d330b156dc
📒 Files selected for processing (42)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/remote-connectors.mddocs/contributing/architecture/request-lifecycle.mddocs/contributing/package-invocation-api.mddocs/contributing/security.mde2e/remote-connectors.spec.tspackages/shared/src/chat.tspackages/shared/src/public-urls.tspackages/shared/src/remote-connectors.tspackages/worker/client/routes/account-remote-connectors.tsxpackages/worker/src/app/authenticated-user.tspackages/worker/src/app/handlers/account-remote-connectors.node.test.tspackages/worker/src/app/handlers/account-remote-connectors.tspackages/worker/src/app/handlers/package-app.node.test.tspackages/worker/src/app/handlers/package-app.tspackages/worker/src/app/router.tspackages/worker/src/app/user-lookup.tspackages/worker/src/index.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/tools/open-generated-ui.node.test.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/oauth-handlers.tspackages/worker/src/oauth-handlers.workers.test.tspackages/worker/src/package-invocations/http.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/service.tspackages/worker/src/package-retrievers/service.tspackages/worker/src/package-runtime/package-app.tspackages/worker/src/package-runtime/package-service.tspackages/worker/src/package-runtime/package-workflows.tspackages/worker/src/package-runtime/realtime-session.tspackages/worker/src/remote-connector/connector-session-key.node.test.tspackages/worker/src/remote-connector/connector-session-key.tspackages/worker/src/remote-connector/remote-connectors-shared.node.test.tspackages/worker/src/remote-connector/session.tspackages/worker/src/security/public-route-hardening.workers.test.tspackages/worker/wrangler.jsonctools/ci/resource-utils.node.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/app/user-lookup.ts`:
- Around line 41-45: The code prematurely returns any non-empty input.username
(variable username) and thus skips fallback DB resolution; modify the
early-return in the function in user-lookup.ts to validate the trimmed username
(e.g., with the existing validateUsername or a username regex) before returning
it, and if validation fails continue to the fallback path (email -> DB lookup)
so stale/invalid usernames do not bypass lookupUserByUsername or the public URL
generation logic.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 1682-1687: hostedUrl construction currently calls
requireUsernameForPublicUrl and can throw when input.username is missing; change
the logic so hostedUrl is only computed when record.hasApp AND a username is
available (or compute a safeUsername via a non-throwing check) — update the
hostedUrl expression that calls buildPackageAppUrl to first verify
input.username (or use a safe getter) instead of invoking
requireUsernameForPublicUrl directly, or wrap the call in a short try/catch that
returns undefined on failure so the package entity lookup won't hard-fail.
In `@packages/worker/src/oauth-handlers.ts`:
- Line 658: The assignment approvedUsername = userRecord.username must validate
that userRecord.username is a non-empty string before using it downstream (e.g.,
for displayName and props.username); update the logic in the OAuth handling flow
(the approvedUsername assignment site and any code that sets
displayName/props.username) to check typeof userRecord.username === 'string' &&
userRecord.username.trim() !== '' and either use a safe fallback (such as
userRecord.email, a generated fallback, or undefined) or abort with a clear
error/validation response when the username is missing/empty so downstream
consumers never receive null/empty usernames.
🪄 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: 999e9ec4-3c09-4555-935e-5fd7020f3f85
📒 Files selected for processing (8)
packages/shared/src/public-urls.tspackages/worker/src/app/user-lookup.tspackages/worker/src/mcp/tools/open-generated-ui.node.test.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/oauth-handlers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/public-urls.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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 `@packages/worker/src/mcp/tools/search.ts`:
- Around line 1794-1799: resolvePublicUsername is being called before validation
and outside the main try/catch, so failures in that lookup can short-circuit and
bypass the structured validation/error handling; move the call to
resolvePublicUsername into the handled path (inside the try/catch and after the
args validation) and only invoke it when needed (e.g., before building the
search context) so errors are caught and returned via the existing error
handling; update references around callerContext.user and agent.getEnv().APP_DB
to use the resolved username variable within the try block where search logic
(the code that checks !args.query && !args.entity and subsequent handling)
executes.
🪄 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: 586ae4cf-65c2-4a80-aa3e-acda09fc38f7
📒 Files selected for processing (3)
packages/worker/src/app/user-lookup.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/oauth-handlers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/src/app/user-lookup.ts
- packages/worker/src/oauth-handlers.ts
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d3621d0. Configure here.
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
2 participants
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.
Summary
Validation
npm run validatepasses.ws://localhost:3742/@kentcdodds/connectors/manual-demo/default-demo.Summary by CodeRabbit
New Features
Bug Fixes
Documentation & Tests