Opened 13 days ago

Last modified 9 days ago

#35897 assigned Cleanup/optimization

Template system: escape() calls in get_exception_info() should be removed

Reported by: Klaas van Schelven Owned by: Klaas van Schelven
Component: Error reporting Version: dev
Severity: Normal Keywords:
Cc: Keryn Knight, Florian Apolloner Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Here there are some calls to escape()

They shouldn't be there: escaping happens in templates for non-safe strings anyway, so there's no need.

And there _is_ a drawback: as an example, the Python Sentry SDK copies this info, but because it gets sent over the wire (as a JSON string) the information that this has already been escaped is lost, and on the receiving end it is escaped again.

This means that on the server-side the Error-tracking, in my case Bugsink will show doubly escaped html code snippets. This looks something like this:

<p class="relative text-slate-600 text-base md:text-xl mb-4 md:mb-5"> 

Removing the calls to escape simply solves this. Which makes sense: calling escape is simply not the responsibility of this piece of code, it should just stay marked as unsafe and be escape at the edges (on rendering).

Attachments (1)

2024-11-08_1877x538.png (121.7 KB ) - added by Klaas van Schelven 13 days ago.

Download all attachments as: .zip

Change History (7)

by Klaas van Schelven, 13 days ago

Attachment: 2024-11-08_1877x538.png added

comment:1 by Klaas van Schelven, 13 days ago

Has patch: set

comment:2 by Sarah Boyce, 13 days ago

Cc: Keryn Knight added
Component: UncategorizedError reporting
Type: UncategorizedCleanup/optimization

Given #33461 added force_escape to message, we might need to move that escaping into the template

comment:3 by Sarah Boyce, 13 days ago

Cc: Florian Apolloner added
Triage Stage: UnreviewedAccepted

Tentatively accepting

comment:4 by Sarah Boyce, 13 days ago

Owner: set to Klaas van Schelven
Status: newassigned

comment:5 by Klaas van Schelven, 12 days ago

I don't fully understand the linked issue, which puts me in dangerous waters, but AFAICT in that issue one of the things that was required for the problem to exist was user input, incorrectly marked as safe. That would mean it doesn't really apply here, since the thing that was previously (before my proposed change) being escape was lines of source code. Presumably not affected by user input, and also not mark-safe'd.

comment:6 by Sarah Boyce, 9 days ago

Triage Stage: AcceptedReady for checkin
Note: See TracTickets for help on using tickets.
Back to Top