Repository navigation
Hide approved OAuth host links - #135
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughPreloads user secrets and uses them to skip generating OAuth host-approval links for hosts already present in a secret's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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>
kentcdodds
marked this pull request as ready for review
April 2, 2026 14:55
Contributor
|
🔎 Preview deployed: https://kody-pr-135.kentcdodds.workers.dev Worker: Mocks:
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![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 and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Host approval links never generated in production
- Captured approved hosts before persisting updates and reused that snapshot when building OAuth host approval links, so newly written hosts no longer suppress link generation.
Preview (a2c7d89d09)
diff --git a/packages/worker/src/app/handlers/account-secrets.node.test.ts b/packages/worker/src/app/handlers/account-secrets.node.test.ts
--- a/packages/worker/src/app/handlers/account-secrets.node.test.ts
+++ b/packages/worker/src/app/handlers/account-secrets.node.test.ts
@@ -230,3 +230,83 @@
}),
)
})
+
+test('connect oauth omits direct host approval links when hosts are already approved', async () => {
+ mockModule.listSecrets.mockResolvedValueOnce([
+ {
+ name: 'teslaAccessToken',
+ scope: 'user',
+ description: '',
+ appId: null,
+ allowedHosts: [
+ 'auth.tesla.com',
+ 'fleet-api.prd.na.vn.cloud.tesla.com',
+ 'fleet-auth.prd.vn.cloud.tesla.com',
+ ],
+ allowedCapabilities: [],
+ createdAt: new Date(0).toISOString(),
+ updatedAt: new Date(0).toISOString(),
+ ttlMs: null,
+ },
+ {
+ name: 'teslaRefreshToken',
+ scope: 'user',
+ description: '',
+ appId: null,
+ allowedHosts: [
+ 'auth.tesla.com',
+ 'fleet-api.prd.na.vn.cloud.tesla.com',
+ 'fleet-auth.prd.vn.cloud.tesla.com',
+ ],
+ allowedCapabilities: [],
+ createdAt: new Date(0).toISOString(),
+ updatedAt: new Date(0).toISOString(),
+ ttlMs: null,
+ },
+ ])
+
+ const handler = createAccountSecretsApiHandler(createEnv())
+ const response = await handler.action({
+ request: new Request('https://example.com/account/secrets.json', {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ action: 'connect_oauth',
+ provider: 'Tesla',
+ tokenUrl: 'https://auth.tesla.com/oauth2/v3/token',
+ apiBaseUrl: 'https://fleet-api.prd.na.vn.cloud.tesla.com',
+ flow: 'pkce',
+ clientIdValueName: 'tesla-client-id',
+ accessTokenSecretName: 'teslaAccessToken',
+ refreshTokenSecretName: 'teslaRefreshToken',
+ allowedHosts: [
+ 'fleet-api.prd.na.vn.cloud.tesla.com',
+ 'fleet-auth.prd.vn.cloud.tesla.com',
+ ],
+ tokenPayload: {
+ access_token: 'access-token',
+ refresh_token: 'refresh-token',
+ },
+ }),
+ }),
+ params: {},
+ } as never)
+
+ expect(response.status).toBe(200)
+ const payload = await response.json()
+ expect(payload).toMatchObject({
+ ok: true,
+ accessTokenSaved: true,
+ refreshTokenSaved: true,
+ hostApprovalLinks: [],
+ connectorName: 'Tesla',
+ })
+ expect(payload.allowedHosts).toEqual(
+ expect.arrayContaining([
+ 'auth.tesla.com',
+ 'fleet-api.prd.na.vn.cloud.tesla.com',
+ 'fleet-auth.prd.vn.cloud.tesla.com',
+ ]),
+ )
+ expect(mockModule.createSecretHostApprovalToken).not.toHaveBeenCalled()
+})
diff --git a/packages/worker/src/app/handlers/account-secrets.ts b/packages/worker/src/app/handlers/account-secrets.ts
--- a/packages/worker/src/app/handlers/account-secrets.ts
+++ b/packages/worker/src/app/handlers/account-secrets.ts
@@ -315,6 +315,16 @@
)
}
+ const approvedHostsBySecretName = new Map(
+ (
+ await listSecrets({
+ env: input.env,
+ userId: input.user.mcpUser.userId,
+ scope: 'user',
+ storageContext: null,
+ })
+ ).map((secret) => [secret.name, new Set(secret.allowedHosts)]),
+ )
const accessSaved = await saveSecret({
env: input.env,
userId: input.user.mcpUser.userId,
@@ -381,6 +391,7 @@
userId: input.user.mcpUser.userId,
allowedHosts,
secretNames: approvalSecretNames,
+ approvedHostsBySecretName,
})
} catch (error) {
console.error('Failed to build OAuth host approval links.', {
@@ -406,6 +417,7 @@
userId: string
allowedHosts: Array<string>
secretNames: Array<string>
+ approvedHostsBySecretName?: Map<string, Set<string>>
}) {
const uniqueHosts = Array.from(new Set(input.allowedHosts)).slice(
0,
@@ -415,6 +427,18 @@
0,
maxConnectOauthApprovalSecrets,
)
+ const approvedHostsBySecretName =
+ input.approvedHostsBySecretName ??
+ new Map(
+ (
+ await listSecrets({
+ env: input.env,
+ userId: input.userId,
+ scope: 'user',
+ storageContext: null,
+ })
+ ).map((secret) => [secret.name, new Set(secret.allowedHosts)]),
+ )
const baseUrl = getAppBaseUrl({
env: input.env,
requestUrl: input.request.url,
@@ -422,6 +446,9 @@
const links = await Promise.all(
uniqueSecretNames.flatMap((secretName) =>
uniqueHosts.map(async (host) => {
+ if (approvedHostsBySecretName.get(secretName)?.has(host)) {
+ return null
+ }
const token = await createSecretHostApprovalToken(input.env, {
userId: input.userId,
name: secretName,
@@ -444,12 +471,14 @@
}),
),
)
- return links.sort((left, right) => {
- return (
- left.secretName.localeCompare(right.secretName) ||
- left.host.localeCompare(right.host)
- )
- })
+ return links
+ .filter((link): link is ConnectOauthHostApprovalLink => link !== null)
+ .sort((left, right) => {
+ return (
+ left.secretName.localeCompare(right.secretName) ||
+ left.host.localeCompare(right.host)
+ )
+ })
}
async function handleOAuthExchangeAction(input: {You can send follow-ups to this agent here.
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: 1
🧹 Nitpick comments (1)
packages/worker/src/app/handlers/account-secrets.ts (1)
318-327: Consider extracting approved-host map loading into a shared helper.The same
listSecrets(...).map(secret => [secret.name, new Set(secret.allowedHosts)])logic appears twice. A helper would reduce drift risk.Also applies to: 430-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/app/handlers/account-secrets.ts` around lines 318 - 327, Extract the repeated logic that builds a Map of secretName -> Set(allowedHosts) into a shared helper function (e.g., buildApprovedHostsMap or getApprovedHostsBySecret) that accepts the same parameters passed to listSecrets (env, userId, scope, storageContext) or the returned secret array; replace the inline uses that create approvedHostsBySecretName and the other occurrence (lines referenced around 430-441) to call this helper instead of duplicating listSecrets(...).map(...), and ensure the helper returns Map<string, Set<string>> and is exported/placed in a shared util file so both account-secrets.ts call sites can reuse it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 318-327: The preloading call to listSecrets that builds
approvedHostsBySecretName can throw and currently runs outside the existing
fallback try/catch, which allows transient failures to block the connect_oauth
flow; wrap the listSecrets call in a try/catch (or move it inside the existing
fallback block) and on error log the failure and fall back to an empty Map for
approvedHostsBySecretName so token save/update and link generation still
proceed; refer to the approvedHostsBySecretName variable and the listSecrets
invocation when making this change.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 318-327: Extract the repeated logic that builds a Map of
secretName -> Set(allowedHosts) into a shared helper function (e.g.,
buildApprovedHostsMap or getApprovedHostsBySecret) that accepts the same
parameters passed to listSecrets (env, userId, scope, storageContext) or the
returned secret array; replace the inline uses that create
approvedHostsBySecretName and the other occurrence (lines referenced around
430-441) to call this helper instead of duplicating listSecrets(...).map(...),
and ensure the helper returns Map<string, Set<string>> and is exported/placed in
a shared util file so both account-secrets.ts call sites can reuse it.
🪄 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
Run ID: d0251a47-da5c-46d7-b50e-35b56f27e82f
📒 Files selected for processing (1)
packages/worker/src/app/handlers/account-secrets.ts
Comment on lines
+318
to
+327
| const approvedHostsBySecretName = new Map( | ||
| ( | ||
| await listSecrets({ | ||
| env: input.env, | ||
| userId: input.user.mcpUser.userId, | ||
| scope: 'user', | ||
| storageContext: null, | ||
| }) | ||
| ).map((secret) => [secret.name, new Set(secret.allowedHosts)]), | ||
| ) |
There was a problem hiding this comment.
Preload failure can now block the entire OAuth connect flow.
Line 318 runs listSecrets outside the existing fallback try/catch (Line 387+), so a transient read failure now fails connect_oauth before token save/update work runs. This was previously non-blocking behavior for link generation.
Suggested fix
- const approvedHostsBySecretName = new Map(
- (
- await listSecrets({
- env: input.env,
- userId: input.user.mcpUser.userId,
- scope: 'user',
- storageContext: null,
- })
- ).map((secret) => [secret.name, new Set(secret.allowedHosts)]),
- )
+ let approvedHostsBySecretName: Map<string, Set<string>> | undefined
+ try {
+ approvedHostsBySecretName = new Map(
+ (
+ await listSecrets({
+ env: input.env,
+ userId: input.user.mcpUser.userId,
+ scope: 'user',
+ storageContext: null,
+ })
+ ).map((secret) => [secret.name, new Set(secret.allowedHosts)]),
+ )
+ } catch (error) {
+ console.error('Failed to preload approved OAuth hosts.', {
+ userId: input.user.mcpUser.userId,
+ error,
+ })
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const approvedHostsBySecretName = new Map( | |
| ( | |
| await listSecrets({ | |
| env: input.env, | |
| userId: input.user.mcpUser.userId, | |
| scope: 'user', | |
| storageContext: null, | |
| }) | |
| ).map((secret) => [secret.name, new Set(secret.allowedHosts)]), | |
| ) | |
| let approvedHostsBySecretName: Map<string, Set<string>> | undefined | |
| try { | |
| approvedHostsBySecretName = new Map( | |
| ( | |
| await listSecrets({ | |
| env: input.env, | |
| userId: input.user.mcpUser.userId, | |
| scope: 'user', | |
| storageContext: null, | |
| }) | |
| ).map((secret) => [secret.name, new Set(secret.allowedHosts)]), | |
| ) | |
| } catch (error) { | |
| console.error('Failed to preload approved OAuth hosts.', { | |
| userId: input.user.mcpUser.userId, | |
| error, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/app/handlers/account-secrets.ts` around lines 318 - 327,
The preloading call to listSecrets that builds approvedHostsBySecretName can
throw and currently runs outside the existing fallback try/catch, which allows
transient failures to block the connect_oauth flow; wrap the listSecrets call in
a try/catch (or move it inside the existing fallback block) and on error log the
failure and fall back to an empty Map for approvedHostsBySecretName so token
save/update and link generation still proceed; refer to the
approvedHostsBySecretName variable and the listSecrets invocation when making
this change.
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
vi.clearAllMocks()calls now that shared Vitest config already enablesclearMocks: trueTesting
npm run test -- packages/worker/src/app/handlers/account-secrets.node.test.tsnpm run test -- --skip-nx-cache packages/worker/src/app/handlers/account-secrets.node.test.tstools/seed-test-data.tscurrently fails against local Wrangler withCouldn't find a D1 DB with the name or binding 'APP_DB' in your Wrangler configuration file.Summary by CodeRabbit
Bug Fixes
Tests