restify / restify/node-restify

An odd request can crash the process when `restify.pre.sanitizePath()` is used

Open
#1,959 0 comments 0 reactions 0 assignees View on GitHub

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

  1. Use normal server.pre(restify.pre.sanitizePath()) middleware

  2. Send odd request via curl e.g. curl http://localhost:3000//

  3. 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.

  1. Using server.pre(restify.pre.sanitizePath()) strips multiple slashes to an empty string (as stated in #1953)
  2. The default router.lookup(req, res) doesn't expect req.getUrl().pathname to 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.