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.

CommentFileSizeAuthor
#3 filter_html_image_secure.cleanup.3.patch2.05 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

We 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 ;)

sun’s picture

Oh, 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

.comment img {
  max-width: 560px;
}

Depending on visible sidebars or not, other page content or not, the value needs to be different.

sun’s picture

Status: Postponed » Needs review
FileSize
2.05 KB

#1 and #2 combined.

webchick’s picture

I would say image sources vs just images in the info description. Otherwise looks good.

webchick’s picture

Created #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.

sun’s picture

Status: Needs review » Active

While 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.

grendzy’s picture

FWIW, 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:

  • Files directory a symlink
  • Reference to a local file that doesn't exist
  • Image element with no src attribute
  • external domain that contains, but doesn't match, e.g. drupal.org.com
  • image with onClick attribute
  • The infamous src=/logout
sun’s picture

@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.

grendzy’s picture

Yikes, I did not realize that regex was such a hornet's nest. Thanks for the explanation though.

chx’s picture

It seems we are good to go?

webchick’s picture

Ok, 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. :)

sun’s picture

Status: Active » Fixed

Thanks everyone!

sun’s picture

Thanks everyone!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.