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.

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

Ignore trailing semicolons when parsing Accept-Language header #32259

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

Conversation

schnapster
Copy link
Contributor

Something I'm seeing in the wild is a trailing semicolon for Accept-Language headers. Partial example stack trace:

java.lang.IllegalArgumentException: range=en;
    at java.util.Locale$LanguageRange.<init>(Locale.java:3099)
    at sun.util.locale.LocaleMatcher.parse(LocaleMatcher.java:525)
    at java.util.Locale$LanguageRange.parse(Locale.java:3214)
    at org.springframework.http.HttpHeaders.getAcceptLanguage(HttpHeaders.java:505)
    at org.springframework.http.HttpHeaders.getAcceptLanguageAsLocales(HttpHeaders.java:526)

I don't think it would hurt to be more lenient here and drop the trailing semicolon before attempting to parse it.

@pivotal-cla
Copy link

@schnapster Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@schnapster Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 13, 2024
@schnapster schnapster force-pushed the accept-language-header-trailing-semicolon branch from f456d26 to a3323de Compare February 13, 2024 15:59
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Feb 13, 2024
@sbrannen sbrannen changed the title Handle trailing semicolon when parsing Accept-Language header Ignore trailing semicolons when parsing Accept-Language header Feb 13, 2024
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

@schnapster, the header is parsed via Locale.LanguageRange.parse, which supports a weighted list of ranges where each may have a q parameter. However, the parsing logic doesn't allow a lot of flexibility and looks specifically for ";q=". Note also it first splits by , to create a list of range value. So the problem may still occur if there is a trailing ; in one of the other ranges.

We don't want to replicate the parsing logic from Locale, but we may be able to do a split by , and then check for invalid ";" in each. However, this is better done defensively, i.e. allowing parsing to fail first and then reacting to that in fallback mode.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Feb 15, 2024
@@ -502,6 +502,9 @@ public void setAcceptLanguage(List<Locale.LanguageRange> languages) {
*/
public List<Locale.LanguageRange> getAcceptLanguage() {
String value = getFirst(ACCEPT_LANGUAGE);
if (value != null) {
value = StringUtils.trimTrailingCharacter(value, ';');
Copy link

Choose a reason for hiding this comment

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

';' should be a variable

@@ -502,6 +502,9 @@ public void setAcceptLanguage(List<Locale.LanguageRange> languages) {
*/
public List<Locale.LanguageRange> getAcceptLanguage() {
String value = getFirst(ACCEPT_LANGUAGE);
if (value != null) {
Copy link

@kmesiab kmesiab Feb 28, 2024

Choose a reason for hiding this comment

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

While I think a null check is fine, why not also validate whether this string is non empty and at least [n] chars and avoid the noop?

@poutsma poutsma self-assigned this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants