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.

Sign up for GitHub

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

Jump to bottom

Rails Sanitize Gem: Frozen Allowed Tags List is Mutated Internally #195

Closed
ayzahamid opened this issue Dec 11, 2024 · 5 comments · Fixed by #196
Closed

Rails Sanitize Gem: Frozen Allowed Tags List is Mutated Internally #195

ayzahamid opened this issue Dec 11, 2024 · 5 comments · Fixed by #196

Comments

@ayzahamid
Copy link

We have observed a problematic behavior in the latest version of the sanitize gem when using a frozen list for allowed_tags. Specifically, the gem mutates the provided frozen list internally, which leads to a RuntimeError (can't modify frozen Array) or unexpected behavior.
This mutation is unexpected because frozen objects are intended to remain immutable, and the gem should ideally create a copy of the provided list if internal modifications are required.
So, to avoid this problem we had to use .dup to create a shallow copy, which should be happening internally.
@flavorjones
Copy link
Member

@ayzahamid Sorry you're having a problem. I'm not sure I understand, can you provide more information, please? What is the error or exception you're seeing? Can you provide a reproduction so I can diagnose what's happening?

@ayzahamid
Copy link
Author

@ayzahamid Sorry you're having a problem. I'm not sure I understand, can you provide more information, please? What is the error or exception you're seeing? Can you provide a reproduction so I can diagnose what's happening?

For rails-html-sanitizer gem version 1.6.1, I am facing this issue where i have a frozen array named ALLOWED_TAGS and i pass that as tags in:

new.sanitize(html, tags: ALLOWED_TAGS, attributes: ALLOWED_ATTRIBUTES)

it mutates the ALLOWED_TAGS internally and i get the below error:
image

@flavorjones
Copy link
Member

@ayzahamid I'm sorry you're having this problem. You're right, in this case we should be duping the list internally, and I'll work on a fix.

In them meantime, you should know that the reason this gem is attempting to modify the allowed tags is because mglyph is unsafe to use. It, along with noscript and malignmark are removed from the allowed tags if they are present, as noted in the GHSAs linked from the CHANGELOG.md file.

If you remove them from your list, you should be fine.

I will get a bugfix release out as soon as I'm able.

@flavorjones flavorjones mentioned this issue Dec 12, 2024
Merged
@flavorjones
Copy link
Member

See #196

flavorjones added a commit that referenced this issue Dec 12, 2024
@flavorjones
fix: PermitScrubber accepts frozen tags
5843d4d 
Previously, if an invalid/unsafe tag was present, the scrubber
attempted to modify the tags array. Now it properly copies the tags
when they are assigned.

Fixes #195

(cherry picked from commit 9a1e376)
@flavorjones
Copy link
Member

Fixed in Release v1.6.2 / 2024-12-12 · rails/rails-html-sanitizer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: PermitScrubber accepts frozen tags rails/rails-html-sanitizer
2 participants
@flavorjones @ayzahamid