fix(env): surface CLERK_PLATFORM_API_URL credential mismatch#344
fix(env): surface CLERK_PLATFORM_API_URL credential mismatch#344rafa-thayto wants to merge 4 commits into
CLERK_PLATFORM_API_URL credential mismatch#344Conversation
🦋 Changeset detectedLatest commit: d654b19 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 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.
rafa-thayto
force-pushed
the
fix/platform-api-url-credential-warning
branch
from
3c4cc3a to
e59876a
Compare
rafa-thayto
force-pushed
the
fix/platform-api-url-credential-warning
branch
from
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
rafa-thayto
force-pushed
the
fix/platform-api-url-credential-warning
branch
2 times, most recently
from
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?
rafa-thayto
force-pushed
the
fix/platform-api-url-credential-warning
branch
from
944826a to
ce52649
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
rafa-thayto
force-pushed
the
fix/platform-api-url-credential-warning
branch
from
ce52649 to
d654b19
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