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 chdir warning #917

Merged
merged 1 commit into from Apr 24, 2018

Conversation

Projects
None yet
3 participants
@jtdowney
Contributor

jtdowney commented Mar 27, 2018

Dir.chdir will change the directory for the whole process. If multiple threads are running, this means those threads have their current directory changed as well. During feature tests, this could cause an issue with rack servers running in a background thread.

Since this use of chdir was to calculate the relative path for static files, I refactored it to not change the current directory for the whole process as a side effect.

@jodosha jodosha self-assigned this Mar 29, 2018

@jtdowney

This comment has been minimized.

Contributor

jtdowney commented Apr 7, 2018

Hi @jodosha. I wanted to check in on this and provide a bit more information. I only saw this with ruby 2.5 and it appears others saw it as well: https://discourse.hanamirb.org/t/confilicting-chdir-in-static-rb/422.

@kaikuchn

This comment has been minimized.

Member

kaikuchn commented Apr 9, 2018

This problem was also discussed in the hanami chat:
glaszig had randomly failing feature tests and noticed warnings about chdir: March 8, 2018 3:43 PM
I found that the above code is to blame and suggested as a quick fix the following: March 9, 2018 3:32 PM

I'm not sure about relative paths @jtdowney I think right now it is absolute paths? But yeah, the chdir is completely unnecessary and I don't know what made me use it when I wrote that code..

Edit: It was relative paths, so 👍

@jodosha

This comment has been minimized.

Member

jodosha commented Apr 10, 2018

Hi folks, I apologize for my delay here.

We're gonna to release v1.2.0 on tomorrow, and didn't want to merge a PR that I haven't manually tested carefully.

Security is my main concern here: does this change allows evil users to access the local file system outside of the public directory? I'm not saying that this PR will cause this problem, but I need to verify before to merge.

@kaikuchn would you be so kind to check as well?

Thank you all.

@kaikuchn

This comment has been minimized.

Member

kaikuchn commented Apr 10, 2018

What this piece of code does is register existing files relative to some root directory (by default the hanami public directory) and pass those along to Rack for serving static assets.

At no time is this code dependent on external(/evil user) input.

While the underlying rack-static middleware does make routing decisions based on the request, it cannot influence the root directory and any file outside of the root directory will not be registered, and therefore result in a 404 (or some other routing decision based on other middlewares).

Edit: I may also stress again, that this PR changes nothing except that we no longer mutate global state (the current working directory).

@jtdowney jtdowney changed the base branch from master to develop Apr 11, 2018

@jtdowney jtdowney changed the base branch from develop to master Apr 12, 2018

Fix chdir warning
Dir.chdir will change the directory for the whole process. If multiple
threads are running, this means those threads have their current
directory changed as well. During feature tests, this could cause an
issue with rack servers running in a background thread.
@jtdowney

This comment has been minimized.

Contributor

jtdowney commented Apr 12, 2018

I mistakenly set the branch to develop but changed it back to master after I saw the note at the bottom of the 1.2.0 announcement blog post. Since this is a bug fix it seems like it should go against master. I also updated the test since it seems a few extra env params are now required in the setup.

@jodosha jodosha added the fix label Apr 24, 2018

@jodosha jodosha added this to the v1.2.1 milestone Apr 24, 2018

@jodosha jodosha merged commit 49216e2 into hanami:master Apr 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jodosha added a commit that referenced this pull request Apr 24, 2018

@jodosha

This comment has been minimized.

Member

jodosha commented Apr 24, 2018

@jtdowney I just merged it thank you again for this fix and the wait. 💯
@kaikuchn Thanks for the review. 👍

@jodosha jodosha modified the milestones: v1.2.1, v1.3.0 Jul 19, 2018

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