node-restify icon indicating copy to clipboard operation
node-restify copied to clipboard

fix NotAuthorized error when serving static files while directory var…

Open unsol opened this issue 5 years ago • 5 comments

Pre-Submission Checklist

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

Issues

Closes:

  • Issue 549

Changes

This is to resolve issue #549, so I did not create a new issue for this pull request.

The serveStatic function sets the appendRequestPath option to true by default. This works the majority of the time, but if the directory option is set to "./", then the regex test (https://github.com/restify/node-restify/blob/e663699c8c5383eb6affad16ac2eb14fcd66a858/lib/plugins/static.js#L194) fails, which results in NotAuthorizedError. This test fails, as it is trying to compare a slash, against a full path. By moving the "p", and "re" variable declarations to the serve function, after the file path is joined with opts.directory, we are then able to normalize the path, run that output through escapeRE, and test the expected expression.

The "p", and "re" variables are not used anywhere else in this file -- only within the serve function.

I tested this solution with the following routes:

    server.get("/firsttest", restify.plugins.serveStatic({
        directory: "./app",
        file: "firsttest.html",
        appendRequestPath: false,
    }));

    server.get("/secondtest", restify.plugins.serveStatic({
        directory: "./app",
        file: "secondtest().html",
        appendRequestPath: false,
    }));

    server.get("/docs/*", restify.plugins.serveStatic({
        directory: "./",
        default: "index.html",
        appendRequestPath: true,
    }));

My folder structure for the tests was something along the lines of:  /   docs/    v1/     index.html    v2/     index.html   app/    firsttest.html    secondtest().html    (contains source code)   dist/    contains typescript build output

I had the first/second test routes set to serve files from the ./app directory, as they just didn't want to serve from docs (kept getting a not found event). I'm assuming that has something to do with the wild card route taking precedence for some reason.

unsol avatar Aug 03 '18 22:08 unsol

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Oct 06 '18 18:10 stale[bot]

Bad stale bot! Bad!

Has anyone had a chance to test this out? I've been using this patch in production since August, and have had no issues.

unsol avatar Oct 06 '18 20:10 unsol

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Dec 05 '18 21:12 stale[bot]

Bumping it up again.

Still no issues with daily use.

unsol avatar Dec 12 '18 16:12 unsol

Been running on 7.4.0. Doesn't look like 7.5.0 will have any effect on the serveStatic changes.

unsol avatar Jan 10 '19 00:01 unsol