-
Notifications
You must be signed in to change notification settings - Fork 4
fix(env): surface CLERK_PLATFORM_API_URL credential mismatch
#344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
811a569
961b9dc
3bec41e
c9c91ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "clerk": patch | ||
| --- | ||
|
|
||
| Warn (in human mode) when `CLERK_PLATFORM_API_URL` routes requests to a host that differs from the active environment's URL, since credentials are keyed by environment name and not by URL. `clerk doctor` now also reports the active environment and its API URL so the mismatch is visible. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ import { | |
| getCurrentEnvName, | ||
| getAvailableEnvs, | ||
| getPlapiBaseUrl, | ||
| isPlatformApiUrlOverridden, | ||
| } from "./lib/environment.ts"; | ||
| import { | ||
| CliError, | ||
|
|
@@ -41,7 +42,7 @@ import { | |
| throwUsageError, | ||
| } from "./lib/errors.ts"; | ||
| import { clerkHelpConfig, formatExamplesBlock, type Example } from "./lib/help.ts"; | ||
| import { isAgent } from "./mode.ts"; | ||
| import { isAgent, isHuman } from "./mode.ts"; | ||
| import { log } from "./lib/log.ts"; | ||
| import { maybeNotifyUpdate, getCurrentVersion } from "./lib/update-check.ts"; | ||
| import { registerExtras } from "@clerk/cli-extras"; | ||
|
|
@@ -135,6 +136,18 @@ export function createProgram(): Program { | |
| if (activeEnv !== "production") { | ||
| process.stderr.write(`[${activeEnv.toUpperCase()}]\n`); | ||
| } | ||
|
|
||
| // Warn (human mode only) when CLERK_PLATFORM_API_URL routes requests to a | ||
| // different host than the active environment's platform URL. Credentials are | ||
| // keyed by environment name, so the active env's token will be sent to the | ||
| // override host. Agent/scripted mode stays clean — stray lines on stderr | ||
| // corrupt machine-readable output; those callers get this info from `doctor`. | ||
| const override = isPlatformApiUrlOverridden(); | ||
| if (override.overridden && isHuman()) { | ||
| log.warn( | ||
| `CLERK_PLATFORM_API_URL is routing requests to ${override.overrideUrl}, but credentials stay keyed to the "${override.envName}" environment — the "${override.envName}" token will be sent to that host.`, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with keeping this off stderr in agent mode — the test fallout in this same PR is a good demonstration of why stray lines break machine-readable output. But the result is that agent/scripted mode gets zero signal, and that's arguably the higher-risk context: a CI job with A cheap middle ground that costs nothing in cleanliness: emit it as if (override.overridden) {
const msg = `CLERK_PLATFORM_API_URL is routing requests to ${override.overrideUrl}, ...`;
if (isHuman()) log.warn(msg);
else log.debug(`env: ${msg}`);
} |
||
| ); | ||
| } | ||
|
rafa-thayto marked this conversation as resolved.
|
||
| }); | ||
|
|
||
| // Show update notification after each command, except for commands that | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import { test, expect, describe, beforeEach, afterEach } from "bun:test"; | ||
| import { isPlatformApiUrlOverridden } from "./environment.ts"; | ||
|
|
||
| describe("isPlatformApiUrlOverridden", () => { | ||
| const original = process.env.CLERK_PLATFORM_API_URL; | ||
|
|
||
| beforeEach(() => { | ||
| delete process.env.CLERK_PLATFORM_API_URL; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (original === undefined) delete process.env.CLERK_PLATFORM_API_URL; | ||
| else process.env.CLERK_PLATFORM_API_URL = original; | ||
| }); | ||
|
|
||
| test("returns overridden=true with URLs when the override differs from the active env URL", () => { | ||
| process.env.CLERK_PLATFORM_API_URL = "https://api.staging.example.com"; | ||
| const result = isPlatformApiUrlOverridden(); | ||
| expect(result.overridden).toBe(true); | ||
| if (!result.overridden) return; | ||
| expect(result.overrideUrl).toBe("https://api.staging.example.com"); | ||
| expect(result.profileUrl).toBe("https://api.clerk.com"); | ||
| expect(result.envName).toBe("production"); | ||
| }); | ||
|
|
||
| test("returns overridden=false when no override is set", () => { | ||
| const result = isPlatformApiUrlOverridden(); | ||
| expect(result.overridden).toBe(false); | ||
| }); | ||
|
|
||
| test("returns overridden=false when the override equals the active env URL", () => { | ||
| process.env.CLERK_PLATFORM_API_URL = "https://api.clerk.com"; | ||
| const result = isPlatformApiUrlOverridden(); | ||
| expect(result.overridden).toBe(false); | ||
| }); | ||
|
|
||
|
rafa-thayto marked this conversation as resolved.
|
||
| test("returns overridden=false when URLs differ only by trailing slash", () => { | ||
| process.env.CLERK_PLATFORM_API_URL = "https://api.clerk.com/"; | ||
| const result = isPlatformApiUrlOverridden(); | ||
| expect(result.overridden).toBe(false); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,6 +141,42 @@ export function getPlapiBaseUrl(): string { | |
| return process.env.CLERK_PLATFORM_API_URL ?? getCurrentEnv().platformApiUrl; | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether CLERK_PLATFORM_API_URL is set to a URL that differs from the | ||
| * active environment's configured platform URL, along with both URLs so the | ||
| * caller can surface a warning. | ||
| * | ||
| * Comparison normalises both URLs via `new URL().href` so trailing-slash and | ||
| * case differences are ignored; falls back to raw string comparison if either | ||
| * value is not a valid URL. | ||
| */ | ||
| export function isPlatformApiUrlOverridden(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two small things on this function: Naming. An Unused field. Also worth a test: the JSDoc documents a raw-string fallback when either value fails |
||
| | { | ||
| overridden: false; | ||
| } | ||
| | { | ||
| overridden: true; | ||
| overrideUrl: string; | ||
| profileUrl: string; | ||
| envName: string; | ||
| } { | ||
| const override = process.env.CLERK_PLATFORM_API_URL; | ||
| if (!override) return { overridden: false }; | ||
|
|
||
| const profileUrl = getCurrentEnv().platformApiUrl; | ||
| const normalize = (u: string) => { | ||
| try { | ||
| return new URL(u).href; | ||
| } catch { | ||
| return u; | ||
| } | ||
| }; | ||
|
|
||
| if (normalize(override) === normalize(profileUrl)) return { overridden: false }; | ||
|
|
||
| return { overridden: true, overrideUrl: override, profileUrl, envName: getCurrentEnvName() }; | ||
| } | ||
|
|
||
| export function getBapiBaseUrl(): string { | ||
| return process.env.CLERK_BACKEND_API_URL ?? getCurrentEnv().backendApiUrl; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,12 @@ import { useIntegrationTestHarness, clerk, mockState } from "./lib/harness.ts"; | |
| useIntegrationTestHarness(); | ||
|
|
||
| function parseJsonError(stderr: string): { code: string; message: string; docsUrl?: string } { | ||
| const parsed = JSON.parse(stderr); | ||
| const jsonLine = stderr | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line-scanning helper is now copy-pasted here and in |
||
| .split("\n") | ||
| .map((l) => l.trim()) | ||
| .find((l) => l.startsWith("{")); | ||
| if (!jsonLine) throw new SyntaxError(`No JSON line found in stderr:\n${stderr}`); | ||
| const parsed = JSON.parse(jsonLine); | ||
| return parsed.error; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,15 @@ useIntegrationTestHarness(); | |
|
|
||
| const devInstance = getInstance(MOCK_APP, "development"); | ||
|
|
||
| function parseJsonFromStderr(stderr: string): { error: { code?: string } } { | ||
| const jsonLine = stderr | ||
| .split("\n") | ||
| .map((l) => l.trim()) | ||
| .find((l) => l.startsWith("{")); | ||
| if (!jsonLine) throw new SyntaxError(`No JSON line found in stderr:\n${stderr}`); | ||
| return JSON.parse(jsonLine) as { error: { code?: string } }; | ||
| } | ||
|
|
||
| beforeEach(async () => { | ||
| await setProfile("github.com/test/project", { | ||
| workspaceId: "", | ||
|
|
@@ -40,7 +49,7 @@ test("explicit CLI flags override --input-json values", async () => { | |
| const result = await clerk.raw("init", "--input-json", '{"mode":"human"}', "--mode", "agent"); | ||
| expect(result.exitCode).not.toBe(0); | ||
| // Agent mode emits structured JSON to stderr; human mode emits plain text. | ||
| const parsed = JSON.parse(result.stderr); | ||
| const parsed = parseJsonFromStderr(result.stderr); | ||
| expect(parsed.error).toBeDefined(); | ||
| }); | ||
|
|
||
|
|
@@ -234,16 +243,14 @@ test("--input-json is registered as a global option", async () => { | |
| // stderr is the agent-mode signature, which proves --mode agent was applied. | ||
| const result = await clerk.raw("--input-json", '{"mode":"agent"}', "doctor", "--json"); | ||
| expect(result.exitCode).not.toBe(0); | ||
| const jsonOutput = result.stderr.split("\n").findLast((line) => line.startsWith("{")); | ||
| expect(jsonOutput).toBeDefined(); | ||
| const parsed = JSON.parse(jsonOutput!); | ||
| const parsed = parseJsonFromStderr(result.stderr); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This swap quietly changes semantics. The original code deliberately took the last JSON line: const jsonOutput = result.stderr.split("\n").findLast((line) => line.startsWith("{"));The new shared helper uses It passes right now only because nothing before the error line happens to start with const jsonLine = stderr
.split("\n")
.map((l) => l.trim())
.findLast((l) => l.startsWith("{"));
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should maybe just ensure that if you expect there only to be a single line, that we test for that. |
||
| expect(parsed.error).toBeDefined(); | ||
| }); | ||
|
|
||
| test("structured JSON error in agent mode for invalid JSON", async () => { | ||
| const result = await clerk.raw("--mode", "agent", "init", "--input-json", "{bad}"); | ||
| expect(result.exitCode).not.toBe(0); | ||
| const parsed = JSON.parse(result.stderr); | ||
| const parsed = parseJsonFromStderr(result.stderr); | ||
| expect(parsed.error.code).toBe("invalid_json"); | ||
| }); | ||
|
|
||
|
|
@@ -256,7 +263,7 @@ test("structured JSON error in agent mode for file not found", async () => { | |
| "@/tmp/does-not-exist-clerk.json", | ||
| ); | ||
| expect(result.exitCode).not.toBe(0); | ||
| const parsed = JSON.parse(result.stderr); | ||
| const parsed = parseJsonFromStderr(result.stderr); | ||
| expect(parsed.error.code).toBe("file_not_found"); | ||
| }); | ||
|
|
||
|
|
@@ -269,6 +276,6 @@ test("structured JSON error in agent mode for nested objects", async () => { | |
| '{"nested":{"key":"value"}}', | ||
| ); | ||
| expect(result.exitCode).not.toBe(0); | ||
| const parsed = JSON.parse(result.stderr); | ||
| const parsed = parseJsonFromStderr(result.stderr); | ||
| expect(parsed.error.code).toBe("invalid_json"); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This warning fires on effectively every integration test, and the workarounds in
input-json.test.ts/error-codes.test.tstreat the symptom rather than the cause.Two harness facts combine here:
setupTest()setsCLERK_PLATFORM_API_URL=https://test-api.clerk.comwhile the active environment staysproduction(https://api.clerk.com), soisPlatformApiUrlOverridden()returnsoverridden: truein every test.mode.tswith a module-levellet _mode = "human"that is never reset insetupTest/teardownTest.So the default state for an integration test is human + overridden, and this warning becomes the first line of stderr for any command that reaches
preAction. I confirmed it with a scratch test:Worse, because
_modeleaks across tests within a file, whether the warning appears depends on test order. Three tests in one file:A and C are byte-identical commands with different stderr, purely because B ran in between and left
_modeas"agent".Everything passes today, but this leaves a tripwire for any future test that asserts on stderr contents or the first stderr line. I'd rather fix it in the harness than in each consumer: reset
_mode = "human"insetupTest()to kill the order-dependence, and either pointCLERK_PLATFORM_API_URLat the active profile's URL or register a test env profile whoseplatformApiUrlishttps://test-api.clerk.com, so the harness stops tripping a warning it doesn't mean to exercise. The JSON-parsing helpers in the two test files can then go away entirely.