This is an archive of the discontinued Mercurial Phabricator instance.

httppeer: detect redirect to URL without query string (issue5860)
ClosedPublic

Authored by indygreg on Apr 30 2018, 7:31 PM.

Details

Summary

197d10e157ce subtly changed the HTTP peer's handling of HTTP redirects.

Before that changeset, we instantiated an HTTP peer instance and
performed the capabilities lookup with that instance. The old code had
the following relevant properties:

  1. The HTTP request layer would automatically follow HTTP redirects.
  2. An encountered HTTP redirect would update a peer instance variable pointing to the repo URL.
  3. The peer would automagically perform a "capabilities" command request if a caller requested capabilities but capabilities were not yet defined.

The first HTTP request issued by a peer is for ?cmd=capabilities. If
the server responds with an HTTP redirect to a ?cmd=capabilities URL,
the HTTP request layer automatically followed it, retrieved a valid
capabilities response, and the peer's base URL was updated
automatically so subsequent requests used the proper URL. In other
words, things "just worked."

In the case where the server redirected to a URL without the
?cmd=capabilities query string, the HTTP request layer would follow
the redirect and likely encounter HTML. The peer's base URL would be
updated and the unexpected Content-Type would raise a RepoError. We
would catch RepoError and immediately call between() (testing the case
for pre 0.9.1 servers not supporting the "capabilities" command). e.g.

try:
    inst._fetchcaps()
except error.RepoError:
    inst.between([(nullid, nullid)])

between() would eventually call into _callstream(). And _callstream()
made a call to self.capable('httpheader'). capable() would call
self.capabilities(), which would see that no capabilities were set
(because HTML was returned for that request) and call the "capabilities"
command to fetch capabilities. Because the base URL had been updated
from the redirect, this 2nd "capabilities" command would succeed and
the client would immediately call "between," which would also succeed.
The legacy handshake succeeded. Only because "capabilities" was
successfully executed as a side effect did the peer recognize that it
was talking to a modern server. In other words, this all appeared to
work accidentally.

After 197d10e157ce, we stopped calling the "capabilities" command on
the peer instance. Instead, we made the request via a low-level opener,
detected the redirect as part of response handling code, and passed the
redirected URL into the constructed peer instance.

For cases where the redirected URL included the query string, this
"just worked." But for cases where the redirected URL stripped the query
string, we threw RepoError and because we removed the "between" handshake
fallback, we fell through to the "is a static HTTP repo" check and
performed an HTTP request for .hg/requires.

While 197d10e157ce was marked as backwards incompatible, the only
intended backwards incompatible behavior was not performing the
"between" fallback. It was not realized that the "between" command
had the side-effect of recovering from an errant redirect that
dropped the query string.

This commit restores the previous behavior and allows clients to
handle a redirect that drops the query string. In the case where
the request is redirected and the query string is dropped, we raise
a special case of RepoError. We then catch this special exception in
the handshake code and perform another "capabilities" request against
the redirected URL. If that works, all is well. Otherwise, we fall back
to the "is a static HTTP repo" check.

The new could is arguably better than before 197d10e157ce, as it is
explicit about the expected behavior and we avoid performing a
"between" request, saving a server round trip.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Apr 30 2018, 7:31 PM

So it is documented somewhere, 197d10e157ce only regressed behavior for the "redirect drops query string case:" the new code handled HTTP redirects with the query string properly. Although it doesn't appear as if there were any tests for it. This commit adds tests for both the "good" and "bad" HTTP redirect cases.

This fixes the issue, thanks again. The only thing I noticed in practice is that the "real URL is xxx" notice is printed twice. The trace looks sane, (and the test captures this too), so I think it's purely cosmetic. The *.t test fails on Windows though:

--- e:/Projects/hg/tests/test-http-protocol.t
+++ e:/Projects/hg/tests/test-http-protocol.t.err
@@ -395,19 +395,42 @@
   s>     Content-Length: 10\r\n
   s>     \r\n
   s>     redirected
