From 92b092af1d5df09ccafe4a0332e9a0bbcdeaee23 Mon Sep 17 00:00:00 2001 From: River Date: Wed, 22 Apr 2026 14:32:19 +0000 Subject: [PATCH] Send OAuth token request params in POST body instead of URL The cli-kit `tokenRequest()` helper was placing all OAuth parameters (`subject_token`, `refresh_token`, `device_code`, `client_id`, etc.) into the URL query string of POST requests to `/oauth/token`. RFC 6749 specifies that these parameters are sent in the request body as `application/x-www-form-urlencoded` data, and putting credentials in URLs causes them to leak into verbose CLI debug output, HTTP access logs, proxies, and anywhere else URLs are recorded. This commit: - Moves the OAuth parameters from the URL query string into the POST request body (form-encoded). - Expands `sanitizeURL` to mask all sensitive OAuth parameter names (`access_token`, `refresh_token`, `id_token`, `subject_token`, `actor_token`, `device_code`, `client_secret`, `code`) so that any URL that ever carries one of these values in the future is still redacted as defence in depth. - Updates tests to assert the new behaviour and adds coverage for every sanitized parameter. Reported through Shopify's bug bounty programme (report #3686978). Requested by Josh Bregar Co-authored-by: Josh Bregar --- .../cli-kit/src/private/node/api/urls.test.ts | 34 +++++++++++++++++++ packages/cli-kit/src/private/node/api/urls.ts | 25 +++++++++++--- .../src/private/node/session/exchange.test.ts | 17 ++++++++-- .../src/private/node/session/exchange.ts | 15 ++++++-- 4 files changed, 80 insertions(+), 11 deletions(-) diff --git a/packages/cli-kit/src/private/node/api/urls.test.ts b/packages/cli-kit/src/private/node/api/urls.test.ts index 59def344dab..62492fb2eb7 100644 --- a/packages/cli-kit/src/private/node/api/urls.test.ts +++ b/packages/cli-kit/src/private/node/api/urls.test.ts @@ -45,4 +45,38 @@ describe('sanitizeURL', () => { // Then expect(sanitizedUrl).toBe('https://example.com/?subject_token=****&other_param=def456') }) + + test.each([ + 'access_token', + 'refresh_token', + 'id_token', + 'subject_token', + 'actor_token', + 'device_code', + 'client_secret', + 'code', + 'token', + ])('sanitizes %s query parameter', (param) => { + // Given + const url = `https://example.com?${param}=secret-value` + + // When + const sanitizedUrl = sanitizeURL(url) + + // Then + expect(sanitizedUrl).toBe(`https://example.com/?${param}=****`) + }) + + test('sanitizes all sensitive query parameters together', () => { + // Given + const url = 'https://example.com?access_token=a&refresh_token=b&device_code=c&subject_token=d&other=keep' + + // When + const sanitizedUrl = sanitizeURL(url) + + // Then + expect(sanitizedUrl).toBe( + 'https://example.com/?access_token=****&refresh_token=****&device_code=****&subject_token=****&other=keep', + ) + }) }) diff --git a/packages/cli-kit/src/private/node/api/urls.ts b/packages/cli-kit/src/private/node/api/urls.ts index c2e0da6a5e8..fd1297f7c58 100644 --- a/packages/cli-kit/src/private/node/api/urls.ts +++ b/packages/cli-kit/src/private/node/api/urls.ts @@ -1,3 +1,19 @@ +// Query-string parameter names that may carry sensitive credentials and must +// not be written to logs, verbose debug output, or any other user-visible +// destination. Covers OAuth 2.0 / device-authorization / token-exchange +// parameters. +const SENSITIVE_QUERY_PARAMS = [ + 'access_token', + 'refresh_token', + 'id_token', + 'subject_token', + 'actor_token', + 'device_code', + 'client_secret', + 'code', + 'token', +] + /** * Removes the sensitive data from the url and outputs them as a string. * @param url - HTTP headers. @@ -5,11 +21,10 @@ */ export function sanitizeURL(url: string): string { const parsedUrl = new URL(url) - if (parsedUrl.searchParams.has('subject_token')) { - parsedUrl.searchParams.set('subject_token', '****') - } - if (parsedUrl.searchParams.has('token')) { - parsedUrl.searchParams.set('token', '****') + for (const param of SENSITIVE_QUERY_PARAMS) { + if (parsedUrl.searchParams.has(param)) { + parsedUrl.searchParams.set(param, '****') + } } return parsedUrl.toString() } diff --git a/packages/cli-kit/src/private/node/session/exchange.test.ts b/packages/cli-kit/src/private/node/session/exchange.test.ts index b0b1f0a7dc0..d0f2c95c97e 100644 --- a/packages/cli-kit/src/private/node/session/exchange.test.ts +++ b/packages/cli-kit/src/private/node/session/exchange.test.ts @@ -271,8 +271,10 @@ describe.each(tokenExchangeMethods)( test(`Executing ${tokenExchangeMethod.name} returns access token and user ID for a valid CLI token`, async () => { // Given let capturedUrl = '' + let capturedInit: {method?: string; headers?: Record; body?: string} = {} vi.mocked(shopifyFetch).mockImplementation(async (url, options) => { capturedUrl = url.toString() + capturedInit = (options ?? {}) as typeof capturedInit return Promise.resolve( new Response( JSON.stringify({ @@ -292,12 +294,21 @@ describe.each(tokenExchangeMethods)( await expect(getLastSeenUserIdAfterAuth()).resolves.toBe(userId) await expect(getLastSeenAuthMethod()).resolves.toBe('partners_token') - // Assert token exchange parameters are correct + // Request is sent as POST form-encoded body (not query string), so the + // URL must not contain any OAuth parameters. const actualUrl = new URL(capturedUrl) expect(actualUrl).toBeDefined() - expect(actualUrl.href).toContain('https://fqdn.com/oauth/token') + expect(actualUrl.href).toBe('https://fqdn.com/oauth/token') + expect(actualUrl.search).toBe('') - const params = actualUrl.searchParams + expect(capturedInit.method).toBe('POST') + expect(capturedInit.headers).toMatchObject({ + 'Content-Type': 'application/x-www-form-urlencoded', + }) + expect(typeof capturedInit.body).toBe('string') + + // Assert token exchange parameters are correct and sent in the body. + const params = new URLSearchParams(capturedInit.body) expect(params.get('grant_type')).toBe(grantType) expect(params.get('requested_token_type')).toBe(accessTokenType) expect(params.get('subject_token_type')).toBe(accessTokenType) diff --git a/packages/cli-kit/src/private/node/session/exchange.ts b/packages/cli-kit/src/private/node/session/exchange.ts index 8ddf858eb86..e5a60bac820 100644 --- a/packages/cli-kit/src/private/node/session/exchange.ts +++ b/packages/cli-kit/src/private/node/session/exchange.ts @@ -226,10 +226,19 @@ async function tokenRequest( params: Record, ): Promise> { const fqdn = await identityFqdn() - const url = new URL(`https://${fqdn}/oauth/token`) - url.search = new URLSearchParams(Object.entries(params)).toString() + const url = `https://${fqdn}/oauth/token` - const res = await shopifyFetch(url.href, {method: 'POST'}) + // Send OAuth parameters in the request body (per RFC 6749) rather than the + // URL query string. This prevents sensitive credentials (subject_token, + // refresh_token, device_code, client_id, etc.) from being written to + // verbose CLI debug output or otherwise leaking through URLs. + const body = new URLSearchParams(Object.entries(params)).toString() + + const res = await shopifyFetch(url, { + method: 'POST', + headers: {'Content-Type': 'application/x-www-form-urlencoded'}, + body, + }) try { const responseText = await res.text()