Problem
- It took several weeks of active development, multiple security team reviews, and overall, years to allow stupidly simple images in user input onto drupal.org.
- 99.9% of all Drupal site builders and administrators aren't even aware of possible security issues.
Goal
- Allow end-users to use images on Drupal sites.
- Ensure that Drupal sites are secure.
Details
- Local image input filter got invented to allow images on drupal.org.
Proposed solution
- Move the local image input filter into core for Drupal 8.
- Keep it as a separate filter, since its functionality is distinct from the HTML filter functionality. (It is possible to combine it with alternative HTML filters like HTMLPurifier, and it's even possible to run the filter on Markdown input [after Markdown processing].)
Notes
- This patch moves the functionality into Drupal core. A direct copy and paste without changes, except for API changes since D6.
- In order to make this happen for D8, I'd highly recommend to get this base functionality in first. Optimize and revise the user experience, along with a possible integration into HTML Filter (Limit allowed HTML tags) in a separate issue, afterwards.
Comment | File | Size | Author |
---|---|---|---|
#22 | Local_images.png | 17.03 KB | mgifford |
#20 | Screenshot-Full_HTML_Restrict_Images.png | 151.69 KB | mgifford |
#20 | Edit_Content_With_Remote_Images.png | 150.52 KB | mgifford |
#16 | platform.filter-image.16.patch | 9.19 KB | sun |
#16 | interdiff.txt | 6.98 KB | sun |
Comments
Comment #1
sunForgot to replace this getcwd() with DRUPAL_ROOT.
Comment #3
sun#1: drupal8.filter-html-image-secure.1.patch queued for re-testing.
Comment #4
sunThis patch is complete in terms of features and tests. RTBC, anyone?
Comment #5
sun#1: drupal8.filter-html-image-secure.1.patch queued for re-testing.
Comment #7
Everett Zufelt CreditAttribution: Everett Zufelt commented+ $alt_text = t('Only local images are allowed.');
I'm not sure that this is the best alt text, hopefully others will provide feedback.
When drafting alt text the question is: what text would be required if the image were missing. Interestingly here we are drafting alt text about an image that represents a missing image (which is missing for a particular reason).
Comment #8
sunAlso, this should probably be themeable:
Comment #9
andrewmacpherson CreditAttribution: andrewmacpherson commentedRe: comment #7
That's a really interesting scenario, Everett. Here are some thoughts of mine.
So for the local image filter, we might have something like this:
[Edit:] on further thought, the text presented would depend on whether we're looking at a preview in an edit tab, or the view tab.
Comment #10
mgiffordCan someone re-roll this patch?
Comment #11
mgiffordThe solution in #9 looks good to those of us at the A11ySprint. Changing tag.
Comment #12
sunRe-rolled against HEAD.
Includes better alt/title attribute as suggested in #9, but without the original image filename/URL.
Comment #13
sun#12: drupal8.filter-html-image-secure.12.patch queued for re-testing.
Comment #15
sunUpdated test for new $modules property.
Comment #16
sunRemoved irrelevant information about error condition from filter tip.
Fixed comments, use proper PHP multibyte wrapper functions, and strict string comparison.
Added a CSS class to aid in styling, and an alter hook for the invalid image DOMElement.
Replaced alter hook with a theme function.
Cleaned-up test comments.
Comment #17
sunFWIW, I'm pretty happy with this code now.
Includes a theme function as mentioned in #8 (albeit a bit special).
Still comes with tests, and passes them. :)
Comment #17.0
sunUpdated issue summary.
Comment #18
sunTagging.
Comment #19
sunFeedback, anyone?
Comment #20
mgiffordI'd like to see this in D8. Will note that the bulk of the description is actually going into the title rather than the alt text.
I'm also tagging this as security. Ultimately this should get more review from that team rather than the accessibility folks.
I applied this patch, but my tests of it didn't seem to give me the effect I wanted. I still saw the LOL cat I pasted into my full html (with that filter applied).
Comment #21
sunRegarding security, this code is still identical to the contrib module, which went through extensive security team reviews already, due to our active usage on drupal.org. Therefore, I don't think there's any need to redo that audit.
That is odd. Are you sure that you configured the filter first? (Alternatively, flushed [field/content] caches after enabling the filter?) The correct behavior is fully covered by tests, so there must have been something wrong in the manual testing.
Comment #22
mgiffordI should have been clear that I wasn't asking for this code to specifically have a security review, but that this has security implications (by tightening up Drupal's security). Maybe there's a way to reach out to others who are watching issues related to security.
I'm curious if there's a better explanation of the security problem than:
https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet
EDIT: Hit submit a bit early. I had yet to say that when I installed this from scratch & cleared caches then it all worked as expected. The code above was used in my testing where the results are attached below as a local image...
Comment #23
sunAwesome! Thanks for the positive manual testing confirmation! :)
From my perspective, we just need an RTBC here.
As mentioned in the issue summary, let's not get into the architectural purity trap. I am fully aware of all related issues, which may or may not change the filter system to allow for such filters to run in a more sophisticated text processing phase. However, those issues are anything but complete, and they not even have remotely complete/committable patches. I am also aware that exposing this filter as an own, separate filter is suboptimal, but as mentioned and clarified in the issue summary, it would be wrong to limit this filter to the (Allowed) HTML filter.
The primary purpose is to increase security on Drupal sites, whatever it takes. What's good enough for Drupal.org is good enough for 200,000+ Drupal sites. Needless to say, the mere exposure of this filter increases sensibility for security.
Comment #24
mgiffordI'm happy to do it, but think it would be beneficial to get someone else involved first. @grendzy seems like a logical choice to give it one more look over. Agreed about making more sites secure. I'd done a review of security modules recently and totally missed Local image input filter.
Comment #25
sun#16: platform.filter-image.16.patch queued for re-testing.
Comment #26
mgiffordLet's get this in Core.
Comment #27
webchickAwesome, really happy to see this hit RTBC!
Unfortunately, we are currently over thresholds, so this patch can't be committed until we start closing down some criticals. :(
Comment #28
webchickAwesome, we are under thresholds, for the moment anyway! :)
This code was extremely thorough reviewed when we added it to Drupal.org, so I'm not too worried there. I eyeballed it again and it seems to be all the same stuff. This is a really useful addition, because it allows people to enable the img tag on their sites without worrying about being totally pwned. Therefore...
Committed and pushed to 8.x (along with a CHANGELOG.txt entry). Thanks! I expanded the commit message to credit everyone listed in the commit log at http://drupalcode.org/project/filter_html_image_secure.git/shortlog. I seem to remember some others, too, but don't recall atm.
Comment #29
Wim LeersThis is GREAT stuff. (If I could make that "GREAT" red, bold and blinking, I would.) Thank you, @sun et al :)
Comment #30.0
(not verified) CreditAttribution: commentedUpdated issue summary.