-  s>     GET /redirected?cmd=capabilities HTTP/1.1\r\n
-  s>     Accept-Encoding: identity\r\n
-  s>     user-agent: test\r\n
-  s>     host: $LOCALIP:$HGPORT\r\n (glob)
-  s>     \r\n
-  s> makefile('rb', None)
-  s>     HTTP/1.1 200 Script output follows\r\n
-  s>     Server: testing stub value\r\n
-  s>     Date: $HTTP_DATE$\r\n
-  s>     Content-Type: application/mercurial-0.1\r\n
-  s>     Content-Length: 453\r\n
-  s>     \r\n
-  s>     batch branchmap $USUAL_BUNDLE2_CAPS_SERVER$ changegroupsubset compression=$BUNDLE2_COMPRESSIONS$ get
bundle httpheader=1024 httpmediatype=0.1rx,0.1tx,0.2tx known lookup pushkey streamreqs=generaldelta,revlogv1 u
nbundle=HG10GZ,HG10BZ,HG10UN unbundlehash
+  ** Unknown exception encountered with possibly-broken third-party extension drawdag
+  ** which supports versions unknown of Mercurial.
+  ** Please disable drawdag and try your action again.
+  ** If that fixes the bug please report it to the extension author.
+  ** Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 20:42:59) [MSC v.1500 32 bit (Intel)]
+  ** Mercurial Distributed SCM (version 4.6rc1+5-80695628adcb)
+  ** Extensions loaded: drawdag
+  Traceback (most recent call last):
+    File "e:/Projects/hg/hg", line 41, in <module>
+      dispatch.run()
+    File "e:\Projects\hg\mercurial\dispatch.py", line 90, in run
+      status = (dispatch(req) or 0)
+    File "e:\Projects\hg\mercurial\dispatch.py", line 210, in dispatch
+      ret = _runcatch(req)
+    File "e:\Projects\hg\mercurial\dispatch.py", line 351, in _runcatch
+      return _callcatch(ui, _runcatchfunc)
+    File "e:\Projects\hg\mercurial\dispatch.py", line 359, in _callcatch
+      return scmutil.callcatch(ui, func)
+    File "e:\Projects\hg\mercurial\scmutil.py", line 160, in callcatch
+      return func()
+    File "e:\Projects\hg\mercurial\dispatch.py", line 341, in _runcatchfunc
+      return _dispatch(req)
+    File "e:\Projects\hg\mercurial\dispatch.py", line 971, in _dispatch
+      cmdpats, cmdoptions)
+    File "e:\Projects\hg\mercurial\dispatch.py", line 727, in runcommand
+      ret = _runcommand(ui, options, cmd, d)
+    File "e:\Projects\hg\mercurial\dispatch.py", line 979, in _runcommand
+      return cmdfunc()
+    File "e:\Projects\hg\mercurial\dispatch.py", line 968, in <lambda>
+      d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
+    File "e:\Projects\hg\mercurial\util.py", line 1553, in check
+      return func(*args, **kwargs)
+    File "e:\Projects\hg\mercurial\debugcommands.py", line 3091, in debugwireproto
+      e.read()
+  AttributeError: 'URLError' object has no attribute 'read'
+  [1]

I have no clue how this test would be failing on Windows but not on other platforms :/

I have no clue how this test would be failing on Windows but not on other platforms :/

Me either. But I didn't see it in the docs for this class, or when I ran dir(URLError). (I also tried on FreeBSD 11, where the test works.) So maybe this is being added in at runtime? While I'd like to see the tests green on stable, I'm more worried about if this is laying on other error paths for non-debug commands on Windows, that generally aren't exercised.

It looks like the redirect is subtly changing the host. I printed req.get_full_url() in keepalive.do_open(), and got:

keepalive: http://$LOCALIP:$HGPORT/redirector?cmd=capabilities
s>     GET /redirector?cmd=capabilities HTTP/1.1\r\n
s>     Accept-Encoding: identity\r\n
s>     user-agent: test\r\n
s>     host: $LOCALIP:$HGPORT\r\n (glob)
s>     \r\n
s> makefile('rb', None)
s>     HTTP/1.1 301 Redirect\r\n
s>     Server: testing stub value\r\n
s>     Date: $HTTP_DATE$\r\n
s>     Location: http://$LOCALIP:$HGPORT/redirected?cmd=capabilities\r\n (glob)
s>     Content-Type: text/plain\r\n
s>     Content-Length: 10\r\n
s>     \r\n
s>     redirected
keepalive: http://Envy:$HGPORT/redirected?cmd=capabilities

