Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When the module's ready for deployment, we'll need a 1.0 release rolled so we can get it into BZR on d.o.
Comment | File | Size | Author |
---|---|---|---|
#3 | filter_html_image_secure.cleanup.3.patch | 2.05 KB | sun |
Comments
Comment #1
sunWe should review and fix the UI, strings, configuration, .info, and also the project page prior to a release, I think.
Note: I'll be mostly away until Monday (drupalcity.de)... just in case, I'll add you to the maintainer list. That said, due to the security aspect, let's make sure to treat this thingy, patches, and reviews as if it was a module in core ;)
Comment #2
sunOh, and I also wanted to add, we should revert that max-width feature -- that doesn't belong into a filter module.
That's pure CSS and looks like
Depending on visible sidebars or not, other page content or not, the value needs to be different.
Comment #3
sun#1 and #2 combined.
Comment #4
webchickI would say image sources vs just images in the info description. Otherwise looks good.
Comment #5
webchickCreated #1289434: Restrict user-uploaded images to a maximum width for fixing this in bluecheese. I still think this would be a useful feature to retain in this module though. It's sad that using this now requires theme patches rather than being an all in one solution and since we're manipulating the output of the image anyway I don't see the harm.
Comment #6
sunWhile I can perfectly understand that it would be nice to have an all-in-one solution, this is a conceptual problem caused by architectural limitations in Drupal core. As long as filters do not have context, and filtered text is not cached per page/theme/context, that max width feature cannot really work as intended.
Incorporated #4, committed, and pushed to 6.x-1.x.
Guess that means back to active, as long as we're waiting for a security team review.
Comment #7
grendzy CreditAttribution: grendzy commentedFWIW, here's a review on behalf of the security team:
In short everything looks good and IMO is ready for release.
What is the purpose of the regex
'|<([^> ]*)/>|i'
? The only thing I can think that would ever match is a self-closing tag with no attributes and no spaces (like<br/>
). But even when I remove the regex,<br />
still comes out with the space as expected. Anyway, this could use an inline comment. I was also surprised at first by the use of '|' as a regex delimiter - could '#' be used instead?On the security side, here are some of the things I tested:
Comment #8
sun@grendzy: That code is an intentional, straight backport from D7+. @chx asked himself the same; soon to be fixed in #1276042: preg is used on html + then backported to this module.
Comment #9
grendzy CreditAttribution: grendzy commentedYikes, I did not realize that regex was such a hornet's nest. Thanks for the explanation though.
Comment #10
chx CreditAttribution: chx commentedIt seems we are good to go?
Comment #11
webchickOk, since the last remaining issue was the security team review, and since #1276042: preg is used on html is still in discussion, and since the preg that issue is fixing doesn't actually affect the security of this module, I'm planning to roll a 1.0 release of this module later today. sun, if you are around and have objections to this, let me know. We miss you in IRC. :)
Comment #12
sunThanks everyone!
Comment #13
sunThanks everyone!