-
Notifications
You must be signed in to change notification settings - Fork 84
fix: More strictly, determine when new URL() should be used #310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b46cd0e
de8c772
9147e43
9d03243
606a905
10b44e0
bc49af9
dd5a383
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| export class RequestError extends Error { | ||
| constructor( | ||
| message: string, | ||
| options?: { | ||
| cause?: unknown | ||
| } | ||
| ) { | ||
| super(message, options) | ||
| this.name = 'RequestError' | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| import { RequestError } from './error' | ||
|
|
||
| const isPathDelimiter = (charCode: number): boolean => | ||
| charCode === 0x2f || charCode === 0x3f || charCode === 0x23 | ||
|
|
||
| // `/.`, `/..` (including `%2e` variants, which are handled by `%` detection) are normalized by `new URL()`. | ||
| const hasDotSegment = (url: string, dotIndex: number): boolean => { | ||
| const prev = dotIndex === 0 ? 0x2f : url.charCodeAt(dotIndex - 1) | ||
| if (prev !== 0x2f) { | ||
| return false | ||
| } | ||
|
|
||
| const nextIndex = dotIndex + 1 | ||
| if (nextIndex === url.length) { | ||
| return true | ||
| } | ||
|
|
||
| const next = url.charCodeAt(nextIndex) | ||
| if (isPathDelimiter(next)) { | ||
| return true | ||
| } | ||
| if (next !== 0x2e) { | ||
| return false | ||
| } | ||
|
|
||
| const nextNextIndex = dotIndex + 2 | ||
| if (nextNextIndex === url.length) { | ||
| return true | ||
| } | ||
| return isPathDelimiter(url.charCodeAt(nextNextIndex)) | ||
| } | ||
|
|
||
| const allowedRequestUrlChar = new Uint8Array(128) | ||
| for (let c = 0x30; c <= 0x39; c++) { | ||
| allowedRequestUrlChar[c] = 1 | ||
| } | ||
| for (let c = 0x41; c <= 0x5a; c++) { | ||
| allowedRequestUrlChar[c] = 1 | ||
| } | ||
| for (let c = 0x61; c <= 0x7a; c++) { | ||
| allowedRequestUrlChar[c] = 1 | ||
| } | ||
| ;(() => { | ||
| const chars = '-./:?#[]@!$&\'()*+,;=~_' | ||
| for (let i = 0; i < chars.length; i++) { | ||
| allowedRequestUrlChar[chars.charCodeAt(i)] = 1 | ||
| } | ||
| })() | ||
|
|
||
| const safeHostChar = new Uint8Array(128) | ||
| // 0-9 | ||
| for (let c = 0x30; c <= 0x39; c++) { | ||
| safeHostChar[c] = 1 | ||
| } | ||
| // a-z | ||
| for (let c = 0x61; c <= 0x7a; c++) { | ||
| safeHostChar[c] = 1 | ||
| } | ||
| ;(() => { | ||
| const chars = '.-_' | ||
| for (let i = 0; i < chars.length; i++) { | ||
| safeHostChar[chars.charCodeAt(i)] = 1 | ||
| } | ||
| })() | ||
|
|
||
| export const buildUrl = (scheme: string, host: string, incomingUrl: string) => { | ||
| const url = `${scheme}://${host}${incomingUrl}` | ||
|
|
||
| let needsHostValidationByURL = false | ||
| for (let i = 0, len = host.length; i < len; i++) { | ||
| const c = host.charCodeAt(i) | ||
| if (c > 0x7f || safeHostChar[c] === 0) { | ||
| needsHostValidationByURL = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if (needsHostValidationByURL) { | ||
| const urlObj = new URL(url) | ||
|
|
||
| // if suspicious, check by host. host header sometimes contains port. | ||
| if ( | ||
| urlObj.hostname.length !== host.length && | ||
| urlObj.hostname !== (host.includes(':') ? host.replace(/:\d+$/, '') : host).toLowerCase() | ||
| ) { | ||
| throw new RequestError('Invalid host header') | ||
| } | ||
| return urlObj.href | ||
| } else if (incomingUrl.length === 0) { | ||
| return url + '/' | ||
| } else { | ||
| if (incomingUrl.charCodeAt(0) !== 0x2f) { | ||
| // '/' | ||
| throw new RequestError('Invalid URL') | ||
| } | ||
|
|
||
| for (let i = 1, len = incomingUrl.length; i < len; i++) { | ||
| const c = incomingUrl.charCodeAt(i) | ||
| if ( | ||
| c > 0x7f || | ||
| allowedRequestUrlChar[c] === 0 || | ||
| (c === 0x2e && hasDotSegment(incomingUrl, i)) | ||
| ) { | ||
| return new URL(url).href | ||
| } | ||
| } | ||
|
|
||
| return url | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,6 +154,17 @@ describe('Request', () => { | |
| }).toThrow(RequestError) | ||
| }) | ||
|
|
||
| it('Should throw error if host header port is invalid', async () => { | ||
| expect(() => { | ||
| newRequest({ | ||
| headers: { | ||
| host: 'localhost:65536', | ||
| }, | ||
| url: '/foo.txt', | ||
| } as IncomingMessage) | ||
| }).toThrow(RequestError) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's better to compare also the error content
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! |
||
| }) | ||
|
|
||
| it('Should be created request body from `req.rawBody` if it exists', async () => { | ||
| const rawBody = Buffer.from('foo') | ||
| const socket = new Socket() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import { RequestError } from '../src/error' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ? The CI does not work now!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the trouble! Oh, right—under the current setup, CI doesn't run on branches outside of main. |
||
| import { buildUrl } from '../src/url' | ||
|
|
||
| describe('buildUrl', () => { | ||
| describe('IPv6 host', () => { | ||
| it('Should throw error for unmatched closing bracket in host', async () => { | ||
| expect(() => { | ||
| buildUrl('http', 'host]', '/foo.txt') | ||
| }).toThrow(new TypeError('Invalid URL')) | ||
| }) | ||
|
|
||
| it('Should throw error for unmatched opening bracket in host', async () => { | ||
| expect(() => { | ||
| buildUrl('http', '[host', '/foo.txt') | ||
| }).toThrow(new TypeError('Invalid URL')) | ||
| }) | ||
| }) | ||
|
|
||
| describe('URL normalization', () => { | ||
| test.each([ | ||
| ['[::1]', '/foo.txt'], | ||
| ['[::1]:8080', '/foo.txt'], | ||
| ['localhost', '/'], | ||
| ['localhost', '/foo/bar/baz'], | ||
| ['localhost', '/foo_bar'], | ||
| ['localhost', '/foo//bar'], | ||
| ['localhost', '/static/%2e%2e/foo.txt'], | ||
| ['localhost', '/static\\..\\foo.txt'], | ||
| ['localhost', '/..'], | ||
| ['localhost', '/foo/.'], | ||
| ['localhost', '/foo/bar/..'], | ||
| ['localhost', '/a/b/../../c'], | ||
| ['localhost', '/a/../../../b'], | ||
| ['localhost', '/a/b/c/../../../'], | ||
| ['localhost', '/./foo.txt'], | ||
| ['localhost', '/foo/../bar.txt'], | ||
| ['localhost', '/a/./b/../c?q=%2E%2E#hash'], | ||
| ['localhost', '/foo/%2E/bar/../baz'], | ||
| ['localhost', '/hello%20world'], | ||
| ['localhost', '/foo%23bar'], | ||
| ['localhost', '/foo"bar'], | ||
| ['localhost', '/%2e%2E/foo'], | ||
| ['localhost', '/caf%C3%A9'], | ||
| ['localhost', '/foo%2fbar/..//baz'], | ||
| ['localhost', '/foo?q=../bar'], | ||
| ['localhost', '/path?q=hello%20world'], | ||
| ['localhost', '/file.txt'], | ||
| ['localhost', ''], | ||
| ['LOCALHOST', '/foo.txt'], | ||
| ['LOCALHOST:80', '/foo.txt'], | ||
| ['LOCALHOST:443', '/foo.txt'], | ||
| ['LOCALHOST:8080', '/foo.txt'], | ||
| ['Localhost:3000', '/foo.txt'], | ||
| ])('Should normalize %s to %s', async (host, url) => { | ||
| expect(buildUrl('http', host, url)).toBe(new URL(url, `http://${host}`).href) | ||
| }) | ||
|
|
||
| it('Should throw a RequestError for non-origin-form request-target', async () => { | ||
| expect(() => { | ||
| buildUrl('http', 'localhost', '*') | ||
| }).toThrow(new RequestError('Invalid URL')) | ||
| }) | ||
|
|
||
| it('Should throw a RequestError for invalid host header', async () => { | ||
| expect(() => { | ||
| buildUrl('http', 'localhost/foo', '/bar') | ||
| }).toThrow(new RequestError('Invalid host header')) | ||
| }) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about creating
url.tsandurl.test.tsto include the code for a URL?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! I've moved it to a separate file.
9d03243