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
5 changes: 3 additions & 2 deletions pkg/handler/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ func newHookEvent(c *httpContext, info FileInfo) HookEvent {
// > For incoming requests, the Host header is promoted to the
// > Request.Host field and removed from the Header map.
// That's why we add it back manually.
c.req.Header.Set("Host", c.req.Host)
copiedHeader := c.req.Header.Clone()
copiedHeader.Set("Host", c.req.Host)

return HookEvent{
Context: c,
Expand All @@ -42,7 +43,7 @@ func newHookEvent(c *httpContext, info FileInfo) HookEvent {
Method: c.req.Method,
URI: c.req.RequestURI,
RemoteAddr: c.req.RemoteAddr,
Header: c.req.Header,
Header: copiedHeader,
},
}
}
95 changes: 95 additions & 0 deletions pkg/handler/hooks_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package handler

import (
"encoding/json"
"net/http/httptest"
"sync"
"testing"

"github.com/stretchr/testify/assert"
)

// TestHookEventHeaderRace demonstrates a race condition that occurs when
// the http.Header map is shared between the original request and HookEvent.
// This test will fail with `go test -race` if Header is not cloned.
//
// See: https://github.com/tus/tusd/issues/1320
func TestHookEventHeaderRace(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for adding these tests as a demonstration. While they are useful now, I'm not sure if we should keep them in the repo after the fix is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, they are useful to catch if there might be a regression. It doesn't complicate or slow down, better to keep but if you insist, I can drop so that we can merge.

req := httptest.NewRequest("POST", "/files", nil)
req.Header.Set("Upload-Length", "1000")
req.Header.Set("Tus-Resumable", "1.0.0")
req.Host = "example.com"

c := &httpContext{
req: req,
}

info := FileInfo{
ID: "test-id",
Size: 1000,
MetaData: map[string]string{"filename": "test.txt"},
}

event := newHookEvent(c, info)

var wg sync.WaitGroup
const iterations = 100

// Goroutine 1: Simulate async hook processing (JSON encoding)
// This is what happens in invokeHookAsync -> json.Marshal
wg.Add(1)
go func() {
defer wg.Done()
for range iterations {
// This iterates over the Header map
_, _ = json.Marshal(event.HTTPRequest)
}
}()

// Goroutine 2: Simulate concurrent request header modification
// This could happen if another hook event is created for the same request
// or if middleware modifies headers
wg.Add(1)
go func() {
defer wg.Done()
for range iterations {
// This writes to the Header map
c.req.Header.Set("X-Request-ID", "some-value")
c.req.Header.Del("X-Request-ID")
}
}()

wg.Wait()
}

// TestHookEventHeaderIsolation verifies that modifying the original request
// headers after creating a HookEvent does not affect the event headers.
func TestHookEventHeaderIsolation(t *testing.T) {
req := httptest.NewRequest("POST", "/files", nil)
req.Header.Set("X-Original", "original-value")
req.Host = "example.com"

c := &httpContext{
req: req,
}

info := FileInfo{
ID: "test-id",
Size: 1000,
}

event := newHookEvent(c, info)

// Verify original header is present
assert.Equal(t, "original-value", event.HTTPRequest.Header.Get("X-Original"))

// Modify the original request header
c.req.Header.Set("X-Original", "modified-value")
c.req.Header.Set("X-New", "new-value")

// Verify event headers are NOT affected (isolation)
assert.Equal(t, "original-value", event.HTTPRequest.Header.Get("X-Original"),
"event header should remain unchanged after modifying original request")
assert.Empty(t, event.HTTPRequest.Header.Get("X-New"),
"new header added to original request should not appear in event")
}