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

Allow regex in attribute whitelist. #3183

Closed
wants to merge 2 commits into from

Conversation

nzgrover
Copy link
Contributor

Adds the ability to use a regex in the attribute whitelist. The specific need I have is to allow all 'data-whatever' attributes through.
Just wanted to get the ball rolling with this commit, look forward feedback on improvements or the 'refinerycms' way to do this.

@parndt
Copy link
Member

parndt commented May 24, 2016

Thanks @nzgrover

Is there no support in Loofah to do this?

@bricesanchez
Copy link
Member

@nzgrover Could rebase your branch with the latest commit on master ?

Thanks!

@nzgrover
Copy link
Contributor Author

nzgrover commented May 24, 2016

@parndt I couldn't find anything in loofah's documentation, and there were data-* attributes being removed, but....
flavorjones/loofah#84
https://github.com/flavorjones/loofah/blob/master/CHANGELOG.rdoc
Says they are supported in version 2.0.2
I'll investigate further,

EDIT
https://github.com/rails/rails-html-sanitizer/blob/master/lib/rails/html/scrubbers.rb#L106
It looks like if you pass in a list of attributes, as refinerycms does, then it no longer uses Loofah::HTML5::Scrub, which allows 'data-'
I won't be able to look further into this until tomorrow. :-(

@parndt
Copy link
Member

parndt commented May 24, 2016

@bricesanchez no point rebasing until it's passed review 😄
@nzgrover thanks a lot!

@bricesanchez
Copy link
Member

@parndt my mistakes i was searching something in the wrong file. I thought a rebase will display it..

@@ -32,6 +32,7 @@ module Core
self.visual_editor_javascripts = []
self.visual_editor_stylesheets = []
self.plugin_priority = []
self.regex_white_list = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All configs for SectionPresenter are in the pages/lib/refinery/pages.configuration. I'm in favor to move regex_white_list in Pages and rename it whitelist_regex to match current configs.

Could you add by default your regex?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want to know whether we can support this in Loofah with existing config options

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it still use Loofah for the configurations Refinery::Pages::whitelist_elementsand Refinery::Pages::whitelist_attributes but use Rails::Html::PermitScrubber to create the scrubber intead of Loofah::Scrubber

@nzgrover
Copy link
Contributor Author

nzgrover commented May 25, 2016

@parndt I've had a further look at seeing if we can just use loofah, but as far as I can tell, if you supply attributes then ONLY those attributes explicitly supplied are allowed through. If you don't supply attributes, then the loofah default scrubber is used, which includes a regex to allow all 'data-' attributes.

The only way I can think of round it would be to update loofahs default allowed attributes directly, but loofah doesn't provide a way to do so. See this very old issue open regarding this: flavorjones/loofah#14

How do you wish to proceed?

@nzgrover
Copy link
Contributor Author

nzgrover commented May 25, 2016

Or what about something (slightly cheeky) like this: master...nzgrover:allow_all_data_attributes

No new parameters, just the custom scrubber which skips scrubbing all data- attributes.

@parndt
Copy link
Member

parndt commented May 25, 2016

@nzgrover I really like that solution.. allowing data- attributes seems sane; we'll need to document this next to the existing config options I think something like:

# Note: data- attributes are whitelisted by default. See (link to PR or something)
config.whitelist_attributes = []

@nzgrover
Copy link
Contributor Author

@parndt Ok, shall I put in a new pull request with the allow_all_data_attributes branch?

@parndt
Copy link
Member

parndt commented May 26, 2016

@nzgrover yes please

@parndt parndt closed this May 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants