fix(cli): keep a path prefix in the login URLs (#6793)

* fix(cli): keep a path prefix in the login URLs

`sim login` built both of its URLs with `new URL('/path', endpoint)`. A
leading-slash path is absolute, so it resolves against the endpoint's origin
and drops any path the endpoint carries: a deployment served at
`https://host/sim` sent the browser to `https://host/cli/auth` and polled
`https://host/api/cli/auth/poll`, neither of which exists there.

Every other command builds its URL by concatenation and was unaffected, so
the endpoint looked correct and login alone failed.

Both now go through the client's `buildUrl`, which is exported for it rather
than duplicated — one URL builder for the whole CLI is the point, since two
of them is how the halves drifted apart. Its TSDoc now names the trap.

* test(cli): restore fetch with spyOn so the stub cannot outlive its test

vi.stubGlobal is not undone by restoreAllMocks, so the completed-auth
response would have leaked into whatever ran next. The rest of this file
already spies on globalThis.fetch, which the existing teardown restores.
This commit is contained in:
Waleed
2026-08-17 16:55:37 -07:00
committed by GitHub
parent 5c37778bbd
commit fe4480d3f7
3 changed files with 67 additions and 13 deletions
@@ -137,6 +137,40 @@ describe('createAuthRequest', () => {
}
})
it('keeps a path prefix the endpoint carries, in both login URLs', async () => {
// Both URLs were built with `new URL('/path', endpoint)`. A leading-slash
// path is absolute, so it resolved against the ORIGIN and dropped the
// prefix: a deployment served at https://host/sim sent the browser to
// https://host/cli/auth and polled https://host/api/cli/auth/poll, neither
// of which exists there. Every other command concatenated and worked, so
// the endpoint looked correct and only login failed.
const prefixed = 'https://host.test/sim'
const auth = createAuthRequest()
expect(buildApprovalUrl(prefixed, auth, 'platform')).toMatch(
/^https:\/\/host\.test\/sim\/cli\/auth\?/
)
// `spyOn`, like the rest of this file: `restoreAllMocks` in teardown undoes
// it, whereas a `stubGlobal` would outlive the test and leak this
// completed-auth response into whatever ran next.
const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(
new Response(JSON.stringify({ status: 'complete', key: { id: 'k', apiKey: 'sk' } }), {
status: 200,
headers: { 'content-type': 'application/json' },
})
)
await pollForKey(prefixed, auth)
expect(fetchSpy.mock.calls[0][0]).toBe('https://host.test/sim/api/cli/auth/poll')
})
it('omits an absent workspace rather than sending it blank', () => {
const auth = createAuthRequest()
expect(buildApprovalUrl(ENDPOINT, auth, 'platform')).not.toContain('workspace=')
expect(buildApprovalUrl(ENDPOINT, auth, 'platform', 'ws_1')).toContain('workspace=ws_1')
})
it('never puts the poll secret in the browser URL', () => {
const auth = createAuthRequest()
const url = buildApprovalUrl(ENDPOINT, auth, 'platform', 'ws_1')
+15 -12
View File
@@ -1,6 +1,6 @@
import { createHash, randomBytes, randomInt } from 'node:crypto'
import { sleep } from '../helpers'
import { REDIRECT_STATUSES, redirectEndpoint, SimApiError } from '../http/client'
import { buildUrl, REDIRECT_STATUSES, redirectEndpoint, SimApiError } from '../http/client'
import { USER_AGENT } from '../version'
/**
@@ -20,6 +20,12 @@ const PAIRING_ALPHABET = 'ABCDEFGHJKLMNPQRSTUVWXYZ23456789'
const POLL_INTERVAL_MS = 2000
const POLL_TIMEOUT_MS = 15 * 60 * 1000
/** The page the browser is sent to for approval. */
const APPROVAL_PATH = '/cli/auth'
/** The route the login poll targets; also the suffix a redirect target is measured against. */
const POLL_PATH = '/api/cli/auth/poll'
/**
* Poll statuses that leave the approval still redeemable, so the login should
* keep waiting rather than making the user restart the browser handoff.
@@ -89,13 +95,13 @@ export function buildApprovalUrl(
scope: CliAuthScope,
workspaceId?: string
): string {
const url = new URL('/cli/auth', endpoint)
url.searchParams.set('request', auth.request)
url.searchParams.set('challenge', auth.challenge)
url.searchParams.set('pairing', auth.pairing)
url.searchParams.set('scope', scope)
if (workspaceId) url.searchParams.set('workspace', workspaceId)
return url.toString()
return buildUrl(endpoint, APPROVAL_PATH, {
request: auth.request,
challenge: auth.challenge,
pairing: auth.pairing,
scope,
workspace: workspaceId,
})
}
interface PollResponse {
@@ -106,9 +112,6 @@ interface PollResponse {
workspaceBound?: boolean
}
/** The route the login poll targets; also the suffix a redirect target is measured against. */
const POLL_PATH = '/api/cli/auth/poll'
/**
* Explains a redirected poll rather than following it.
*
@@ -166,7 +169,7 @@ export async function pollForKey(
let response: Response | null = null
try {
response = await fetch(new URL(POLL_PATH, endpoint), {
response = await fetch(buildUrl(endpoint, POLL_PATH), {
method: 'POST',
headers: {
'content-type': 'application/json',
+18 -1
View File
@@ -53,7 +53,24 @@ export interface WorkspaceOptions {
auth?: AuthRequirement
}
function buildUrl(endpoint: string, path: string, query?: Record<string, QueryValue>): string {
/**
* Joins an endpoint and a route into a request URL.
*
* Concatenation rather than `new URL(path, endpoint)`, which is the trap it
* exists to avoid: a leading-slash path is absolute, so `new URL()` resolves it
* against the endpoint's ORIGIN and silently drops any path the endpoint
* carries. A deployment served under a prefix `https://host/sim` behind a
* proxy that fronts several apps would have every request rewritten to
* `https://host/...`, losing the prefix that identifies it.
*
* Empty values are skipped rather than sent blank so an omitted optional
* parameter reads as absent, not as the empty string.
*/
export function buildUrl(
endpoint: string,
path: string,
query?: Record<string, QueryValue>
): string {
const url = new URL(`${endpoint}${path}`)
for (const [key, value] of Object.entries(query ?? {})) {
if (value === null || value === undefined || value === '') continue