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.

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

html/template: stateURL context percent encodes "(", ")" yet they are valid path characters #15891

Closed
gmccue opened this issue May 30, 2016 · 7 comments
Closed

html/template: stateURL context percent encodes "(", ")" yet they are valid path characters #15891

gmccue opened this issue May 30, 2016 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@gmccue
Copy link

gmccue commented May 30, 2016 •
edited

The html/template context stateURL appears to be percent encoding valid characters. I believe this is because shouldEscape() in net/url encodes these characters by default: https://github.com/ecosia/go/blob/master/src/net/url/url.go#L101

RFC3986 allows for the use of ( and ) characters in the query component: https://tools.ietf.org/html/rfc3986#section-3.4

  1. What version of Go are you using (go version)?
    1.6.2
  2. What operating system and processor architecture are you using (go env)?
    linux/amd64
  3. What did you do?
    Create a template with a variable to be used for the href attribute. Pass a URL including ( or ) to the template and compile. ( and ) are percent encoded during compilation.
    An example on play.golang.org: https://play.golang.org/p/QFtR42GyfI
  4. What did you expect to see?
    <a href="http://my.domain/path-to-lang(en)">Click this link.</a>
  5. What did you see instead?
    <a href="http://my.domain/path-to-lang%28en%29">Click this link.</a>
@gmccue
Copy link
Author

gmccue commented May 30, 2016

Related to: #12036, and this PR

@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone May 30, 2016
@ianlancetaylor
Copy link
Contributor

Does this extra encoding cause any harm?

@gmccue
Copy link
Author

gmccue commented May 31, 2016

In my particular use-case is does, as the URL that is output is to a third-party re-direction URL that does not perform proper URL un-encoding.

I will submit a PR shortly.

@gmccue gmccue mentioned this issue Jun 20, 2016
Closed
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@odeke-em
Copy link
Member

I don't see any connection to net/http so am going to change the title of this bug to relate it to net/url instead. Please correct me if am wrong.

@bradfitz
Copy link
Contributor

Or html/template?

@odeke-em odeke-em changed the title net/http: stateURL template context percent encodes "(" and ")" html/template: stateURL context percent encodes "(", ")" yet they are valid query component characters Oct 18, 2016
@odeke-em
Copy link
Member

Good plan, thanks. Updated.

@rsc
Copy link
Contributor

rsc commented Oct 19, 2016

The problem here is not net/url but the fact that the html/template escaper uses the same code for escaping <a href="https://site.com/$PATH"> and CSS like url(https://site.com/$PATH). In the latter case, it is important to escape the parens.

Any RFC-compliant server should treat /path(en) and /path%28en%29 the same. This code is very subtle, and I am not convinced that the benefit to broken servers is worth the risk of introducing a security problem in this code.

I think we should leave this alone.

/cc @mikesamuel

@rsc rsc changed the title html/template: stateURL context percent encodes "(", ")" yet they are valid query component characters html/template: stateURL context percent encodes "(", ")" yet they are valid path characters Oct 19, 2016
@rsc rsc closed this as completed Oct 19, 2016
@golang golang locked and limited conversation to collaborators Oct 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
7 participants
@bradfitz @rsc @quentinmit @gmccue @ianlancetaylor @odeke-em @gopherbot