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: URL auto-escaper may produce invalid markup #19205

Closed
moshee opened this issue Feb 20, 2017 · 4 comments
Closed

html/template: URL auto-escaper may produce invalid markup #19205

moshee opened this issue Feb 20, 2017 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.10

Comments

@moshee
Copy link

moshee commented Feb 20, 2017

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

go version go1.8 linux/amd64

What operating system and processor architecture are you using (go env)?

Any environment.

What did you do?

https://play.golang.org/p/vkxq91HMhf

What did you expect to see?

The template produces fully valid markup, i.e. <a href="http://www.example.com/%5B%5D">a</a>, as if using url.PathEscape.

What did you see instead?

The square brackets remain unescaped in the path. According to the W3 Validator, this is invalid:

screen shot 2017-02-20 at 11 04 59 am
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 21, 2017
@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 21, 2017
@odeke-em
Copy link
Member

@rsc do you think there might be some security implications in changing this?

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017
@odeke-em
Copy link
Member

/cc @mikesamuel

@mikesamuel
Copy link
Contributor

mikesamuel commented Aug 14, 2017 •
edited

Square brackets are allowed in URIs as in IPv6 numeric hosts per RFC 6874.

IP-literal = "[" ( IPv6address / IPv6addrz / IPvFuture  ) "]"

so we ought not fix this by breaking https://play.golang.org/p/ZO713Uf8_Z

`<a href="http://{{ . }}/">a</a>`

The template produces fully valid markup

This is impossible for URIs without non-local reasoning. For example,

template.New("asdf")
    .Parse(`<a href="{{if .IsHost}}http://{{end}}{{.UrlPart}}{{if .IsHost}}/{{end}}">`)

In this instance, we would need to either

  • have the escaping directive know about the context based on information available at runtime
  • do a second pass at the end of the href value to fixup the URL.

Unless there are security consequences to not validating here, I would leave as-is.

@ianlancetaylor
Copy link
Contributor

It sounds like we shouldn't make any change here. Closing. Please comment if you disagree.

@golang golang locked and limited conversation to collaborators Dec 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
6 participants
@bradfitz @mikesamuel @moshee @ianlancetaylor @odeke-em @gopherbot