-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proxied requests confuse the URI parser #248
Comments
Are there any headers that come with this |
Host is, as expected, |
OK, so this is the culprit: https://github.com/mirage/ocaml-cohttp/blob/master/lib/request.ml#L97 |
By the way it's easier to reproduce by setting the Firefox HTTP proxy setting to a local cohttp server. |
@rgrinberg are you on this? It looks like d04701f#diff-daa4e580cf195143c70372b6183a5a40R140 should really be using proper |
@dsheets I'd like to fix this by next week. A fix from you would be much appreciated since I'm not sure of proper Uri usage. |
This is the absolutely horrible hack I'm using for now, but it works until a fix is (hopefully) released in 0.16. I put it here in case anyone googled and found this issue. I would like to apologize to the gods of ocaml for this horror. let fix_uri uri =
let ungarble uri host protocol_len =
uri
|> fun uri -> ((Uri.with_host uri (Some (String.slice host 0 (-protocol_len)))), (String.length host - protocol_len))
|> fun (uri, len) -> Uri.with_path uri (String.slice (Uri.path uri) (len + 2) 0)
in
match (uri |> Uri.host |> Option.value ~default:"") with
| host when String.is_suffix ~suffix:"http" host -> ungarble uri host 4
| host when String.is_suffix ~suffix:"https" host -> ungarble uri host 5
| host when (((String.length host) / 2) mod 2 = 0) && ((String.slice host (String.length host / 2) 0) = (String.slice host 0 (String.length host / 2))) ->
Uri.with_host uri (Some (String.slice host 0 (String.length host / 2)))
| _ -> uri
|
Partially addresses mirage#248
I stuck a functional update fix into avsm/ocaml-cohttp@0260455, but uri is doing some odd things:
Those should really be equivalent. |
This is still happening even with Uri 1.8.0.
|
@SGrondin I assume using cohttp master as well? |
Oh, I didn't see it was closed in 37bcbd8. For some reason I thought it was in Uri 1.8.0. |
I just tested it against master and it's still happening.
|
@SGrondin So i assume the request looks something like this then:
FYI I just tested the request above and am getting the correct uri
|
Try it by setting your Firefox (or system) proxy to a cohttp server and then try to browse wikipedia.org in a browser. http://i.imgur.com/81vdpWS.png |
I cna't reproduce this, but I'm seeing some suspicious activity in When browsing wikipedia, 400 Bad Requests show up, which they really shouldn't. Probably some hop-by-hop heading that isn't being handled right.
|
That debug output shows the browser writing a request, it being rewritten correctly for upstream (with the URL rewritten to make it a relative URL). The problem appears immediately afterwards when a HTTP bad request shows up. This is possibly related to pipelining behaviour. |
As requested, here's a small test proxy that demonstrates the bug. Set your system proxy to As you try to browse HTTP sites, you'll see in the console that the hostname for the incoming requests is broken and nothing can be fetched. Then edit line 25 to uncomment the call to Compile with |
Indeed, some unrelated change in 0.16.0 fixed it. Nice |
This was a pretty severe issue. It would be nice to know which commit fixed the issue so we can understand the cause of the original defect. |
When I set the system-wide HTTP proxy on Debian to forward all traffic to a Cohttp application, the calls come in as
GET //simongrondin.name/files/fifty.txt
instead ofGET http://simongrondin.name/files/fifty.txt
.The URI parser gets confused and interprets them like this:
The text was updated successfully, but these errors were encountered: