Skip to content

Rule out sync bugs due to slice reuse (fixes #198) #199

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

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

Rule out sync bugs due to slice reuse (fixes #198) #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 10 additions & 22 deletions cors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ import (
"github.com/rs/cors/internal"
)

var headerVaryOrigin = []string{"Origin"}
var headerOriginAll = []string{"*"}
var headerTrue = []string{"true"}

// Options is a configuration container to setup the CORS middleware.
type Options struct {
// AllowedOrigins is a list of origins a cross-domain request can be executed from.
Expand Down Expand Up @@ -133,7 +129,7 @@ type Cors struct {
allowCredentials bool
allowPrivateNetwork bool
optionPassthrough bool
preflightVary []string
preflightVary string
}

// New creates a new Cors handler with the provided options.
Expand Down Expand Up @@ -229,9 +225,9 @@ func New(options Options) *Cors {

// Pre-compute prefight Vary header to save allocations
if c.allowPrivateNetwork {
c.preflightVary = []string{"Origin, Access-Control-Request-Method, Access-Control-Request-Headers, Access-Control-Request-Private-Network"}
c.preflightVary = "Origin, Access-Control-Request-Method, Access-Control-Request-Headers, Access-Control-Request-Private-Network"
} else {
c.preflightVary = []string{"Origin, Access-Control-Request-Method, Access-Control-Request-Headers"}
c.preflightVary = "Origin, Access-Control-Request-Method, Access-Control-Request-Headers"
}

// Precompute max-age
Expand Down Expand Up @@ -337,11 +333,7 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) {
// Always set Vary headers
// see https://github.com/rs/cors/issues/10,
// https://github.com/rs/cors/commit/dbdca4d95feaa7511a46e6f1efb3b3aa505bc43f#commitcomment-12352001
if vary, found := headers["Vary"]; found {
headers["Vary"] = append(vary, c.preflightVary[0])
} else {
headers["Vary"] = c.preflightVary
}
headers.Add("Vary", c.preflightVary)
allowed, additionalVaryHeaders := c.isOriginAllowed(r, origin)
if len(additionalVaryHeaders) > 0 {
headers.Add("Vary", strings.Join(convert(additionalVaryHeaders, http.CanonicalHeaderKey), ", "))
Expand Down Expand Up @@ -372,7 +364,7 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) {
return
}
if c.allowedOriginsAll {
headers["Access-Control-Allow-Origin"] = headerOriginAll
headers.Set("Access-Control-Allow-Origin", "*")
} else {
headers["Access-Control-Allow-Origin"] = r.Header["Origin"]
}
Expand All @@ -385,10 +377,10 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) {
headers["Access-Control-Allow-Headers"] = reqHeaders
}
if c.allowCredentials {
headers["Access-Control-Allow-Credentials"] = headerTrue
headers.Set("Access-Control-Allow-Credentials", "true")
Copy link
Owner

@rs rs Nov 22, 2025

Choose a reason for hiding this comment

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

To keep the allocation optimization and avoid triggering the concurrency check, we could create a new slice with the same backing array. Something like headerTrue[:1]

Copy link
Contributor Author

@jub0bs jub0bs Nov 22, 2025

Choose a reason for hiding this comment

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

But then handlers could still mutate the first element of the shared backing array through the new slice...

Copy link
Owner

@rs rs Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

They only append or set right? There should no slide[0] = val as far as I remember

Copy link
Contributor Author

@jub0bs jub0bs Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I don't understand your last comment... 🤔

What I meant in my earlier comment is that, even if you write

headers["Access-Control-Allow-Credentials"] = headerTrue[:1]

the tests that I posted in #198 keep failing, partly because the first element of headerTrue can still be mutated.

Copy link
Owner

@rs rs Nov 22, 2025

Choose a reason for hiding this comment

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

How would it be mutated?

Copy link
Contributor Author

@jub0bs jub0bs Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The failing tests included in #198 show how. Here is a version of the mutating handler focused on ACAO:

func(w http.ResponseWriter, r *http.Request) {
	acao := w.Header()["Access-Control-Allow-Credentials"] // aliases slices headerTrue, headerTrue[:1], etc.
	if len(acao) > 0 {
		acao[0] = "oops!"
		fmt.Println(headerTrue[0]) // oops!
	}
}

Copy link
Owner

@rs rs Nov 22, 2025

Choose a reason for hiding this comment

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

This is an unlikely use case. The caller would have to assume there is a first element they did not add and would know what it is.

Copy link
Contributor Author

@jub0bs jub0bs Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You may be right about the unlikelihood of such a use case, but the fact remains: a handler can corrupt a CORS middleware that wraps it and inadvertently introduce non-obvious synchronisation bugs (non-obvious because it would be natural to assume that the acao slice doesn't alias anything at package level of rs/cors). I think most users of your library would value the guarantee that no such bugs are possible more than saving a couple of allocations during middleware execution.

Copy link
Contributor Author

@jub0bs jub0bs Nov 28, 2025

Choose a reason for hiding this comment

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

@rs For completeness, here are some benchmark results ("old" and "new" correspond to master and my branch, respectively):

goos: darwin
goarch: arm64
pkg: github.com/rs/cors
cpu: Apple M4
                            │      old      │                 new                  │
                            │    sec/op     │    sec/op      vs base               │
Without-10                     3.279n ±  2%    3.265n ±  6%        ~ (p=0.732 n=6)
Default-10                     98.96n ±  2%   184.55n ± 14%  +86.48% (p=0.002 n=6)
AllowedOrigin-10               142.0n ±  1%    160.5n ± 10%  +13.10% (p=0.002 n=6)
Preflight-10                   247.7n ±  3%    339.9n ± 11%  +37.20% (p=0.002 n=6)
PreflightHeader-10             269.6n ±  2%    358.3n ±  3%  +32.93% (p=0.002 n=6)
PreflightAdversarialACRH-10    501.1n ± 18%    536.6n ±  2%        ~ (p=0.065 n=6)
Wildcard/match-10              6.625n ±  1%    6.612n ±  1%        ~ (p=0.240 n=6)
Wildcard/too_short-10         0.9658n ± 22%   0.9652n ±  3%        ~ (p=0.413 n=6)
geomean                        42.10n          50.20n        +19.26%

                            │     old      │                 new                  │
                            │     B/op     │    B/op     vs base                  │
Without-10                    0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=6) ¹
Default-10                    16.00 ± 0%     48.00 ± 0%  +200.00% (p=0.002 n=6)
AllowedOrigin-10              16.00 ± 0%     32.00 ± 0%  +100.00% (p=0.002 n=6)
Preflight-10                  16.00 ± 0%     48.00 ± 0%  +200.00% (p=0.002 n=6)
PreflightHeader-10            16.00 ± 0%     48.00 ± 0%  +200.00% (p=0.002 n=6)
PreflightAdversarialACRH-10   40.00 ± 0%     56.00 ± 0%   +40.00% (p=0.002 n=6)
Wildcard/match-10             0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=6) ¹
Wildcard/too_short-10         0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=6) ¹
geomean                                  ²                +71.72%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                            │     old      │                 new                  │
                            │  allocs/op   │ allocs/op   vs base                  │
Without-10                    0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=6) ¹
Default-10                    1.000 ± 0%     3.000 ± 0%  +200.00% (p=0.002 n=6)
AllowedOrigin-10              1.000 ± 0%     2.000 ± 0%  +100.00% (p=0.002 n=6)
Preflight-10                  1.000 ± 0%     3.000 ± 0%  +200.00% (p=0.002 n=6)
PreflightHeader-10            1.000 ± 0%     3.000 ± 0%  +200.00% (p=0.002 n=6)
PreflightAdversarialACRH-10   2.000 ± 0%     3.000 ± 0%   +50.00% (p=0.002 n=6)
Wildcard/match-10             0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=6) ¹
Wildcard/too_short-10         0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=6) ¹
geomean                                  ²                +73.21%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

}
if c.allowPrivateNetwork && r.Header.Get("Access-Control-Request-Private-Network") == "true" {
headers["Access-Control-Allow-Private-Network"] = headerTrue
headers.Set("Access-Control-Allow-Private-Network", "true")
}
if len(c.maxAge) > 0 {
headers["Access-Control-Max-Age"] = c.maxAge
Expand All @@ -404,11 +396,7 @@ func (c *Cors) handleActualRequest(w http.ResponseWriter, r *http.Request) {
allowed, additionalVaryHeaders := c.isOriginAllowed(r, origin)

// Always set Vary, see https://github.com/rs/cors/issues/10
if vary := headers["Vary"]; vary == nil {
headers["Vary"] = headerVaryOrigin
} else {
headers["Vary"] = append(vary, headerVaryOrigin[0])
}
headers.Add("Vary", "Origin")
if len(additionalVaryHeaders) > 0 {
headers.Add("Vary", strings.Join(convert(additionalVaryHeaders, http.CanonicalHeaderKey), ", "))
}
Expand All @@ -430,15 +418,15 @@ func (c *Cors) handleActualRequest(w http.ResponseWriter, r *http.Request) {
return
}
if c.allowedOriginsAll {
headers["Access-Control-Allow-Origin"] = headerOriginAll
headers.Set("Access-Control-Allow-Origin", "*")
} else {
headers["Access-Control-Allow-Origin"] = r.Header["Origin"]
}
if len(c.exposedHeaders) > 0 {
headers["Access-Control-Expose-Headers"] = c.exposedHeaders
}
if c.allowCredentials {
headers["Access-Control-Allow-Credentials"] = headerTrue
headers.Set("Access-Control-Allow-Credentials", "true")
}
c.logf(" Actual response added headers: %v", headers)
}
Expand Down
57 changes: 57 additions & 0 deletions cors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"regexp"
"slices"
"strings"
"sync"
"testing"
)

