Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
5 changes: 5 additions & 0 deletions .changeset/encode-device-authorization-request.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/cli-kit': patch
---

Encode device authorization request parameters safely.
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,48 @@ describe('requestDeviceAuthorization', () => {
expect(shopifyFetch).toBeCalledWith('https://fqdn.com/oauth/device_authorization', {
method: 'POST',
headers: {'Content-type': 'application/x-www-form-urlencoded'},
body: 'client_id=clientId&scope=scope1 scope2',
body: new URLSearchParams({client_id: 'clientId', scope: 'scope1 scope2'}).toString(),
})
Comment thread
dmerand marked this conversation as resolved.
expect(got).toEqual(dataExpected)
})

test('encodes special characters in authorization scopes', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we testing the behavior of URLSearchParams? I feel like we could get away with much simpler testing here...

// Given
const response = new Response(JSON.stringify(data))
vi.mocked(shopifyFetch).mockResolvedValue(response)
vi.mocked(identityFqdn).mockResolvedValue('fqdn.com')
vi.mocked(clientId).mockReturnValue('clientId')
const scopes = ['scope&name', 'scope=value', 'scope%value']

// When
await requestDeviceAuthorization(scopes)

// Then
expect(shopifyFetch).toHaveBeenCalledWith('https://fqdn.com/oauth/device_authorization', {
method: 'POST',
headers: {'Content-type': 'application/x-www-form-urlencoded'},
body: 'client_id=clientId&scope=scope%26name+scope%3Dvalue+scope%25value',
})
})

test('omits empty authorization scope values', async () => {
// Given
const response = new Response(JSON.stringify(data))
vi.mocked(shopifyFetch).mockResolvedValue(response)
vi.mocked(identityFqdn).mockResolvedValue('fqdn.com')
vi.mocked(clientId).mockReturnValue('clientId')

// When
await requestDeviceAuthorization([])

// Then
expect(shopifyFetch).toHaveBeenCalledWith('https://fqdn.com/oauth/device_authorization', {
method: 'POST',
headers: {'Content-type': 'application/x-www-form-urlencoded'},
body: 'client_id=clientId',
})
})

test('opens the browser directly in an interactive terminal', async () => {
// Given
const outputInfo = vi.spyOn(output, 'outputInfo')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,8 @@ export async function pollForDeviceAuthorization(code: string, interval = 5): Pr
}

function convertRequestToParams(queryParams: {client_id: string; scope: string}): string {
return Object.entries(queryParams)
.map(([key, value]) => value && `${key}=${value}`)
.filter((hasValue) => Boolean(hasValue))
.join('&')
// URLSearchParams applies form encoding so scope values cannot change the request structure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a lot of justification for the use of URLSearchParams over a hand-rolled conversion function 😆

return new URLSearchParams(Object.entries(queryParams).filter(([, value]) => Boolean(value))).toString()
}

/**
Expand Down
Loading