Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions pkg/server/locales/locales.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package locales

import (
"errors"
"strings"

"golang.org/x/text/language"
"k8s.io/klog/v2"
)
Expand All @@ -25,6 +28,17 @@ func GetLocale(acceptLangHeader string) Localization {
}

func getPreferredLang(acceptLangHeader string) string {
// OCPBUGS-92015: In order to prevent the Quadratic-time DoS attack
// that is possible as documented by https://github.com/golang/go/issues/79684,
// return early with the fallback to english if there are more than 1000 underscore and hyphen
// characters in the Accept-Language header.
// This can be removed once we have updated the language dependency to a version
// that has fixed this issue.
if strings.Count(acceptLangHeader, "-")+strings.Count(acceptLangHeader, "_") > 1000 {
klog.V(5).Infof("Error parsing 'Accept-Language' header, falling back to English language: %v", errors.New("tag list exceeds max length"))
return language.English.String()
}

matcher := language.NewMatcher(supportedLangs)
userPrefs, _, err := language.ParseAcceptLanguage(acceptLangHeader)
if err != nil {
Expand Down
16 changes: 16 additions & 0 deletions pkg/server/locales/locales_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package locales

import (
"reflect"
"strings"
"testing"
)

Expand Down Expand Up @@ -51,6 +52,21 @@ func TestLocales(t *testing.T) {
header: "cz;q=0.5, de;q=0.8",
locale: locale_en,
},
{
name: "Test 'Accept-Language' request header with too many underscores, so defaults to English language",
header: strings.Repeat("_", 2000),
locale: locale_en,
},
Comment on lines +55 to +59

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test passes even without the new guard; ParseAcceptLanguage() would return an error because the value isn't a valid language. I understand that the test now passes because of the new guard, but I'm worried about regressions if e.g. we break the guard accidentally.

However, to be honest, I don't know whether it's possible to write a test that will help prevent such regressions, because the function's output doesn't help distinguish the cases (unless we capture and check the logged error somehow).

Therefore I won't block the PR on this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, without a refactor here such that the function actually returns an error case I'm not sure there is a way to reliably prove that we are testing the exact path we are intending other than making sure we are getting our fallback language.

In order to properly test this here, we would have to construct a language header with >1000 language tags separated by _ and I'm not sure that is actually possible without significantly more work (I don't think go is aware of that many language tags).

I'm not sure the additional verification is worth the squeeze here but 🤷

{
name: "Test 'Accept-Language' request header with too many hyphens, so defaults to English language",
header: strings.Repeat("-", 2000),
locale: locale_en,
},
{
name: "Test 'Accept-Language' request header with too many underscores + hyphens, so defaults to English language",
header: strings.Repeat("_", 800) + strings.Repeat("-", 800),
locale: locale_en,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down