From 441060eb8dcd54c0cb2ff5f2f507a2034978226a Mon Sep 17 00:00:00 2001 From: jub0bs Date: Thu, 29 Aug 2024 08:35:21 +0200 Subject: [PATCH] Handle multiple ACRH headers (fixes #184) --- cors.go | 8 +- cors_test.go | 40 +++++++ internal/sortedset.go | 152 +++++++++++++++++++------ internal/sortedset_test.go | 223 ++++++++++++++++++++++++++----------- utils.go | 9 -- 5 files changed, 326 insertions(+), 106 deletions(-) diff --git a/cors.go b/cors.go index 20a66d0..5d5c0c6 100644 --- a/cors.go +++ b/cors.go @@ -364,9 +364,11 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) { // Note: the Fetch standard guarantees that at most one // Access-Control-Request-Headers header is present in the preflight request; // see step 5.2 in https://fetch.spec.whatwg.org/#cors-preflight-fetch-0. - reqHeaders, found := first(r.Header, "Access-Control-Request-Headers") - if found && !c.allowedHeadersAll && !c.allowedHeaders.Subsumes(reqHeaders[0]) { - c.logf(" Preflight aborted: headers '%v' not allowed", reqHeaders[0]) + // However, some gateways split that header into multiple headers of the same name; + // see https://github.com/rs/cors/issues/184. + reqHeaders, found := r.Header["Access-Control-Request-Headers"] + if found && !c.allowedHeadersAll && !c.allowedHeaders.Subsumes(reqHeaders) { + c.logf(" Preflight aborted: headers '%v' not allowed", reqHeaders) return } if c.allowedOriginsAll { diff --git a/cors_test.go b/cors_test.go index 14050ef..46430fc 100644 --- a/cors_test.go +++ b/cors_test.go @@ -536,6 +536,46 @@ func TestSpec(t *testing.T) { }, true, }, + { + "MultipleACRHHeaders", + Options{ + AllowedOrigins: []string{"http://foobar.com"}, + AllowedHeaders: []string{"Content-Type", "Authorization"}, + }, + "OPTIONS", + http.Header{ + "Origin": {"http://foobar.com"}, + "Access-Control-Request-Method": {"GET"}, + "Access-Control-Request-Headers": {"authorization", "content-type"}, + }, + http.Header{ + "Vary": {"Origin, Access-Control-Request-Method, Access-Control-Request-Headers"}, + "Access-Control-Allow-Origin": {"http://foobar.com"}, + "Access-Control-Allow-Methods": {"GET"}, + "Access-Control-Allow-Headers": {"authorization", "content-type"}, + }, + true, + }, + { + "MultipleACRHHeadersWithOWSAndEmptyElements", + Options{ + AllowedOrigins: []string{"http://foobar.com"}, + AllowedHeaders: []string{"Content-Type", "Authorization"}, + }, + "OPTIONS", + http.Header{ + "Origin": {"http://foobar.com"}, + "Access-Control-Request-Method": {"GET"}, + "Access-Control-Request-Headers": {"authorization\t", " ", " content-type"}, + }, + http.Header{ + "Vary": {"Origin, Access-Control-Request-Method, Access-Control-Request-Headers"}, + "Access-Control-Allow-Origin": {"http://foobar.com"}, + "Access-Control-Allow-Methods": {"GET"}, + "Access-Control-Allow-Headers": {"authorization\t", " ", " content-type"}, + }, + true, + }, } for i := range cases { tc := cases[i] diff --git a/internal/sortedset.go b/internal/sortedset.go index 513da20..9f1b886 100644 --- a/internal/sortedset.go +++ b/internal/sortedset.go @@ -52,46 +52,134 @@ func (set SortedSet) String() string { return strings.Join(elems, ",") } -// Subsumes reports whether csv is a sequence of comma-separated names that are -// - all elements of set, -// - sorted in lexicographically order, +// Subsumes reports whether values is a sequence of list-based field values +// whose elements are +// - all members of set, +// - sorted in lexicographical order, // - unique. -func (set SortedSet) Subsumes(csv string) bool { - if csv == "" { - return true +func (set SortedSet) Subsumes(values []string) bool { + var ( // effectively constant + maxLen = maxOWSBytes + set.maxLen + maxOWSBytes + 1 // +1 for comma + ) + var ( + posOfLastNameSeen = -1 + name string + commaFound bool + emptyElements int + ok bool + ) + for _, s := range values { + for { + // As a defense against maliciously long names in s, + // we process only a small number of s's leading bytes per iteration. + name, s, commaFound = cutAtComma(s, maxLen) + name, ok = trimOWS(name, maxOWSBytes) + if !ok { + return false + } + if name == "" { + // RFC 9110 requires recipients to tolerate + // "a reasonable number of empty list elements"; see + // https://httpwg.org/specs/rfc9110.html#abnf.extension.recipient. + emptyElements++ + if emptyElements > maxEmptyElements { + return false + } + if !commaFound { // We have now exhausted the names in s. + break + } + continue + } + pos, ok := set.m[name] + if !ok { + return false + } + // The names in s are expected to be sorted in lexicographical order + // and to each appear at most once. + // Therefore, the positions (in set) of the names that + // appear in s should form a strictly increasing sequence. + // If that's not actually the case, bail out. + if pos <= posOfLastNameSeen { + return false + } + posOfLastNameSeen = pos + if !commaFound { // We have now exhausted the names in s. + break + } + } + } + return true +} + +const ( + maxOWSBytes = 1 // number of leading/trailing OWS bytes tolerated + maxEmptyElements = 16 // number of empty list elements tolerated +) + +func cutAtComma(s string, n int) (before, after string, found bool) { + // Note: this implementation draws inspiration from strings.Cut's. + end := min(len(s), n) + if i := strings.IndexByte(s[:end], ','); i >= 0 { + after = s[i+1:] // deal with this first to save one bounds check + return s[:i], after, true + } + return s, "", false +} + +// TrimOWS trims up to n bytes of [optional whitespace (OWS)] +// from the start of and/or the end of s. +// If no more than n bytes of OWS are found at the start of s +// and no more than n bytes of OWS are found at the end of s, +// it returns the trimmed result and true. +// Otherwise, it returns the original string and false. +// +// [optional whitespace (OWS)]: https://httpwg.org/specs/rfc9110.html#whitespace +func trimOWS(s string, n int) (trimmed string, ok bool) { + if s == "" { + return s, true + } + trimmed, ok = trimRightOWS(s, n) + if !ok { + return s, false } - posOfLastNameSeen := -1 - chunkSize := set.maxLen + 1 // (to accommodate for at least one comma) - for { - // As a defense against maliciously long names in csv, - // we only process at most chunkSize bytes per iteration. - end := min(len(csv), chunkSize) - comma := strings.IndexByte(csv[:end], ',') - var name string - if comma == -1 { - name = csv - } else { - name = csv[:comma] + trimmed, ok = trimLeftOWS(trimmed, n) + if !ok { + return s, false + } + return trimmed, true +} + +func trimLeftOWS(s string, n int) (string, bool) { + sCopy := s + var i int + for len(s) > 0 { + if i > n { + return sCopy, false } - pos, found := set.m[name] - if !found { - return false + if !(s[0] == ' ' || s[0] == '\t') { + break } - // The names in csv are expected to be sorted in lexicographical order - // and appear at most once in csv. - // Therefore, the positions (in set) of the names that - // appear in csv should form a strictly increasing sequence. - // If that's not actually the case, bail out. - if pos <= posOfLastNameSeen { - return false + s = s[1:] + i++ + } + return s, true +} + +func trimRightOWS(s string, n int) (string, bool) { + sCopy := s + var i int + for len(s) > 0 { + if i > n { + return sCopy, false } - posOfLastNameSeen = pos - if comma < 0 { // We've now processed all the names in csv. + last := len(s) - 1 + if !(s[last] == ' ' || s[last] == '\t') { break } - csv = csv[comma+1:] + s = s[:last] + i++ } - return true + return s, true } // TODO: when updating go directive to 1.21 or later, diff --git a/internal/sortedset_test.go b/internal/sortedset_test.go index 9727686..8c9f119 100644 --- a/internal/sortedset_test.go +++ b/internal/sortedset_test.go @@ -1,108 +1,207 @@ package internal import ( + "strings" "testing" ) func TestSortedSet(t *testing.T) { cases := []struct { - desc string - elems []string + desc string + elems []string + // expectations + size int combined string - subsets []string - notSubsets []string - wantSize int + slice []string + subsets [][]string + notSubsets [][]string }{ { desc: "empty set", + size: 0, combined: "", - notSubsets: []string{ - "bar", - "bar,foo", + subsets: [][]string{ + // some empty elements, possibly with OWS + {""}, + {","}, + {"\t, , "}, + // multiple field lines, some empty elements + make([]string, maxEmptyElements), + }, + notSubsets: [][]string{ + {"x-bar"}, + {"x-bar,x-foo"}, + // too many empty elements + {strings.Repeat(",", maxEmptyElements+1)}, + // multiple field lines, too many empty elements + make([]string, maxEmptyElements+1), }, - wantSize: 0, }, { desc: "singleton set", - elems: []string{"foo"}, - combined: "foo", - subsets: []string{ - "", - "foo", + elems: []string{"x-foo"}, + size: 1, + combined: "x-foo", + slice: []string{"X-Foo"}, + subsets: [][]string{ + {"x-foo"}, + // some empty elements, possibly with OWS + {""}, + {","}, + {"\t, , "}, + {"\tx-foo ,"}, + {" x-foo\t,"}, + {strings.Repeat(",", maxEmptyElements) + "x-foo"}, + // multiple field lines, some empty elements + append(make([]string, maxEmptyElements), "x-foo"), + make([]string, maxEmptyElements), }, - notSubsets: []string{ - "bar", - "bar,foo", + notSubsets: [][]string{ + {"x-bar"}, + {"x-bar,x-foo"}, + // too much OWS + {"x-foo "}, + {" x-foo "}, + {" x-foo "}, + {"x-foo\t\t"}, + {"\tx-foo\t\t"}, + {"\t\tx-foo\t\t"}, + // too many empty elements + {strings.Repeat(",", maxEmptyElements+1) + "x-foo"}, + // multiple field lines, too many empty elements + append(make([]string, maxEmptyElements+1), "x-foo"), + make([]string, maxEmptyElements+1), }, - wantSize: 1, }, { desc: "no dupes", - elems: []string{"foo", "bar", "baz"}, - combined: "bar,baz,foo", - subsets: []string{ - "", - "bar", - "baz", - "foo", - "bar,baz", - "bar,foo", - "baz,foo", - "bar,baz,foo", + elems: []string{"x-foo", "x-bar", "x-baz"}, + size: 3, + combined: "x-bar,x-baz,x-foo", + slice: []string{"X-Bar", "X-Baz", "X-Foo"}, + subsets: [][]string{ + {"x-bar"}, + {"x-baz"}, + {"x-foo"}, + {"x-bar,x-baz"}, + {"x-bar,x-foo"}, + {"x-baz,x-foo"}, + {"x-bar,x-baz,x-foo"}, + // some empty elements, possibly with OWS + {""}, + {","}, + {"\t, , "}, + {"\tx-bar ,"}, + {" x-baz\t,"}, + {"x-foo,"}, + {"\tx-bar ,\tx-baz ,"}, + {" x-bar\t, x-foo\t,"}, + {"x-baz,x-foo,"}, + {" x-bar , x-baz , x-foo ,"}, + {"x-bar" + strings.Repeat(",", maxEmptyElements+1) + "x-foo"}, + // multiple field lines + {"x-bar", "x-foo"}, + {"x-bar", "x-baz,x-foo"}, + // multiple field lines, some empty elements + append(make([]string, maxEmptyElements), "x-bar", "x-foo"), + make([]string, maxEmptyElements), }, - notSubsets: []string{ - "qux", - "bar,baz,baz", - "qux,baz", - "qux,foo", - "quxbaz,foo", + notSubsets: [][]string{ + {"x-qux"}, + {"x-bar,x-baz,x-baz"}, + {"x-qux,x-baz"}, + {"x-qux,x-foo"}, + {"x-quxbaz,x-foo"}, + // too much OWS + {"x-bar "}, + {" x-baz "}, + {" x-foo "}, + {"x-bar\t\t,x-baz"}, + {"x-bar,\tx-foo\t\t"}, + {"\t\tx-baz,x-foo\t\t"}, + {" x-bar\t,\tx-baz\t ,x-foo"}, + // too many empty elements + {"x-bar" + strings.Repeat(",", maxEmptyElements+2) + "x-foo"}, + // multiple field lines, elements in the wrong order + {"x-foo", "x-bar"}, + // multiple field lines, too many empty elements + append(make([]string, maxEmptyElements+1), "x-bar", "x-foo"), + make([]string, maxEmptyElements+1), }, - wantSize: 3, }, { desc: "some dupes", - elems: []string{"foo", "bar", "bar", "foo", "e"}, - combined: "bar,e,foo", - subsets: []string{ - "", - "bar", - "e", - "foo", - "bar,foo", - "bar,e", - "e,foo", - "bar,e,foo", + elems: []string{"x-foo", "x-bar", "x-foo"}, + size: 2, + combined: "x-bar,x-foo", + slice: []string{"X-Bar", "X-Foo"}, + subsets: [][]string{ + {"x-bar"}, + {"x-foo"}, + {"x-bar,x-foo"}, + // some empty elements, possibly with OWS + {""}, + {","}, + {"\t, , "}, + {"\tx-bar ,"}, + {" x-foo\t,"}, + {"x-foo,"}, + {"\tx-bar ,\tx-foo ,"}, + {" x-bar\t, x-foo\t,"}, + {"x-bar,x-foo,"}, + {" x-bar , x-foo ,"}, + {"x-bar" + strings.Repeat(",", maxEmptyElements+1) + "x-foo"}, + // multiple field lines + {"x-bar", "x-foo"}, + // multiple field lines, some empty elements + append(make([]string, maxEmptyElements), "x-bar", "x-foo"), + make([]string, maxEmptyElements), }, - notSubsets: []string{ - "qux", - "qux,bar", - "qux,foo", - "qux,baz,foo", + notSubsets: [][]string{ + {"x-qux"}, + {"x-qux,x-bar"}, + {"x-qux,x-foo"}, + {"x-qux,x-baz,x-foo"}, + // too much OWS + {"x-qux "}, + {"x-qux,\t\tx-bar"}, + {"x-qux,x-foo\t\t"}, + {"\tx-qux , x-baz\t\t,x-foo"}, + // too many empty elements + {"x-bar" + strings.Repeat(",", maxEmptyElements+2) + "x-foo"}, + // multiple field lines, elements in the wrong order + {"x-foo", "x-bar"}, + // multiple field lines, too much whitespace + {"x-qux", "\t\tx-bar"}, + {"x-qux", "x-foo\t\t"}, + {"\tx-qux ", " x-baz\t\t,x-foo"}, + // multiple field lines, too many empty elements + append(make([]string, maxEmptyElements+1), "x-bar", "x-foo"), + make([]string, maxEmptyElements+1), }, - wantSize: 3, }, } for _, tc := range cases { f := func(t *testing.T) { elems := clone(tc.elems) - s := NewSortedSet(tc.elems...) - size := s.Size() - if s.Size() != tc.wantSize { + set := NewSortedSet(tc.elems...) + size := set.Size() + if set.Size() != tc.size { const tmpl = "NewSortedSet(%#v...).Size(): got %d; want %d" - t.Errorf(tmpl, elems, size, tc.wantSize) + t.Errorf(tmpl, elems, size, tc.size) } - combined := s.String() + combined := set.String() if combined != tc.combined { const tmpl = "NewSortedSet(%#v...).String(): got %q; want %q" t.Errorf(tmpl, elems, combined, tc.combined) } for _, sub := range tc.subsets { - if !s.Subsumes(sub) { + if !set.Subsumes(sub) { const tmpl = "%q is not a subset of %q, but should be" - t.Errorf(tmpl, sub, s) + t.Errorf(tmpl, set, sub) } } for _, notSub := range tc.notSubsets { - if s.Subsumes(notSub) { + if set.Subsumes(notSub) { const tmpl = "%q is a subset of %q, but should not be" - t.Errorf(tmpl, notSub, s) + t.Errorf(tmpl, set, notSub) } } } diff --git a/utils.go b/utils.go index 7019f45..41b0c28 100644 --- a/utils.go +++ b/utils.go @@ -1,7 +1,6 @@ package cors import ( - "net/http" "strings" ) @@ -24,11 +23,3 @@ func convert(s []string, f func(string) string) []string { } return out } - -func first(hdrs http.Header, k string) ([]string, bool) { - v, found := hdrs[k] - if !found || len(v) == 0 { - return nil, false - } - return v[:1], true -}