httplib: cover filename metacharacter escaping in ServeSetHeaders

[pedrogaudencio]

Closes #302

AI Disclosure

Co-authored with: Opus 5

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[pedrogaudencio]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

Verified the test passes and confirmed the expected encodings against the real implementation (url.PathEscape("café résumé.pdf") → caf%C3%A9%20r%C3%A9sum%C3%A9.pdf, url.PathEscape("a\"b\\c.zip") → a%22b%5Cc.zip). Review below.

1. 🟡 The exact case issue #303 asks for is missing; the added case covers #302's territory instead

• Addressed
• Dismissed

The PR description says "Closes #303", and issue #303's concluded fix is a test using a non-ASCII filename with a space, asserting the full header:

add a test case to TestServeSetHeadersContentDisposition using café résumé.pdf, asserting the full header string: attachment; filename="café résumé.pdf"; filename*=UTF-8''caf%C3%A9%20r%C3%A9sum%C3%A9.pdf

The added case instead exercises " and \, which per the issue discussion is the "related quote/backslash gap (issue #302)". As a result:

• Space (→ %20) and multi-byte UTF-8 (é → %C3%A9) percent-encoding by url.PathEscape remain unpinned.
• The raw-UTF-8-bytes-in-the-quoted-filename= behavior that the issue explicitly wanted documented ("making any future change visible rather than silent") is still undocumented.

The metacharacter case does partially satisfy #303's underlying concern (the suite can now distinguish the implementation from one that omits escaping entirely), so this is a coverage gap rather than a bug, but merging with "Closes #303" would close the issue without its requested case. Add it alongside the new case:

	// non-ASCII characters and spaces must be percent-encoded in the RFC 5987
	// parameter; the legacy quoted parameter intentionally keeps the raw UTF-8 bytes
	w = httptest.NewRecorder()
	ServeSetHeaders(w, &ServeHeaderOptions{Filename: "café résumé.pdf"})
	assert.Equal(t, `attachment; filename="café résumé.pdf"; filename*=UTF-8''caf%C3%A9%20r%C3%A9sum%C3%A9.pdf`, w.Header().Get("Content-Disposition"))

(I verified this exact assertion passes against the current modules/httplib/serve.go:94-95.)

2. ⚪️ Comment mischaracterizes the threat as "header injection"

• Addressed
• Dismissed

modules/httplib/serve_test.go:124-125:

	// quotes and backslashes must be escaped in the quoted-string form and
	// percent-encoded in the RFC 5987 parameter, to prevent header injection

Escaping " and \ prevents breaking out of the quoted-string in the Content-Disposition value (i.e. injecting additional disposition parameters), not HTTP header injection, which requires CR/LF — and Go's net/http already refuses to write header values containing control characters. Suggest:

	// quotes and backslashes must be escaped in the quoted-string form and
	// percent-encoded in the RFC 5987 parameter, so the filename cannot break
	// out of the quoted-string and inject disposition parameters

3. ⚪️ Growing copy-paste blocks; make the test table-driven like its sibling

• Addressed
• Dismissed

The diff extends a repeated NewRecorder / ServeSetHeaders / assert.Equal block pattern to a third copy (serve_test.go:126-128), while the sibling test TestServeSetHeaderContentRelated in the same file (serve_test.go:136-158) is already table-driven. Folding the cases (including the #303 case from issue 1) into a loop removes the repetition and makes future cases one-liners:

func TestServeSetHeadersContentDisposition(t *testing.T) {
	cases := []struct {
		name     string
		opts     *ServeHeaderOptions
		expected string
	}{
		{
			// a filename without an explicit disposition must default to "attachment", so that
			// user-controlled content is never rendered inline by the browser
			name:     "DefaultsToAttachment",
			opts:     &ServeHeaderOptions{Filename: "foo.zip"},
			expected: `attachment; filename="foo.zip"; filename*=UTF-8''foo.zip`,
		},
		{
			name:     "InlineDisposition",
			opts:     &ServeHeaderOptions{Filename: "foo.zip", Disposition: "inline"},
			expected: `inline; filename="foo.zip"; filename*=UTF-8''foo.zip`,
		},
		{
			name:     "EscapesQuoteAndBackslash",
			opts:     &ServeHeaderOptions{Filename: `a"b\c.zip`},
			expected: `attachment; filename="a\"b\\c.zip"; filename*=UTF-8''a%22b%5Cc.zip`,
		},
		{
			name:     "PercentEncodesNonASCIIAndSpaces",
			opts:     &ServeHeaderOptions{Filename: "café résumé.pdf"},
			expected: `attachment; filename="café résumé.pdf"; filename*=UTF-8''caf%C3%A9%20r%C3%A9sum%C3%A9.pdf`,
		},
	}
	for _, c := range cases {
		t.Run(c.name, func(t *testing.T) {
			w := httptest.NewRecorder()
			ServeSetHeaders(w, c.opts)
			assert.Equal(t, c.expected, w.Header().Get("Content-Disposition"))
			assert.Equal(t, "Content-Disposition", w.Header().Get("Access-Control-Expose-Headers"))
		})
	}

	w := httptest.NewRecorder()
	ServeSetHeaders(w, &ServeHeaderOptions{})
	assert.Empty(t, w.Header().Get("Content-Disposition"))
}

The assertion values themselves in the new case are correct, the test passes, and no production code changes were needed or made.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[pedrogaudencio]

@taoeffect approved! ✅