search

rails / rails-html-sanitizer

MIT License
306 stars 83 forks source link

Fix ReDoS Vulnerability in PermitScrubber and Add Performance Test #186

Open ch4n3-yoon opened 2 months ago

ch4n3-yoon commented 2 months ago

This pull request addresses a Regular Expression Denial of Service (ReDoS) vulnerability in the PermitScrubber class of the rails-html-sanitizer library. The vulnerability, caused by a regex pattern with potential for excessive backtracking, has been mitigated by optimizing the regular expression used in the scrub_attribute method.

flavorjones commented 2 months ago

I'm looking into the JRuby failures which are unrelated to this change, but likely a bug that has been exposed by the test.

flavorjones commented 2 months ago

JRuby failures should be fixed with #188, I've rebased this branch and force-pushed it.

flavorjones commented 2 months ago

@ch4n3-yoon I appreciate the fact that you wrote a benchmark test, but it seems brittle, failing often for me in development and in CI (see https://github.com/rails/rails-html-sanitizer/actions/runs/10372673519/job/28715948562?pr=186).

I'm open to you putting more work into it to be less brittle, but I am also fine with removing the test. Up to you.

ch4n3-yoon commented 2 months ago

Hi @flavorjones, thank you for the feedback. Given the brittleness of the benchmark test, I've decided to remove it to prevent potential instability in development and CI. The primary fix for the ReDoS vulnerability remains effective.

flavorjones commented 2 months ago

Since this patch has some unwanted behavior changes, I'd like to suggest we close this and discuss the approach in the hackerone issue. @ch4n3-yoon if you agree, I'll close this.