fix(env): surface CLERK_PLATFORM_API_URL credential mismatch - #344
fix(env): surface CLERK_PLATFORM_API_URL credential mismatch#344rafa-thayto wants to merge 6 commits into
CLERK_PLATFORM_API_URL credential mismatch#344Conversation
🦋 Changeset detectedLatest commit: 8625e13 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
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:
📝 WalkthroughWalkthroughA new exported helper Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 7 linked repositories, but your current plan allows 1. Analyzed 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 |
wyattjoh
left a comment
wyattjoh
left a comment
There was a problem hiding this comment.
Clean, well-scoped diagnostic fix with good test coverage. One minor robustness note on the URL comparison and a small test-quality suggestion.
3c4cc3a to
e59876a
Compare
39bbf59 to
15534ac
Compare
![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/cli-core/src/test/integration/input-json.test.ts (1)
21-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: dedupe the stderr JSON extractor.
This helper is byte-for-byte equivalent to
parseJsonError's new line-scanning logic inerror-codes.test.ts(only the return type/.erroraccess differs). Consider extracting a sharedextractFirstJsonLine(stderr)into a test util so future changes to the parsing contract stay in one place.🤖 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/cli-core/src/test/integration/input-json.test.ts` around lines 21 - 28, The stderr JSON parsing logic in parseJsonFromStderr duplicates the new line-scanning behavior already used by parseJsonError, so factor that shared extraction into a common test utility. Create a reusable helper like extractFirstJsonLine(stderr) and update parseJsonFromStderr and parseJsonError to call it, keeping only their type-specific JSON parsing/field access in each test.
🤖 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/cli-core/src/cli-program.ts`:
- Around line 138-143: The override warning in cli-program.ts should not emit
during agent/scripted runs because log.warn still writes to stderr. Update the
warning path around isPlatformApiUrlOverridden() to only call log.warn when
isHuman() is true, or otherwise gate the logger invocation so non-human/JSON
consumers never see the message. Keep the existing override check and warning
text intact, just add the human-mode guard in this branch.
---
Nitpick comments:
In `@packages/cli-core/src/test/integration/input-json.test.ts`:
- Around line 21-28: The stderr JSON parsing logic in parseJsonFromStderr
duplicates the new line-scanning behavior already used by parseJsonError, so
factor that shared extraction into a common test utility. Create a reusable
helper like extractFirstJsonLine(stderr) and update parseJsonFromStderr and
parseJsonError to call it, keeping only their type-specific JSON parsing/field
access in each test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4be00fb3-cd7c-4e46-9794-6ce90373fddf
📒 Files selected for processing (7)
.changeset/platform-api-url-credential-warning.mdpackages/cli-core/src/cli-program.tspackages/cli-core/src/commands/doctor/checks.tspackages/cli-core/src/lib/environment.test.tspackages/cli-core/src/lib/environment.tspackages/cli-core/src/test/integration/error-codes.test.tspackages/cli-core/src/test/integration/input-json.test.ts
cbf951d to
944826a
Compare
wyattjoh
left a comment
wyattjoh
left a comment
There was a problem hiding this comment.
If the authentication information is tied to the URL being authenticated with, doesn't it make sense just to instead refuse to use that credential and consider the user as locked out?
d654b19 to
28577c7
Compare
9b71500 to
99de8c0
Compare
8dde4ad to
c9c91ce
Compare
wyattjoh
left a comment
wyattjoh
left a comment
There was a problem hiding this comment.
The core idea is right, and the normalized-URL comparison plus the human-mode gate are both good calls. The main thing I'd like resolved before merge is the integration-harness fallout: this warning now fires on essentially every integration test, and it landed as JSON-parsing workarounds in two test files rather than a fix at the source. Details inline.
…g.debug in agent mode isPlatformApiUrlOverridden() returned a discriminated union, not a boolean, so rename it to match. Include profileUrl in the warning so the mismatch is self-diagnosing (shows both the override host and where traffic should be going), and log.debug the same info in agent/scripted mode instead of staying fully silent, so a --verbose re-run surfaces it without touching stderr in the default case. Also covers the new URL parse fallback branch with a test. Addresses review feedback from #344.
…g.debug in agent mode isPlatformApiUrlOverridden() returned a discriminated union, not a boolean, so rename it to match. Include profileUrl in the warning so the mismatch is self-diagnosing (shows both the override host and where traffic should be going), and log.debug the same info in agent/scripted mode instead of staying fully silent, so a --verbose re-run surfaces it without touching stderr in the default case. Also covers the new URL parse fallback branch with a test. Addresses review feedback from #344.
2cdc94a to
15dfd51
Compare
…g.debug in agent mode isPlatformApiUrlOverridden() returned a discriminated union, not a boolean, so rename it to match. Include profileUrl in the warning so the mismatch is self-diagnosing (shows both the override host and where traffic should be going), and log.debug the same info in agent/scripted mode instead of staying fully silent, so a --verbose re-run surfaces it without touching stderr in the default case. Also covers the new URL parse fallback branch with a test. Addresses review feedback from #344.
15dfd51 to
7bf7e23
Compare
CLERK_PLATFORM_API_URL redirects API traffic to an arbitrary host, but credentials are keyed by environment name, not by URL, so the active env's token is sent to the override host with no isolation. Warn about this in human mode (agent/scripted output stays clean), and report the active environment and API URL in clerk doctor so the mismatch is visible. Refs #329
…warnings
Use new URL().href to normalize both the override and profile URL before
comparing, so trailing slashes and host-case differences don't produce
false positives. Falls back to raw string comparison when either URL is
malformed.
Also pin the test to a concrete literal ("https://api.clerk.com") instead
of the self-referencing getPlapiBaseUrl() call, and strengthen the positive
warning case by asserting the override host appears in the message.
…ll site Export `isPlatformApiUrlOverridden()` from environment.ts that returns the override/profile URLs and env name as data, rather than emitting the warning itself. Move the `log.warn` call to the preAction hook in cli-program.ts so the decision of whether/how to warn is at the call site, and so warnings go to stderr unconditionally (not just in human mode) since machine-readable stdout data is never polluted by stderr. Update integration tests to extract the JSON line from stderr before parsing so the warning line doesn't break `JSON.parse(result.stderr)`.
log.warn is not mode-aware; without an isHuman() guard the warning leaks to stderr in agent mode, corrupting machine-readable output. Claude-Session: https://claude.ai/code/session_01Fch1D1a2XLtPBeMD7r5tED
…g.debug in agent mode isPlatformApiUrlOverridden() returned a discriminated union, not a boolean, so rename it to match. Include profileUrl in the warning so the mismatch is self-diagnosing (shows both the override host and where traffic should be going), and log.debug the same info in agent/scripted mode instead of staying fully silent, so a --verbose re-run surfaces it without touching stderr in the default case. Also covers the new URL parse fallback branch with a test. Addresses review feedback from #344.
…llout Addresses the integration-harness issues raised in review: - Reset the mocked `_mode` in setupTest so agent mode can't leak across tests and make stderr order-dependent. - Align the active profile's platformApiUrl with the test CLERK_PLATFORM_API_URL so the credential-mismatch warning no longer fires as a stray first stderr line; drop the JSON-parsing workarounds in the two integration tests. - Honor an explicit `--mode` flag before expandInputJson so errors it throws (invalid JSON, missing file) are formatted in the correct mode instead of relying on mode leaking from a prior test.
7bf7e23 to
8625e13
Compare
2 participants
Summary
Addresses the immediate, concrete gap from #329.
CLERK_PLATFORM_API_URLredirects API traffic to an arbitrary host, but credentials are keyed by environment name (viaswitch-env), not by URL — so when the override points somewhere else, the active environment's token is silently sent to that host with no credential isolation.This is a Tier-1 / diagnostic fix:
CLERK_PLATFORM_API_URLdiffers from the active environment's platform URL (agent/scripted stderr stays clean — it would otherwise corrupt machine-readable output; agents get the same info fromdoctor).clerk doctornow reports the active environment name and effective API URL in the "Logged in" check.The broader request in #329 (user-definable environment profiles for full multi-env credential isolation) is a separate, larger change and is not included here — this PR makes the silent failure visible.
Test plan
src/lib/environment.test.ts: warns on override mismatch in human mode; silent when unset, when equal, and in agent modedoctortests passRefs #329