fix(e2e): never carry production keys in staging mode without a staging key#8758
Open
jacekradko wants to merge 1 commit into
Open
fix(e2e): never carry production keys in staging mode without a staging key#8758jacekradko wants to merge 1 commit into
jacekradko wants to merge 1 commit into
Conversation
…ng key withInstanceKeys set the production PK/SK first, then in staging mode with no clerkstage- key it only deleted CLERK_API_URL and returned, leaving the production credentials live. Long-running apps are filtered by isStagingReady, but a test that bypasses that filter (e.g. one that builds its own app via app.withEnv) would silently drive a production instance from staging CI. Extract the credential resolution into a side-effect-free instanceKeys module and, in staging mode without a staging key, apply intentionally-invalid placeholder keys (never the production keys) and clear the API URL. The env stays not-staging-ready (so it is still filtered out of long-running apps), and any accidental use now fails fast against a non-existent instance instead of hitting production. No behavioral change to existing staging tests: every env used directly via withEnv has a staging key or clones one that does.
🦋 Changeset detectedLatest commit: 55a0c20 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughA new TypeScript module ChangesInstance Credentials Staging Resolution
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@integration/presets/__tests__/instanceKeys.test.ts`:
- Around line 17-48: Add a test that asserts resolveInstanceConfig throws when
isStaging=false and the requested instance key is missing: create keys via
makeKeys without the target instance (e.g., makeKeys({} or only unrelated
keys)), call resolveInstanceConfig('missing-instance', keys, false) and expect
it to throw; reference the resolveInstanceConfig function and makeKeys helper
and reuse constants like PROD/STAGING_UNAVAILABLE_* only for assertions about
non-staging behavior, ensuring the test covers the non-staging missing-key error
path.
🪄 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), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 70a4291a-c052-48c5-a0ae-222427477841
📒 Files selected for processing (4)
.changeset/staging-e2e-prod-key-fallback.mdintegration/presets/__tests__/instanceKeys.test.tsintegration/presets/envs.tsintegration/presets/instanceKeys.ts
Comment on lines
+17
to
+48
| describe('resolveInstanceConfig', () => { | ||
| it('uses production keys in non-staging mode and does not touch the API URL', () => { | ||
| const keys = makeKeys({ 'with-email-codes': PROD }); | ||
| const cfg = resolveInstanceConfig('with-email-codes', keys, false); | ||
| expect(cfg).toEqual({ pk: PROD.pk, sk: PROD.sk, clearApiUrl: false }); | ||
| }); | ||
|
|
||
| it('swaps to staging keys and sets the staging API URL when a staging key exists', () => { | ||
| const keys = makeKeys({ 'with-email-codes': PROD, 'clerkstage-with-email-codes': STAGING }); | ||
| const cfg = resolveInstanceConfig('with-email-codes', keys, true); | ||
| expect(cfg).toEqual({ pk: STAGING.pk, sk: STAGING.sk, apiUrl: STAGING_API_URL, clearApiUrl: false }); | ||
| }); | ||
|
|
||
| it('NEVER returns production keys in staging mode when the staging key is missing', () => { | ||
| const keys = makeKeys({ 'with-no-staging-mirror': PROD }); | ||
| const cfg = resolveInstanceConfig('with-no-staging-mirror', keys, true); | ||
|
|
||
| // The core safety invariant: a staging run must not carry production credentials. | ||
| expect(cfg.pk).not.toBe(PROD.pk); | ||
| expect(cfg.sk).not.toBe(PROD.sk); | ||
| expect(cfg.pk).toBe(STAGING_UNAVAILABLE_PK); | ||
| expect(cfg.sk).toBe(STAGING_UNAVAILABLE_SK); | ||
| // And it must be marked not-staging-ready by clearing any inherited API URL. | ||
| expect(cfg.clearApiUrl).toBe(true); | ||
| expect(cfg.apiUrl).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('placeholder keys are not real Clerk keys (cannot authenticate anywhere real)', () => { | ||
| expect(STAGING_UNAVAILABLE_PK).toContain('staging-key-unavailable'); | ||
| expect(STAGING_UNAVAILABLE_SK).toContain('staging-key-unavailable'); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add coverage for the non-staging missing-key error path.
The suite doesn’t currently assert the throw branch when isStaging=false and the requested key is absent.
Proposed test addition
describe('resolveInstanceConfig', () => {
+ it('throws in non-staging mode when production keys are missing', () => {
+ const keys = makeKeys({});
+ expect(() => resolveInstanceConfig('missing-instance', keys, false)).toThrow(
+ /Missing instance keys/,
+ );
+ });
+
it('uses production keys in non-staging mode and does not touch the API URL', () => {As per coding guidelines: **/*.{test,spec}.{ts,tsx} requires verifying proper error handling and edge cases for new functionality.
🤖 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 `@integration/presets/__tests__/instanceKeys.test.ts` around lines 17 - 48, Add
a test that asserts resolveInstanceConfig throws when isStaging=false and the
requested instance key is missing: create keys via makeKeys without the target
instance (e.g., makeKeys({} or only unrelated keys)), call
resolveInstanceConfig('missing-instance', keys, false) and expect it to throw;
reference the resolveInstanceConfig function and makeKeys helper and reuse
constants like PROD/STAGING_UNAVAILABLE_* only for assertions about non-staging
behavior, ensuring the test covers the non-staging missing-key error path.
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
1 participant
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.
A latent safety bug surfaced during the staging-e2e investigation.
withInstanceKeyssets the production PK/SK first, and in staging mode (E2E_STAGING=1) with noclerkstage-key it only deletedCLERK_API_URLand returned, leaving the production credentials live on the env. Long-running apps are filtered out byisStagingReady, but a test that bypasses that filter by building its own app viaapp.withEnvwould silently drive a production instance from staging CI.The fix pulls credential resolution into a small side-effect-free
instanceKeysmodule and, in staging mode without a staging key, applies intentionally-invalid placeholder keys (never the production keys) and clears the API URL. The env stays not-staging-ready, so it is still filtered out of long-running apps, and any accidental direct use now fails fast against a non-existent instance instead of hitting production.This is behavior-preserving for the suite: every env used directly via
withEnveither has a staging key or clones one that does (e.g.withDynamicKeys/withEmailCodesQuickstartderive fromwithEmailCodes), so the placeholder path is only reachable by envs that were already being filtered out. The separate module exists so the "never production in staging" invariant can be unit-tested without constructing every env inenvs.ts.Summary by CodeRabbit
New Features
Tests