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.

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

Jump to bottom

Content type is not set when HTB is used with a small buffer #882

Closed
adriansmares opened this issue Nov 10, 2023 · 1 comment · Fixed by #883
Closed

Content type is not set when HTB is used with a small buffer #882

adriansmares opened this issue Nov 10, 2023 · 1 comment · Fixed by #883

Comments

@adriansmares
Copy link

References #762 (comment)

It seems that the Content-Type header is not set when the size of the response body is smaller than the jitter entropy buffer, which doesn't seem like normal behavior at a first glance. As requested, please find a reproduction below:

func TestContentTypeDetectWithJitter(t *testing.T) {
	t.Parallel()
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		content := `<!DOCTYPE html>` + strings.Repeat("foo", 400)
		w.Write([]byte(content))
	})

	for _, tc := range []struct {
		name    string
		wrapper func(http.Handler) (http.Handler, error)
	}{
		{
			name: "no wrapping",
			wrapper: func(h http.Handler) (http.Handler, error) {
				return h, nil
			},
		},
		{
			name: "default",
			wrapper: func(h http.Handler) (http.Handler, error) {
				wrapper, err := NewWrapper()
				if err != nil {
					return nil, err
				}
				return wrapper(h), nil
			},
		},
		{
			name: "jitter, default buffer",
			wrapper: func(h http.Handler) (http.Handler, error) {
				wrapper, err := NewWrapper(RandomJitter(32, 0, false))
				if err != nil {
					return nil, err
				}
				return wrapper(h), nil
			},
		},
		{
			name: "jitter, small buffer",
			wrapper: func(h http.Handler) (http.Handler, error) {
				wrapper, err := NewWrapper(RandomJitter(32, DefaultMinSize, false))
				if err != nil {
					return nil, err
				}
				return wrapper(h), nil
			},
		},
	} {
		tc := tc
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()

			handler, err := tc.wrapper(handler)
			assertNil(t, err)

			req, resp := httptest.NewRequest(http.MethodGet, "/", nil), httptest.NewRecorder()
			req.Header.Add("Accept-Encoding", "gzip")

			handler.ServeHTTP(resp, req)

			assertEqual(t, "text/html; charset=utf-8", resp.Header().Get("Content-Type"))
		})
	}
}

On go version go1.21.3 darwin/arm64 this outputs:

--- FAIL: TestContentTypeDetectWithJitter (0.00s)
    --- FAIL: TestContentTypeDetectWithJitter/jitter,_default_buffer (0.00s)
        /Users/adriansmares/projects/compress/gzhttp/compress_test.go:1716: want "text/html; charset=utf-8", got "application/x-gzip"
FAIL
FAIL	github.com/klauspost/compress/gzhttp	0.181s
FAIL

As mentioned in the original issue, I think the culprit is the jitter buffer size guard added here:

compress/gzhttp/compress.go

Lines 127 to 130 in 847c1b4

// If the current buffer is less than minSize and a Content-Length isn't set, then wait until we have more data.
if len(w.buf) < w.minSize && cl == 0 || (w.jitterBuffer > 0 && len(w.buf) < w.jitterBuffer) {
return len(b), nil
}

I cannot tell what was the original intent, but I would've expected that the size of the entropy pool won't affect the Content-Type logic.

cc @klauspost (and apologies for the original necropost).
klauspost added a commit that referenced this issue Nov 10, 2023
@klauspost
Fix missing content type on Close
a4a7e17 
If compression had not yet been triggered in Write, be sure to detect content type on Close.

Fixes #882
@klauspost klauspost mentioned this issue Nov 10, 2023
Merged
@klauspost
Copy link
Owner

Thanks for the clean repro! Should be fixed by #883

klauspost added a commit that referenced this issue Nov 10, 2023
@klauspost
gzhttp: Fix missing content type on Close (#883)
80ba129 
If compression had not yet been triggered in Write, be sure to detect content type on Close.

Fixes #882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

gzhttp: Fix missing content type on Close klauspost/compress
2 participants
@klauspost @adriansmares