Meh... What a mess.
One part of the clean up: we should make very clear where a URL variable holds a 'canonical' URL (in this conversation, that means according to svn's own cannonicalisation rules) and where it does not. As our general rule is URLs in Subversion source code should be canonical, the way to make this clear is to label any non-canonical URL as such, by it's variable name, if it exists for more than about one line of code from where it's received to where it's validated/canonicalized
Sent with FairEmail [https://email.faircode.eu/] , an open source, privacy friendly email app for Android
30 Jan 2020 19:34:42 Stefan Sperling <stsp_at_elego.de>:
> I managed to trigger a client-side assertion failure today with a
> simple HTTP redirect. I have fixed this issue in r1873375 (see below).
>
> When I tried to merge this fix to 1.10.x I learned that this behaviour
> was intentionally changed already in r1866899. We used to canonicalize
> these URLs, but then we stopped doing so because we could end up
> misreporting an unrelated error condition (see below).
>
> My patch doesn't cleanly merge to 1.10.x because svn_uri_canonicalize_safe()
> does not yet exist there. Should we backport it to 1.10.x as a private
> function, or revert r1866899 from 1.10.x, or do we leave 1.10.x as is?
>
> To me it looks as if the consequences of r1866899 weren't fully considered.
> The server should not be able to trigger assertions in the client's libraries.
>
> Would svn_uri_canonicalize_safe() allow for detecting a redirect loop problem?
> My patch is not making use of the non-canonicalized version of the URL but
> this version remains available when svn_uri_canonicalize_safe() is used.
> Perhaps we could somehow take advantage of that to fix both problems?
>
> ------------------------------------------------------------------------
> r1873375 | stsp | 2020-01-30 20:14:49 +0100 (Thu, 30 Jan 2020) | 16 lines
>
> Canonicalize redirect URLs in ra_serf, rather than using them as-is.
> This prevents an assertion failure in the client if the server sends
> a redirect to a non-canonical URL.
>
> If Apache HTTPD uses a redirect statement such as this:
> Redirect permanent "/svn" https://svn.example.com/svn/
> then the redirect URL won't be canonical. For example, access to the path
> "/svn/trunk" will be redirected to https://svn.example.com/svn//trunk
>
> Note the double-slash which eventually triggers an assertion failure when
> the redirect URL gets checked at an API boundary outside of ra_serf.
>
> * subversion/libsvn_ra_serf/options.c
> (svn_ra_serf__exchange_capabilities): Treat redirect URLs as untrusted
> input and attempt to canonicalize them.
>
> ------------------------------------------------------------------------
> r1866899 | julianfoad | 2019-09-13 14:24:01 +0200 (Fri, 13 Sep 2019) | 26 lines
>
> When following an HTTP redirect, use the Location header URL exactly.
>
> Previously we canonicalized the redirect URL, which could lead to a redirect
> loop. Then Subversion would report a redirect loop as the error, potentially
> hiding a more interesting error such as when the target is not in fact a
> Subversion repository.
>
> A manual test case (on a non-repository):
> before:
> $ svn ls https://archive.apache.org/dist
> Redirecting to URL 'https://archive.apache.org/dist':
> Redirecting to URL 'https://archive.apache.org/dist':
> svn: E195019: Redirect cycle detected for URL 'https://archive.apache.org/dist'
>
> after:
> $ svn ls https://archive.apache.org/dist
> Redirecting to URL 'https://archive.apache.org/dist/':
> svn: E170013: Unable to connect to a repository at URL 'https://archive.apache.org/dist/'
> svn: E175003: The server at 'https://archive.apache.org/dist/' does not support the HTTP/DAV protocol
>
> * subversion/libsvn_ra_serf/options.c
> (svn_ra_serf__exchange_capabilities): Don't canonicalize the redirect URL.
>
> * subversion/libsvn_ra_serf/util.c
> (response_get_location): Don't canonicalize the redirect URL.
>
> ------------------------------------------------------------------------
>
Received on 2020-01-30 20:43:09 CET