Refactor Rack::Static and improve directory index support #477

Closed
wants to merge 1 commit into from

3 participants

@trans

This is a reissue of #403, rebased and with test.

This patch makes the the Rack::Static code a little more efficient and readable. At the same time it adds support for existent directory index support --instead of just checking for a trailing /, failing that it will check to see if the the path is an actual directory on disk. This is indispensable in some use cases where a trailing slash can not be insured, but the directories index file still needs to be served.

If necessary we can add a new configuration option to conditionally support this new directory behaviour, but I did not do so presently b/c I suspect it would be a YAGNI.

@raggi raggi commented on the diff Dec 30, 2012
lib/rack/static.rb
end
def call(env)
- path = env["PATH_INFO"]
+ path = env["PATH_INFO"].strip
@raggi
Official Rack repositories member
raggi added a note Dec 30, 2012

Why the addition of strip?

@trans
trans added a note Dec 30, 2012

Don't recall at this point. That's what I had from before. I suppose it was just in case trailing white space ended up in the path --it could mess up the route matching.

@tenderlove
Official Rack repositories member

This middleware is for serving up static files. It's legitimate for a static file to end in a space, so strip should not be used.

@trans
trans added a note Jul 11, 2014

Sounds technically correct, OTOH who in their right mind would do that and is it wise to facilitate them?

@raggi
Official Rack repositories member
raggi added a note Jul 12, 2014

The point is, your code is incorrect if it disregards correct use cases. This shouldn't be an argument, so please don't make it one.

@trans
trans added a note Jul 12, 2014

Spaces aren't valid in URLs, trialing spaces aren't valid in Windows, and even if they are possible in Linux it's a world of headache. Here's some learning for you: http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html. But hey if you still think my argument doesn't have a point, have at it.

@raggi
Official Rack repositories member
raggi added a note Jul 12, 2014

Congratulations, you just sent me an essay about how the shell is broken, incorrectly reporting that the filesystem is broken, as a justification for an invalid patch.

If you want to force your opinions on people in your own libraries and frameworks, that's totally fine, but I neither agree with you, nor can force such opinions on rack users. Please stop making this difficult, it's making me upset.

@trans
trans added a note Jul 13, 2014

After two years of nothing. No problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi raggi commented on the diff Dec 30, 2012
lib/rack/static.rb
@@ -149,5 +160,11 @@ def set_headers(headers)
headers.each { |field, content| @headers[field] = content }
end
+ # Determine if a path is a directory by looking for
+ # trailing `/` or, failing that, checking the file system.
+ def directory?(path)
+ path.end_with?('/') || ::File.directory?(::File.join(@root, path.to_s))
@raggi
Official Rack repositories member
raggi added a note Dec 30, 2012

I think we need to extract utility code out of Rack::File into Rack::Utils for path depth checks and readability checks, then use that here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi raggi commented on the diff Dec 30, 2012
lib/rack/static.rb
@@ -149,5 +160,11 @@ def set_headers(headers)
headers.each { |field, content| @headers[field] = content }
end
+ # Determine if a path is a directory by looking for
+ # trailing `/` or, failing that, checking the file system.
+ def directory?(path)
@raggi
Official Rack repositories member
raggi added a note Dec 30, 2012

directory matching may want an unescaped path. previously this was handled by Rack::File as static didn't need to care.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi raggi commented on the diff Dec 30, 2012
lib/rack/static.rb
end
+ # Look up `path` in urls.
+ # If urls is an Array, return `path` if there is a match, otherwise nil.
+ # If urls is a Hash, return path "overwrite" if there is a match.
+ # Otherwise return nil.
def route_file(path)
@raggi
Official Rack repositories member
raggi added a note Dec 30, 2012

route matching may want an unescaped representation of the path.

@trans
trans added a note Dec 30, 2012

That makes sense. I wasn't cognisant of the escape vs unescaped paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi
Official Rack repositories member

Thanks for taking the time to rebase this and add the test case. I left a few line comments. I'll be happy to do the security refactors, but please comment if I've missed anything.

Thanks again.

@trans

I don't see anything else off hand. You've covered some points I'd not readily notice, as I am not familiar enough with the Rack code as a whole. So if you can make these adjustments, that would be great.

@raggi
Official Rack repositories member

Due to the work that needs doing to this, I don't have time to cover it before the 1.5.0 release. I have rescheduled this for the 1.6 milestone.

@trans

Ok. But I hope 1.6 doesn't take too long to be released. I've been wanting to use this feature for a very long time!

@trans

Hmm... besides unescaping the path, what has to be done? Seems like the rest is just code structure improvements. Or am I overlooking something (e.g. moving some code to Utils)?

@mulderp

I was wondering if this refactoring could help to introduce a similar behavior as with Python's SimpleHTTPServer:

When I go to a directory with an index.html and I do:

python -m SimpleHTTPServer
Serving HTTP on 0.0.0.0 port 8000 ...

I automatically get the index.html served from the 0.0.0.0:8000 root path.
Could this be done in a similar way with Rack?
Right now, I need to specify index.html to get the same effect with the following config.ru:

run Rack::Directory.new("./public/")
@trans

@mulderp Yes, that's the idea.

Would be nice if this would make it into Rack.

@trans

😦 Unfortunate this feature never made it in.

@raggi
Official Rack repositories member

Oh, I was supposed to finish it? 👅

@trans

I think it is finished. You mentioned refactoring a bit, moving some code into Utils, but I don't know what you want exactly in that regard, so that's your call.

@trans trans closed this Jul 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment