Skip to content
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

Fixed #35627 -- Raised a LookupError rather than an unhandled ValueError in get_supported_language_variant(). #18403

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixed #35627 -- Raised a LookupError rather than an unhandled ValueError in get_supported_language_variant(). #18403

wants to merge 1 commit into from
31 −7

Conversation

lorinkoz
Copy link

@lorinkoz lorinkoz commented Jul 23, 2024 •
edited
Loading

Trac ticket number

ticket-35627

Branch description

get_supported_language_variant can raise a ValueError when the language code is too long under certain circumstances. This error is not handled by any caller and can make e.g. LocaleMiddleware end up in a server error by constructing a URL that contains a thousand characters on the first path element.

This PR modifies the callers of get_supported_language_variant to not only catch LookupError but also ValueError.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

github-actions[bot]
github-actions bot reviewed Jul 23, 2024
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

@lorinkoz lorinkoz force-pushed the lorinkoz-patch-1 branch 3 times, most recently from d4304db to 48dfbeb Compare July 23, 2024 09:20
@sarahboyce sarahboyce changed the title Prevent ValueError in LocaleMiddleware to bubble up Fixed #35627 -- Prevented ValueError in LocaleMiddleware to bubble up Jul 23, 2024
@lorinkoz lorinkoz marked this pull request as ready for review July 23, 2024 10:06
@sarahboyce
Copy link
Contributor

Thank you for picking this up @lorinkoz ⭐

We're going to need a release note for Django 5.0 and 4.2 as we will do a backport (see this as maybe a similar-ish idea: 38e5779)
We don't have release notes for Django 4.2.15 yet so you will need to make a new file starting with:

===========================
Django 4.2.15 release notes
===========================

*Expected August 6, 2024*

Django 4.2.15 fixes a regression in 4.2.14.

Bugfixes
========

and add 4.2.15 to docs/releases/index.txt

@lorinkoz lorinkoz force-pushed the lorinkoz-patch-1 branch 4 times, most recently from 30c77c1 to b06fd8b Compare July 23, 2024 12:52
@lorinkoz
Copy link
Author

@sarahboyce Would you please take another look?

sarahboyce
sarahboyce reviewed Jul 23, 2024
View reviewed changes
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you for the updates @lorinkoz 👍
I discussed this with @nessita and we should update the error raised to be a LookupError
This would also mean that (looking at 9e97922) we need to update:

  • docs/ref/utils.txt to say LookupError and .. versionchanged:: 4.2.15
  • the ValueError asserts in i18n.tests.MiscTests.test_get_supported_language_variant_real

I don't think the original release notes for the CVE need to be updated so let's leave them
The tests you've added are great, please keep these ⭐

docs/releases/5.0.8.txt Outdated Show resolved Hide resolved
@lorinkoz lorinkoz changed the title Fixed #35627 -- Prevented ValueError in LocaleMiddleware to bubble up Fixed #35627 -- Better handling of LocaleMiddleware errors Jul 23, 2024
@lorinkoz lorinkoz requested a review from sarahboyce July 23, 2024 15:31
@lorinkoz @sarahboyce
Fixed #35627 -- Raised a LookupError rather than an unhandled ValueEr…
a015cbf 
…ror in get_supported_language_variant().

LocaleMiddleware didn't handle the ValueError raised by
get_supported_language_variant() when language codes were
over 500 characters.

Regression in 9e97922.
@sarahboyce sarahboyce changed the title Fixed #35627 -- Better handling of LocaleMiddleware errors Fixed #35627 -- Raised a LookupError rather than an unhandled ValueError in get_supported_language_variant(). Jul 23, 2024
sarahboyce
sarahboyce approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you @lorinkoz this looks good to me
Welcome on board ⛵

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions left review comments

@sarahboyce sarahboyce sarahboyce approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@lorinkoz @sarahboyce