Opened 7 weeks ago

Last modified 4 weeks ago

#30686 new Bug

Improve utils.text.Truncator &co to use a full HTML parser.

Reported by: Thomas Hooper Owned by: nobody
Component: Utilities Version: master
Severity: Normal Keywords:
Cc: Jon Dufresne Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Carlton Gibson)

Original description:

I'm using Truncator.chars to truncate wikis, and it sometimes truncates in the middle of &quot; entities, resulting in '<p>some text &qu</p>'

This is a limitation of the regex based implementation (which has had security issues, and presents an intractable problem).

Better to move to use a HTML parser, for Truncate, and strip_tags(), via html5lib and bleach.

Attachments (1)

possible-html5lib-truncator-implementation.patch (4.6 KB) - added by Carlton Gibson 6 weeks ago.
Example implemetation of _truncate_html() using html5lib, by Florian Apolloner

Download all attachments as: .zip

Change History (18)

comment:1 Changed 7 weeks ago by Thomas Hooper

Description: modified (diff)

comment:2 Changed 7 weeks ago by Carlton Gibson

Hi Thomas. Any chance of an example string (hopefully minimal) that creates the behaviour so we can have a look?

comment:3 Changed 7 weeks ago by Florian Apolloner

I think now that the security release are out let's just add bleach as dependency on master and be done with it?

comment:4 in reply to:  2 Changed 7 weeks ago by Thomas Hooper

comment:5 Changed 7 weeks ago by Florian Apolloner

btw I confused truncator with strip_tags. So in this case the answer would be to rewrite the parser using html5lib, while split_tags would use bleach which in turn then uses html5lib as well.

comment:6 Changed 7 weeks ago by Thomas Hooper

Looks like it can be fixed with this regex change https://github.com/django/django/pull/11633/files

Changed 6 weeks ago by Carlton Gibson

Example implemetation of _truncate_html() using html5lib, by Florian Apolloner

comment:7 Changed 6 weeks ago by Carlton Gibson

Description: modified (diff)
Summary: Truncator.chars splits HTML entitiesImprove utils.text.Truncator &co to use a full HTML parser.
Triage Stage: UnreviewedAccepted
Version: 2.2master

Right, good news is this isn't a regression from 7f65974f8219729c047fbbf8cd5cc9d80faefe77.

  • The new example case fails on v2.2.3 &co.
  • The suggestion for the regex change is in the part not changed as part of 7f65974f8219729c047fbbf8cd5cc9d80faefe77. (Which is why the new case fails, I suppose :)

I don't want to accept a tweaking of the regex here. Rather, we should move to using html5lib as Florian suggests.
Possibly this would entail small changes in behaviour around edge cases, to be called out in release notes, but
would be a big win overall.

This has previously been discussed by the Security Team as the required way forward.
I've updated the title/description and will Accept accordingly.

I've attached an initial WIP patch by Florian of an html5lib implementation of the core _truncate_html() method.

An implementation of strip_tags() using bleach would go something like:

bleach.clean(text, tags=[], strip=True, strip_comments=True)

Thomas, would taking on making changes like these be something you'd be willing/keen to do? If so, I'm very happy to input to assist in any way. :)


comment:8 Changed 6 weeks ago by Thomas Hooper

Hi Carlton, that would be fun, but this is bigger than I have time for now. It looks like you all have it in hand.

comment:9 Changed 6 weeks ago by Claude Paroz

Do we want to make both html5lib and bleach required dependencies of Django?
html5lib latest release is now 20 months ago, and when I read issues like https://github.com/html5lib/html5lib-python/issues/419 without any maintainer feedback, I'm a bit worried. What about the security report workflow for those libs? What if a security issue is discovered in html5 lib and the maintainers are unresponsive? Sorry to sound a bit negative, but I think those questions must be asked.

comment:10 Changed 6 weeks ago by Carlton Gibson

Yep Claude, absolutely.

