Opened 12 years ago

Closed 12 years ago

#3013 defect closed fixed (fixed)

twisted.web.proxy incorrectly mangles URIs

Reported by: Jean-Paul Calderone Owned by:
Priority: highest Milestone:
Component: web Keywords:
Cc: Branch: branches/proxy-urlquote-3013
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun


ReverseProxyResource.render parses the request URI it receives. It then passes on the path and the query portions to the target of the proxying.

It does not pass on the fragment.

It unquotes the URI and does not re-quote it when sending it on.

Change History (10)

comment:1 Changed 12 years ago by Jean-Paul Calderone

Huh, actually, I have no idea what the correct behavior WRT the fragment is. The quoting issue is real and serious, though.

comment:2 Changed 12 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/proxy-urlquote-3013

(In [22401]) Branching to 'proxy-urlquote-3013'

comment:3 Changed 12 years ago by Jean-Paul Calderone

Keywords: review added
Owner: jknight deleted
Priority: highhighest

comment:4 Changed 12 years ago by danderson

Keywords: review removed

The changes on the branch look good.

As for the fragment question, I don't think that a normal HTTP client would ever send a fragment to a server, since it's just a local referencing system, none of the server's business. At least Firefox and wget filter out the fragment part of the URL before sending the request to the server. Telnetting to an Apache server and GETing an URL with a fragment returns the same resource as the URL without fragment, but the fragment is made available in logs, and to scripts (eg. php).

Given all that, it seems that the issue is moot for most applications, since they don't actually send a fragment. But, to make the proxy as transparent as posible, I think the fragment should be passed on. Let the destination server decide what to do with it.

comment:5 Changed 12 years ago by danderson

Owner: set to Jean-Paul Calderone

comment:6 Changed 12 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Thanks for the review.

Passing on the fragment makes sense to me, but it's going to be pretty difficult to implement.

ReverseProxyResource learns about the URI through the getChild API. The segment passed to getChild is already decoded. This means there's no way to differentiate between the # which delimits the fragment and a # which was really a %23 in the request. So, while ideally I'd change this code to pass on fragments, I'm not actually excited about changing the whole proxying implementation in order to support this non-standard feature.

comment:7 Changed 12 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Why did you mangle the quote import as urlquote?

Otherwise, looks good.

comment:8 Changed 12 years ago by Jean-Paul Calderone

15:06 < exarkun> therve: "quote" seems too vague to me
15:07 < therve> exarkun: alright
15:07 < therve> exarkun: you can merge then

comment:9 Changed 12 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [22431]) Merge proxy-urlquote-3013

Author: exarkun Reviewer: danderson, therve Fixes #3013

Change twisted.web.proxy.ReverseProxyResource so that it correctly encodes the request URI it sends on to the server for which it is a proxy.

comment:10 Changed 9 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.