Skip to content

Fixed #33461 -- Escaped template errors in the technical 500 debug page. #15361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 28, 2022
Merged

Fixed #33461 -- Escaped template errors in the technical 500 debug page. #15361

merged 1 commit into from
Jan 28, 2022

Conversation

kezabelle
Copy link
Contributor

@kezabelle kezabelle commented Jan 26, 2022 •
edited
Loading

Ticket is 33461.

I've used the reproducer from my original submission to the security address, but slightly adapted to force 2 alerts to appear, when the patch isn't in place. This has required using html=False in the assertNotContains to avoid getting django.test.html.HTMLParseError: ('Unexpected end tag textarea ...

I've opted to just apply escape in the place where it's officially rendered, rather than use the force_escape filter and risk potentially missing something (like the breaking out of the textarea!) - though it was suggested that maybe force_escape was enough to tackle the issue, so I'm OK with changing it if that's preferred.

(Edit: oh get in the bin, flake8 and isort)
(Edit 2: oh, I forgot I was only running the single test case I added, rather than the whole suite. Oops! Guess I'll have opportunity to go and fix linters anyway, sigh ;))
(Edit 3: hey this time I ran the actual suite, and moved from escape to force_escape to restore the expected Plain Text output. Given I've had over a week to think on this, you'd think I'd have perhaps got this right first time. Dear reader, you'd be wrong, I am so fallible)
(Edit 4: sigh, windows failures, and I've got no idea why!)

felixxm
felixxm reviewed Jan 27, 2022
View reviewed changes
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kezabelle Thanks 👍 On Windows it's raising an OSError:

image

@kezabelle
Copy link
Contributor Author

kezabelle commented Jan 27, 2022 •
edited
Loading

@felixxm Thanks for the assist on the Windows failures, how do you want me to handle the issue? My read of your image makes it seem like get_contents needs fixing to catch and raise TemplateDoesNotExist for OSError (indiscriminately, or would it be back to .errno checking?) as well as FileNotFoundError ... or I could just skip the test ;)

@felixxm
Copy link
Member

felixxm commented Jan 27, 2022

@felixxm Thanks for the assist on the Windows failures, how do you want me to handle the issue? My read of your image makes it seem like get_contents needs fixing to catch and raise TemplateDoesNotExist for OSError (indiscriminately, or would it be back to .errno checking?) as well as FileNotFoundError ... or I could just skip the test ;)

We can switch to something less exploitable 😄 that will continue to prove that it's properly escaped, e.g. <x&y> 🤔

@felixxm
Copy link
Member

felixxm commented Jan 28, 2022

@felixxm Thanks for the assist on the Windows failures, how do you want me to handle the issue? My read of your image makes it seem like get_contents needs fixing to catch and raise TemplateDoesNotExist for OSError (indiscriminately, or would it be back to .errno checking?) as well as FileNotFoundError ... or I could just skip the test ;)

We can switch to something less exploitable smile that will continue to prove that it's properly escaped, e.g. <x&y> thinking

OK, let's just skip this test on Windows 😄 As far as I'm aware OSError shouldn't be caught on Windows.

@felixxm felixxm changed the title Fixed #33461 -- Forced escaping of Exception representation in the technical 500 Fixed #33461 -- Escaped template errors in the technical 500 debug page. Jan 28, 2022
@felixxm
Copy link
Member

felixxm commented Jan 28, 2022

@kezabelle Thanks ⭐

@felixxm felixxm merged commit c5c7a15 into django:main Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixxm felixxm felixxm left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kezabelle @felixxm