restify / restify/node-restify
An odd request can crash the process when `restify.pre.sanitizePath()` is used
Nobody has claimed this yet.
- Dominant language
- JavaScript
- Stars
- 10.7k
- Forks
- 975
- Avg merge
- 1d 6h
- Merged PRs (30d)
- 5
Description
- Used appropriate template for the issue type
- Searched both open and closed issues for duplicates of this issue
- Title adequately and concisely reflects the feature or the bug
Restify Version: 11.1.0
Node.js Version: 16, 18, 20
Expected behaviour
Restify should do its best to not crash when weird/unexpected requests are received. It should either return a standard error (e.g. if the method or route is not supported) or invoke the route handler.
Actual behaviour
-
Use normal
server.pre(restify.pre.sanitizePath())middleware -
Send odd request via curl e.g.
curl http://localhost:3000// -
The Node process running the restify server will crash with this error:
/Users/abg/git/nexdrew/github/restify-repro/node_modules/assert-plus/assert.js:22 throw new assert.AssertionError({ ^ AssertionError [ERR_ASSERTION]: pathname (string) is required at RouterRegistryRadix.lookup (/Users/abg/git/nexdrew/github/restify-repro/node_modules/restify/lib/routerRegistryRadix.js:97:12) at Router.lookup (/Users/abg/git/nexdrew/github/restify-repro/node_modules/restify/lib/router.js:76:40) at Server._runRoute (/Users/abg/git/nexdrew/github/restify-repro/node_modules/restify/lib/server.js:1119:36) at Server._afterPre (/Users/abg/git/nexdrew/github/restify-repro/node_modules/restify/lib/server.js:1101:10) at preChainDone (/Users/abg/git/nexdrew/github/restify-repro/node_modules/restify/lib/server.js:1069:14) at nextTick (/Users/abg/git/nexdrew/github/restify-repro/node_modules/restify/lib/chain.js:145:24) at process.processTicksAndRejections (node:internal/process/task_queues:77:11) { generatedMessage: false, code: 'ERR_ASSERTION', actual: 'object', expected: 'string', operator: '===' }
Repro case
Please see https://github.com/nexdrew/restify-repro-1959
It includes code and instructions to reproduce the error, and it also contains possible workarounds.
Cause
This issue overlaps with https://github.com/restify/node-restify/issues/1953. The reason I created it as a separate issue is to consider not only a fix to the prePath logic but also to consider handling an empty pathname in the default Router for whatever reason it might occur.
- Using
server.pre(restify.pre.sanitizePath())strips multiple slashes to an empty string (as stated in #1953) - The default
router.lookup(req, res)doesn't expectreq.getUrl().pathnameto be null and doesn't gracefully handle any assertion errors that it might cause
Are you willing and able to fix this?
Yes, I plan to open a PR that includes code changes for both of the workarounds demonstrated in the repro case.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with lib/plugins/pre/prePath.js and lib/router.js, following the repro case at restify-repro-1959 with server.pre(restify.pre.sanitizePath()) and curl http://localhost:3000//. Trace how the empty pathname reaches Router.lookup and verify that the process no longer crashes, while the request receives a standard error or reaches its route handler.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 28/100