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 |
---|---|---|---|
#86 | filter_tips_improperly-2346209-86.patch | 3.28 KB | cburschka |
#84 | filter_tips_improperly-2346209-83-pure.patch | 5.06 KB | cburschka |
#73 | filter_tips_improperly-2346209-73.patch | 15.17 KB | keopx |
#73 | interdiff-filter_tips_improperly-2346209-70-73.txt | 762 bytes | keopx |
#70 | filter_tips_improperly-2346209-70.patch | 15.15 KB | keopx |
Comments
Comment #1
realityloopThe attached patch partially fixes the issue however some of the output is still incorrectly excaped, see image for after patch applied
Comment #2
realityloopComment #3
realitylooptagging
Comment #4
realityloopComment #5
pguillard CreditAttribution: pguillard commentedComment #6
LinL CreditAttribution: LinL commentedAdding parent issue.
Comment #7
pguillard CreditAttribution: pguillard commentedThis is what we should have, right ?
Comment #8
realityloop@pguillard that looks better but I don't know the code tags are required around the character examples.
Comment #9
pguillard CreditAttribution: pguillard commentedI also wonder. I'm preparing thé patch
Comment #10
pguillard CreditAttribution: pguillard commented@realityloop : I also wonder. I'm preparing thé patch
Comment #11
Hanno CreditAttribution: Hanno commentedthe code tags should not be visible as typed tags, but we need them in the html for semantics (a11y)
Comment #12
pguillard CreditAttribution: pguillard commentedWhat do you think of this patch ?
I introduced a private function there for visibility, maybe not a good idea...
Comment #13
pguillard CreditAttribution: pguillard commentedComment #14
pguillard CreditAttribution: pguillard commentedIt seems I forgot to upload that patch
Comment #16
pguillard CreditAttribution: pguillard commentedOops
Comment #17
Zekvyrin CreditAttribution: Zekvyrin commentedPatch #16 is working.
I'm not sure about the function, but I think that htmlentities function should be replaced.
The proper way as written in https://www.drupal.org/node/2311123, would be using #context variables (like option two example).
If not, maybe Drupal8's String::checkPlain is preferred.
Also think the boolean variables name should be different (and I fixed a minor typo in "boolean").
I've uploaded a patch using the function, and I'll try to make one without the function.
Comment #18
Zekvyrin CreditAttribution: Zekvyrin commentedSorry, forgot to upload the patch
Comment #19
organicwire CreditAttribution: organicwire commentedPatch #17 is working for me. And, according to #2311123, using the #context array is a good idea.
Comment #20
organicwire CreditAttribution: organicwire commentedSummary
The problem was that on the filter tips page (filter/tips) the permitted html was inproperly escaped. See screenshot Compose tips | Site-Install 2014-09-28 11-01-57.png.
Work being done
#16 fixes this
#18 improved the patch
#19 confirms that the patch works
TODO
We need someone to review the code.
Comment #21
martin_qThe code looks OK to me (also a novice in D8) but I note that under option two in #2311123,
Is this not such a situation?
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedI reviewed the code and from #18 and I think its is fine.
Comment #23
Zekvyrin CreditAttribution: Zekvyrin commented@martin_q: I don't think this applies here, but still I'm not sure.
Because we don't know if this needs work to be done or not for sure, this issue's status should be CNR not CNW.
If someone is sure that in needs to change or not can either change it to CNR or RTBC.
Comment #24
realityloopThe code looks good and it fixes the problem
Before:
After applying #18:
Comment #25
alexpottWhy
Why do we need to do this?
The docblock is not done according to our coding standards.
why is the function private? What is wrong with protected?
No need to call drupal_render() here. It will be called on the table.
Comment #26
Zekvyrin CreditAttribution: Zekvyrin commented@alexpott:
1. If this code is not added, the resulted page will be:
I think I fixed #2-3-4 with this patch.
If it's something more with docblock's coding standards, please tell me and I'll try to fix it.
Comment #27
jibranComment #28
joelpittetThanks for the fix @Zekvyrin it looks really good and I like the approach!
We don't need this Xss::filterAdmin with the inline_template because it will be checkPlained by autoescape in the inline template. And it was originally checkPlain and not filterAdmin.
This looks like it could be more generic for all of Drupal, what do you think?
' : $content,
The content variable needs spaces inside the two curly brackets for coding standards:
{{content}}
should be{{ content }}
Comment #29
aneek CreditAttribution: aneek commented@Zekvyrin, nice patch. Really like the idea about
getInlineTemplate()
method. I do agree with @joelpittet, we could make this method work more. Like available to the whole system not just the FilterHtml.php or it's child classes.Few feedbacks,
1. More detailed description will be nice. About the method, and what it does in more details.
If
$is_html_code
is FALSE or NULL then$content
is printed as is. Then I suppose we don't need to add variables in#context
.Thanks!
Comment #30
Zekvyrin CreditAttribution: Zekvyrin commentedI've uploaded a patch fixing coding standards (joelpittet 3rd point) and changing '#context' array (aneek's comment) not to pass context to inline template if not needed.
Patch still needs work for the reasons below:
@joelpittet 1st comment:
Try my patch without this change and you'll see something like the screenshot I got in #26.
Things is that this page contains some sections (one for each text format),
and each section has a unordered list (ul) with one item each filter in this format.
For example in Restricted html there is one list item for:
Each item contains html & possibly other templates, which gets escaped when checked plain...
Using this line, we are checking the whole html of each item for possibly xss, and "marking" is safe (so it's not checked plain afterwards). If we let it to be checked plain, we'll get the result like the screenshot I got in #26.
Maybe we need to change the logic somehow (I was thinking that we might need to check every possible filter tip to be a template as an alternative), instead of adding this line, but without it we have all these sections getting double escaped. But we definitely need to do something there...
@aneek: well for the record it wasn't my idea adding a function :P
I just improved a previous patch.
I do agree with both that this function can be more generic and be used elsewhere as well.
But I guess it needs another name and be inserted somewhere else ( I don't know where).
maybe a more generic which allows the developer to choose a tag:
getInlineTemplateforTag($content, $is_html_code, $tag)
where $tag variable holds the string for the tag (for example in our case $tag = "code").
What do you think?
Comment #31
subhojit777Invoking testbot
Comment #32
aneek CreditAttribution: aneek commented@Zekvyrin, checked with patch #30, page UI output is as expected.
Now, about the new method we are talking about. Before making one, let's think about a bit.
First, what the method will do? I think, this method can create "inline_template" based on the tag and the content we supply via the arguments. So, instead writing the "inline_template" based on the api https://www.drupal.org/node/2311123 we can use this new method to generate "inline_template" dynamically.
Second, if the method is written then what will be it's scope? What will be the possibility that other modules or core or other developers will use this to write "inline_template"? Since writing "inline_template" and generating "inline_template" via calling the method with parameters can take more or less same efforts from a developer. So the main question is will this method become useful to others?
Third, creating this method to generate "inline_template" can have a performance factor as well. In this patch we only see that a simple "code" tag is used. To make this function generic we need to allow others to pass any HTML tags and n numbers of HTML attributes in it. So I can pass
<div class="xx" id="yy" attr1="zz"></div>
etc. Also some one can pass malicious attributes as well. In that case, we need to check about each tags and attributes for sensitizations. Then only actual "inline_template" can be generated from this method. So what we are doing? We are just using many if-else conditions and some array iterations and a bunch of class declarations likenew Attribute()
,SafeMarkup::checkAdminXss()
etc. Again the same question comes, do we really need this to happen just to generate a page?But if we see there is a very good chance that this method can be used widely throughout the system then we might have this method.
I'd really like to hear what you think @Zekvyrin & @joelpittet and what others has to say about this.
Thanks!
Comment #34
subhojit777From what @aneek mentioned in #32, I do not think we need a separate function.
' : $content,
+ '#context' => $context,
+ );
This can be done here
Aneek's third comment is very important. It will be an overhead if we want to make the function generic.
Comment #35
subhojit777Removed function.
As for
$tiplist[$tip_key]['tip'] = Xss::filterAdmin($tiplist[$tip_key]['tip']);
, I think we cannot remove this. I am no expert to decide this, but I think this is what is happening - #1825952: Turn on twig autoescape by default is coming into action, it is escaping HTML while rendering,XSS::filterAdmin()
makes sure that the string will not be escaped during rendering.Comment #36
subhojit777Back to needs work for tests
Comment #37
idebr CreditAttribution: idebr commentedI think these 'type' and 'get' classes are missing in the patch as well
Comment #38
joelpittetThe classes are in there,
though they should be on the next line.
Comment #39
idebr CreditAttribution: idebr commented@joelpittet Those are the classes indeed, but it's a different code block.
This code maintains the type and get class
In this case the class 'type' and 'get' are missing
Comment #40
joelpittetOh I missed that, thanks, those are missing in the second hunk.
Comment #41
jackbravo CreditAttribution: jackbravo commented@idebr found this issue which is also being worked at #2388299: Fix markup on filter/tips page..
Comment #42
subhojit777I think this does not needs tests. This bug can be tested "visually", and since the filter tips is kind of static I guess it does not needs tests. Sorry for the confusion. I am leaving this to committer (and others) whether this bug needs tests.
Comment #43
idebr CreditAttribution: idebr commented@subhojit777 The tests are not strictly necessary for this issue, but think of it as a safeguard so our valued patches are not broken by accident in the future :). For reference, have a look at the meta issue #2297711: Fix HTML escaping due to Twig autoescape to verify the issues that have been fixed before that included a test.
To summarize:
- This issue needs a basic test to ensure markup is properly escaped
- The missing classes I discussed with @joelpittet in #38/#39/#40 still need to be fixed
Comment #44
subhojit777Comment #45
subhojit777Comment #46
subhojit777Comment #48
subhojit777Patch with only tests failing as expected.
Comment #49
m4oliveiTagging as Needs reroll. No longer applies clean against HEAD. Re-rolling..
Comment #51
keopxComment #53
m4oliveiRe-roll. This patch applies cleanly at commit 4b1714a. I also spread the array out for code style.
Comment #54
keopxComment #55
pguillard CreditAttribution: pguillard commentedI quit this issue a long time ago, I can't tell about the code now, but the result on the filter page seems to be what we need !
Comment #57
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedUse SafeMarkup instead of String on testFilterTipHtmlEscape to fix the error
Comment #58
joelpittetThis patch is looking pretty close! Just one question and one little nitpick.
I'm curious if this is still needed?
Can probably go short array syntax on the changed hunks.
Comment #59
keopxCreate new version of patch removing deprecated drupal_render function.
Comment #60
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedConsidering PHP documentation:
"As of PHP 5.4 you can also use the short array syntax, which replaces array() with []."
Can you review this interdiff?
In case you were correct, I apply it to the rest of change and upload the patch?
Thanks.
Comment #61
rteijeiro CreditAttribution: rteijeiro commented@dimaro, go ahead. interdiff looks good.
Comment #62
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedAdd the changes made in #59.
I used the short array syntax throughout the method because I think we should use only one syntax.
Comment #63
keopxLooks good.
Comment #64
rteijeiro CreditAttribution: rteijeiro commented@regiguren, could you provide a couple of screenshots, before and after the patch, please?
Comment #65
keopxComment #66
keopxHere screenshots
Comment #67
rteijeiro CreditAttribution: rteijeiro commentedI think this should be correctly formatted and add the missing trailing comma.
Comment #68
keopxComment #69
keopxComment #70
keopxComment #73
keopx@rteijeiro thanks for cancel test
Comment #74
keopxComment #77
rteijeiro CreditAttribution: rteijeiro commentedLooks RTBC to me!
Comment #78
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedRTBC++
Comment #81
lauriiiI guess its RTBC again
Comment #82
alexpottSo #59 to #62 converted this to use a short array syntax basically making this issue exceptionally difficult to review. Can someone please reroll this patch to only do what is necessary to fix the bug reported in the issue summary.
Comment #83
cburschkaDidn't want to rip it out manually and there isn't a script that reverts the short array syntax... but I was able to do the conversion first, commit it, then 3-merge in the patch, commit it, then cherry-pick the second commit onto the original. Git is awesome. :D
Comment #84
cburschkaOne slipped through. This patch only changes the arrays that are getting changed anyway.
Comment #85
alexpottLet's just use
SafeMarkup::format('<span class="x">@var</span>', array('@var' => $var));
here instead and then there'll be only one line of difference. See option 4 on https://www.drupal.org/node/2311123Out of scope change
As above.
Comment #86
cburschkaThis should do it.
Comment #87
keopxWell done @cburschka
Comment #88
alexpottThis issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 0af6777 and pushed to 8.0.x. Thanks!