Expand Down Expand Up @@ -830,3 +831,59 @@ func TestAccessControlExposeHeadersPresence(t *testing.T) {
}

}

var mutatingHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
for _, k := range keys {
vv := w.Header()[k]
if len(vv) > 0 {
vv[0] = "oops!"
}
}
})

var keys = []string{
"Access-Control-Allow-Credentials",
"Access-Control-Allow-Origin",
"Vary",
}

// Note: run this test with -race
func TestSynchronizationBugWithPrelightRequest(t *testing.T) {
testSynchronizationBug(t, true)
}

// Note: run this test with -race
func TestSynchronizationBugWithActualRequest(t *testing.T) {
testSynchronizationBug(t, false)
}

func testSynchronizationBug(t *testing.T, preflight bool) {
t.Helper()
c := New(Options{
AllowedOrigins: []string{"*"},
AllowCredentials: true,
AllowedMethods: []string{http.MethodPut},
OptionsPassthrough: true,
})
var req *http.Request
if preflight {
req = httptest.NewRequest(http.MethodOptions, "https://example.org", nil)
req.Header.Add("Access-Control-Request-Method", http.MethodPut)
} else {
req = httptest.NewRequest(http.MethodGet, "https://example.org", nil)
}
req.Header.Add("Origin", "https://example.com")

// simulate concurrent requests
const n = 128
var wg sync.WaitGroup
wg.Add(n)
for range n {
go func() {
defer wg.Done()
rec := httptest.NewRecorder()
c.Handler(mutatingHandler).ServeHTTP(rec, req)
}()
}
wg.Wait()
}