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

Add ability to specify inline style policies #48

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pauln
Copy link
Contributor

@pauln pauln commented Oct 18, 2017

Following on from #43 / #44, I've implemented a policy type and builder for style policies, following the same pattern as attribute policies. That means that you can whitelist individual CSS properties either globally (on any element that's had the style attribute whitelisted) or on specific elements. In addition, a MatchingEnum method allows for a list of allowed values to be specified (useful for properties which have a set of predefined values, so that you don't need to build a regular expression to do this).

Note that this PR does not touch the predefined policies, though it does open the door to doing so in the future (either updating existing ones or adding new policies which include style policies).

It's currently implemented using simple string processing, rather than adding an external dependency; if that's deemed to be too naive or weak, the sanitizeStyles func in sanitize.go can be enhanced to use an actual parser (either an external dependency like douceur - as per #43 / #44 - or a custom minimal implementation to avoid external dependencies).

Closes #43.

Style policies allow inline styles ("style" attributes on HTML elements) to be processed, either using regular expressions or matching against a list of allowed values (enum).
Adds a test for AllowStyles which covers Matching, MatchingEnum, OnElements and Globally.
@nixypanda
Copy link

Any work going on this I have started to use this in one of the projects at my company. Works like a charm. Also is there any list of styles I can allow without worrying about XSS injection as if you have to create a policy that handles each style individually will get really painful to write and a nightmare to maintain.

@sapiens-sapide
Copy link

Any idea if this PR will be merged soon ? Should I fork to get it ?

@grafana-dee
Copy link
Contributor

You should fork it.

It is not safe to use from a security perspective.

I totally get the convenience, and if you are in control of your HTML input then this is good enough to be used as you control the inputs. But CSS can be harmful as it can be used for anything from keylogging to inserting SVG and scripting content into the HTML.

Until such a time that either I find time to write a safe version, or a PR is made that is whitelist-based and tests the values within the style attribute and style blocks... I won't merge code that introduces security risks into a security package.

@sapiens-sapide
Copy link

OK, thanks for replying. I understand your point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants