Ticket #3013 defect closed fixed

Opened 6 years ago

Last modified 6 years ago

twisted.web.proxy incorrectly mangles URIs

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: web Keywords:
Cc: Branch: branches/proxy-urlquote-3013
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

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

1

Changed 6 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.

2

Changed 6 years ago by exarkun

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

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

3

Changed 6 years ago by exarkun

  • owner jknight deleted
  • priority changed from high to highest
  • keywords review added

4

Changed 6 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.

5

Changed 6 years ago by danderson

  • owner set to exarkun

6

Changed 6 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.

7

Changed 6 years ago by therve

  • owner set to exarkun
  • keywords review removed

Why did you mangle the quote import as urlquote?

Otherwise, looks good.

8

Changed 6 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

9

Changed 6 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(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.

10

Changed 3 years ago by <automation>

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