Re: Question and request for comments on patch
On 7/26/2011 6:29 PM, Daniel Ruggeri wrote: > Both points taken and implemented. Regarding invalid directives, I set > it as a warning informing that the directive is being discarded. I never > actually tested apr_tables to see if they were case sensitive but had > assumed they were. The offending lines of code have been removed. Thank > you, guys. Finally committed as revision 1151654. ... not sure if this was one of the things holding up the beta, but I'm glad it's in before then :-) -- -- Daniel Ruggeri
Re: Question and request for comments on patch (now failure to commit)
On 7/27/2011 1:11 AM, Stefan Fritsch wrote: > On Wednesday 27 July 2011, Daniel Ruggeri wrote: >> That seems to have changed the error message, but no luck actually >> making things stick. I also tried removing the local source tree, >> pulling from https, applying the patch and trying again without >> luck. Not sure if there are remote logs I'd be allow to view, but >> I imagine I could self-diagnose if I had something more to go on. > > >> Sendingdocs/manual/developer/new_api_2_4.xml >> svn: Commit failed (details follow): >> svn: Server sent unexpected return value (403 Forbidden) in >> response to CHECKOUT request for >> '/repos/asf/!svn/ver/1137340/httpd/httpd/trunk/docs/manual/develope >> r/new_api_2_4.xml' # > > Maybe you are still missing some svn permissions (those are needed in > addition to the actual account). Wrowe, can you have a look, please? Not sure why he wasn't there... fixed now. Give it another try.
Re: Question and request for comments on patch (now failure to commit)
On Wednesday 27 July 2011, Daniel Ruggeri wrote: > That seems to have changed the error message, but no luck actually > making things stick. I also tried removing the local source tree, > pulling from https, applying the patch and trying again without > luck. Not sure if there are remote logs I'd be allow to view, but > I imagine I could self-diagnose if I had something more to go on. > Sendingdocs/manual/developer/new_api_2_4.xml > svn: Commit failed (details follow): > svn: Server sent unexpected return value (403 Forbidden) in > response to CHECKOUT request for > '/repos/asf/!svn/ver/1137340/httpd/httpd/trunk/docs/manual/develope > r/new_api_2_4.xml' # Maybe you are still missing some svn permissions (those are needed in addition to the actual account). Wrowe, can you have a look, please?
Re: Question and request for comments on patch (now failure to commit)
On 7/26/2011 9:05 PM, Eric Covener wrote: > you have to check out as, or switch to, https to commit. > > I believe "svn switch --relocate http://... https://..."; is close. That seems to have changed the error message, but no luck actually making things stick. I also tried removing the local source tree, pulling from https, applying the patch and trying again without luck. Not sure if there are remote logs I'd be allow to view, but I imagine I could self-diagnose if I had something more to go on. # svn checkout https://svn.apache.org/repos/asf/httpd/httpd/trunk httpd-trunk ... # cd httpd-trunk # patch -p1 < ../httpd-trunk.AllowOverrideList.patch ... # svn changelist httpd-trunk.AllowOverrideList docs/manual/developer/new_api_2_4.xml docs/manual/mod/core.xml server/config.c server/core.c server/request.c include/http_config.h include/ap_mmn.h include/httpd.h include/http_core.h ... # svn commit --username druggeri --password REDACTED --message "Add AllowOverrideList directive and documentation" --changelist httpd-trunk.AllowOverrideList Sendingdocs/manual/developer/new_api_2_4.xml svn: Commit failed (details follow): svn: Server sent unexpected return value (403 Forbidden) in response to CHECKOUT request for '/repos/asf/!svn/ver/1137340/httpd/httpd/trunk/docs/manual/developer/new_api_2_4.xml' # -- -- Daniel Ruggeri
Re: Question and request for comments on patch
> > I attempted to commit this, but ran into the following error. This is my > first try to commit, so I very well may be doing something wrong. I'm > attempting from my machine here at home rather than minotaur. Any > suggestions? > > svn checkout http://svn.apache.org/repos/asf/httpd/httpd/trunk you have to check out as, or switch to, https to commit. I believe "svn switch --relocate http://... https://..."; is close.
Re: Question and request for comments on patch
On 7/26/2011 5:02 AM, Stefan Fritsch wrote: > This is valid use-case which justifies making it available somehow. > But actually, I think something like > > module *mod = &ap_top_module; > ap_find_command_in_modules(cmd, mod); > > should already work without the need for an additional API. This way > is not very efficient but as this is only done on server startup, > that's not important. On 7/26/2011 5:35 AM, "Plüm, Rüdiger, VF-Group" wrote: > IMHO the keys in apr_tables are case insensitive. So this tolower shouldn't > be needed. Both points taken and implemented. Regarding invalid directives, I set it as a warning informing that the directive is being discarded. I never actually tested apr_tables to see if they were case sensitive but had assumed they were. The offending lines of code have been removed. Thank you, guys. I attempted to commit this, but ran into the following error. This is my first try to commit, so I very well may be doing something wrong. I'm attempting from my machine here at home rather than minotaur. Any suggestions? svn checkout http://svn.apache.org/repos/asf/httpd/httpd/trunk httpd-trunk && cd http-trunk svn changelist httpd-trunk.AllowOverrideList docs/manual/developer/new_api_2_4.xml docs/manual/mod/core.xml server/config.c server/core.c server/request.c include/http_config.h include/ap_mmn.h include/httpd.h include/http_core.h ... svn commit --username druggeri --password REDACTED --message "Add AllowOverrideList directive and documentation" --changelist httpd-trunk.AllowOverrideList svn: Commit failed (details follow): svn: Server sent unexpected return value (403 Forbidden) in response to MKACTIVITY request for '/repos/asf/!svn/act/2f9005f7-2cd2-4c3f-9311-77bdcf4322e2' User credentials to svn.apache.org web interface and people.apache.org work fine so I suspect there's more to the equation. -- -- Daniel Ruggeri
RE: Question and request for comments on patch
> -Original Message- > From: Daniel Ruggeri > Sent: Montag, 25. Juli 2011 01:36 > To: dev@httpd.apache.org > Subject: Re: Question and request for comments on patch > and unavailable to core.c where the configuration directive is > implemented? If I'm off, I'd definitely like to add this as well as > remove the need for the 'tolower' calls by looking up the expected > CamelCase of a directive as it shows up in config.c. IMHO the keys in apr_tables are case insensitive. So this tolower shouldn't be needed. Regards Rüdiger
Re: Question and request for comments on patch
On Monday 25 July 2011, Daniel Ruggeri wrote: > On 7/24/2011 2:12 AM, Stefan Fritsch wrote: > > When parsing the list, look in ap_config_hash if a directive > > exists. If not, either log a warning or error out (not sure > > which is better). > > I may be missing something, but isn't ap_config_hash private to > config.c and unavailable to core.c where the configuration > directive is implemented? If I'm off, I'd definitely like to add > this as well as remove the need for the 'tolower' calls by looking > up the expected CamelCase of a directive as it shows up in > config.c. This is valid use-case which justifies making it available somehow. But actually, I think something like module *mod = &ap_top_module; ap_find_command_in_modules(cmd, mod); should already work without the need for an additional API. This way is not very efficient but as this is only done on server startup, that's not important. > > It should be possible to reset AllowOverrideList to an empty list > > in a subdir, even if it is set in the parent dir. This is > > important in the case that there is a permissive > > AllowOverrideList in main server scope and an empty one for some > > virtual hosts. In the case of an empty list, d->override_list > > should be set to NULL instead of an empty table for better > > performance. Maybe one could allow "disabled" or "reset" as > > alias for an empty list (like in UserDir/DirectoryIndex). > > I realized I did not add this about five minutes after sending the > patch along to the list. I have added this also. Your followup > email about making the list NULL complicating things is also true > - I had a lot of trouble merging properly when NULL was considered > "nothing in the list" as opposed to "not configured". htaccess parsing is notoriously slow anyway. One more table lookup won't hurt that much, so IMHO no need to complicate things here. > > It may be possible that some modules react badly if a directive > > is used in .htaccess that has previously not been allowed there. > > For example if the module needs to do some global initialization > > when a directive is used for the first time. I am not aware of > > such a case, but it is something we may want to keep in mind if > > this is backported. And it is definitely something that should > > go into the new_api_2.4 document. > > Yes, very good point - I did not realize this would become a side > effect. I thought there were upstream context checks before a > command is invoked to prevent this. > I have taken the second suggestion and added a note in > new_api_2.4.xml also. Maybe we want to extend ap_check_cmd_context() to have something like NOT_IN_HTACCESS. In any case, I think your patch is good enough for the next beta. We can solve any remaining issues before GA. Cheers, Stefan
Re: Question and request for comments on patch
On 7/24/2011 2:12 AM, Stefan Fritsch wrote: > On Friday 22 July 2011, Daniel Ruggeri wrote: >> Attached is the final cut of the patch including doco and MMN bump >> as you brought up. I plan to commit this on Monday, time >> permitting (and of course in the absence of objections). I'll >> cobble something together afterwards for a 2.2 backport. > +1 to the principle. This also allows to use LogLevel in .htaccess, > which I wanted to implement but never got around to. > > Some thoughts / nitpicks: > style: Include a space between an if and the opening brace. Done - looks like I only did this in a few spots some how. > We need to support C90: > core.c: In function 'set_override_list': > core.c:1626:5: error: ISO C90 forbids mixed declarations and code [- > Werror=declaration-after-statement] Done also > > When parsing the list, look in ap_config_hash if a directive exists. > If not, either log a warning or error out (not sure which is better). I may be missing something, but isn't ap_config_hash private to config.c and unavailable to core.c where the configuration directive is implemented? If I'm off, I'd definitely like to add this as well as remove the need for the 'tolower' calls by looking up the expected CamelCase of a directive as it shows up in config.c. > I think the word1,word2,... syntax is unwieldly, especially if the > list gets long. Maybe use AP_INIT_TAKE_ARGV instead (see e.g. > AllowMethods). This is far from a nitpick and makes a lot more sense. I have implemented this as well. > It should be possible to reset AllowOverrideList to an empty list in a > subdir, even if it is set in the parent dir. This is important in the > case that there is a permissive AllowOverrideList in main server scope > and an empty one for some virtual hosts. In the case of an empty > list, d->override_list should be set to NULL instead of an empty table > for better performance. Maybe one could allow "disabled" or "reset" > as alias for an empty list (like in UserDir/DirectoryIndex). I realized I did not add this about five minutes after sending the patch along to the list. I have added this also. Your followup email about making the list NULL complicating things is also true - I had a lot of trouble merging properly when NULL was considered "nothing in the list" as opposed to "not configured". > It may be possible that some modules react badly if a directive is > used in .htaccess that has previously not been allowed there. For > example if the module needs to do some global initialization when a > directive is used for the first time. I am not aware of such a case, > but it is something we may want to keep in mind if this is backported. > And it is definitely something that should go into the new_api_2.4 > document. Yes, very good point - I did not realize this would become a side effect. I thought there were upstream context checks before a command is invoked to prevent this. I have taken the second suggestion and added a note in new_api_2.4.xml also. Thank you for the review. Much appreciated. Attached is the patch including the suggested changes except the directive look up. -- -- Daniel Ruggeri Index: httpd-trunk.AllowOverrideList/docs/manual/developer/new_api_2_4.xml === --- httpd-trunk.AllowOverrideList/docs/manual/developer/new_api_2_4.xml (revision 1150484) +++ httpd-trunk.AllowOverrideList/docs/manual/developer/new_api_2_4.xml (working copy) @@ -92,6 +92,13 @@ common structures for heartbeat modules (should this be public API?) + +ap_parse_htaccess (changed) +The function signature for ap_parse_htaccess has been +changed. A apr_table_t of individual directives allowed +for override must now be passed (override remains). + + http_config (changed) Index: httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml === --- httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml (revision 1150484) +++ httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml (working copy) @@ -327,10 +327,11 @@ Files sections. -When this directive is set to None, then -.htaccess files are completely ignored. -In this case, the server will not even attempt to read -.htaccess files in the filesystem. +When this directive is set to None and AllowOverrideList is set to +None .htaccess files are +completely ignored. In this case, the server will not even attempt +to read .htaccess files in the filesystem. When this directive is set to All, then any directive which has the .htaccess + +AllowOverrideList +Individual directives that are allowed in +.htaccess files +AllowOverrideList None|directive +[directive-type] ... +AllowOverrideList None +directory + + +When the server finds an .htaccess file (as +specified by AccessFileName) +it needs to know which directives de
Re: Question and request for comments on patch
On Sun, 24 Jul 2011, Stefan Fritsch wrote: and an empty one for some virtual hosts. In the case of an empty list, d->override_list should be set to NULL instead of an empty table for better performance. Well, this would require some additional logic for the config merge. Maybe it's not worth the effort...
Re: Question and request for comments on patch
On Friday 22 July 2011, Daniel Ruggeri wrote: > Attached is the final cut of the patch including doco and MMN bump > as you brought up. I plan to commit this on Monday, time > permitting (and of course in the absence of objections). I'll > cobble something together afterwards for a 2.2 backport. +1 to the principle. This also allows to use LogLevel in .htaccess, which I wanted to implement but never got around to. Some thoughts / nitpicks: style: Include a space between an if and the opening brace. We neee to support C90: core.c: In function 'set_override_list': core.c:1626:5: error: ISO C90 forbids mixed declarations and code [- Werror=declaration-after-statement] When parsing the list, look in ap_config_hash if a directive exists. If not, either log a warning or error out (not sure which is better). I think the word1,word2,... syntax is unwieldly, especially if the list gets long. Maybe use AP_INIT_TAKE_ARGV instead (see e.g. AllowMethods). It should be possible to reset AllowOverrideList to an empty list in a subdir, even if it is set in the parent dir. This is important in the case that there is a permissive AllowOverrideList in main server scope and an empty one for some virtual hosts. In the case of an empty list, d->override_list should be set to NULL instead of an empty table for better performance. Maybe one could allow "disabled" or "reset" as alias for an empty list (like in UserDir/DirectoryIndex). It may be possible that some modules react badly if a directive is used in .htaccess that has previously not been allowed there. For example if the module needs to do some global initialization when a directive is used for the first time. I am not aware of such a case, but it is something we may want to keep in mind if this is backported. And it is definitely something that should go into the new_api_2.4 document.
Re: Question and request for comments on patch
On Jul 19, 2011, at 7:44 PM, Daniel Ruggeri wrote: > All; > I am attaching a patch that will allow for a comma separated list of > directives permissable for override. This is something that has been requested by folks on the users mailing list, and on IRC, as far back as I've been watching. Seems like a great improvement. -- Rich Bowen rbo...@rcbowen.com rbo...@apache.org
Re: Question and request for comments on patch
On 7/21/2011 3:32 AM, Igor Galić wrote: > I think you're missing an MMN bump, regarding backporting - or API in > general, the wrapper is the right way to go. > Also: Why not patch against trunk? > > i > > Daniel Ruggeri wrote: > >> All; >> I am attaching a patch that will allow for a comma separated list of >> directives permissable for override. I am doing this because the >> existing AllowOverride list of override-able directives sometimes has >> too many things, sometimes allows more than is documented and it also >> forces third party modules into one of the four predefined lists. With >> this patch, an AllowOverrideList directive is added for the server admin >> to specify individual directives for override. No other functionality >> changes. FWIW, In my particular use case, I would like to grant a >> content admin the ability to implement a server-side redirect in >> .htacess, but would NOT like to grant them the ability to stand up a >> proxy via "RewriteRule / http://someOtherLocation/ [P]" (which is what >> would happen if FileInfo was set in AllowOverride). Note how Redirect >> and friends are not documented as directives in core.html#allowoverride >> for FileInfo... another patch, another day :) . >> >> >> With that in mind, I'd like to request comments on the idea and the >> implementation supplied. I can see that there could be efficiency gained >> without having to "tolower" each directive before comparison in >> invoke_cmd, but I could not find a way outside of server/config.c >> (server/core.c constructs the list I am using) to look up a directive's >> normal case like ReDiREcT becomes "Redirect" before the call to >> invoke_cmd as a result of ml = apr_hash_get(ap_config_hash, dir, >> APR_HASH_KEY_STRING);. >> >> >> The second thing I'd like to bring up is that I see a lot of use in >> backporting this, but I have made a change that I think would be >> considered an API change (added param to ap_parse_htaccess in >> include/http_config.h). Since I know this is not allowed, would it be >> acceptable to rename the new ap_parse_htaccess function and implement a >> wrapper as ap_parse_htaccess? I would foresee that such a wrapper would >> issue a deprecation warning when called, but will call ap_parse_htaccess >> with a NULL in place of the (new) override_list. >> >> cheers! >> -- >> -- >> Daniel Ruggeri Attached is the final cut of the patch including doco and MMN bump as you brought up. I plan to commit this on Monday, time permitting (and of course in the absence of objections). I'll cobble something together afterwards for a 2.2 backport. Regarding the question, Igor, there was not much thought put into which source tree I started hacking on - I wanted to get something together before this weekend hit and already had this version configured/built. -- -- Daniel Ruggeri Index: httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml === --- httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml (revision 1149429) +++ httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml (working copy) @@ -327,7 +327,8 @@ Files sections. -When this directive is set to None, then +When this directive is set to None and AllowOverrideList is not set, then .htaccess files are completely ignored. In this case, the server will not even attempt to read .htaccess files in the filesystem. @@ -442,7 +443,60 @@ + +AllowOverrideList +Individual directives that are allowed in +.htaccess files +AllowOverrideList directive1,directve2,... +directory + + +When the server finds an .htaccess file (as +specified by AccessFileName) +it needs to know which directives declared in that file can override +earlier configuration directives. + +Only available insections +AllowOverrideList is valid only in +Directory +sections specified without regular expressions, not in Location, DirectoryMatch or +Files sections. + + +When this directive is not set and +AllowOverride is set to None, then +.htaccess files are completely ignored. +In this case, the server will not even attempt to read +.htaccess files in the filesystem. + +Example: + + + AllowOverride None + AllowOverrideList "Redirect,RedirectMatch" + + +In the example above only the Redirect and +RedirectMatch directives are allowed. All others will +cause an internal server error. + + + AllowOverride AuthConfig + AllowOverrideList "CookieTracking,CookieName" + + +In the example above AllowOverride + grants permission to the AuthConfig +directive grouping and AllowOverrideList grants +permission to only two directves from the FileInfo directive +grouping. All others will cause an internal server error. + + + AccessFileName +AllowOverride Configuration Files .htaccess Files Index: httpd-trunk.AllowOverrideL
RE: Question and request for comments on patch
I think you're missing an MMN bump, regarding backporting - or API in general, the wrapper is the right way to go. Also: Why not patch against trunk? i Daniel Ruggeri wrote: >All; > I am attaching a patch that will allow for a comma separated list of >directives permissable for override. I am doing this because the >existing AllowOverride list of override-able directives sometimes has >too many things, sometimes allows more than is documented and it also >forces third party modules into one of the four predefined lists. With >this patch, an AllowOverrideList directive is added for the server admin >to specify individual directives for override. No other functionality >changes. FWIW, In my particular use case, I would like to grant a >content admin the ability to implement a server-side redirect in >.htacess, but would NOT like to grant them the ability to stand up a >proxy via "RewriteRule / http://someOtherLocation/ [P]" (which is what >would happen if FileInfo was set in AllowOverride). Note how Redirect >and friends are not documented as directives in core.html#allowoverride >for FileInfo... another patch, another day :) . > > > With that in mind, I'd like to request comments on the idea and the >implementation supplied. I can see that there could be efficiency gained >without having to "tolower" each directive before comparison in >invoke_cmd, but I could not find a way outside of server/config.c >(server/core.c constructs the list I am using) to look up a directive's >normal case like ReDiREcT becomes "Redirect" before the call to >invoke_cmd as a result of ml = apr_hash_get(ap_config_hash, dir, >APR_HASH_KEY_STRING);. > > > The second thing I'd like to bring up is that I see a lot of use in >backporting this, but I have made a change that I think would be >considered an API change (added param to ap_parse_htaccess in >include/http_config.h). Since I know this is not allowed, would it be >acceptable to rename the new ap_parse_htaccess function and implement a >wrapper as ap_parse_htaccess? I would foresee that such a wrapper would >issue a deprecation warning when called, but will call ap_parse_htaccess >with a NULL in place of the (new) override_list. > >cheers! >-- >-- >Daniel Ruggeri
