In Apache 2.4, the "Order" becomes deprecated
With Apache 2.4 the old Order get's deprecated in favor of Require.
If hosters by default have Access compat, then Order keeps working. Otherwise however, with our current .htaccess Apache 2.4 will not start after installing Drupal.
I tried to figure out a way to check which the current version of Apache is, but unfortunately IfVersion is only available in 2.4, so that will break 2.2. My stackoverflow question hasn't yielded anything that we can use yet. I asked on IRC, and got two suggestions.
- Ship two files, one < 2.4 and one >= 2.4. Pick one during install. Not sure how feasable that is for us.
- Find modules that are unique and frequently enabled in the different environments, and do IfModule on those to determine where we are. This seems like a total hack to be honest, but may actually be the best way to go if we can identify a reliable set of modules.
Comment | File | Size | Author |
---|---|---|---|
#97 | interdiff.txt | 253 bytes | David_Rothstein |
#97 | drupal7-htaccess_protections-1599774-97.patch | 1.21 KB | David_Rothstein |
#90 | drupal7-htaccess_protections-1599774-90.patch | 1.19 KB | stefan.r |
#90 | interdiff.txt | 687 bytes | stefan.r |
#73 | drupal7-htaccess_protections-1599774-73.patch | 1.21 KB | mpdonadio |
Comments
Comment #1
BTMash CreditAttribution: BTMash commentedPerhaps another solution might be that we have whatever we need inside the .htaccess file and if users are running 2.4, they comment out the 'Order' set of lines for the 'Require'. And if changing the htaccess file is considered hacking core, then it makes more sense for there to be an example.htaccess (or .htaccess.default as per #1117704: Move .htaccess to .htaccess.default) file anyways so a person can make the htaccess file as needed.
Comment #2
Letharion CreditAttribution: Letharion commentedSince I haven't been able to find anything close to a clean solution, it may just come down to that, but honestly, that is just _not_ a good solution. By the time we ship 8, 2.4 will be popping up all over the internet. By the time 8 takes over 7 in usage, 2.4 is likely to be a common server.
By requiring users to manually edit the .htaccess file, we're making an already complex installation worse. It's not that I mind myself, I don't use the file anyway, but since we're moving towards being more user friendly, we should be doing the opposite, that is, make installation easier.
Comment #3
marcingy CreditAttribution: marcingy commentedAfter some digging ifversion does work with 2.2 (it is available from 2.0 onwards) see http://httpd.apache.org/docs/current/mod/mod_version.html#ifversion
On xampp I had to load LoadModule version_module modules/mod_version.so to get it working so that could become part of the install process. Not sure how widely supported the module is on shared hosts though.
Obviously we need an update to install instructions for this approach for sites without the module installed.
Comment #5
BTMash CreditAttribution: BTMash commentedI went through this with @marcingy (and am updating the issue as a result) by looking at our default apache environments when ubuntu gets installed. mod_versions is not enabled by default. At my workplace, we actually have a shared hosting environment so I was able to see what a shared env would have and unfortunately, same issue. Its a real shame because I like the patch quite a lot. I don't have an apache2.4 instance to test things out on...but is mod_access_compat enabled by default by any chance?
Comment #6
marcingy CreditAttribution: marcingy commentedLooking in xamppp mod_access_compat is enabled in 2.4 by default.
Comment #7
marcingy CreditAttribution: marcingy commentedI am assuming you are thinking of using ifmodule ! if so we should use http://httpd.apache.org/docs/trunk/mod/mod_authz_core.html as technical http://httpd.apache.org/docs/current/mod/mod_access_compat.html is just for 2.3.
Comment #8
BTMash CreditAttribution: BTMash commentedwell...mod_access_compat is made for 2.3+ (so the settings from 2.2- can still continue to work). If it is enabled by default, then we know that someone intentionally went in and disabled it (I'd imagine if it is a shared hosting company, they would want the access_compat enabled or see a lot of angry emails but I am unsure on this). In which case, we can add some documentation for those that don't.
IfModule would be a nice solution if there was an directive to go with it.
Comment #9
marcingy CreditAttribution: marcingy commentedThe issue I see that there is no way to determine if we on 2.2 where the module does not exist vs 2.3+ and the module has been disabled. Which why I am thinking this approach works better as the core module will be enabled and it gives us a simple option to switch, instead of having to have a multi layer ifModule structure.
Comment #10
BTMash CreditAttribution: BTMash commentedah...that is what I was looking for (I did not understand what you meant by the ! until I looked it up). But yes...I'd imagine that is a fairly good workaround? I'll try and write up a patch if you're not able to get to it before then.
Comment #11
BTMash CreditAttribution: BTMash commentedI just realized...the patch wouldn't take too long. Attaching patch for review.
Comment #12
Letharion CreditAttribution: Letharion commentedThe syntax used in the apache documentation http://httpd.apache.org/docs/2.4/mod/mod_authz_core.html#require looks like "Require all denied" instead of "Require allow,deny". Is there a reason to use the syntax of the current patch?
Comment #13
BTMash CreditAttribution: BTMash commentedAck, I had messed it up - you are right. What is the simplest rule we should use then? Would it be 'Require all granted'? It *seems* like the equivalent for 'Order allow, deny' but I'm not entirely sure.
Comment #14
bleen CreditAttribution: bleen commented... as long as you're making changes to the patch in #11, I would add a comment explaining why you checking for that module ... in an ideal world those if statements would be "if apache_version >=2.4 "... and there would be no confusion, but since we are using the existence of an apache module instead I think we need to explain why.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat looks very fragile. The existence of a mod_authz_core module is an implementation detail, not a contract we should rely on.
Given that it is 100% certain that no host in the world that is running Apache 2.4 is not going to run with
mod_access_compat
, I think we should just postpone this to after Drupal 8.Comment #16
Letharion CreditAttribution: Letharion commented@#13 Yes,
require all granted
(or all denied) is what the documentation does, and I've started using myself so I can say that it works as expected.@#15 Yes, it's really fragile, I agree. However, I have spent quite some time trying to find out what a robust way would be, and that type of solution was the best one I could come up with as well. That said I have neither used all options to find a better method, nor am I an expert on apache config.
I initially assumed that there would be an environment variable hardcoded into apache, much like we do in bootstrap.inc, that tells us which version is being used, but if that is the case, then I haven't been able to find this info. Perhaps it would be a reasonable feature request against apache.
I agree that it's reasonble to assume that all generic hosts will run with mod_access_compat for a long time. We could definitely postpone this until around this time next year, yes.
Personally, I just want a useable patch in the queue, because I do run without compat myself, so that I can root out problems early and get the upgrade over with. Now that there's an issue and a suggestion for a patch, I can just stick that in my make-files and be happy.
Comment #17
catchWhat about two sets of rules? One for each apache version, and then you'd be required to comment/uncomment depending on what you're running.
Comment #18
Letharion CreditAttribution: Letharion commentedThat would be great, but I never even bothered suggestion it since that will make installation (even) more complicated, something I assumed wasn't gonna fly.
Perhaps stick with the 2.2 one by default, yet distribute an optional 2.4, requiring only 2.4 users to take extra action. Then switch those around when we release D9 or 10.
Comment #19
catchYeah shipping with 2.2 by default, then rules for 2.4 commented out I thought. Then we've got rules in core that can be tested by more people, and we can try to improve it as time goes on - or as you say swap the defaults if nothing much changes but 2.4 adoption picks up.
Comment #20
Letharion CreditAttribution: Letharion commented@#19 It's probably one of the best ways of going forward with this.
The only issue I see with that is that technically, changing the .htaccess file will be "hacking core", something that I will find annoying since I move around and/or create new sites frequently.
I will probably just post a patch that does away with the 2.2 support and stick that patch in my makefile.
Comment #21
torvall CreditAttribution: torvall commentedGentoo has just started pushing Apache 2.4 in its stable release and the Drupal sites I have in a Gentoo box stopped working after the update. (Edit: It seems that this was a mistake by the ebuild maintainers and 2.4 was marked as unstable again. Reference: https://bugs.gentoo.org/show_bug.cgi?id=459264)
I had to edit .htaccess in all of them to get them working again. However, the Require directive syntax in #11 is not correct, should be "Require all granted" instead of "Require allow,deny" (see http://httpd.apache.org/docs/2.4/upgrading.html).
This will be a problem not only in D8 so, in my opinion, the directive should be included (as a comment, of course) in all versions.
Comment #22
FreekyMage CreditAttribution: FreekyMage commentedAs #21 says, this should be fixed in D7 as well.
Comment #23
dellintosh CreditAttribution: dellintosh commentedMight consider using this: http://httpd.apache.org/docs/2.2/mod/mod_version.html
A bit verbose, but our tests seem to work fine with it.
Comment #24
kristofferwiklund CreditAttribution: kristofferwiklund commentedIt require the version module. And it seems that Ubuntu doesn't have it active as default. Read comment #5
I have the same problem with a update Gentoo version. Apache 2.4
Comment #25
kristofferwiklund CreditAttribution: kristofferwiklund commentedHere come a patch with updated syntax.
It has been tested on latest Gentoo with Apache 2.4 and Ubuntu 12.04 LTS with Apache 2.2.
And it works nice.
This is not a optimal solution but I could not find any other better. As the option of commenting and uncommenting will force Apache 2.4 users to first edit .htaccess before installation. This is not good for future OS upgrades. And also for every core update one needs to do the same thing. I have some idea about solving this but I needs to do some more testing.
And the other solution using IfVersion; this is not enabled by default. And also using the If syntax is not working in Apache 2.2. So that solution is not possible.
edit: typo
Comment #26
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedWorks for RHE6 and Apache 2.4.
Comment #27
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI have issues however with the rewrite module. Maybe its not related but when I submit a login form from /user apache logs a 404, while when I submit a login form from /?q=user, apache logs a 200 OK.
Comment #28
kristofferwiklund CreditAttribution: kristofferwiklund commentedI have not seen that problem. For me it works with both url.
Which OS are you running under. And how is your server installed? I might be as you said an other problem that needs it's own issue.
Comment #29
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedYes probably, I encounter it on RHEL6, with Apache 2.4 built from sources with dynamic module Rewrite.so.
URLs works fine but for some reason the Clean URL test (in admin panel) fail to validate. I think this is definitely related to the Apache configuration and .htaccess configuration but may belong to another issue (or not).
Comment #30
kristofferwiklund CreditAttribution: kristofferwiklund commentedUpdating the tags.
Comment #31
valthebaldWorks well on Debian testing (Apache/PHP installed from packages, not manually compiled)
Comment #32
gnassar CreditAttribution: gnassar commentedApache 2.4, FreeBSD 9.2. Worked perfectly. Clean URL test passed fine, both URLs listed in #27 work -- can't replicate.
Probably about time this gets bumped to RTBC.
Comment #33
gnassar CreditAttribution: gnassar commentedI should note: also rolled back to Apache 2.2 and tested. Still worked perfectly.
Comment #34
Xano#25: update_htaccess_for_apache_2_4-1599774-25.patch queued for re-testing.
Comment #35
Morbus IffWhy is it "Require all granted" and not "Require all denied"?
The docs for the old 2.2 "Order allow,deny" explain why it works without a "Deny from all": "First, all Allow directives are evaluated; at least one must match, or the request is rejected. Next, all Deny directives are evaluated. If any matches, the request is rejected. Last, any requests which do not match an Allow or a Deny directive are denied by default."
But, for 2.4, "Require all granted" just point blanks states: "Access is allowed unconditionally."
Thus, it appears something is wrong (in 2.4, we're allowing everything)
or something is tricky (in 2.4, what you think you see is wrong because...).
Comment #36
alexpottfile_save_htaccess()
will not correctly create an htaccess for private files so this needs work.The value of the
MTimeProtectedFastFileStorage::HTACCESS
constant is"SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006\nDeny from all\nOptions None\nOptions +FollowSymLinks"
Comment #37
kristofferwiklund CreditAttribution: kristofferwiklund commentedAgree. And for Drupal 7 that´s in function file_create_htaccess($directory, $private = TRUE) in /includes/file.inc
Comment #38
antarchi CreditAttribution: antarchi commentedcomment #35: which should it be? I've just had to manually edit .htaccess and used "Require all granted". Is this OK?? Is that the best solution currently? (I'm a noob)
Comment #39
Gorka Guridi CreditAttribution: Gorka Guridi commented3: apache-2.4.patch queued for re-testing.
Comment #41
longwave#35 is correct, this should use "Require all denied". I am running Ubuntu 13.10, which uses Apache 2.4, but also has mod_access_compat enabled for backward compatibility.
Without any patch:
With #25 applied:
With #25 applied, and modified to use "Require all denied":
Comment #42
longwaveAttached patch fixes the above, plus FileStorage::htaccessLines(), and adds some extra assertions.
Comment #43
sunNot sure why this is categorized as feature. Also not sure whether the priority shouldn't actually be major or even critical, because unless I misunderstood, this incompatibility causes all of our .htaccess files in public but even more worrisome also private files directories to be dysfunctional — and no one is aware of it.
Comment #44
tstoecklerI also do not see how this can not be critical, unless I'm completely missing something.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedagree this is critical, patch looks ready to me.
Comment #46
alexpottCommitted 9e72c8b and pushed to 8.x. Thanks!
Comment #48
hgurol CreditAttribution: hgurol commentedThere is a function somewhere that creates an .htaccess file on the fly for some certain folders; eg. the /tmp folder.
Does this patch takes care of that too?
PS: This is for D7 by the way...
Comment #49
hgurol CreditAttribution: hgurol commentedincludes/file.inc
around lines 534
That "Deny from all" should be "Required something..." right?
or maybe more changes are needed.
I hope you have looked at the issue from all angles.
Comment #50
catchThat's https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21PhpS... in Drupal 8, and it's covered in the patch.
Comment #51
marcingy CreditAttribution: marcingy commentedComment #52
longwaveI think this should move to a feature request for Drupal 7, not a bug. Drupal 7 was released before Apache 2.4 and so there was no way we could guarantee forward compatibility at the time. Trying to retrofit it now with the technique used in the Drupal 8 patch should work on most setups, but can we guarantee it works on all existing configurations out there?
Also, it looks like we might have a further difficult case to deal with: upgrading existing autogenerated .htaccess files that existed prior to the patch, to catch users that move their D7 site from Apache 2.2 to 2.4.
Comment #53
serundeputy CreditAttribution: serundeputy commentedbackport for D7.
Comment #54
valthebaldTagging for manual testing - someone needs to test this on both 2.2 and 2.4
Comment #55
widukind CreditAttribution: widukind commentedI'm verifying patch #53 to be working with Apache 2.4.7 on Ubuntu 14.04 (access_compat turned off) and Apache 2.2.22 on Ubuntu 12.04.
Comment #56
quicksketchThis line needs to be changed to use concatenation instead of replacing the entire
$lines
variable. If the entire variable is overridden, then the special string "Drupal_Security_Do_Not_Remove_See_SA_2006_003" goes missing, and then system_requirements() throws an error every time the Statue Report page is viewed.Comment #57
quicksketchComment #58
Rob C CreditAttribution: Rob C commentedThis fixes the missing dot, no other changes.
Comment #59
quicksketchThanks Rob C. One additional issue, the
<<<EOF
styntax doesn't add any new lines before/after, so the code comment# Deny all requests from Apache 2.4+.
ends up squashed at the end of the line instead of on its own line. I'd recommend putting a new line above the comment, and for extra kudos, adding an extra line at the end of the first block of code. Here's the fix that we made to this in Backdrop: https://github.com/backdrop/backdrop/pull/997/files.Comment #60
Rob C CreditAttribution: Rob C commentedAdded 1 additional line before start and at the end.
Comment #61
ben.bunk CreditAttribution: ben.bunk commentedI've added an update hook to the system module to force regenerating the htaccess files in the files directories. This may cause problems for people if they modified these files but I think the benefit would outweigh the risk here for the majority of sites that exist and may not even realize the file was generated on their behalf.
Comment #63
ben.bunk CreditAttribution: ben.bunk commentedI updated the patch, it turns out that the overwrite flag for file_create_htaccess doesn't actually allow overwriting unless you first change the file permissions to allow it.
Comment #64
ben.bunk CreditAttribution: ben.bunk commentedComment #66
joelpittetCan we go back to 60? I disagree the benefits outweigh the risk and drush up will override the .htaccess.
Comment #67
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedPatch on #60 still applies cleanly.
Comment #68
mgiffordPatch from #60 re-uploaded for the bot.
Comment #70
DuneBLHere is the re-rolled #60 patch for 7.42
Comment #71
longwave#70 is malformed. #68 looks like a random fail, let's try that again.
Comment #73
mpdonadioReroll.
Pretty simple hand merge to add the new FilesMatch pattern for composer.
Comment #75
mpdonadioOK, I ran the fails from #68 and #73 locally w/ Apache 2.2 and all was OK. I also queued up a few successive runs, and they came back OK. We should check to see if there is a problem with spurious fails w/ 7.x. If this last run comes up OK, someone should do the manual testing, and then we should be able to set RTBC if all is well.
Comment #76
rooby CreditAttribution: rooby commentedYeah I also did a bit of investigation and came to the conclusion that I didn't think this patch was the cause of the fail.
Comment #77
miranche CreditAttribution: miranche commentedHeads up, Sabayon and Gentoo seem to have dropped support for mod_access_compat, so Drupal crashes on liftoff on a local apache 2.4 server due to this error.
Comment #80
J-LeeI have applied successfully patch #73 at 7.x-dev and 7.50.
The patched Drupal 7.50 works well on Apache 2.2 and Apache 2.4.
RTBC for me.
Comment #81
stefan.r CreditAttribution: stefan.r commentedComment #82
joelpittetThis applies with 2 lines of fuzz.
Comment #83
yareckon CreditAttribution: yareckon commentedAt least the main .htaccess patch must go in in the next release. Drupal has to work on apache for goodness sake.
If any more bugs come in on the autogenerated stuff, we can look at them, but this needs to go through.
Comment #84
yareckon CreditAttribution: yareckon commentedComment #85
Fabianx CreditAttribution: Fabianx as a volunteer commentedMarking for commit. I agree with this being critical.
We cannot port the tests, right?
Comment #86
Fabianx CreditAttribution: Fabianx as a volunteer commentedI am happy to commit this, but don't we need an upgrade path to re-write already existing .htaccess files, which are written dynamically to the public files directory - if they only match the old version?
Comment #87
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe patch looks good although unclear why it is moving those lines to the bottom of the .htaccess file? I think it would be cleaner if they stayed at the top where they currently are (and where the Drupal 8 patch left them - this should really just be a straight backport without any changes from what went into Drupal 8).
I think that could be left for a potential followup issue. If someone decides to deliberately change the Apache configuration/version to avoid using mod_access_compat for an existing site, it's reasonable that they might need to edit .htaccess files too. And we still haven't done an upgrade path for https://www.drupal.org/SA-CORE-2013-003 even :) (I remember it got difficult when looking into it in the private issue queue, and I think there's a public issue for it somewhere but I'm not sure.)
We could in theory, but we'd have to port the whole test. An overall backported test for file_create_htaccess() would definitely be useful, but I see little value in extending that to test this particular issue (it just comes down to asserting a particular set of lines exist in the .htaccess file, which is fragile and not actually testing much that's meaningful). So in my opinion we can skip it here.
Comment #90
stefan.r CreditAttribution: stefan.r commentedBackport of the Drupal 8 version (untested!) keeping those lines at the top
Comment #91
J-Lee#90 is nearly the same as #73. Applyed and working for me.
For me RTBC.
Comment #92
yareckon CreditAttribution: yareckon commentedWorks. Lets get this into the next release please.
Comment #93
yareckon CreditAttribution: yareckon for Ärzte ohne Grenzen e.V. / Médecins Sans Frontières commentedPinging release mgmt people!
Comment #94
mpdonadio#93, updating issues to get attention doesn't usually work. The committers have a big RTBC queue that they are working through.
Comment #95
yareckon CreditAttribution: yareckon for Ärzte ohne Grenzen e.V. / Médecins Sans Frontières commentedThere are only 3 RTBC criticals, and this is one of them. Cmon folks.
https://www.drupal.org/project/issues/drupal?text=&status=14&priorities=...
Comment #96
stefan.r CreditAttribution: stefan.r commentedComment #97
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe patch no longer applied, but it only required a trivial reroll, so I'm leaving this at RTBC. Everything looks good to me to get this in for the upcoming release.
While I was rerolling it, I fixed a minor issue - I put the
\n\n
that was there before back in (see attached interdiff) so that the spacing in the resulting .htaccess file is consistent throughout. I'll file a quick followup to do that for Drupal 8 too.I will also create a followup issue to consider adding an update function.
Comment #98
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #99
stefan.r CreditAttribution: stefan.r commentedUpdate function sounds good, but not blocking and the patch still looks OK, so marking for commit.
Comment #100
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!