{"action":"created","issue":{"url":"https://api.github.com/repos/EFForg/https-everywhere/issues/14598","repository_url":"https://api.github.com/repos/EFForg/https-everywhere","labels_url":"https://api.github.com/repos/EFForg/https-everywhere/issues/14598/labels{/name}","comments_url":"https://api.github.com/repos/EFForg/https-everywhere/issues/14598/comments","events_url":"https://api.github.com/repos/EFForg/https-everywhere/issues/14598/events","html_url":"https://github.com/EFForg/https-everywhere/pull/14598","id":296160878,"number":14598,"title":"Update https_everywhere_checker/http_client.py, Fix #14365","user":{"login":"cschanaj","id":23636124,"avatar_url":"https://avatars0.githubusercontent.com/u/23636124?v=4","gravatar_id":"","url":"https://api.github.com/users/cschanaj","html_url":"https://github.com/cschanaj","followers_url":"https://api.github.com/users/cschanaj/followers","following_url":"https://api.github.com/users/cschanaj/following{/other_user}","gists_url":"https://api.github.com/users/cschanaj/gists{/gist_id}","starred_url":"https://api.github.com/users/cschanaj/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cschanaj/subscriptions","organizations_url":"https://api.github.com/users/cschanaj/orgs","repos_url":"https://api.github.com/users/cschanaj/repos","events_url":"https://api.github.com/users/cschanaj/events{/privacy}","received_events_url":"https://api.github.com/users/cschanaj/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2018-02-11T04:55:23Z","updated_at":"2018-02-14T23:53:15Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/EFForg/https-everywhere/pulls/14598","html_url":"https://github.com/EFForg/https-everywhere/pull/14598","diff_url":"https://github.com/EFForg/https-everywhere/pull/14598.diff","patch_url":"https://github.com/EFForg/https-everywhere/pull/14598.patch"},"body":"Travis failed to follow a non-trivial rewrite because the redirect location lack a trailing `/`. A testing PR is available at #14599. Close #14365. Close #14599 \r\n\r\nP.S. This bug should not affect the extension itself.\r\n\r\ncc @Hainish @jeremyn \r\n\r\n"},"comment":{"url":"https://api.github.com/repos/EFForg/https-everywhere/issues/comments/365784159","html_url":"https://github.com/EFForg/https-everywhere/pull/14598#issuecomment-365784159","issue_url":"https://api.github.com/repos/EFForg/https-everywhere/issues/14598","id":365784159,"user":{"login":"jeremyn","id":299895,"avatar_url":"https://avatars1.githubusercontent.com/u/299895?v=4","gravatar_id":"","url":"https://api.github.com/users/jeremyn","html_url":"https://github.com/jeremyn","followers_url":"https://api.github.com/users/jeremyn/followers","following_url":"https://api.github.com/users/jeremyn/following{/other_user}","gists_url":"https://api.github.com/users/jeremyn/gists{/gist_id}","starred_url":"https://api.github.com/users/jeremyn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jeremyn/subscriptions","organizations_url":"https://api.github.com/users/jeremyn/orgs","repos_url":"https://api.github.com/users/jeremyn/repos","events_url":"https://api.github.com/users/jeremyn/events{/privacy}","received_events_url":"https://api.github.com/users/jeremyn/received_events","type":"User","site_admin":false},"created_at":"2018-02-14T23:53:15Z","updated_at":"2018-02-14T23:53:15Z","author_association":"COLLABORATOR","body":"@Hainish I agree that my earlier naive string concatentation will not work.\r\n\r\n`ParseResult` with parameters [is public](https://docs.python.org/2/library/urlparse.html#results-of-urlparse-and-urlsplit) and can be directly instantiated. This or similar should work:\r\n\r\n```python\r\njoinedUrlParts = ParseResult(join\r\n\tjoinedUrlParts.scheme,\r\n\tjoinedUrlParts.netloc,\r\n\t'/' if joinedUrlParts.path == '' else joinedUrlParts.path,\r\n\tjoinedUrlParts.params,\r\n\tjoinedUrlParts.query,\r\n\tjoinedUrlParts.fragment,\r\n)\r\n```\r\n\r\nwith a later `geturl` call. We could also try some string manipulation based on the `ParseResult` members directly, like `'/'.join(joinedUrlParts.scheme, ...) + ...` or something like that, but that would be awkward and error-prone.\r\n\r\n`_replace` should be avoided entirely because it is a private method and it relies on using `namedtuple` which is a private implementation detail. I'm not interested in arguing this particular point further, so if you still want to keep `_replace`, then I concede.\r\n\r\n@Hainish and @cschanaj I think I understand the need for this change now, thanks.\r\n\r\nAfter some digging ( https://webmasters.stackexchange.com/a/35646 , https://tools.ietf.org/html/rfc2616#section-5.1.2 ), it seems the relevant RFC requires a trailing `/` if the path is blank. Browsers add this themselves, and HTTPS Everywhere matches the URL after the `/` is added. So, adding a `/` manually, like we're trying to do in this pull request, is actually the correct thing for us to do. \r\n\r\nTo make it totally clear, for my issue https://github.com/EFForg/https-everywhere/issues/14365 the full chain of events is this:\r\n\r\n* HTTPS Everywhere sees `http://link.armytimes.com/` \r\n* it rewrites this to `https://link.armytimes.com/`\r\n* the server for `https://link.armytimes.com/` returns a `302` response with a `Location` of `http://armytimes.com` (without a trailing `/`)\r\n* (crucially) the ruleset checker doesn't match `http://armytimes.com` to `^http://armytimes\\.com/` because it is missing a trailing `/`, so it doesn't rewrite it to `www`, and instead it hits the trivial rewrite to become `https://armytimes.com`\r\n* curl/PycURL makes a request to `https://armytimes.com/`\r\n* `https://armytimes.com/` gets `Failed to connect`\r\n\r\nPlease correct me if this is wrong."}}
{"actor":{"display_login":"jeremyn"}}