You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@apache.org> on 2019/09/11 16:22:37 UTC
Redirect cycle detected for URL, if redirect adds a trailing slash
[[[
$ curl --head https://archive.apache.org/dist
HTTP/1.1 301 Moved Permanently
Location: https://archive.apache.org/dist/
...
$ 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'
]]]
Subversion "canonicalized" the redirect URL in its own way, and in doing
so removed the trailing slash, and then tried again and got the same
response.
I feel we should not really blame the server configuration. If the
server was intended for use by svn clients only, we could say it should
be changed to match what the Subversion client software expects, but
that server is aimed just as much at web browser users, not that this
trailing slash is important for them either. Anyhow, this kind of
redirect seems perfectly reasonable and it seems wrong that Subversion
does not work with it.
I suppose Subversion should not canonicalize a URL received in a
redirect, but should use it as given.
Does that make sense?
- Julian
Re: Redirect cycle detected for URL, if redirect adds a trailing
slash
Posted by Branko Čibej <br...@apache.org>.
On 13.09.2019 14:25, Julian Foad wrote:
> Branko Čibej wrote:
>> It looks correct to me, and the result you report against
>> archive.apache.org is correct, too. I'd say, go ahead and commit it,
>> we'll soon see if something goes wrong because of that.
>
> Ack. http://svn.apache.org/r1866899
>
>> The only thing I can think of is that we *require* that repository paths
>> of directories don't have a trailing slash, for example. However,
>> mod_dav_svn doesn't do 'foo/' -> 'foo' redirects, so this shouldn't be
>> an issue, IMO.
>
> I don't quite follow that, but am happy that you think it's ok.
Redirects can only come from Apache configuration outside the
<Location/> controlled by mod_dav_svn. So you won't get the kind of
'dir' -> 'dir/' redirect that archive.apache.org does -- which could
result in an invalid in-repo path -- for paths that are already within a
repository ... so with your patch we won't not canonicalize such paths.
Is that any clearer? :D
-- Brane
Re: Redirect cycle detected for URL, if redirect adds a trailing
slash
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
> It looks correct to me, and the result you report against
> archive.apache.org is correct, too. I'd say, go ahead and commit it,
> we'll soon see if something goes wrong because of that.
Ack. http://svn.apache.org/r1866899
> The only thing I can think of is that we *require* that repository paths
> of directories don't have a trailing slash, for example. However,
> mod_dav_svn doesn't do 'foo/' -> 'foo' redirects, so this shouldn't be
> an issue, IMO.
I don't quite follow that, but am happy that you think it's ok.
- Julian
Re: Redirect cycle detected for URL, if redirect adds a trailing
slash
Posted by Branko Čibej <br...@apache.org>.
On 13.09.2019 13:36, Julian Foad wrote:
> Branko Čibej wrote:
>>> It is a confusing error message, though. The redirect "cycle" is only
>>> Subversion that made it a cycle, and is not the real problem. We could
>>> do better.
>>
>> We could. Not canonicalizing the value of the Location header is an
>> option; if it's not a valid URL, that's the server admin's problem, not
>> ours (and as noted a number of times in other conversations, Subversion
>> is not a browser so we don't have to worry about producing user-friendly
>> URLs).
>
> The attached patch, 'serf-no-canonicalize-redirect-1.patch', stops
> canonicalizing the redirect, solving this problem.
>
> I have only tested that it fixes the non-repository case I reported,
> and verified that the existing test suite still passes. I haven't
> tested what difference it makes to any currently untested forms of
> redirect when accessing a repo.
>
> WDYT?
It looks correct to me, and the result you report against
archive.apache.org is correct, too. I'd say, go ahead and commit it,
we'll soon see if something goes wrong because of that.
The only thing I can think of is that we *require* that repository paths
of directories don't have a trailing slash, for example. However,
mod_dav_svn doesn't do 'foo/' -> 'foo' redirects, so this shouldn't be
an issue, IMO.
> ps. It's my favourite kind of patch: a code reduction/simplification.
Eventually (at t < ∞)we'll reach the point where there's no code at all
in subversion/ :)
-- Brane
Re: Redirect cycle detected for URL, if redirect adds a trailing
slash
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
>> It is a confusing error message, though. The redirect "cycle" is only
>> Subversion that made it a cycle, and is not the real problem. We could
>> do better.
>
> We could. Not canonicalizing the value of the Location header is an
> option; if it's not a valid URL, that's the server admin's problem, not
> ours (and as noted a number of times in other conversations, Subversion
> is not a browser so we don't have to worry about producing user-friendly
> URLs).
The attached patch, 'serf-no-canonicalize-redirect-1.patch', stops
canonicalizing the redirect, solving this problem.
I have only tested that it fixes the non-repository case I reported, and
verified that the existing test suite still passes. I haven't tested
what difference it makes to any currently untested forms of redirect
when accessing a repo.
WDYT?
ps. It's my favourite kind of patch: a code reduction/simplification.
- Julian
Re: Redirect cycle detected for URL, if redirect adds a trailing
slash
Posted by Branko Čibej <br...@apache.org>.
On 13.09.2019 11:04, Julian Foad wrote:
> Branko Čibej wrote:
>> On 11.09.2019 18:22, Julian Foad wrote:
>>> $ 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'
>>> [...] >>
>>> Does that make sense?
>>
>> It does not.
>>
>> [...] No DAV headers there, so it's not a Subversion repository and
>> not our
>> problem. Compare to:
>>
>> $ curl --request OPTIONS --include https://svn.apache.org/repos/asf
>
> Gah... I was comparing https://archive.apache.org/dist/ with
> https://dist.apache.org/repos/dist/ and somehow it did not occur to me
> that the former might not be a repository.
>
> Thanks for setting me straight!
>
> It is a confusing error message, though. The redirect "cycle" is only
> Subversion that made it a cycle, and is not the real problem. We could
> do better.
We could. Not canonicalizing the value of the Location header is an
option; if it's not a valid URL, that's the server admin's problem, not
ours (and as noted a number of times in other conversations, Subversion
is not a browser so we don't have to worry about producing user-friendly
URLs).
Note that JavaHL has its own redirect-cycle detection (this is
client-side stuff, not library-level stuff).
-- Brane
Re: Redirect cycle detected for URL, if redirect adds a trailing
slash
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
> On 11.09.2019 18:22, Julian Foad wrote:
>> $ 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'
>> [...] >>
>> Does that make sense?
>
> It does not.
>
> [...] No DAV headers there, so it's not a Subversion repository and not our
> problem. Compare to:
>
> $ curl --request OPTIONS --include https://svn.apache.org/repos/asf
Gah... I was comparing https://archive.apache.org/dist/ with
https://dist.apache.org/repos/dist/ and somehow it did not occur to me
that the former might not be a repository.
Thanks for setting me straight!
It is a confusing error message, though. The redirect "cycle" is only
Subversion that made it a cycle, and is not the real problem. We could
do better.
- Julian
Re: Redirect cycle detected for URL, if redirect adds a trailing
slash
Posted by Branko Čibej <br...@apache.org>.
On 11.09.2019 18:22, Julian Foad wrote:
> [[[
> $ curl --head https://archive.apache.org/dist
> HTTP/1.1 301 Moved Permanently
> Location: https://archive.apache.org/dist/
> ...
>
> $ 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'
> ]]]
>
> Subversion "canonicalized" the redirect URL in its own way, and in
> doing so removed the trailing slash, and then tried again and got the
> same response.
>
> I feel we should not really blame the server configuration. If the
> server was intended for use by svn clients only, we could say it
> should be changed to match what the Subversion client software
> expects, but that server is aimed just as much at web browser users,
> not that this trailing slash is important for them either. Anyhow,
> this kind of redirect seems perfectly reasonable and it seems wrong
> that Subversion does not work with it.
>
> I suppose Subversion should not canonicalize a URL received in a
> redirect, but should use it as given.
>
> Does that make sense?
It does not.
$ curl --request OPTIONS --include https://archive.apache.org/dist/
HTTP/1.1 200 OK
Date: Wed, 11 Sep 2019 20:11:40 GMT
Server: Apache/2.4.7 (Ubuntu)
Allow: GET,HEAD,POST,OPTIONS
Content-Length: 0
Content-Type: httpd/unix-directory
No DAV headers there, so it's not a Subversion repository and not our
problem. Compare to:
$ curl --request OPTIONS --include https://svn.apache.org/repos/asf
HTTP/1.1 200 OK
Date: Wed, 11 Sep 2019 20:14:12 GMT
Server: Apache/2.4.7 (Ubuntu)
DAV: 1,2
DAV: version-control,checkout,working-resource
DAV: merge,baseline,activity,version-controlled-collection
DAV: http://subversion.tigris.org/xmlns/dav/svn/depth
DAV: http://subversion.tigris.org/xmlns/dav/svn/log-revprops
DAV: http://subversion.tigris.org/xmlns/dav/svn/atomic-revprops
DAV: http://subversion.tigris.org/xmlns/dav/svn/partial-replay
DAV: http://subversion.tigris.org/xmlns/dav/svn/inherited-props
DAV: http://subversion.tigris.org/xmlns/dav/svn/inline-props
DAV: http://subversion.tigris.org/xmlns/dav/svn/reverse-file-revs
DAV: http://subversion.tigris.org/xmlns/dav/svn/mergeinfo
DAV: <http://apache.org/dav/propset/fs/1>
MS-Author-Via: DAV
Allow: OPTIONS,GET,HEAD,POST,DELETE,TRACE,PROPFIND,PROPPATCH,COPY,MOVE,LOCK,UNLOCK,CHECKOUT
Content-Length: 0