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

html/template: does not recognize rgb() as a CSS color #25446

Open
daviddengcn opened this Issue May 17, 2018 · 10 comments

Comments

Projects
None yet
8 participants
@daviddengcn

daviddengcn commented May 17, 2018

Please answer these questions before submitting your issue. Thanks!

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

tip

Does this issue reproduce with the latest release?

yes

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

does not matter

What did you do?

An code fragment to reproduced is here: https://play.golang.org/p/r8LOFN_roBo
Using string instead of template.HTML produces the same result.

What did you expect to see?

color: rgb(10,20,30)

What did you see instead?

color: ZgotmplZ

@daviddengcn daviddengcn changed the title from html template does not recognize rgb() as a css color to bug: html template does not recognize rgb() as a CSS color May 17, 2018

@daviddengcn

This comment has been minimized.

daviddengcn commented May 17, 2018

I had thought any template.HTML-typed value will be output directly. template.CSS works. But why string does not.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented May 17, 2018

This may be a bug as I'd expect css function syntax to be allowed.

However, if you use template.CSS instead of template.HTML as the type of ColorRGB it works correctly.

@bradfitz bradfitz changed the title from bug: html template does not recognize rgb() as a CSS color to html/template: does not recognize rgb() as a CSS color May 17, 2018

@bradfitz bradfitz added this to the Go1.11 milestone May 17, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 17, 2018

I don't know this area, but if this is ultimately deemed not a bug, somebody should consider what documentation could be added to clarify this for future users.

@zegl

This comment has been minimized.

Contributor

zegl commented May 17, 2018

It happens because rgb() looks like unsafe JavaScript, which template.HTML tries to prevent. See the documentation on the ZgotmlpZ error. The documentation for template.CSS notes that that type is there to mark the content as safe, and that the source should be trusted.

@daviddengcn

This comment has been minimized.

daviddengcn commented May 18, 2018

rgb() is actually a safe css function. More functions can be found: https://www.w3schools.com/cssref/css_functions.asp

(a better way may be find css standards)

I don't think encouraging people to use template.CSS is a good idea. It can only be a workaround. Using string will be safer in case some careless bug may introduce unsafe code.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 29, 2018

@jamdagni86

This comment has been minimized.

Contributor

jamdagni86 commented Aug 8, 2018

@ianlancetaylor does this need to be fixed? I'll have a look at it if it needs one.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 8, 2018

@jamdagni86 Frankly, I don't know.

CC @mikesamuel @stjj89

@mikesamuel

This comment has been minimized.

Contributor

mikesamuel commented Aug 8, 2018

cssValueFilter is the function that is rejecting these. It's conservative and does not make any attempt to recognize safe functions.

@daviddengcn said

rgb() is actually a safe css function.

I agree that rgb, rgba, hsl, and hsla are safe functions. They're just not recognized as such.

The tests (code) give a sense of what is currently recognized.

@jamdagni86

This comment has been minimized.

Contributor

jamdagni86 commented Aug 16, 2018

@mikesamuel - one approach to fix this is to figure out if the value is a safe CSS function call. If it is, then we'll have to make sure the function call is of a valid syntax. Do we have a parser somewhere which I can use?

If you had other thoughts, please let me know.

@empijei

This comment has been minimized.

Contributor

empijei commented Sep 4, 2018

@jamdagni86 the current implementation just ranges over the decoded bytes and detects potentially dangerous characters, so there is no parser to use.

Switching to a parser to allow certain functions would imply a complete rewrite of that logic, and would very likely introduce issues and potential vulnerabilities in code that is currently protected by that check (e.g. existing code with a globally defined and vulnerable rgb javascript function or something similar).

I don't think switching to an allowed-list can be easily done in a secure way, and would probably not be considered backward compatible.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment