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 serving files that clash with directories (#6222) #6231

Merged
merged 1 commit into from Jul 25, 2017

Conversation

@MattSturgeon
Copy link
Contributor

MattSturgeon commented Jul 18, 2017

Fix #6222.

To avoid having to re-implement the entire set_filename method, I instead extended search_index_file and temporarily mutate res.filename. Kinda hacky, so I tried to leave plenty of inline comments. (Also first time using ruby, so beware!)

For reference, here's the base class search_index_file implementation.

Feel free to checkout the commit message too.

@pathawks pathawks requested a review from jekyll/stability Jul 18, 2017
@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jul 18, 2017

@jekyllbot jekyllbot assigned ghost Jul 18, 2017
@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jul 18, 2017

Thank you @MattSturgeon for learning Ruby just to fix this! 🍻

@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 19, 2017

No problem, feel free to recommend any changes if required.

Just noticed the first mistake, I linked to the wrong super method in the commit message! Should be this one instead, so I'll amend the commit and we can wait for the CI builds to start from scratch!

@MattSturgeon MattSturgeon force-pushed the MattSturgeon:fix-6222 branch from 16897d3 to 811ed0f Jul 19, 2017
Copy link
Member

ashmaroli left a comment

Hi, some minor changes:

  • == and === check for equality
  • = assigns a value to a variable
if File.extname == ".foo"
  puts "Foo Ahoy!"
  baz = "foobar"
end
# First, let's see if the default implementation can figure it out.
# (Check for index files in the res.filename directory)
if file = super
return file

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

arguments for if, unless, while etc check for equality via (== or ===) and the body assigns a value to a variable via =

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

I wasn't checking for equality. I was checking if the assigned value was truthy. if file = super evaluates to nul we continue, otherwise we return the value assigned to file.

(the super method)

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

Appreciate you mentioning it though, since I did say I was new to ruby. (Not as new to programming though).

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

I see.. that wasn't readily apparent to me from reading the code.
Learnt something new today then.. 🍻

path_arr = res.filename.scan(%r!/[^/]*!)
while basename = path_arr.pop
break unless basename == "/"
end

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

I feel this while..break unless -- can be replaced by an alternative..

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

It can probably just be basename = path_arr.pop, but to cover edge cases I wanted to loop until basename wasn't empty (well, "/") since there may be (a?) trailing slash.

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

okay, can you list 3-5 such edge-cases of path and basename? Will help you write tests later..

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

The normal code flow would be

basename = arr.pop
if basename == "/" then basename = arr.pop # I can't imagine this happening
                                           # more than once, but imho it
                                           # makes more sense to allow for
                                           # the possibility.
done

To me, while ... break unless ... is actually quite an elegant solution to popping off an unknown number of elements and assigning the final pop.

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

Sorry, didn't see your comment before replying. 3-5? Not sure. Edge case may not be the best choice of words.

Basically our code doesn't know if res.filename will have a trailing / or not. We could go with whether it does or doesn't right now, but it may change in the future or (without analysing the WEBrick code) be inconsistent.

Essentially I'm treating it as unsanitised input.

Even if there are no trailing slashes (in the current WEBrick implementation), having the loop immediately break is effectively the same as not having the loop, in terms of performance and complexity. It just gives us the robustness against the possibility.

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

have you tried until?

basename == arr.pop until basename == "/"

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

hostname == arr.pop until hostname == "/" doesn't assign the value of arr.pop to hostname. Also hostname will be undefined when evaluating the until condition.

Looking at the set_filename implementation, (which sets up the res.filename and calls search_index_file), res.filename is fairly sane and unlikely to change.

It looks like the filename shouldn't have any trailing slashes (so basename = arr.pop without testing for trailing slashes may work) when we get in search_index_file, but I haven't tested.

Still, the while method isn't complex or slow and it makes our method more robust and resilient to change.

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

I could use index instead of arrays, something like this:

# Index of final / that isn't followed by another / or EOL
# Could just be rindex('/') if we don't care about trailing slashes
i = res.filename.rindex %r!/(?!/|$)!
basename = res.filename[i..-1]
res.filename = res.filename[0, i]
@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 19, 2017

I've tried to make the code more readable, following @ashmaroli's feedback.

In 20a333f I replaced a conditional return with an unless branch and removed two out of three cases of assignment within conditions. I also re-write most of the comments.

It's a fixup commit, so intended to be rebased/autosquashed into the original commit, just thought I'd keep it separate for now in case anyone wanted to compare it with the original PR (?w=1 is your friend here).

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Jul 19, 2017

FYI, Currently, Jekyll's merge bot will squash all commits irrespective of the --fixup flag

@MattSturgeon MattSturgeon force-pushed the MattSturgeon:fix-6222 branch from bb94deb to 737461c Jul 19, 2017
@ghost
ghost approved these changes Jul 19, 2017
Copy link

ghost left a comment

very nice! +1 for the extra documentation

res.filename << basename unless file
end

return file

This comment has been minimized.

Copy link
@ghost

ghost Jul 19, 2017

you technically don't need the return statement, since ruby returns the last statement in a function anyways

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

But the last statement is res.filename << basename, unless file is evaluated first, right? Either way I think it is more readable in this case to explicitly return file.

This comment has been minimized.

Copy link
@ghost

ghost Jul 19, 2017

i agree!

@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 19, 2017

Ok, good to know. I think this is probably ready now, I've pushed up a slightly re-worded commit with all changes including switching to index based splitting:

i = res.filename.rindex "\/"
basename = res.filename[i..-1]
res.filename = res.filename[0, i]

And simply appending basename back onto filename instead of copying a backup.

From what I could tell, trailing slashes are unlikely to ever be an issue with WEBrick so I shifted to simply splitting on the index of the final / char without messing around with arrays or loops.

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Jul 19, 2017

What we need now are tests.. 😈

@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 19, 2017

Where are the existing tests for the serve command?

@ashmaroli

This comment has been minimized.

@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 19, 2017

How did I miss that? I did look, I promise!

@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 19, 2017

I'm probably missing something, but I'm not sure there's any testable changes here. Short of running the server, and making a mini client within the test suit to assert that HTTP requests return expected results.

The tests for command serve seem to be mostly sanity checking configuration options to make sure config files and command line switches have taken affect.

I'd need to run the command asynchronously, then fire off a HTTP request to a few URLs that I know are either directories, files or both and assert that the result matches the local file I wanted.

Is this within scope of the tests? It sounds more complex than the entire feature to me. It doesn't seem to be something that was added for #3452

What kind of things should I be testing for this change?

Copy link
Member

ashmaroli left a comment

needs some more edits..

@@ -21,10 +21,36 @@ def initialize(server, root, callbacks)
# up with a different preference on which comes first.

def search_file(req, res, basename)
# /file.* > /file/index.html > /file.html
# /file.* -> /file.html

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

please undo this change.

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

Any reason why? It was an intended change, since search_file doesn't change "/file" into "/file/index.html" (that's search_index_file's job)

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

I don't think the comment meant changing /file.* into /file.index.html and then into /file.html either.
Its probably documenting the order of traversal.

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

Ah, in that case I'll clarify and move it above search_file since it applies to both.

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

How's this?

# Order of precedence:
# /file -> /file/index.html -> /file.html -> fancy-indexing -> 404.html

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

I'll defer this to the original author for his input.
/cc @envygeeks

This comment has been minimized.

Copy link
@MattSturgeon

MattSturgeon Jul 19, 2017

Author Contributor

For now I have this:

# Add the ability to tap file.html the same way that Nginx does on our
# Docker images (or on GitHub Pages.) The difference is that we might end
# up with a different preference on which comes first.
#
# Order of precedence:
# /file -> /file/index.html -> /file.html -> fancy-indexing -> 404.html

def search_file
  # First see if we can find /file normally, if not try /file.html
end

def search_index_file
  # First, let's see if our super method can find an index file.
  unless
    # Ok, looks like there's no index file in the directory.
    # Let's look for directory.html instead...

    # Try and find a file named dirname.html in the parent directory.
  end
end

actual code removed for brevity

/cc @envygeeks

def search_index_file(req, res)
# /file/index.html -> /file.html

# First, let's see if the our super method can figure it out.

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

small typo => ..if the our..

res.filename = res.filename[0, i]

# Try and find a file named dirname.html in the parent directory.
file = search_file(req, res, basename + ".html")

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 19, 2017

Member

in the really rare chance that basename is not a String, this will fail. Instead interpolate:

- file = search_file(req, res, basename + ".html")
+ file = search_file(req, res, "#{basename}.html")
@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Jul 19, 2017

What kind of things should I be testing for this change?

Mainly, if the original issue you reported has been fixed and stays fixed..

@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 19, 2017

What kind of things should I be testing for this change?

Mainly, if the original issue you reported has been fixed and stays fixed..

IMO that's not practical to test. Unless you have some test suite that can compare files served over HTTP to files on the filesystem?

@envygeeks

This comment has been minimized.

Copy link
Contributor

envygeeks commented Jul 20, 2017

This is a lot of complicated code for such a simple problem.

@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 20, 2017

Well the problem isn't as simple as it sounds, but I wouldn't say the solution was complex or even large (10 lines).

set_filename passes us a directory path (res.filename) and it expects us to return a file from within that directory to use as an index file (usually index.html).

We want to abuse this slightly and return a file from the parent directory of the one we have, so we have to modify res.filename and look for files in the modified directory path.

If we fail, we undo our modification so that we don't break WEBrick's Fancy Indexing.

Are you saying that it isn't worth this amount of complexity? Also, are you happy with the modification to your /file.* > /file/index.html > /file.html comment?

@envygeeks

This comment has been minimized.

Copy link
Contributor

envygeeks commented Jul 20, 2017

No, I'm not happy with the comment change, because it's arbitrary. I'm happy you learned Ruby and welcome to the Ruby community! This can be fixed by changing search file to super(req, res, ".html") || super || super(req, res, "#{basename}.html") without the extra 9 lines of code.

Sorry it took me so long to respond.

@MattSturgeon MattSturgeon force-pushed the MattSturgeon:fix-6222 branch from 6107d9f to 6dfc2cc Jul 20, 2017
@MattSturgeon MattSturgeon force-pushed the MattSturgeon:fix-6222 branch from 6dfc2cc to 1dc2bc7 Jul 20, 2017
@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 20, 2017

Ah, much simpler. Probably should be ordered super || super(req, res, ".html") || super(req, res, "#{basename}.html") to prioritise the default.

As soon as I noticed search_index_file was used when searching a directory, I focused on that and didn't notice that you could just use ".html" in search_file and it would concatenate into a sane filename.

I've added your changes as 1dc2bc7.

@envygeeks

This comment has been minimized.

Copy link
Contributor

envygeeks commented Jul 20, 2017

No, it should be in the order I put it in, as that was my intention, and it was my intention with this source to begin with. You should also rebase, squash, and only have commits in your name, it is bad form to commit in another's name, it should instead be credited in the extended message (if you wish,) as committing in my name forces me to have to say "that commit is not mine, it's not signed" in the future, should a problem arise, and it also alters the blame in a confusing way on Github. Sorry to be weird.

Thanks!

@envygeeks

This comment has been minimized.

Copy link
Contributor

envygeeks commented Jul 20, 2017

I guess what I am saying is, I do wish to take blame if something goes wrong (and I'll happily help fix it,) but the commit should not be in my name, you should instead opt to link to my comment in the extended commit, so that people know that the source was originally my idea, and it's my fault if something is wrong.

@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 20, 2017

I didn't commit in your name, I listed you as the author of the changes. That's what --author is for, committing other's changes. It still list's me as the committer, so I still get the blame for any issues with the commit.

commit 1dc2bc74580060d4f00dd4a45e02a70574a56672
tree 4207e3ea2b7b52733fb6d3e3a85d5cc00d6b6522
parent 56546a28fda95098c44b31090050db0f415be574
author Jordon Bedwell <jordon@********> 1500420691 +0100
committer Matt Sturgeon <matt@********> 1500521221 +0100

    Fix serving files that clash with directories

Why did you want it to check ".html" before the default check of basename? Surely basename -> ".html" -> "basename.html" or basename -> "basename.html" -> ".html" makes more sense?


Either way since it is your change now, it is probably easiest if you make a separate PR.

@parkr
parkr approved these changes Jul 21, 2017
@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jul 21, 2017

Thank you, all! I'm guessing the comment directly above this line is fine?

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jul 21, 2017

Additionally, a test would be nice.

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jul 21, 2017

Additionally, a test would be nice.

Suggestions would be appreciated. See above comment:

I'm probably missing something, but I'm not sure there's any testable changes here. Short of running the server, and making a mini client within the test suit to assert that HTTP requests return expected results.
...
What kind of things should I be testing for this change?

@envygeeks

This comment has been minimized.

Copy link
Contributor

envygeeks commented Jul 21, 2017

This source was never really tested to begin with, I don't know if I would force that constraint on the new author, I'll take responsibility outside of this pull request if necessary, it shouldn't really fall on him.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jul 25, 2017

Ok! I'd love to get that module tested so we don't break this behaviour in the future, but it's not a super high priority. Thanks, all!

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit ec84bec into jekyll:master Jul 25, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyllbot added a commit that referenced this pull request Jul 25, 2017
@envygeeks

This comment has been minimized.

Copy link
Contributor

envygeeks commented Jul 26, 2017

This commit is not what my code originally was, what it intended, and it still has my name stamped all over it as the author even though the person who used my name falsely refused to fix the problem, it is merged now and my name is associated with something I didn't write, please fix this problem.

@MattSturgeon MattSturgeon deleted the MattSturgeon:fix-6222 branch Jul 26, 2017
@MattSturgeon

This comment has been minimized.

Copy link
Contributor Author

MattSturgeon commented Jul 26, 2017

@envygeeks your name is not on the merged commit (ec84bec)

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.