Skip to content

Display helper should not sanitize its input#5299

Merged
deivid-rodriguez merged 3 commits intoactiveadmin:masterfrom
SPBTV:bugfix/resource_filters_xss
Jan 5, 2018
Merged

Display helper should not sanitize its input#5299
deivid-rodriguez merged 3 commits intoactiveadmin:masterfrom
SPBTV:bugfix/resource_filters_xss

Conversation

@faucct
Copy link
Contributor

@faucct faucct commented Dec 22, 2017 •
edited
Loading

Following 0456e2d#r26411218

Fivell
Fivell previously approved these changes Dec 22, 2017
View reviewed changes
@faucct
Copy link
Contributor Author

faucct commented Dec 22, 2017

Sadly ActionView::Helpers::OutputSafetyHelper#to_sentence was only introduced in rails 5 and cannot be used here. Can we copy it?

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Dec 22, 2017

I'm 👍 for copying it until we drop rails 4.2 support.

@faucct faucct force-pushed the bugfix/resource_filters_xss branch from c98ebed to 6a2f8f3 Compare December 28, 2017 11:08
@codecov
Copy link

codecov bot commented Dec 28, 2017 •
edited
Loading

Codecov Report

Merging #5299 into master will increase coverage by <.01%.
The diff coverage is 98.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5299      +/-   ##
==========================================
+ Coverage   98.29%   98.29%   +<.01%     
==========================================
  Files         292      294       +2     
  Lines       10910    10973      +63     
==========================================
+ Hits        10724    10786      +62     
- Misses        186      187       +1
Impacted Files Coverage Δ
spec/unit/view_helpers/display_helper_spec.rb 100% <ø> (ø) ⬆️
lib/active_admin/filters/active_sidebar.rb 100% <100%> (ø) ⬆️
...b/active_admin/views/components/sidebar_section.rb 100% <100%> (ø) ⬆️
spec/unit/pretty_format_spec.rb 100% <100%> (ø) ⬆️
lib/active_admin/view_helpers/display_helper.rb 98% <100%> (ø) ⬆️
spec/unit/helpers/output_safety_helper_spec.rb 100% <100%> (ø)
lib/active_admin/helpers/output_safety_helper.rb 92.85% <92.85%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 043ba0c...4d1b971. Read the comment docs.

@faucct
Copy link
Contributor Author

faucct commented Dec 28, 2017

Copied #to_sentence, though I had no idea where to include the helper module. I wanted to put it in Filters::ActiveSidebar, but apparently the block method is executed in context of Views::SidebarSection, so I've had to put it there.

@varyonic
Copy link
Contributor

varyonic commented Dec 28, 2017

Looks like you might need to copy a couple of unit tests also...

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Jan 4, 2018

@varyonic I added the unit tests!

@faucct
Copy link
Contributor Author

faucct commented Jan 4, 2018

Travis CI has failed to bundle, probably has to be restarted.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Jan 4, 2018

Restarted! 🤞

This was referenced Jan 4, 2018
Closed
Closed
@varyonic
Copy link
Contributor

varyonic commented Jan 5, 2018

@deivid-rodriguez Perhaps we need to update travis.yml? https://www.ruby-lang.org/en/news/2017/12/14/net-ftp-command-injection-cve-2017-17405/

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Jan 5, 2018

Maybe, I restarted the job a few times yesterday but it kept failing. It didn't seem related to this PR though.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Jan 5, 2018

It's good to update Travis CI in any case, yeah. We can also take the chance to try out how AA is working with ruby 2.5.0.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Jan 5, 2018

I'm actually surprised that all those minitest-style assertions worked out of the box. Merging anyways since these tests are not meant to stay long.

@deivid-rodriguez deivid-rodriguez merged commit 8155e88 into activeadmin:master Jan 5, 2018
@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Jan 5, 2018

Thanks for cleaning up my mess @faucct! 😃

@faucct
Copy link
Contributor Author

faucct commented Jan 5, 2018

You're welcome.

@tiagotex tiagotex mentioned this pull request Jan 8, 2018
Closed
@faucct
Copy link
Contributor Author

faucct commented Jan 9, 2018

Can anyone backport this in 1.2.0 yanking the buggy version?

varyonic pushed a commit that referenced this pull request Jan 10, 2018
@faucct @varyonic
Backport pull request #5299 from SPBTV/bugfix/resource_filters_xss
66950fe 
Adapt unit tests for copied helper
DisplayHelper should not sanitize its input
Filters sidebar should not be vulnerable to XSS
@varyonic varyonic mentioned this pull request Jan 10, 2018
Merged
@deivid-rodriguez deivid-rodriguez mentioned this pull request Jul 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deivid-rodriguez deivid-rodriguez deivid-rodriguez approved these changes

@Fivell Fivell Fivell left review comments

+1 more reviewer

@varyonic varyonic varyonic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@faucct @deivid-rodriguez @varyonic @Fivell