Opened 13 years ago

Closed 10 years ago

#113 closed defect (fixed)

URLs not calculated correctly behind a proxy

Reported by: dobes Owned by:
Priority: major Milestone: 0.8.2
Version: 0.7.6 Keywords: web
Cc: exarkun

Description

I've just set up the buildbot behind an apache proxy, like this:

<Location "/buildbot/">
AuthType Basic
AuthName "Buildbot Admin"
AuthUserFile /srv/buildbot/htpasswd
Require valid-user
</Location>

ProxyPass /buildbot/ http://localhost:8001/
ProxyPassReverse /buildbot/ http://localhost:8001/

I've discovered that if I browse to http://server/buildbot/, many of the links are invalid. However, browsing to http://server/buildbot/waterfall will work.

For http://server/buildbot/, I get broken links like:

<td align="center" class="Builder"><a href="../builders/comment">comment</a></td>

For http://server/buildbot/waterfall, the HTML has elements like:

<td align="center" class="Builder"><a href="waterfall/../builders/comment">comment</a></td>

Clearly there is a bug in the way buildbot is calculating its URIs; neither of these hrefs looks correct to me, although the second one does work.

Change History (15)

comment:1 Changed 13 years ago by dobes

Upon further investigation, I see that this problem applies only to the deprecated Waterfall display; WebStatus? doesn't use the root URL for a waterfall display. May I suggest an auto-redirect for Waterfall, instead of displaying the same resource as /waterfall?

E.g., if you're using the Waterfall class instead of WebStatus?, it should send a redirect to the web browser to the correct waterfall URI instead of using the broken behavior it has currently.

comment:2 Changed 13 years ago by warner

  • Milestone changed from undecided to 0.7.7

comment:3 Changed 13 years ago by warner

Yeah, I was uncertain about doing that "/" -> "/waterfall" redirect, because my reading of RFC2616 (the HTTP specification) says that redirects are supposed to be absolute URIs, and of course any absolute URI from behind a reverse proxy is going to be wrong. So I punted for 0.7.6 and made "/" be a copy of "/waterfall", but then some of the URLs are broken.

I suppose we need to keep Waterfall around for another release or so, it's deprecated in 0.7.7 but it's been around for so long, it's only fair to give people a chance to switch over. Do you think an out-of-spec relative redirect URI is likely to cause fewer problems than the current issues? Or is it enough to point folks with this problem towards the modern WebStatus? class?

comment:4 Changed 13 years ago by warner

so, it didn't occur to me earlier that we could use c['buildbotURL'] to get an absolute URL of the /waterfall page, and just require that anyone who's using a proxy and still using the deprecated Waterfall make sure that they get the buildbotURL right. This would use an absolute redirect, in keeping with the HTTP spec.

I'll try to implement that one tomorrow and see how it looks.

comment:5 Changed 13 years ago by exarkun

There are a lot of other redirects in the web code. For example, after posting to the force build form, a redirect to "../.." is generated. This is clearly not an absolute URL either.

A trivial grep reveals a number of things which probably need to be fixed:

./buildbot/status/web/build.py:        #return Redirect("../%d" % self.build.getNumber())
./buildbot/status/web/build.py:        r = Redirect("../../..") # TODO: no longer correct
./buildbot/status/web/build.py:        #r = Redirect("../../../..") # this takes us back to the welcome page
./buildbot/status/web/build.py:        #r = Redirect("../../../../waterfall") # or the Waterfall
./buildbot/status/web/build.py:        #r = Redirect("../../../../waterfall?show=%s" % builder_name)
./buildbot/status/web/build.py:        r = Redirect("../..") # the Builder's page
./buildbot/status/web/builder.py:            return Redirect("..")
./buildbot/status/web/builder.py:            return Redirect("..")
./buildbot/status/web/builder.py:            return Redirect("..")
./buildbot/status/web/builder.py:        return Redirect("../..")
./buildbot/status/web/builder.py:        return Redirect("../..")

This breaks real HTTP clients which don't expect to have to figure out relative paths.

comment:6 Changed 13 years ago by exarkun

Ah, a random comment.

Using c['buildbotURL'] seemed like a good idea to me at first, but then I looked at my buildbot configuration and remembered that I have more than one web status reporter. The single global configuration only provides one URL, but there are three commonly used URLs which expose status information. Clearly each needs to generate URLs which point to it, not to one of the other statuses.

If buildbot isn't doing anything excessively clever with internal redirects (which it probably isn't, since it uses twisted.web which probably can't even do such a thing currently), then it may be possible to use Request.prepath to automatically determine the correct URL. Alternatively, if only Resources which are being rendered need to generate redirects (as opposed to generating a redirect partway through URL path segment traversal, eg by returning something funny from a getChild), then you can look at Request.uri and Request.getHeader('host'). Another option would be to pass the root into WebStatus as a parameter, which is nice and explicit and puts the burden of correctness on whoever is writing the configuration. ;) Of these, the first is the coolest but also probably the most likely to have subtle annoying bugs. The second is probably the best, if it applies.

comment:7 Changed 13 years ago by warner

The second approach sounds doable.. are reverse proxies prohibited from modifying the Host: header? And, well, I guess most browsers are speaking HTTP/1.1 these days and will therefore have a Host header. (most of my buildmasters are on vhosts, so they aren't even reachable without a Host header).

comment:8 Changed 12 years ago by bhearsum

  • Cc bhearsum@… added

comment:9 Changed 12 years ago by warner

  • Milestone changed from 0.7.7 to 0.7.8

hrm, I can't figure out a good way to fix this that will be done in time for 0.7.7 . Gotta push it out a release.

comment:10 Changed 12 years ago by dhart

I have a similar setup, and noticed that the nice "Welcome to the Buildbot!" page does not use buildbotURL, so all its links are broken.

comment:11 Changed 12 years ago by dustin

that page is static HTML -- edit it in your public_html directory.

comment:12 Changed 11 years ago by dustin

  • Milestone changed from 0.8.0 to 1.0.0

We'll need to consider URL-building more carefully in 1.0

comment:13 Changed 11 years ago by bhearsum

  • Cc bhearsum@… removed

comment:14 Changed 10 years ago by dustin

  • Cc exarkun added
  • Keywords web added
  • Milestone changed from 1.0.0 to 0.8.1

Has the Jinja work fixed this?

Do we need to make a per-WebStatus buildbotURL configuration?

comment:15 Changed 10 years ago by dustin

  • Resolution set to fixed
  • Status changed from new to closed

The JinjaProject? should have fixed this.

Note: See TracTickets for help on using tickets.