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()