Skip to content

Commit

Permalink
fix: use URL constructor instead of url.parse() (#33)
Browse files Browse the repository at this point in the history 
BREAKING CHANGE: this removes the (hopefully) unused feature that
arbitrary strings are allowed as URLs in the Request constructor. we now
require that URLs are valid and absolute.
  • Loading branch information
@nlf
nlf committed Feb 24, 2022
1 parent 5565cde commit f96f3b1
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 57 deletions.
12 changes: 3 additions & 9 deletions lib/index.js
View file
@@ -1,5 +1,5 @@
'use strict'
const Url = require('url')
const { URL } = require('url')
const http = require('http')
const https = require('https')
const zlib = require('minizlib')
Expand All @@ -15,10 +15,6 @@ const { getNodeRequestOptions } = Request
const FetchError = require('./fetch-error.js')
const AbortError = require('./abort-error.js')

// disabling no-deprecated-api because we need to refactor to switch to the URL constructor
// eslint-disable-next-line node/no-deprecated-api
const resolveUrl = Url.resolve

const fetch = (url, opts) => {
if (/^data:/.test(url)) {
const request = new Request(url, opts)
Expand Down Expand Up @@ -130,7 +126,7 @@ const fetch = (url, opts) => {

// HTTP fetch step 5.3
const locationURL = location === null ? null
: resolveUrl(request.url, location)
: (new URL(location, request.url)).toString()

// HTTP fetch step 5.5
if (request.redirect === 'error') {
Expand Down Expand Up @@ -174,9 +170,7 @@ const fetch = (url, opts) => {
}

// Update host due to redirection
// disabling no-deprecated-api pending refactor to URL constructor
// eslint-disable-next-line node/no-deprecated-api
request.headers.set('host', Url.parse(locationURL).host)
request.headers.set('host', (new URL(locationURL)).host)

// HTTP-redirect fetch step 6 (counter increment)
// Create a new Request object.
Expand Down
34 changes: 19 additions & 15 deletions lib/request.js
View file
@@ -1,5 +1,5 @@
'use strict'
const Url = require('url')
const { URL } = require('url')
const Minipass = require('minipass')
const Headers = require('./headers.js')
const { exportNodeCompatibleHeaders } = Headers
Expand All @@ -12,8 +12,6 @@ const defaultUserAgent =

const INTERNALS = Symbol('Request internals')

const { format: formatUrl } = Url

const isRequest = input =>
typeof input === 'object' && typeof input[INTERNALS] === 'object'

Expand All @@ -28,12 +26,9 @@ const isAbortSignal = signal => {

class Request extends Body {
constructor (input, init = {}) {
// we use url.parse() here because swapping for new URL() is not straight forward.
// until the necessary refactoring is done, we'll just silence the linter
// eslint-disable-next-line node/no-deprecated-api
const parsedURL = isRequest(input) ? Url.parse(input.url)
: input && input.href ? Url.parse(input.href) // eslint-disable-line node/no-deprecated-api
: Url.parse(`${input}`) // eslint-disable-line node/no-deprecated-api
const parsedURL = isRequest(input) ? new URL(input.url)
: input && input.href ? new URL(input.href)
: new URL(`${input}`)

if (isRequest(input)) {
init = { ...input[INTERNALS], ...init }
Expand Down Expand Up @@ -137,7 +132,7 @@ class Request extends Body {
}

get url () {
return formatUrl(this[INTERNALS].parsedURL)
return this[INTERNALS].parsedURL.toString()
}

get headers () {
Expand Down Expand Up @@ -170,10 +165,6 @@ class Request extends Body {
}

// Basic fetch
if (!parsedURL.protocol || !parsedURL.hostname) {
throw new TypeError('Only absolute URLs are supported')
}

if (!/^https?:$/.test(parsedURL.protocol)) {
throw new TypeError('Only HTTP(S) protocols are supported')
}
Expand Down Expand Up @@ -239,8 +230,21 @@ class Request extends Body {
// HTTP-network fetch step 4.2
// chunked encoding is handled by Node.js

// we cannot spread parsedURL directly, so we have to read each property one-by-one
// and map them to the equivalent https?.request() method options
const urlProps = {
auth: parsedURL.username || parsedURL.password
? `${parsedURL.username}:${parsedURL.password}`
: '',
host: parsedURL.host,
hostname: parsedURL.hostname,
path: parsedURL.pathname,
port: parsedURL.port,
protocol: parsedURL.protocol,
}

return {
...parsedURL,
...urlProps,
method: request.method,
headers: exportNodeCompatibleHeaders(headers),
agent,
Expand Down
29 changes: 22 additions & 7 deletions test/index.js
View file
Expand Up @@ -24,7 +24,7 @@ const fs = require('fs')
const http = require('http')
// use of url.parse here is intentional and for coverage purposes
// eslint-disable-next-line node/no-deprecated-api
const { parse: parseURL, URLSearchParams } = require('url')
const { parse: parseURL, URLSearchParams, URL: CoreURL } = require('url')

const vm = require('vm')
const {
Expand Down Expand Up @@ -91,17 +91,21 @@ t.test('expose Headers, Response and Request constructors', t => {
t.test('support proper toString output', { skip: !supportToString }, t => {
t.equal(new Headers().toString(), '[object Headers]')
t.equal(new Response().toString(), '[object Response]')
t.equal(new Request().toString(), '[object Request]')
t.equal(new Request('http://localhost:30000').toString(), '[object Request]')
t.end()
})

t.test('reject with error if url is protocol relative', t =>
t.rejects(fetch('//example.com/'), new TypeError(
'Only absolute URLs are supported')))
t.rejects(fetch('//example.com/'), {
code: 'ERR_INVALID_URL',
name: 'TypeError',
}))

t.test('reject if url is relative path', t =>
t.rejects(fetch('/some/path'), new TypeError(
'Only absolute URLs are supported')))
t.rejects(fetch('/some/path'), {
code: 'ERR_INVALID_URL',
name: 'TypeError',
}))

t.test('reject if protocol unsupported', t =>
t.rejects(fetch('ftp://example.com/'), new TypeError(
Expand Down Expand Up @@ -1503,7 +1507,7 @@ t.test('fetch with Request instance', t => {
})
})

t.test('fetch with Node.js URL object', t => {
t.test('fetch with Node.js legacy URL object', t => {
const url = `${base}hello`
const urlObj = parseURL(url)
const req = new Request(urlObj)
Expand All @@ -1514,6 +1518,17 @@ t.test('fetch with Node.js URL object', t => {
})
})

t.test('fetch with Node.js URL object', t => {
const url = `${base}hello`
const urlObj = new CoreURL(url)
const req = new Request(urlObj)
return fetch(req).then(res => {
t.equal(res.url, url)
t.equal(res.ok, true)
t.equal(res.status, 200)
})
})

t.test('fetch with WHATWG URL object', t => {
const url = `${base}hello`
const urlObj = new URL(url)
Expand Down
63 changes: 37 additions & 26 deletions test/request.js
View file
Expand Up @@ -84,7 +84,7 @@ t.test('should support wrapping Request instance', t => {
t.test('should override signal on derived Request instances', t => {
const parentAbortController = new AbortController()
const derivedAbortController = new AbortController()
const parentRequest = new Request('test', {
const parentRequest = new Request('http://localhost/test', {
signal: parentAbortController.signal,
})
const derivedRequest = new Request(parentRequest, {
Expand All @@ -97,7 +97,7 @@ t.test('should override signal on derived Request instances', t => {

t.test('should allow removing signal on derived Request instances', t => {
const parentAbortController = new AbortController()
const parentRequest = new Request(`test`, {
const parentRequest = new Request('http://localhost/test', {
signal: parentAbortController.signal,
})
const derivedRequest = new Request(parentRequest, {
Expand All @@ -109,17 +109,17 @@ t.test('should allow removing signal on derived Request instances', t => {
})

t.test('should throw error with GET/HEAD requests with body', t => {
t.throws(() => new Request('.', { body: '' }), TypeError)
t.throws(() => new Request('.', { body: 'a' }), TypeError)
t.throws(() => new Request('.', { body: '', method: 'HEAD' }), TypeError)
t.throws(() => new Request('.', { body: 'a', method: 'HEAD' }), TypeError)
t.throws(() => new Request('.', { body: 'a', method: 'get' }), TypeError)
t.throws(() => new Request('.', { body: 'a', method: 'head' }), TypeError)
t.throws(() => new Request('http://localhost', { body: '' }), TypeError)
t.throws(() => new Request('http://localhost', { body: 'a' }), TypeError)
t.throws(() => new Request('http://localhost', { body: '', method: 'HEAD' }), TypeError)
t.throws(() => new Request('http://localhost', { body: 'a', method: 'HEAD' }), TypeError)
t.throws(() => new Request('http://localhost', { body: 'a', method: 'get' }), TypeError)
t.throws(() => new Request('http://localhost', { body: 'a', method: 'head' }), TypeError)
t.end()
})

t.test('should default to null as body', t => {
const req = new Request('.')
const req = new Request(base)
t.equal(req.body, null)
return req.text().then(result => t.equal(result, ''))
})
Expand Down Expand Up @@ -194,13 +194,6 @@ t.test('should support blob() method', t => {
})
})

t.test('should support arbitrary url', t => {
const url = 'anything'
const req = new Request(url)
t.equal(req.url, 'anything')
t.end()
})

t.test('should support clone() method', t => {
const url = base
const r = new Minipass().end('a=1')
Expand Down Expand Up @@ -241,23 +234,23 @@ t.test('should support clone() method', t => {
})

t.test('should support ArrayBuffer as body', t => {
const req = new Request('', {
const req = new Request('http://localhost', {
method: 'POST',
body: stringToArrayBuffer('a=1'),
})
return req.text().then(result => t.equal(result, 'a=1'))
})

t.test('should support Uint8Array as body', t => {
const req = new Request('', {
const req = new Request('http://localhost', {
method: 'POST',
body: new Uint8Array(stringToArrayBuffer('a=1')),
})
return req.text().then(result => t.equal(result, 'a=1'))
})

t.test('should support DataView as body', t => {
const req = new Request('', {
const req = new Request('http://localhost', {
method: 'POST',
body: new DataView(stringToArrayBuffer('a=1')),
})
Expand Down Expand Up @@ -302,6 +295,18 @@ t.test('get node request options', t => {
agent: undefined,
}, 'happy path')

t.match(Request.getNodeRequestOptions(new Request('http://user:password@a.b')), {
auth: 'user:password',
}, 'sets both user and password')

t.match(Request.getNodeRequestOptions(new Request('http://user:@a.b')), {
auth: 'user:',
}, 'sets just user')

t.match(Request.getNodeRequestOptions(new Request('http://:password@a.b')), {
auth: ':password',
}, 'sets just password')

t.match(Request.getNodeRequestOptions(new Request('http://a.b', {
method: 'PATCH',
headers: {
Expand Down Expand Up @@ -349,7 +354,9 @@ t.test('get node request options', t => {
body: 'xyz',
compress: false,
})), {
href: 'http://x.y',
path: '/',
protocol: 'http:',
hostname: 'x.y',
method: 'PATCH',
headers: {
Accept: ['*/*'],
Expand All @@ -368,7 +375,9 @@ t.test('get node request options', t => {
body: new Minipass().end('xyz'),
compress: false,
})), {
href: 'http://x.y',
path: '/',
protocol: 'http:',
hostname: 'x.y',
method: 'PATCH',
headers: {
Accept: ['*/*'],
Expand All @@ -382,7 +391,9 @@ t.test('get node request options', t => {
method: 'GET',
family: 6,
})), {
href: 'http://x.y',
path: '/',
protocol: 'http:',
hostname: 'x.y',
method: 'GET',
family: 6,
})
Expand All @@ -396,7 +407,9 @@ t.test('get node request options', t => {

Request.getNodeRequestOptions(new Request('http://a.b', { agent }), {
method: 'GET',
href: 'http://a.b',
path: '/',
protocol: 'http:',
hostname: 'a.b',
agent: 420,
})

Expand All @@ -405,13 +418,11 @@ t.test('get node request options', t => {
})

t.throws(() => Request.getNodeRequestOptions(new Request('ok.html')), {
message: 'Only absolute URLs are supported',
constructor: TypeError,
code: 'ERR_INVALID_URL',
})

t.throws(() => Request.getNodeRequestOptions(new Request('xyz://ok.html')), {
message: 'Only HTTP(S) protocols are supported',
constructor: TypeError,
})

t.throws(() => Request.getNodeRequestOptions(new Request('http://a.b', {
Expand Down

0 comments on commit f96f3b1

Please sign in to comment.