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 #352 - Block PHP scripts in Nginx for uploads #356

Merged
merged 1 commit into from
Sep 17, 2015
Merged

Fix #352 - Block PHP scripts in Nginx for uploads #356

merged 1 commit into from
Sep 17, 2015

Conversation

erikbelusic
Copy link

Prevent php scripts from being accessed or executed inside the app/uploads folder and any subdirectories by adding an nginx directive to the wordpress.conf.

This has been tested to return a 403 error for any file with a .php extension in app/uploads or any subdirectory of uploads. Assets with any other extension are not affected and can be accessed as normal.

@@ -1,5 +1,10 @@
# {{ ansible_managed }}

# Prevent php scripts from being executed inside the uploads folder.
location ~* /app/uploads/.*\.php$ {
deny all;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use 2 spaces instead of 4

@erikbelusic
Copy link
Author

done and done! i shouldn't have did this on my master branch though....right? i should have made a feature branch?

@swalkinshaw swalkinshaw changed the title This resolves roots/trellis#352 by adding an nginx directive to the wordpress.conf Fixes #352 - Block PHP scripts in Nginx for uploads Sep 17, 2015
@swalkinshaw
Copy link
Member

@erikbelusic yeah you probably have created a branch but it's not a big deal. You can always re-create your fork.

One more thing: we usually like to keep PRs to a single commit (unless there's a reason for more) to keep our Git history clean. Would you be able to squash these two commits into one? We can provide instructions if you haven't done it before.

Although I just realized that's the best reason to do work on a feature branch since you need to force push after a rebase/squash and obviously you should avoid that on master :)

@swalkinshaw swalkinshaw changed the title Fixes #352 - Block PHP scripts in Nginx for uploads Fix #352 - Block PHP scripts in Nginx for uploads Sep 17, 2015
@erikbelusic
Copy link
Author

@swalkinshaw i have not done it before. please provide guidance =]

@erikbelusic
Copy link
Author

@swalkinshaw i think i got it. let me know if it turned out correctly.

swalkinshaw added a commit that referenced this pull request Sep 17, 2015
Fix #352 - Block PHP scripts in Nginx for uploads
@swalkinshaw swalkinshaw merged commit f63c2a1 into roots:master Sep 17, 2015
@swalkinshaw
Copy link
Member

@erikbelusic perfect! Thanks.

@erikbelusic
Copy link
Author

@swalkinshaw now that this has been merged, can i delete and refork to make any additional contributions on feature branches without messing this up?

@swalkinshaw
Copy link
Member

@erikbelusic yep I'd definitely do that.

@louim
Copy link
Contributor

louim commented Sep 17, 2015

@erikbelusic No need to delete.

Assuming you named the upstream (this remote) upstream:

git fetch --all
git reset --hard upstream/master
git push -f

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.

4 participants