Skip to content
Merged
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
3 changes: 2 additions & 1 deletion lib/web/fetch/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ function responseLocationURL (response, requestFragment) {
// 3. If location is a header value, then set location to the result of
// parsing location with response’s URL.
if (location !== null && isValidHeaderValue(location)) {
location = new URL(location, responseURL(response))
// Make sure location is properly encoded.
location = new URL(Buffer.from(location, 'ascii').toString('utf8'), responseURL(response))
Copy link
Member

@lemire lemire Mar 18, 2024

Choose a reason for hiding this comment

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

This seems strange to me: Buffer.from(location, 'ascii').toString('utf8'). I suggest at least some comments to explain what the intention is.

Copy link
Member

@lemire lemire Mar 18, 2024

Choose a reason for hiding this comment

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

Screenshot 2024-03-18 at 7 06 00 PM

Copy link
Member

Choose a reason for hiding this comment

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

What does "parsing location with response URL" mean precisely? I interpret it is a pathname setter...

The spec links "parsing" to https://url.spec.whatwg.org/#concept-url-parser

Copy link
Contributor Author

@Xvezda Xvezda Mar 19, 2024

Choose a reason for hiding this comment

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

@lemire
This issue arises when the location header's address is not encoded as with encodeURI, but is responded in binary form.
For example, while the result of encodeURIComponent('안녕') is %EC%95%88%EB%85%95, in binary form, this appears as \xEC\x95\x88\xEB\x85\x95, same as the outcome of Buffer.from('안녕', 'utf8').toString('binary').
What I am attempting in this code is to normalize characters that exist outside the ASCII range into UTF-8 form and URL encode them, to ensure behavior consistent with major web browsers.

Copy link
Member

Choose a reason for hiding this comment

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

@KhafraDev Yes. Parsing with the base will work the same in ada, test here : ada-url/ada#611

}

// 4. If location is a URL whose fragment is null, then set location’s
Expand Down
50 changes: 50 additions & 0 deletions test/fetch/fetch-url-after-redirect.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,53 @@ test('after redirecting the url of the response is set to the target url', async

assert.strictEqual(response.url, `http://127.0.0.1:${port}/target`)
})

test('location header with non-ASCII character redirects to a properly encoded url', async (t) => {
/**
* @example
* ```ts
* const server = createServer((req, res) => {
* // redirect -> %EC%95%88%EB%85%95 (안녕), not %C3%AC%C2%95%C2%88%C3%AB%C2%85%C2%95
* if (res.req.url.endsWith('/redirect')) {
* const path = String.fromCharCode.apply(
* null,
* encodeURIComponent('안녕')
* .split('%')
* .slice(1)
* .map(n => parseInt(n, 16))
* )
* res.writeHead(302, {
* Location: `/${path}`
* })
* return res.end()
* }
* })
* ```
*/
const encodedPath = encodeURIComponent('안녕')
const nonEncodedPath = String.fromCharCode.apply(
null,
encodeURIComponent('안녕')
.split('%')
.slice(1)
.map(n => parseInt(n, 16))
)

const server = createServer((req, res) => {
if (res.req.url.endsWith('/redirect')) {
res.writeHead(302, undefined, { Location: `/${nonEncodedPath}` })
res.end()
} else {
res.writeHead(200, 'dummy', { 'Content-Type': 'text/plain' })
res.end()
}
})
t.after(closeServerAsPromise(server))

const listenAsync = promisify(server.listen.bind(server))
await listenAsync(0)
const { port } = server.address()
const response = await fetch(`http://127.0.0.1:${port}/redirect`)

assert.strictEqual(response.url, `http://127.0.0.1:${port}/${encodedPath}`)
})