Skip to content
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.

Sign up for GitHub

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

Jump to bottom

net/url: URL.String() on Chrome intent URLs with an embedded "https://" string inside of a Fragment is not being encoded properly #13784

Closed
hasSoumya opened this issue Dec 31, 2015 · 5 comments
Closed

net/url: URL.String() on Chrome intent URLs with an embedded "https://" string inside of a Fragment is not being encoded properly #13784

hasSoumya opened this issue Dec 31, 2015 · 5 comments
Labels
FrozenDueToAge
Milestone
Unplanned

Comments

@hasSoumya
Copy link

What version of Go are you using (go version)?
1.5.1

What operating system and processor architecture are you using?
Mac OS X El Capitan (Darwin Kernel Version 15.0.0)

What did you do?
See : http://play.golang.org/p/oNl2dEgd9L

What did you expect to see?
intent://path/?query=test#Intent;scheme=something;package=app.mozilla.firefox;S.query=test;S.browser_fallback_url=https%3A%2F%2Fbooyah.com%2Fmy.apk;end

What did you see instead?
intent://path/?query=test#Intent;scheme=something;package=app.mozilla.firefox;S.query=test;S.browser_fallback_url=https%253A%252F%252Fbooyah.com%252Fmy.apk;end

Details:

Note: The Chrome spec here is purely an example on which I was working on

The intention is to produce something like the following based on Chromes specification about fallback URLs: https://developer.chrome.com/multidevice/android/intents

Ref: http://play.golang.org/p/oNl2dEgd9L

Expected URL: intent://path/?query=test#Intent;scheme=something;package=app.mozilla.firefox;S.query=test;S.browser_fallback_url=https%3A%2F%2Fbooyah.com%2Fmy.apk;end

When I use the library somewhat properly embedding the https:// link inside of the URL Fragment, it looks like the following URL: intent://path/?query=test#Intent;scheme=something;package=app.mozilla.firefox;S.query=test;S.browser_fallback_url=https%253A%252F%252Fbooyah.com%252Fmy.apk;end

I might be missing something, but increasingly it looks like some sort of bug just eyeballing the escape() function inside the url library. Note the spurious %25 being added onto the https:// url. Even when I don't encode the embedded https:// - String() does not encode the url at all and leaves it bare like so:
intent://path/?query=test#Intent;scheme=something;package=app.mozilla.firefox;S.query=test;S.browser_fallback_url=https://booyah.com/my.apk;end
@bradfitz
Copy link
Contributor

Sorry, this slipped through the cracks while everybody was away during the holidays, then it was so far below the fold, nobody found it back until now.

Couple questions:

  • Is this behavior a regression from Go 1.4?
  • Have you tried Go 1.6beta2?
  • Can you minimize the problematic code as much as possible? Those URLs have a lot of stuff going on. It's visually hard to find the differences with so much noise.

@bradfitz bradfitz added this to the Unplanned milestone Jan 21, 2016
@bradfitz bradfitz changed the title URL.String() on Chrome intent URLs with an embedded "https://" string inside of a Fragment is not being encoded properly net/url: URL.String() on Chrome intent URLs with an embedded "https://" string inside of a Fragment is not being encoded properly Jan 21, 2016
@kennygrant
Copy link
Contributor

This happens on 1.4 and 1.6

Here is a simpler test case which might help :

u := url.URL{Scheme: "intent", Path: "path", RawQuery: "query=test", Fragment: "https://example.com"}
fmt.Printf("%s - fragment not encoded\n", u.String())

u.Fragment = url.QueryEscape("https://example.com")
fmt.Printf("%s - fragment double encoded\n", u.String())

output:
intent://path?query=test#https://example.com - fragment not encoded
intent://path?query=test#https%253A%252F%252Fexample.com - fragment double encoded

*URL.String() calls escape which calls shouldEscape. shouldEscape does not ignore % characters in mode encodeFragment, though it ignores other reserved characters. This leads to double encoding of the percent escapes if the fragment is escaped already, and no escaping if not escaped.

@bradfitz
Copy link
Contributor

@kennygrant, as far as I can tell, this behavior is the same on Go 1.4, Go 1.5, and Go 1.6, no? Your message almost implies that Go 1.5 fixed it and then it regressed in Go 1.6.

@kennygrant
Copy link
Contributor

I only tested with 1.4 and 1.6 as didn't have 1.5 installed so could only talk about those, didn't mean to imply anything about 1.5, I've just checked though and 1.5 is the same yes, I don't think this is new behaviour.

@bradfitz
Copy link
Contributor

Note that url.Parse un-escapes %xx sequences in the fragment:

http://play.golang.org/p/uiAtaRKJo0

    u, err := url.Parse("http://foo.com/#x%41x")
    fmt.Printf("%#v\n%v\n%v", u, u, err)

Yields:

&url.URL{Scheme:"http", Opaque:"", User:(*url.Userinfo)(nil), Host:"foo.com", Path:"/", RawPath:"", RawQuery:"", Fragment:"xAx"}
http://foo.com/#xAx
<nil>

Note that Fragment is xAx instead of x%41x. As a result, if url.Parse and url.String are going to roundtrip, it must always escape a % byte to %25.

So I don't think there's anything we can do here for compatibility reasons.

If you need to generate such URLs, you can always use URL.Opaque... http://play.golang.org/p/fjgaEJ_Mpj

@golang golang locked and limited conversation to collaborators Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@bradfitz @kennygrant @gopherbot @hasSoumya