I think there's two difficulties we could face:

  • trying to successfully sanitize HTML with regexes.
  • (Help) Make sure html5lib-python is maintained.

The first of these is intractable. The second not. 🙂

I've put out some feelers to try and find out more.

  • This is pressing for Python and pip now, not for us for a while yet.
  • If we look at https://github.com/html5lib/html5lib-python/issues/361 it seems there's some money on the table from tidelift potentially.
  • We COULD allocate some time in a pinch I think.
  • AND it's just (even with the emphasis, cough) a wrapper around the underlying C library, so whilst 20 months seems a long time, I'm not sure the release cadence is really an issue.

BUT, yes, absolutely. Let's hammer this out properly before we commit. 👍
I will open a mailing list thread when I know more.

Last edited 6 weeks ago by Carlton Gibson (previous) (diff)

comment:11 Changed 6 weeks ago by Carlton Gibson

AND it's just (even with the emphasis, cough) a wrapper around the underlying C library, so whilst 20 months seems a long time, I'm not sure the release cadence is really an issue.

OK, that last one isn't at all true. (Looking at the source it's the entire implementation.)

Update: I had `lxml` in mind.

Last edited 6 weeks ago by Carlton Gibson (previous) (diff)

comment:12 Changed 6 weeks ago by Claude Paroz

To be clear, I'm also convinced parsing is more reliable than regexes. I just think we have to double-think before adding a dependency, because as the name implies, we depend on it and therefore we must be able to trust its maintainers. Some guarantees about the security process and serious bugs fixing should be obtained. Without that, we are just outsourcing problems.

comment:13 Changed 6 weeks ago by Carlton Gibson

@Claude: 💯👍 Totally agree.

comment:14 Changed 6 weeks ago by Carlton Gibson

Duplicate in #30700, with failing test case provided.

I've tried contacting maintainers of HTML5lib with no success.

I've re-opened https://github.com/django/django/pull/11633 (original regex based suggestion) so we can at least assess it as a possible stop-gap.

comment:15 Changed 5 weeks ago by Carlton Gibson

Cc: Jon Dufresne added

Paging Jon, to ask his opinion on this.

Hey Jon, I see you've made a number of PRs to both html5lib, and bleach.

To me, at this point, html5lib essentially looks unmaintained. I don't have personal capacity to give to it, as cool as it is as a project. Arguably we (Fellows) could allocate it _some_ time, since we spend a fair bit already messing around with regexes but that would be small, and we couldn't take it on whole, so can I ask your thoughts?

Is html5lib in trouble? If so, as a user, what are your plans, if any? And from that, what do you think about Django adopting it? What's the alternative?

Thanks for the thought and insight.

comment:16 Changed 4 weeks ago by Jon Dufresne

To me, at this point, html5lib essentially looks unmaintained.

I agree with this observation. The previous main maintainer looks to have stopped working on the project. Responses to issues and PRs have stopped.

Is html5lib in trouble? If so, as a user, what are your plans, if any? And from that, what do you think about Django adopting it? What's the alternative?

For my own projects, I'll probably continue using html5lib until its staleness creates an observable bug for me. I haven't hit that point yet.

Bleach, on the other hand, looks like maintenance has slowed, but not stopped. I believe they have vendored html5lib to allow them to make changes internally. FWIW, I also still use Bleach.

---

I'm not familiar with all the details of this ticket, but would the stdlib HTML parser be sufficient?

https://docs.python.org/3/library/html.parser.html

comment:17 Changed 4 weeks ago by Carlton Gibson

Hi Jon,

Thank you for the comments. I will email Will, the maintainer of Bleach, and ask his thoughts too. Bleach has slowed down, but that's because it's Stable/Mature now I would have thought.

...would the stdlib HTML parser be sufficient?

Yes. Maybe. Ideally we just thought to bring in Bleach, and with it html5lib since, in theory, that's already working code. (Florian already had a Truncate prototype...)

Anyhow... will follow-up.

Last edited 4 weeks ago by Carlton Gibson (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top