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:
Before patch is applied

After:
After patch is added

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

droplet’s picture

Status: Active » Needs review
biguzis’s picture

Status: Needs review » Needs work

The last submitted patch, 3: filter-chars-limits-2579471-3.patch, failed testing.

The last submitted patch, 3: filter-chars-limits-2579471-3.patch, failed testing.

Status: Needs work » Needs review
biguzis’s picture

#3 first time failed, second time passed. Works on my local. IMO ready for RTBC.

droplet’s picture

Priority: Normal » Major
Issue tags: +Quick fix, +Novice

It'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

dawehner’s picture

Once you have a textarea, don't you have to deal with newlines then?

thorandre’s picture

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

thorandre’s picture

thorandre’s picture

Issue summary: View changes
swentel’s picture

Priority: Major » Normal

Did a quick test just copy/pasting the defaults of this field

<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption> <a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption> <a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption> <a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption>

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.

droplet’s picture

Forcing 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.. )

so tbh, hitting that limit almost brings you to full html

You can't use FULL HTML if you blocked any single tags for Security

walangitan’s picture

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

droplet’s picture

Issue summary: View changes
Issue tags: +Security

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

walangitan’s picture

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

swentel’s picture

Status: Needs review » Needs work
Issue tags: -Security +Security improvements

This 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:

  • The newlines are saved, this should not happen (see underneath)
  • As a consequence, you get a javascript error: Uncaught TypeError: Cannot read property 'tagName' of null - you see in before the submit

We should have a way to disallow entering new lines because it breaks the javascript before submission too.

settings:
      allowed_html: "<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption>\r\n<table> <tr> <td>"

The real quick fix then (if we want to keep this novice) is to set the maxlength to 2056 for instance.

droplet’s picture

@swentel,

(because we can discuss that for hours)

:)

--

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)

The real quick fix then (if we want to keep this novice) is to set the maxlength to 2056 for instance.

Thanks for the suggestions :)

walangitan’s picture

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

walangitan’s picture

Status: Needs work » Needs review
swentel’s picture

Note - not sure why I came up with 2056 :) 2048 seems more 'logical' because it's the double of 1024 - anyway, works for me.

walangitan’s picture

Updated patch to use 2048 :).

walangitan’s picture

jibran’s picture

Status: Needs review » Reviewed & tested by the community

All the feedback from @swentel is addressed so RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Can 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!

  • alexpott committed e662dec on 8.1.x
    Issue #2579471 by walangitan, droplet, biguzis, thorandre, swentel:...

  • alexpott committed 988c359 on
    Issue #2579471 by walangitan, droplet, biguzis, thorandre, swentel:...
walangitan’s picture

Follow up issue for converting this input to a textarea and resolving the newline issue that results from it can be found here.

alexpott’s picture

Status: Fixed » Closed (fixed)

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

shaundepp123’s picture

Can 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

jonnysimpkin123’s picture

This 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

KaiJames’s picture

this 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