The subsequent httppeer test is failing with connection refused. This looks like as promising a lead as any. I wonder if this e.read() (and any others) needs to check that the attribute exists first, in case of failures without data available.

With these fixes, it works on Windows. I'm not sure if the advertisedbaseurl a couple lines above needs to be adjusted too.

I've got no idea why there seems to be a subtle difference between baseurl and advertisedbaseurl on Windows. Nor why what I think is the raw redirect header in the output seemingly correct with advertisedbaseurl. I did use baseurl in the LFS server though, because I think I saw this subtle difference.

tests/test-http-protocol.t
361

If this is changed to req.baseurl, it works.

375

$DAEMON_PIDS

475

$DAEMON_PIDS

indygreg planned changes to this revision.May 3 2018, 3:17 PM

Thanks for the info @mharbison72! It's really helpful. I'll try to get updated patches out in the next few hours.

FWIW it looks like this is the sole blocker to releasing 4.6. So if you could keep an eye out for updates and can verify patches when they are submitted, it would be appreciated.

indygreg added inline comments.May 3 2018, 3:34 PM
tests/test-http-protocol.t
361

The distinction between these URLs is that one uses the host:port as it actually is and the other uses the host:port as identified by the client. Since we are sending a URL back to the client, we should be using the advertised host:port.

The fact that the advertised host:port is being mangled seemingly points to a bug with the formulation of the advertised* variables. This is likely the result of code somewhere using gethostname() to populate a hostname field. I'm not sure if this is happening on the client or server. And I'm not sure why it would only be happening on Windows.

It is something to keep our eye on. But I think it is an issue in the test harness and not the hgweb code. We have unit test coverage for URL formulation in test-wsgirequest.py and I'm pretty confident it adheres to PEP 3333.

375

We run killdaemons.py above, so overwriting $DAEMON_PIDS should be fine.

FWIW, I have patches queued up to change killdaemons.py behavior so it removes the file by default. Will send those once the 3.7 window opens.

I'll probably be at work for another ~2 hours if you need me to test with SCM Manager, but I can stay later if needed.

tests/test-http-protocol.t
361

That makes sense. And test-wsgirequest.py does run on Windows.

What baffles me is that the header that's printed (e.g. line 393) is correct on Windows with either URL. So I have no idea where it could have divergent behavior.

Should I change the LFS behavior now, or punt because it's experimental?

indygreg updated this revision to Diff 8452.May 3 2018, 4:38 PM
indygreg added inline comments.May 3 2018, 4:40 PM
tests/test-http-protocol.t
361

You should consider changing it. But since it is experimental, it isn't critical. We can always do it in 4.6.1 if it is a problem in the wild.

Just to confirm that phabimport brought this in correctly, the only thing you changed is advertisedurl to baseurl? If that's the case, the LFS change will have to be punted.

I can confirm that 'test-http*' now runs on Windows, and both Windows and FreeBSD can now clone a repo+subrepo from SCM Manager with this change.

Thanks again for this.

Yes, I just changed the advertised URL bit in the test extension.

Queued with s/new could/new code/ in the commit message. Thanks!

This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.May 4 2018, 8:57 PM
yuja added inline comments.
mercurial/httppeer.py
894

Nit: baseurl=url seems not correct here since we've explicitly followed the redirect.

Anyway, this isn't a blocker as the baseurl (and safeurl) are used only for error
reporting.

mharbison72 added inline comments.May 4 2018, 11:33 PM
tests/test-http-protocol.t
361

The fact that the advertised host:port is being mangled seemingly points to a bug
with the formulation of the advertised* variables. This is likely the result of code
somewhere using gethostname() to populate a hostname field. I'm not sure if this
is happening on the client or server. And I'm not sure why it would only be
happening on Windows.

The plot thickens...

I stripped down the *.t, ran it with --keep, and then launched a non-daemon hg serve + client from the leftover repo, without the test harness. If I use baseurl, it succeeds as expected, using 127.0.0.1. If I use advertisedbaseurl, it redirects to the hostname, and the access log shows the external IP address. So it seems like more than a test harness issue. (Though in the test harness, this fails with a connection refused. But ProcessExplorer says it is listening on 0.0.0.0.)

There aren't too many uses of 'gethostname' (insensitive) in python 2.7.15, and none of them seem like an obvious problem. None of the hits in hg code are relevant.