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

.htaccess hardening #5621

Merged
merged 12 commits into from Jun 16, 2017
Merged

.htaccess hardening #5621

merged 12 commits into from Jun 16, 2017

Conversation

DaazKu
Copy link
Contributor

@DaazKu DaazKu commented May 25, 2017

  • Disallow directories that do not need to be exposed by the webserver
  • Prevent any script but /index.php from being called by default.
  • Clean up existing rules
  • Remove deprecated

Fixes #3431

@DaazKu DaazKu added this to the 2017-Q2-5 milestone May 25, 2017
@DaazKu DaazKu added the Status: WIP This pull request is currently in progress. Do NOT merge it. label May 26, 2017
DaazKu and others added 2 commits May 26, 2017 13:02
Make comments vs commented code clearer
@DaazKu DaazKu assigned linc and unassigned linc May 29, 2017
@DaazKu
Copy link
Contributor Author

DaazKu commented May 29, 2017

@linc Are you ok with this not being commented by default?

RewriteCond %{REQUEST_URI} !^/index.php$

@DaazKu DaazKu added Status: Awaiting Feedback This issue or pull request is awaiting feedback from someone. DON'T MERGE! and removed Status: WIP This pull request is currently in progress. Do NOT merge it. labels May 29, 2017
@initvector initvector assigned linc and unassigned initvector Jun 1, 2017
@linc
Copy link
Contributor

linc commented Jun 1, 2017

Not really, no. That's the sort of thing that confuses the hell out of folks who aren't expecting it and will send them on a days-long odyssey. It should be opt-in for folks who want that level of security.

.htaccess.dist Outdated
RewriteCond %{QUERY_STRING} ^p=/?([^&]+)(&([^?]+))?$
RewriteRule ^index\.php %1?%3 [E=X_REWRITE:1,L]
####
# Deny access to certain directories that SHOULD not be exposed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you wanna capitalize the 'NOT' also or instead.

.htaccess.dist Outdated
# Deny access to certain directories that SHOULD not be exposed.
####
RewriteRule (^|/)\.git - [L,R=403]
#RewriteRule ^build/ - [L,R=403] # Already covered by .htaccess
Copy link
Contributor

Choose a reason for hiding this comment

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

In what scenario is someone uploading a build or tests folder that we'd need to account for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they upload the cloned repository

@@ -0,0 +1 @@
Deny from all
Copy link
Contributor

Choose a reason for hiding this comment

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

It's common for hosts to ignore .htaccess files that aren't in the root directory. What's the rationale for splitting this off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/tests already have an .htaccess I just copied the behaviour.

I added the commented rules in the case someone preferred not to use the .htaccess (which is faster I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That being said should I uncomment them?

Copy link
Contributor

@linc linc Jun 8, 2017

Choose a reason for hiding this comment

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

I'd delete the redundant lines in the main htaccess entirely.

These directories should never, ever make it to production. The sub-.htaccess files being here is already a backup. Double-backing it up by also having comments in the main htaccess seems like tacit approval of this massive screwup because someone would have to opt into it, at which point what the hell are they even doing?

@linc linc added Revisit and removed Status: Awaiting Feedback This issue or pull request is awaiting feedback from someone. DON'T MERGE! labels Jun 1, 2017
@DaazKu DaazKu added Status: Awaiting Feedback This issue or pull request is awaiting feedback from someone. DON'T MERGE! and removed Revisit labels Jun 2, 2017
@linc linc modified the milestones: 2017-Q2-6, 2017-Q2-5 Jun 5, 2017
@linc linc added the Revisit label Jun 8, 2017
@linc linc removed the Status: Awaiting Feedback This issue or pull request is awaiting feedback from someone. DON'T MERGE! label Jun 8, 2017
@DaazKu DaazKu removed the Revisit label Jun 13, 2017
@linc linc merged commit 1050218 into master Jun 16, 2017
@DaazKu DaazKu deleted the feature/apache-htaccess branch June 16, 2017 20:38
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.

None yet

3 participants