Skip to content

Implement whitelist for sanitizer: allowed_html_elements_with_attributes() #53

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
Merged
Alkarex merged 12 commits into from
Oct 29, 2025
Merged

Implement whitelist for sanitizer: allowed_html_elements_with_attributes() #53

Alkarex merged 12 commits into from
Oct 29, 2025

Conversation

@Inverle
Copy link
Member

@Inverle Inverle commented Sep 14, 2025

@Inverle Inverle mentioned this pull request Sep 14, 2025
Merged
@Inverle
Copy link
Member Author

Inverle commented Sep 14, 2025

Maybe now

@Alkarex
Copy link
Member

Alkarex commented Sep 14, 2025

Thanks 👍🏻
If you ca, it would be nice to:

  1. Add a minimal test (there are examples in the test directory)
  2. Open the same PR at https://github.com/simplepie/simplepie

@Inverle
Copy link
Member Author

Inverle commented Sep 25, 2025 •
edited
Loading

done (test added, PR not opened yet)

@Inverle
Copy link
Member Author

Inverle commented Sep 27, 2025

Attempting to fix CI, have to wait for approval so no idea if this works

@Inverle Inverle mentioned this pull request Sep 28, 2025
Closed
@Inverle
Copy link
Member Author

Inverle commented Sep 28, 2025

simplepie#947

@Inverle
Copy link
Member Author

Inverle commented Oct 20, 2025

What should be done now?

@Alkarex
Copy link
Member

Alkarex commented Oct 21, 2025

Sorry for the delay. I rewrote the approach to iterate the elements and attributes. I was not so happy of the two layers of XPath.

  1. Please double-check my changes 638515c (not tested)
  2. If that is good enough, please update upstream PR accordingly
  3. We will merge here

@Inverle
Copy link
Member Author

Inverle commented Oct 25, 2025

Please double-check my changes 638515c (not tested)

WIP

Inverle
Inverle commented Oct 25, 2025
View reviewed changes
@Inverle
Improvements, fixes
6a68b68 
* Fixed sanitization output returning `<!DOCTYPE html>` regardless of input
* Custom elements are not allowed anymore
* Added `allow_data_attr` and `allow_aria_attr` parameters
* Don't remove children of the disallowed element
* Replaced `removeAttribute()` with `removeAttributeNS()` to ensure all disallowed attributes are removed
    * For example `xmlns` was not being removed correctly
* Fixed test using incorrect variable `$sanitize` instead of `$sanitize_whitelist`
@Inverle Inverle changed the title Implement whitelist for sanitizer: whitelist_tags() Implement whitelist for sanitizer: allowed_html_elements_with_attributes() Oct 25, 2025
@Inverle
Copy link
Member Author

Inverle commented Oct 25, 2025 •
edited
Loading

If that is good enough, please update upstream PR accordingly

@Alkarex Please review the new changes before I do that (and run the tests workflow)

I have also updated FreshRSS/FreshRSS#7924 for FreshRSS testing. Seems to work fine with my feeds

Alkarex
Alkarex reviewed Oct 26, 2025
View reviewed changes
Alkarex
Alkarex reviewed Oct 26, 2025
View reviewed changes
Alkarex
Alkarex reviewed Oct 26, 2025
View reviewed changes
Alkarex
Alkarex reviewed Oct 26, 2025
View reviewed changes
Alkarex
Alkarex reviewed Oct 26, 2025
View reviewed changes
@Inverle
Copy link
Member Author

Inverle commented Oct 29, 2025

Ready for review

@Inverle @Alkarex
Update src/Sanitize.php
7d50c2b 
Co-authored-by: Alexandre Alapetite <[email protected]>
@Inverle
Copy link
Member Author

Inverle commented Oct 29, 2025

Though I don't understand why removeAttributeNode() and removeAttribute() behave differently

@Inverle
Copy link
Member Author

Inverle commented Oct 29, 2025

Looks like CI didn't print all the unsupported functions at once..

@Alkarex
Copy link
Member

Alkarex commented Oct 29, 2025

Looks like CI didn't print all the unsupported functions at once..

Yes, that would have been likely caught by simplepie#939

@Inverle
Copy link
Member Author

Inverle commented Oct 29, 2025

Everything still works

@Alkarex
Copy link
Member

Alkarex commented Oct 29, 2025

Please double-check 4a16e57 and I think we are good to go

@Inverle
Copy link
Member Author

Inverle commented Oct 29, 2025

Looks fine

@Alkarex Alkarex merged commit 0d953d4 into FreshRSS:freshrss Oct 29, 2025
10 checks passed
@Inverle Inverle deleted the sanitize-whitelist branch October 29, 2025 22:34
Alkarex added a commit to Inverle/FreshRSS that referenced this pull request Oct 29, 2025
@Alkarex
Sync to SimplePie fork
e0ace22 
FreshRSS/simplepie#53
Alkarex added a commit to FreshRSS/FreshRSS that referenced this pull request Nov 1, 2025
@Inverle @Alkarex
Implement whitelist for SimplePie sanitizer (#7924)
500d05f 
* Implement whitelist for SimplePie sanitizer

ref: #7770 (comment)

FreshRSS/simplepie#53
simplepie/simplepie#947

* Remove `<plaintext>` from whitelist

* Improve order

* Remove some tags from whitelist

* Revert partially

* sync

* Display contents of `<noscript>` and `<noembed>`

* sync

* Allow use of `<track>`

* sync again

* Sync to SimplePie fork
FreshRSS/simplepie#53

* Alphabetic order

* Reduce list of stripped attributes

* Temporarily strip some attributes

---------

Co-authored-by: Alexandre Alapetite <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Alkarex Alkarex Alkarex left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Inverle @Alkarex