fix(repo): harden keyless accountless requests#8676
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: c11afee The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
mwickett
changed the title
API Changes Report
Summary
@clerk/sharedCurrent version: 4.14.0 Subpath
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a shared isAutomatedEnvironment() utility that detects CI/automation env vars and updates canUseKeyless across Astro, Next.js, Nuxt, React Router, and TanStack packages to require !isAutomatedEnvironment(). It also introduces a normalized per-service source value for accountless-application flows, extends backend AccountlessApplications endpoints to accept a source query param, threads source through shared keyless service and SDK server adapters, and adds tests plus a changeset. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tanstack-react-start/src/utils/feature-flags.ts (1)
11-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the JSDoc to include the automated-environment guard.
The comment says keyless is available in development when not explicitly disabled, but Line 19 also blocks it in automated environments. Please align docs with current behavior.
As per coding guidelines, "All public APIs must be documented with JSDoc" and docs should remain accurate.
🤖 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/tanstack-react-start/src/utils/feature-flags.ts` around lines 11 - 19, The JSDoc above canUseKeyless is out of date: it states keyless mode is available in development when not explicitly disabled but omits that automated environments also block it; update the comment to reflect the full guard used by canUseKeyless (isDevelopmentEnvironment() && !isAutomatedEnvironment() && !KEYLESS_DISABLED), explicitly mention automated environments prevent keyless mode, and keep the note about disabling via VITE_CLERK_KEYLESS_DISABLED or CLERK_KEYLESS_DISABLED; reference canUseKeyless, isDevelopmentEnvironment, isAutomatedEnvironment, and KEYLESS_DISABLED when editing the doc so the behavior and disable instructions are aligned.
🤖 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/shared/src/utils/runtimeEnvironment.ts`:
- Around line 32-39: Replace the local getEnvVariable implementation with the
shared cross-runtime helper by importing the shared getEnvVariable and removing
the local const; update isAutomatedEnvironment to call the imported
getEnvVariable (instead of the local one) so detection checks
import.meta.env/globalThis fallbacks; ensure the import name matches the
exported symbol (getEnvVariable) and delete the local getEnvVariable function
declaration to avoid shadowing.
---
Outside diff comments:
In `@packages/tanstack-react-start/src/utils/feature-flags.ts`:
- Around line 11-19: The JSDoc above canUseKeyless is out of date: it states
keyless mode is available in development when not explicitly disabled but omits
that automated environments also block it; update the comment to reflect the
full guard used by canUseKeyless (isDevelopmentEnvironment() &&
!isAutomatedEnvironment() && !KEYLESS_DISABLED), explicitly mention automated
environments prevent keyless mode, and keep the note about disabling via
VITE_CLERK_KEYLESS_DISABLED or CLERK_KEYLESS_DISABLED; reference canUseKeyless,
isDevelopmentEnvironment, isAutomatedEnvironment, and KEYLESS_DISABLED when
editing the doc so the behavior and disable instructions are aligned.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bce89080-379d-4448-ad50-e862116d17f6
📒 Files selected for processing (9)
.changeset/keyless-ci-guard.mdpackages/astro/src/utils/feature-flags.tspackages/nextjs/src/utils/__tests__/feature-flags.test.tspackages/nextjs/src/utils/feature-flags.tspackages/nuxt/src/runtime/utils/feature-flags.tspackages/react-router/src/utils/feature-flags.tspackages/shared/src/__tests__/runtimeEnvironment.spec.tspackages/shared/src/utils/runtimeEnvironment.tspackages/tanstack-react-start/src/utils/feature-flags.ts
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
mwickett
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/shared/src/keyless/__tests__/service.spec.ts (1)
28-47: ⚡ Quick winExpand tests to cover source normalization and fallback.
Current assertions only cover already-normalized
'nextjs'. Add cases for undefined framework (fallback to'javascript') and normalization inputs (e.g.,'Next.js', spaced/symbol-heavy names, long names) to lock increateSourcebehavior.As per coding guidelines:
**/*.{test,spec}.{ts,tsx}should verify edge cases for new functionality.Also applies to: 49-66
🤖 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/shared/src/keyless/__tests__/service.spec.ts` around lines 28 - 47, Add tests to verify source normalization and fallback: extend the existing test using createKeylessService and its getOrCreateKeys call to assert that when framework is undefined the createAccountlessApplication receives source 'javascript', and add additional cases where framework inputs like 'Next.js', 'Next JS', and symbol/long-heavy names are normalized to the expected canonical source (e.g., 'nextjs' or trimmed/slugified result produced by your createSource logic); update both the block around the current test (the one using createAccountlessApplication mock) and the similar block around lines 49-66 to include these inputs and assertions against createAccountlessApplication.mock.calls to confirm headers is a Headers instance and source equals the normalized fallback.packages/backend/src/api/__tests__/AccountlessApplicationsApi.test.ts (1)
16-37: ⚡ Quick winAdd a no-
sourceedge-case test for both API calls.These tests validate
source=nextjs, but not the optional path. Please add coverage wheresourceis omitted and assert the query param is absent (prevents regressions likesource=undefined).As per coding guidelines:
**/*.{test,spec}.{ts,tsx}tests should verify proper error handling and edge cases.Also applies to: 39-60
🤖 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/backend/src/api/__tests__/AccountlessApplicationsApi.test.ts` around lines 16 - 37, Add tests that cover the "no source" edge case for the accountless application API calls: duplicate the existing test in AccountlessApplicationsApi.test.ts but call createBackendApiClient and __experimental_accountlessApplications.createAccountlessApplication (and the other accountless API call referenced around lines 39-60) without passing a source, then in the server mock handler assert that new URL(request.url).searchParams.get('source') is null (or falsy) to ensure no "source=undefined" is sent, while keeping the same header assertions (Clerk-API-Version and User-Agent) and response expectations (e.g., publishableKey).
🤖 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/backend/src/api/endpoints/AccountlessApplicationsAPI.ts`:
- Line 13: The public methods createAccountlessApplication and the other public
method at line 25 need explicit TypeScript return type annotations; update their
signatures to declare the precise Promise-returning types that match their
implementations (e.g., Promise<...> or Promise<void> as appropriate) so the
exported class API has explicit return types; locate the methods by name
(createAccountlessApplication and the public method on line 25) and add the
correct return type annotations to their signatures.
---
Nitpick comments:
In `@packages/backend/src/api/__tests__/AccountlessApplicationsApi.test.ts`:
- Around line 16-37: Add tests that cover the "no source" edge case for the
accountless application API calls: duplicate the existing test in
AccountlessApplicationsApi.test.ts but call createBackendApiClient and
__experimental_accountlessApplications.createAccountlessApplication (and the
other accountless API call referenced around lines 39-60) without passing a
source, then in the server mock handler assert that new
URL(request.url).searchParams.get('source') is null (or falsy) to ensure no
"source=undefined" is sent, while keeping the same header assertions
(Clerk-API-Version and User-Agent) and response expectations (e.g.,
publishableKey).
In `@packages/shared/src/keyless/__tests__/service.spec.ts`:
- Around line 28-47: Add tests to verify source normalization and fallback:
extend the existing test using createKeylessService and its getOrCreateKeys call
to assert that when framework is undefined the createAccountlessApplication
receives source 'javascript', and add additional cases where framework inputs
like 'Next.js', 'Next JS', and symbol/long-heavy names are normalized to the
expected canonical source (e.g., 'nextjs' or trimmed/slugified result produced
by your createSource logic); update both the block around the current test (the
one using createAccountlessApplication mock) and the similar block around lines
49-66 to include these inputs and assertions against
createAccountlessApplication.mock.calls to confirm headers is a Headers instance
and source equals the normalized fallback.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d0eb8c46-14db-40d0-a29e-df25345bf7a5
📒 Files selected for processing (10)
.changeset/keyless-ci-guard.mdpackages/astro/src/server/keyless/index.tspackages/backend/src/api/__tests__/AccountlessApplicationsApi.test.tspackages/backend/src/api/endpoints/AccountlessApplicationsAPI.tspackages/nextjs/src/server/keyless-node.tspackages/nuxt/src/runtime/server/keyless/index.tspackages/react-router/src/server/keyless/index.tspackages/shared/src/keyless/__tests__/service.spec.tspackages/shared/src/keyless/service.tspackages/tanstack-react-start/src/server/keyless/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/keyless-ci-guard.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared/src/__tests__/runtimeEnvironment.spec.ts (1)
24-29: ⚡ Quick winAdd a falsey global-fallback test case too.
Nice coverage for
globalThistruthy fallback. Please also add a companion case (e.g.,vi.stubGlobal('CI', 'false')or'0') to lock in falsey normalization on the same path.As per coding guidelines, test files should verify proper error handling and edge cases.
🤖 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/shared/src/__tests__/runtimeEnvironment.spec.ts` around lines 24 - 29, Add a companion test that verifies falsey global fallback: when you clear the env var via vi.stubEnv('CI', undefined) and set the global via vi.stubGlobal('CI', 'false') (or '0'), assert isAutomatedEnvironment() returns false; this mirrors the existing truthy global test and locks in normalization on the globalThis path—update runtimeEnvironment.spec.ts to include the new it(...) using vi.stubEnv, vi.stubGlobal, and calling isAutomatedEnvironment().
🤖 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.
Nitpick comments:
In `@packages/shared/src/__tests__/runtimeEnvironment.spec.ts`:
- Around line 24-29: Add a companion test that verifies falsey global fallback:
when you clear the env var via vi.stubEnv('CI', undefined) and set the global
via vi.stubGlobal('CI', 'false') (or '0'), assert isAutomatedEnvironment()
returns false; this mirrors the existing truthy global test and locks in
normalization on the globalThis path—update runtimeEnvironment.spec.ts to
include the new it(...) using vi.stubEnv, vi.stubGlobal, and calling
isAutomatedEnvironment().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d408055b-0422-4056-a380-b498e35b726c
📒 Files selected for processing (3)
packages/backend/src/api/endpoints/AccountlessApplicationsAPI.tspackages/shared/src/__tests__/runtimeEnvironment.spec.tspackages/shared/src/utils/runtimeEnvironment.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/backend/src/api/endpoints/AccountlessApplicationsAPI.ts
- packages/shared/src/utils/runtimeEnvironment.ts
| @@ -1,3 +1,32 @@ | |||
| import { getEnvVariable } from '../getEnvVariable'; | |||
|
|
|||
| export const automatedEnvironmentVariables = [ | |||
There was a problem hiding this comment.
I'm unsure of better ways to detect CI envs, definitely open to suggestions from those with more knowledge!
2 participants
Summary
Tests