Skip to content
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

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

Open
3 tasks done
nexdrew opened this issue Jul 3, 2023 · 0 comments
Open
3 tasks done

Comments

@nexdrew
Copy link

nexdrew commented Jul 3, 2023

  • 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 #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 prePath middleware strips multiple slashes to an empty string #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.

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

No branches or pull requests

1 participant