Skip to content

fix: do not throw from getUrl on an invalid Host or request target - #2012

Open
freshlogic wants to merge 1 commit into
restify:masterfrom
stores-com:fix-geturl-invalid-url
Open

fix: do not throw from getUrl on an invalid Host or request target#2012
freshlogic wants to merge 1 commit into
restify:masterfrom
stores-com:fix-geturl-invalid-url

Conversation

@freshlogic

Copy link
Copy Markdown

Pre-Submission Checklist

  • Opened an issue discussing these changes before opening the PR
  • Ran the linter and tests via make prepush
  • Included comprehensive and convincing tests for changes

Issues

Closes:

getUrl() builds a WHATWG URL from two client-supplied inputs — the Host header as the
authority, and the request target. Node's HTTP parser validates neither as a URL, so either
can make new URL() throw. Router.lookup calls req.getUrl().pathname on every request
before any handler runs, nothing on that path catches, and createServer defaults
handleUncaughtExceptions to false — so one malformed request exits the process.

Introduced in 12.0.0 by #1996. 11.1.0 is unaffected — getUrl() was url.parse(this.url),
path-only, and never read the Host header.

Changes

Guards both new URL() constructions with URL.canParse and falls back instead of throwing:

  • an unparseable Host falls back to a placeholder authority
  • an unparseable request target falls back to the base origin, so the request routes as /
    and gets a normal 404 rather than killing the server

Tests

Two regression tests in test/request.test.js. Both confirmed to fail
against unpatched lib/request.js — the first takes the process down mid-suite, which is
the behaviour being fixed.

Verification

make prepush green: 794 nodeunit assertions + 209 mocha.

Behaviour against 12.0.0, same server, raw sockets:

request before after
Host: foo|bar process death 200
GET http://a:99999/x (valid Host) process death 404
GET http://[::1/x process death 404
GET a://[ process death 404
GET http://other.example/x (proxy style) 200 200
OPTIONS * 200 200
origin-form, valid Host 200 200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant