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

html/template: don't escape '+' chars in HTML attributes #27805

Closed
wants to merge 1 commit into from
Closed

html/template: don't escape '+' chars in HTML attributes #27805

wants to merge 1 commit into from

Conversation

FelicianoTech
Copy link

The "+" (plus) symbol is being escaped right now from HTML attributes
via html/template even though it's a valid character.

This causes what would be a valid ISO8601 date such as
2018-09-19T19:52:28+06:00 to be escaped in an HTML meta tag. Which is
unnecessary and may prevent certain search bots and other tools from
parsing the date correctly.

HTML 4.x and HTML 5 attribute values can have a plus (+) character unescaped without issue: https://www.w3.org/TR/2011/WD-html5-20110525/syntax.html#attributes-0

You can further verify that put validating the snipper below at the W3C HTML Validator.

<!DOCTYPE html>
<html lang="en">
<head>
<meta name="generator" content="Hugo 0.49-DEV" />
	<meta charset="UTF-8" />
	<meta property="og:updated_time" content="2006-01-02T15:04:05+07:00" /> 
	<title>My Fake Website</title>
</head>
<body class="home ">
	<h1 class="site-title">Hello!</h1>
</body>
</html>

Notice how there's a datetime timestamp with the plus symbol for the timezone. That's perfectly valid and this PR will enable that to happen with html/template.

@FelicianoTech
html/template: not escape + chars in HTML attributes
a492394 
The "+" (plus) symbol is being escaped right now from HTML attributes
via html/template even though it's a valid character.

This causes what would be a valid ISO8601 date such as
2018-09-19T19:52:28+06:00 to be escaped in an HTML meta tag. Which is
unnecessary and may prevent certain search bots and other tools from
parsing the date correctly.
@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Sep 21, 2018
@gopherbot
Copy link
Contributor

This PR (HEAD: a492394) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/136755 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gerrit User 5976:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/136755.
After addressing review feedback, remember to publish your drafts!

@FelicianoTech FelicianoTech changed the title html/template: not escape '+' chars in HTML attributes html/template: don't escape '+' chars in HTML attributes Sep 21, 2018
@gopherbot
Copy link
Contributor

Message from Gerrit User 12446:

Uploaded patch set 2: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/136755.
After addressing review feedback, remember to publish your drafts!

@FelicianoTech FelicianoTech mentioned this pull request Sep 21, 2018
Closed
@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 2:

This would need a test.

Please don’t reply on this GitHub thread. Visit golang.org/cl/136755.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Mike Samuel:

Patch Set 2:

Escaping '+' is done to thwart utf7 attacks. Utf7 may no longer be an issue for attribute values. I'll check.

Please don’t reply on this GitHub thread. Visit golang.org/cl/136755.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Emmanuel Odeke:

Patch Set 2:

Hello Ricardo, thank you for this change and welcome to the Go project!

So this change has a security impact but also the decisions considered for it
would be useful to be indexable and searchable for posterity and also if
anything changes, to figure out why we took a certain approach.

Please file a corresponding issue on the issue tracker at https://github.com/golang/go/issues

Thank you!

Please don’t reply on this GitHub thread. Visit golang.org/cl/136755.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Mike Samuel:

Patch Set 2:

'+' is escaped because it historically figured in UTF-7 attacks.
http://michaelthelin.se/security/2014/06/08/web-security-cross-site-scripting-attacks-using-utf-7.html

Browsers since IE 9 haven't been vulnerable, so it only affects about 1% of browsers out there, but I'd prefer not to relax escaping without good reason.

may prevent certain search bots and other tools from parsing the date correctly

Do we know how many, if any search bots / tools have this trouble?
If it's a small amount, I'd rather file bugs against them.

Please don’t reply on this GitHub thread. Visit golang.org/cl/136755.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rodolfo Carvalho:

Patch Set 2: Code-Review-1

Hi there! AFAICT this request for change originates from an attempt to fix gohugoio/hugo#5236. I stumbled upon it and realized that the reason the '+' char was being escaped in an Hugo template was unrelated to html/template's implementation.

Please don’t reply on this GitHub thread. Visit golang.org/cl/136755.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/136755 has been abandoned.

Sounds like this is no longer needed. Reply if that's incorrect.

@gopherbot gopherbot closed this Apr 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FelicianoTech @gopherbot @googlebot