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:34:41 CET