Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions packages/cli-kit/src/private/node/api/urls.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}=****`)
})
Comment thread
isaacroldan marked this conversation as resolved.

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',
)
})
})
25 changes: 20 additions & 5 deletions packages/cli-kit/src/private/node/api/urls.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,30 @@
// 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.
* @returns A sanitized version of the url as a string.
*/
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()
}
17 changes: 14 additions & 3 deletions packages/cli-kit/src/private/node/session/exchange.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string>; body?: string} = {}
vi.mocked(shopifyFetch).mockImplementation(async (url, options) => {
capturedUrl = url.toString()
capturedInit = (options ?? {}) as typeof capturedInit
return Promise.resolve(
new Response(
JSON.stringify({
Expand All @@ -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)
Expand Down
15 changes: 12 additions & 3 deletions packages/cli-kit/src/private/node/session/exchange.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,19 @@ async function tokenRequest(
params: Record<string, string>,
): Promise<Result<TokenRequestResult, {error: string; store?: string}>> {
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()

Expand Down
Loading