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
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
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
-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 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
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 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 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 @@ pcommon structures for heartbeat modules (should this be public API?)/p /section + section id=ap_parse_htaccess +titleap_parse_htaccess (changed)/title +pThe function signature for codeap_parse_htaccess/code has been +changed. A codeapr_table_t/code of individual directives allowed +for override must now be passed (override remains)./p + /section + section id=http_config titlehttp_config (changed)/title ul 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 @@ directive type=section module=coreFiles/directive sections. /note -pWhen this directive is set to codeNone/code, then -a href=#accessfilename.htaccess/a files are completely ignored. -In this case, the server will not even attempt to read -code.htaccess/code files in the filesystem./p +pWhen this directive is set to codeNone/code and directive +module=coreAllowOverrideList/directive is set to +codeNone/code a href=#accessfilename.htaccess/a files are +completely ignored. In this case, the server will not even attempt +to read code.htaccess/code files in the filesystem./p pWhen this directive is set to codeAll/code, then any directive which has the .htaccess a @@ -442,7 +443,63 @@ /note /usage
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
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 drugg...@primary.net 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
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 drugg...@primary.net 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 @@ directive type=section module=coreFiles/directive sections. /note -pWhen this directive is set to codeNone/code, then +pWhen this directive is set to codeNone/code and directive +module=coreAllowOverrideList/directive is not set, then a href=#accessfilename.htaccess/a files are completely ignored. In this case, the server will not even attempt to read code.htaccess/code files in the filesystem./p @@ -442,7 +443,60 @@ /note /usage +directivesynopsis +nameAllowOverrideList/name +descriptionIndividual directives that are allowed in +code.htaccess/code files/description +syntaxAllowOverrideList directive1,directve2,.../syntax +contextlistcontextdirectory/context/contextlist + +usage +pWhen the server finds an code.htaccess/code file (as +specified by directive module=coreAccessFileName/directive) +it needs to know which directives declared in that file can override +earlier configuration directives./p + +notetitleOnly available in lt;Directorygt; sections/title +directiveAllowOverrideList/directive is valid only in +directive type=section module=coreDirectory/directive +sections specified without regular expressions, not in directive +type=section module=coreLocation/directive, directive +module=core type=sectionDirectoryMatch/directive or +directive type=section module=coreFiles/directive sections. +/note + +pWhen this directive is not set and directive module=core +AllowOverride/directive is set to codeNone/code, then +a href=#accessfilename.htaccess/a files are completely ignored. +In this case, the server will not even attempt to read +code.htaccess/code files in the filesystem./p + +pExample:/p + +example + AllowOverride None + AllowOverrideList Redirect,RedirectMatch +/example + +pIn the example above only the codeRedirect/code and +
