Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c
On 19 Jan 2022, at 09:40, Ruediger Pluem wrote: >> @@ -723,6 +737,57 @@ static int dav_get_overwrite(request_rec >> return -1; >> } >> >> +static int uripath_is_canonical(const char *uripath) > > Isn't this a filesystem path we are talking about in the case? > How does this function work on Windows when people might use '\' instead of > '/'? This comes from svn at https://github.com/apache/subversion/blob/0f4de5d963ce3b43084f66efab0901be15d2eec2/subversion/libsvn_subr/dirent_uri.c#L1856 If you’ve gone through the expression parser you need a sanity check to verify you’re still canonical. You don;t need to canonicalise, as you;ve done this already. You would need to do the same step for directories, but canonicalise to a file path. I am going to look at this again, as you extra-parameter-to-dav is a lot cleaner. Regards, Graham —
Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c
On 1/17/22 5:10 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Mon Jan 17 16:10:51 2022
> New Revision: 1897156
>
> URL: http://svn.apache.org/viewvc?rev=1897156=rev
> Log:
> core: Allow an optional expression to be specified for an effective
> path in the DirectoryMatch and LocationMatch directives. This allows
> modules like mod_dav to map URLs to URL spaces or to directories on
> the filesystem.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/mod/core.xml
> httpd/httpd/trunk/modules/dav/main/mod_dav.c
> httpd/httpd/trunk/server/core.c
>
avior in the filesystem
>
> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.c?rev=1897156=1897155=1897156=diff
> ==
> --- httpd/httpd/trunk/modules/dav/main/mod_dav.c (original)
> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.c Mon Jan 17 16:10:51 2022
>
> @@ -723,6 +737,57 @@ static int dav_get_overwrite(request_rec
> return -1;
> }
>
> +static int uripath_is_canonical(const char *uripath)
Isn't this a filesystem path we are talking about in the case?
How does this function work on Windows when people might use '\' instead of '/'?
> +{
> +const char *dot_pos, *ptr = uripath;
> +apr_size_t i, len;
> +unsigned pattern = 0;
> +
> +/* URIPATH is canonical if it has:
> + * - no '.' segments
> + * - no closing '/'
> + * - no '//'
> + */
> +
> +if (ptr[0] == '.'
> +&& (ptr[1] == '/' || ptr[1] == '\0'
> +|| (ptr[1] == '.' && (ptr[2] == '/' || ptr[2] ==
> '\0' {
> +return 0;
> +}
> +
> +/* valid special cases */
> +len = strlen(ptr);
> +if (len < 2) {
Empty pathes ("") are ok?
> +return 1;
> +}
> +
> +/* invalid endings */
> +if (ptr[len - 1] == '/' || (ptr[len - 1] == '.' && ptr[len - 2] == '/'))
> {
> +return 0;
> +}
> +
> +/* '.' are rare. So, search for them globally. There will often be no
> + * more than one hit. Also note that we already checked for invalid
> + * starts and endings, i.e. we only need to check for "/./"
Why don't we need to look for /../ segments?
> + */
> +for (dot_pos = memchr(ptr, '.', len); dot_pos;
> +dot_pos = strchr(dot_pos + 1, '.')) {
> +if (dot_pos > ptr && dot_pos[-1] == '/' && dot_pos[1] == '/') {
> +return 0;
> +}
> +}
> +
> +/* Now validate the rest of the path. */
> +for (i = 0; i < len - 1; ++i) {
> +pattern = ((pattern & 0xff) << 8) + (unsigned char) ptr[i];
> +if (pattern == 0x101 * (unsigned char) ('/')) {
> +return 0;
> +}
> +}
> +
> +return 1;
> +}
> +
> /* resolve a request URI to a resource descriptor.
> *
> * If label_allowed != 0, then allow the request target to be altered by
Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c
On Mon, Jan 17, 2022 at 5:10 PM wrote:
>
> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.c?rev=1897156=1897155=1897156=diff
> ==
> --- httpd/httpd/trunk/modules/dav/main/mod_dav.c (original)
> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.c Mon Jan 17 16:10:51 2022
> @@ -723,6 +737,57 @@ static int dav_get_overwrite(request_rec
> return -1;
> }
>
> +static int uripath_is_canonical(const char *uripath)
Maybe reuse ap_normalize_path() for the implementation, like:
static int uripath_is_canonical(apr_pool_t *p, const char *uripath)
{
char *path = apr_pstrdup(uripath);
return (ap_normalize_path(path, (AP_NORMALIZE_MERGE_SLASHES |
AP_NORMALIZE_NOT_ABOVE_ROOT))
&& strcmp(uri_path, path) == 0);
}
?
Regards;
Yann.
Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c
On 18 Jan 2022, at 12:31, Ruediger Pluem wrote: > Not sure I get the intention correct, but this looks like a change to core > for a mod_dav specific short coming. It's not specific to mod_dav, no. I created a patch ages ago for mod_dav_svn that depended on this patch and this got buried under other things, and I have other modules that need the same thing: http://svn.apache.org/viewvc/subversion/branches/mod-dav-svn-expressions/ At the core of the problem is for many modules to work, they need to know the “root” of the URL space, so that a PATH_INFO can be accurately calculated. As soon as you’re in a LocationMatch or DirectoryMatch the root of the URL space is replaced with the regex, and you’re lost. > e.g. in mod_proxy similar problems are solved for ProxyPass that can be used > in and outside Directory / Location elements. The ProxyPass was originally outside of Directory / Location only, I added the option of the one parameter version of ProxyPass that inherited from the Directory / Location, but this (as I recall) doesn't work for regexes. It would be great if the two parameter ProxyPass can go away in future to simplify. > I would be in favor for a similar approach here, by either allowing a second > optional parameter to the 'DAV' directive that sets > this or for a separate directive that allows to set this. In case nothing is > set the cmd->path could be used as currently. > This also allows to use this feature in 'if' sections as well. > Another option would be to provide this information via an environment > variable that can set via SetEnvIfExpr or a RewriteRule. H… Adding an extra parameter to DAV, and then to something generic like SetHandler may do the trick. This gives most modules access to a sane path when used inside a Match. Let me think about this for a bit. Regards, Graham —
Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c
On 1/17/22 5:10 PM, minf...@apache.org wrote: > Author: minfrin > Date: Mon Jan 17 16:10:51 2022 > New Revision: 1897156 > > URL: http://svn.apache.org/viewvc?rev=1897156=rev > Log: > core: Allow an optional expression to be specified for an effective > path in the DirectoryMatch and LocationMatch directives. This allows > modules like mod_dav to map URLs to URL spaces or to directories on > the filesystem. > > Modified: > httpd/httpd/trunk/CHANGES > httpd/httpd/trunk/docs/log-message-tags/next-number > httpd/httpd/trunk/docs/manual/mod/core.xml > httpd/httpd/trunk/modules/dav/main/mod_dav.c > httpd/httpd/trunk/server/core.c > Not sure I get the intention correct, but this looks like a change to core for a mod_dav specific short coming. e.g. in mod_proxy similar problems are solved for ProxyPass that can be used in and outside Directory / Location elements. I would be in favor for a similar approach here, by either allowing a second optional parameter to the 'DAV' directive that sets this or for a separate directive that allows to set this. In case nothing is set the cmd->path could be used as currently. This also allows to use this feature in 'if' sections as well. Another option would be to provide this information via an environment variable that can set via SetEnvIfExpr or a RewriteRule. Regards Rüdiger
Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c
On Mon, Jan 17, 2022 at 04:10:51PM -, minf...@apache.org wrote:
> Author: minfrin
> Date: Mon Jan 17 16:10:51 2022
> New Revision: 1897156
>
> URL: http://svn.apache.org/viewvc?rev=1897156=rev
> Log:
> core: Allow an optional expression to be specified for an effective
> path in the DirectoryMatch and LocationMatch directives. This allows
> modules like mod_dav to map URLs to URL spaces or to directories on
> the filesystem.
https://app.travis-ci.com/github/apache/httpd/jobs/555883817#L2039
This has introduced new warnings:
In file included from mod_dav.c:51:
mod_dav.c: In function ‘uripath_is_canonical’:
mod_dav.c:774:38: error: passing argument 1 of ‘ap_strchr’ discards ‘const’
qualifier from pointer target type [-Werror=discarded-qualifiers]
774 | dot_pos = strchr(dot_pos + 1, '.')) {
| ^~~
/home/travis/build/apache/httpd/include/httpd.h:2469:34: note: in definition of
macro ‘strchr’
2469 | # define strchr(s, c) ap_strchr(s,c)
| ^
/home/travis/build/apache/httpd/include/httpd.h:2457:36: note: expected ‘char
*’ but argument is of type ‘const char *’
2457 | AP_DECLARE(char *) ap_strchr(char *s, int c);
| ~~^
