missing trailing slash in URL when using default file "index.html" #56

Merged
merged 3 commits into from May 28, 2013

2 participants

@TeeTeeHaa

This issue is for node-static version 0.5.9.

In case the URL does not contain a file node-static tries to serve the default file "index.html". In case the file exists it does not matter whether the URL contains a trailing slash or not:

http://localhost/path/ ==> serves index.html
http://localhost/path ==> serves index.html

But if the file (index.html) includes some other files, e.x. a JS script file (code.js), the loading of the JS script file will fail with an "404 Not Found" error because the browser is assuming a wrong path for the JS script file:

http://localhost/path/ ==> serves index.html and serves code.js
http://localhost/path ==> serves index.html and serves 404 for code.js

The same examples in Apache look like this:

http://localhost/path/ ==> serves index.html and serves code.js
http://localhost/path ==> serves "301 Moved Permanently" with a header "Location" containing the URL including the trailing slash where the browser then goes to

I've written a proof-of-concept to demonstrate this bug:

https://gist.github.com/2983697

This pull request includes a fix for this bug. Disclaimer: I did not test that fix excessively so you should have a look yourself before putting this in a productive system.

@phstc phstc commented on an outdated diff May 25, 2013
lib/node-static.js
@@ -52,7 +52,14 @@ this.Server.prototype.serveDir = function (pathname, req, res, finish) {
fs.stat(htmlIndex, function (e, stat) {
if (!e) {
- that.respond(null, 200, {}, [htmlIndex], stat, req, res, finish);
+ var status = 200;
+ var headers = {};
+ var originalPathname = decodeURI(url.parse(req.url).pathname);
+ if (originalPathname.length && originalPathname.charAt(originalPathname.length - 1) !== '/') {
+ status = 301;
@phstc
Collaborator
phstc added a note May 25, 2013

Should it be 404 instead of 301?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@phstc
Collaborator

On GitHub https://github.com/cloudhead/node-static/pull/56 and https://github.com/cloudhead/node-static/pull/56/ respond equally.

With this PR this example shouldn't work. The second one will be considered as a folder and due it doesn't exist, it will send a 301 status.

@TeeTeeHaa

I took the behavior of Apache web server as reference. The Apache web server of my hosting provider behaves like this:
http://www.teeteehaa.de/path/ responds with 200 and content of file "index.html"
http://www.teeteehaa.de/path responds with 301 and header Location: http://www.teeteehaa.de/path/

Current, unmodified node-static 0.6.9 behaves like this:
http://localhost/path/ responds with 200 and content of file "index.html"
http://localhost/path responds with 200 and content of file "index.html" after which the request to "code.js" results in a 404
node-static considers both requests a directory and looks for "index.html"

Should it be 404 instead of 301?

If /path/ shall be considered a directory and /path shall not be considered a directory, yes, a 404 would fit better. But this is not like Apache web server behaves and this will confuse many users just like me.

I think valid options are 301 or 404, but the current 200 does break websites.

With this PR this example shouldn't work. The second one will be considered as a folder and due it doesn't exist, it will send a 301 status.

As I understand my code https://github.com/cloudhead/node-static/pull/56/ will return whatever an unmodified node-static returns because a 301 will only be returned if there is no slash at the end of the URL.

I think https://github.com/cloudhead/node-static/pull/56/ should be considered a directory. But Github is no good reference in my opinion because they probably use a highly custom web server and their URLs probably do not refer to actual directories and files.

P.S.: I like node-static for its simplicity and use it frequently. I would really appreciate this issue getting fixed by adopting the behavior of Apache web server (which is the de facto reference in my opinion).

@phstc
Collaborator

Thank you for your well detailed explanation. 👍

It is true, the GitHub behaviour isn't a good example. We have to follow rfc 2616.

rfc 2616 compliant HTTP static-file server module, with built-in caching.

Before the merge, could you add tests?

https://github.com/cloudhead/node-static/blob/master/test/integration/node-static-test.js

Cheers

@TeeTeeHaa

Thank you for your well detailed explanation. 👍

Just trying to help you understand my problem.

Regarding tests I'll see what I can do but it might take a couple of days.

TeeTeeHaa added some commits May 28, 2013
TeeTeeHaa Merge remote-tracking branch 'upstream/master' de26676
TeeTeeHaa improved fix and added test cases
improved issue regarding missing trailing slash in URL when using
default file "index.html" and added test cases
6af107c
@TeeTeeHaa

Disclaimer: today I used git and "GitHub for Windows" for the very first time (I only used github.com before) and I am not sure whether I did everything right.

In my latest commit I improved my initial fix further and added several test cases. In the test for the 301 redirect I check the body to be an empty string. Maybe a check for undefined would be better, but actually an existing, similar test case with a check for body to be undefined currently fails for me. I reported that in issue #99 .

@phstc phstc merged commit 544f7ad into cloudhead:master May 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment