Opened 8 years ago

Closed 8 years ago

#3013 defect closed fixed (fixed)

twisted.web.proxy incorrectly mangles URIs

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: web Keywords:
Cc: Branch: branches/proxy-urlquote-3013
(github, coverage, patch, buildbot, log)
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 8 years ago by exarkun

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 8 years ago by exarkun

  • author set to exarkun
  • Branch set to branches/proxy-urlquote-3013

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

comment:3 Changed 8 years ago by exarkun

  • Keywords review added
  • Owner jknight deleted
  • Priority changed from high to highest

comment:4 Changed 8 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 8 years ago by danderson

  • Owner set to exarkun

comment:6 Changed 8 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 8 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Why did you mangle the quote import as urlquote?

Otherwise, looks good.

comment:8 Changed 8 years ago by exarkun

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 8 years ago by exarkun

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

(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 5 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.