Problem/Motivation
There's should be no chars limits in "Limit allowed HTML tags" to allow more advanced configs. For example, with default config and add left / right / center alignment features will exceed 1024 chars.
** Limits on Chars implied to force site builder use FULL HTML which is a Security issue. **
Proposed resolution
- Change from textfield type to textarea
Remaining tasks
User interface changes
- replace "input" field with "textarea" field
API changes
Data model changes
Tested patch
Patch has been manually tested:
Before:
After:
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-2579471-20-22.txt | 1003 bytes | walangitan |
#23 | filter-chars-limits-2579471-23.patch | 1.09 KB | walangitan |
#11 | Basic_HTML_8_0_0-dev_before.jpg | 69.17 KB | thorandre |
#10 | Basic_HTML_8_0_0-dev_after.jpg | 81.03 KB | thorandre |
#3 | filter-chars-limits-2579471-3.patch | 1.26 KB | biguzis |
Comments
Comment #2
droplet CreditAttribution: droplet commentedComment #3
biguzis CreditAttribution: biguzis at Wunder commentedrerolled.
Comment #7
biguzis CreditAttribution: biguzis at Wunder commented#3 first time failed, second time passed. Works on my local. IMO ready for RTBC.
Comment #8
droplet CreditAttribution: droplet commentedIt's pretty important. In D8, it's very easy to hit this limits. It's not good to fix in contri or wait until 8.1
Comment #9
dawehnerOnce you have a textarea, don't you have to deal with newlines then?
Comment #10
thorandre CreditAttribution: thorandre as a volunteer and commentedDid a manual test using filter-chars-limits-2579471-3.patch and it works as expected. Adding screenshots to the summary.
@dawehner about newlines:
Not sure if you're asking about tests, risks or edgecases, but if you're not: I tried to add som linebreaks in the area, and they where removed on save. So it does not seem to be an issue, allthough this is just from a sitebuilders perspective.
Comment #11
thorandre CreditAttribution: thorandre as a volunteer and commentedComment #12
thorandre CreditAttribution: thorandre as a volunteer and commentedComment #13
swentel CreditAttribution: swentel commentedDid a quick test just copy/pasting the defaults of this field
That's a lot of tags imo, so tbh, hitting that limit almost brings you to full html anyway, so I'm not sure how much this would help, besides a bit more UX because you'll have faster view on which tags are actually in there. Also, this isn't major at all.
Comment #14
droplet CreditAttribution: droplet commentedForcing to do bad thing is Critical issue in my mind. Looking at https://www.drupal.org/node/45111, I think it's Critical. I wonder if I should call it BUG now. It tends to be a bug more than task after some thoughts.
Now the CKEditor toolbar config do not sync with bottom tag inputs. When you adding "left / right / center alignment". It more than extra 30 chars per tag.
Case 1:
30 tags * 30 chars = 900 + above 1000 chars = 1900.
Case 2:
above 1000 chars + assumed 5 tags with "class" = 1030 ( Not really edge case, anyone required a little more than default will be hit.. )
You can't use FULL HTML if you blocked any single tags for Security
Comment #15
walangitan CreditAttribution: walangitan at Chromatic commentedI do think that the CKEditor toolbar config not syncing with the allowed HTML filters at the bottom is not ideal. I can see this leading to frustration for users as to why their CKEditor configurations aren't appearing as they would assume if they didn't also include it in an allowed HTML tag. But I tend to agree with swentel here in that the 1024 character limit allows for quite a lot of tags.
I recognize that the "left/right/center" alignment buttons in CKEditor do take up ~30 characters each i.e.
<p class="text-align-center">
, but for the default CKEditor setup, they are the largest tags in terms of characters. The others being<sub>, <sup>, <hr />, <table border cellpadding cellspacing style>, <tbody>, <tr>, <td>, <u>,< s>
. I would say that the two cases that droplet suggests in comment #14, (1900 characters & 1030 characters) are still edge cases. The 1000 character example from comment #13 is a copy and paste of the same tags, which wouldn't be the case in a real world scenario.For me it is difficult to understand how one would have a case that would require more than the current 1024 characters for allowed tags, but still need to limit tags which is why one couldn't use the FULL_HTML option -- and for that case to be common enough to warrant a change here.
Comment #16
droplet CreditAttribution: droplet commented@walangitan,
I remember at the time when I posted the issues. I tested with default config with alignment features. So that it may adding more than 30 chars per tags.
---
We need not to find out WHY they hit the limits.
the main problem is: We Will Hit the Limit at Some Point. And We CANNOT use FULL HTML instead because the Security issues.
Comment #17
walangitan CreditAttribution: walangitan at Chromatic commented@droplet - I understand now. I don't see an issue with patch #3 after testing. Looking at how the configurations are stored in the database, there doesn't look to be a limitation for changing from a textfield to a textarea. I would defer to more input from the community on this though.
Comment #18
swentel CreditAttribution: swentel commentedThis behavior has been there since ages (#254725: Maxlength field for Allowed HTML tags too short and no-one has ever complained about it since then. Let's not focus on security or the state of the issue (because we can discuss that for hours)
Note that I don't oppose the patch, but needs work for two reasons:
We should have a way to disallow entering new lines because it breaks the javascript before submission too.
The real quick fix then (if we want to keep this novice) is to set the maxlength to 2056 for instance.
Comment #19
droplet CreditAttribution: droplet commented@swentel,
:)
--
We only have advanced attrs config from D8.
There's about 120 HTML elements in HTML5 SPECS and able to fit into 1024 chars. (So It's not a big issue for D7 in most cases)
Thanks for the suggestions :)
Comment #20
walangitan CreditAttribution: walangitan at Chromatic commentedThe patch attached here is for increasing the maxlength to 2056. If we want to move in the other direction (changing the form element to a textarea & not allowing newlines to be saved), we can create that patch as well.
Comment #21
walangitan CreditAttribution: walangitan at Chromatic commentedComment #22
swentel CreditAttribution: swentel commentedNote - not sure why I came up with 2056 :) 2048 seems more 'logical' because it's the double of 1024 - anyway, works for me.
Comment #23
walangitan CreditAttribution: walangitan at Chromatic commentedUpdated patch to use 2048 :).
Comment #24
walangitan CreditAttribution: walangitan at Chromatic commentedComment #25
jibranAll the feedback from @swentel is addressed so RTBC.
Comment #26
alexpottCan we get a follow to change this to a textarea and to handle the new line issue. I think that that makes a lot of sense.
Committed 988c359 and pushed to 8.0.x and 8.1.x. Thanks!
Comment #29
walangitan CreditAttribution: walangitan at Chromatic commentedFollow up issue for converting this input to a textarea and resolving the newline issue that results from it can be found here.
Comment #30
alexpottComment #32
shaundepp123 CreditAttribution: shaundepp123 as a volunteer and commentedCan we get a comply with to exchange this to a text area and to address the new line trouble. I assume that that makes a number of feel...Brothers Construction Group
Comment #33
jonnysimpkin123 CreditAttribution: jonnysimpkin123 commentedThis is an informative blog by which I have got that info which I really wanted to get. I assume that that makes a number of feel... Advance Build
Comment #34
KaiJames CreditAttribution: KaiJames commentedthis blog is realy appreciated its realy good and this sight give great information i realy want to use this blog will u. Its just like Birch Contractors