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.
Comment | File | Size | Author |
---|---|---|---|
#19 | 2564283.19.patch | 8.83 KB | alexpott |
#18 | 2564283.18.patch | 8.81 KB | alexpott |
#18 | 12-18-interdiff.txt | 519 bytes | alexpott |
#15 | Content__Content____Site-Install.png | 119 KB | joelpittet |
#12 | remove_use_of-2564283-12.patch | 8.57 KB | joelpittet |
Comments
Comment #2
stefan.r CreditAttribution: stefan.r commentedComment #3
stefan.r CreditAttribution: stefan.r commentedTook these "as is" out of the parent patch
Comment #4
stefan.r CreditAttribution: stefan.r commentedincluding a fix to https://www.drupal.org/node/2545972#comment-10225417 (from #74 in the parent)
Comment #5
stefan.r CreditAttribution: stefan.r commentedreverting an unrelated change
Comment #8
alexpottSo
Drupal\views_ui\Tests\XssTest
in HEAD is hilarious - it is testing for double escaping :)Removed a few
Html::escape()
- the whole point here is to rely on Twig to autoescape.Comment #9
alexpottNeeded a reroll.
Comment #10
stefan.r CreditAttribution: stefan.r commentedLooks like we may have some double escaping here?
This one is double escaped both in HEAD and in the patch:
These look the same:
Comment #11
alexpott@stefan.r can I get some steps to reproduce - thanks.
Comment #12
joelpittetAdding a test for double escaping. With the steps to reproduce we can add to this.
Comment #13
dawehnerQuick note: EntityRow.php no longer needs the
SafeMarkup
use statement.This is so much more readable!
Nice test!
Comment #14
alexpottBugs that occur in HEAD and patch found whilst testing:
Afaics this patch does make anything and definitely will make those problems easier to solve.
Comment #15
joelpittetWhile trying to reproduce this I found an actual alert(), which is rare consider we escape things like gangbusters.
<script>alert('Title');</script>
in every field possible on article.Will try to figure out where that is coming from.
Comment #16
joelpittetre: #15 it's the field label value, it's on click, and this patch doesn't change it (bug in head), needs a follow-up.
Comment #17
joelpittetFollow-up added #2567673: Escaped markup with $.text() injected into a tag will cause XSS for #15
Comment #18
alexpottAddress pre 1 of #13.
Comment #19
alexpottRerolled as #2492839: Views replacement token bc layer allows for Twig template injection via arguments landed
Comment #20
dawehner@joelpittet I think this is covered by #2564283: Remove use of SafeMarkup::checkPlain() from adminSummary() and adminLabel() in views plugins
Comment #21
dawehnerWell, then nevermind my last comment.
Comment #22
joelpittetRTBC++ I'm in a recursive comment! in a recursive comment!
Comment #23
joelpittetThe double escaping mentioned in #10 looks to be mostly there before this patch. Going to need to track that down and add some double escaping tests for that. @stefan.r could you hook us up with your steps to reproduce that?
Comment #24
dawehner@joelpittet
Yeah I saw similar things as part of #2506479: Replace !placeholder with @placeholder for non URLs in t() in Views, except for t() output that is used as an attribute value
Comment #25
joelpittetOh wasn't following that one yet, thanks for the heads up
Comment #27
catchCommitted/pushed to 8.0.x, thanks!