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

fix NotAuthorized error when serving static files while directory var… #1692

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

unsol
Copy link

@unsol unsol commented Aug 3, 2018

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:

  • 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 (

if (!re.test(file.replace(/\\/g, '/'))) {
) 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.

@stale
Copy link

stale bot commented Oct 6, 2018

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 stale bot added the Stale label Oct 6, 2018
@unsol
Copy link
Author

unsol commented Oct 6, 2018

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.

@stale stale bot removed the Stale label Oct 6, 2018
@stale
Copy link

stale bot commented Dec 5, 2018

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 stale bot added the Stale label Dec 5, 2018
@unsol
Copy link
Author

unsol commented Dec 12, 2018

Bumping it up again.

Still no issues with daily use.

@stale stale bot removed the Stale label Dec 12, 2018
@unsol
Copy link
Author

unsol commented Jan 10, 2019

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

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