Skip to content

tests: cover non-ASCII filename encoding in Content-Disposition - #331

Merged
taoeffect merged 2 commits into
masterfrom
filename-non-ascii-whitespace-test
Aug 25, 2026
Merged

tests: cover non-ASCII filename encoding in Content-Disposition#331
taoeffect merged 2 commits into
masterfrom
filename-non-ascii-whitespace-test

Conversation

@pedrogaudencio

Copy link
Copy Markdown
Collaborator
  • adds a café résumé.pdf case to TestServeSetHeadersContentDisposition, exercising url.PathEscape for filename* (spaces + multi-byte UTF-8).
  • documents current behaviour: legacy filename keeps raw UTF-8 bytes.

Closes #303

AI Disclosure

Co-authored with: Opus 5

* added a café résumé.pdf case to TestServeSetHeadersContentDisposition, exercising url.PathEscape for filename* (spaces + multi-byte UTF-8).
* documents current behavior: legacy filename keeps raw UTF-8 bytes.
@pedrogaudencio pedrogaudencio self-assigned this Aug 24, 2026
Copilot AI lite review requested due to automatic review settings August 24, 2026 08:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@pedrogaudencio

pedrogaudencio commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

/review


AI review started.

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown

Advanced AI Review

  • Type: Agentic (crush)
  • Model: glm-5.3
Click to expand review

Review: tests: cover non-ASCII filename encoding in Content-Disposition

I verified the new assertion against the implementation (modules/httplib/serve.go:94-95): url.PathEscape("café résumé.pdf") produces exactly caf%C3%A9%20r%C3%A9sum%C3%A9.pdf and backslashEscapedName leaves the raw UTF-8 untouched, so the full-header expectation is correct. The full test package passes and go vet is clean. The change is test-only, matches the scope agreed in #303, and the documenting comment is accurate.

1. ⚪️ Table-driven form would reduce repetition in TestServeSetHeadersContentDisposition

  • Addressed
  • Dismissed

The addition at modules/httplib/serve_test.go:126-128 is now the third copy of the same record → serve → assert pattern, following the two existing cases at serve_test.go:115-118 and serve_test.go:120-122. The sibling test directly below (TestServeSetHeaderContentRelated, serve_test.go:135-146) already uses a table, so converting would also give each case a self-describing name, which helps failure output:

func TestServeSetHeadersContentDisposition(t *testing.T) {
	// a filename without an explicit disposition must default to "attachment", so that
	// user-controlled content is never rendered inline by the browser.
	// the "filename*" parameter is percent-encoded, while the legacy "filename" parameter
	// keeps the raw UTF-8 bytes (only backslashes and quotes are escaped)
	cases := []struct {
		name        string
		opts        *ServeHeaderOptions
		expected    string
		exposeCORS  bool
	}{
		{
			name:       "default disposition is attachment",
			opts:       &ServeHeaderOptions{Filename: "foo.zip"},
			expected:   `attachment; filename="foo.zip"; filename*=UTF-8''foo.zip`,
			exposeCORS: true,
		},
		{
			name:       "explicit inline disposition",
			opts:       &ServeHeaderOptions{Filename: "foo.zip", Disposition: "inline"},
			expected:   `inline; filename="foo.zip"; filename*=UTF-8''foo.zip`,
		},
		{
			name:       "non-ASCII filename with space",
			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"))
			if c.exposeCORS {
				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"))
}

This is stylistic only — the PR as written is correct, passes, and faithfully pins the behavior #303 asked to pin. No 🔴 or 🟡 issues found.


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

@pedrogaudencio

Copy link
Copy Markdown
Collaborator Author

@taoeffect approved! ✅

Copilot AI review requested due to automatic review settings August 25, 2026 07:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@taoeffect
taoeffect merged commit b743afb into master Aug 25, 2026
33 checks passed
@taoeffect
taoeffect deleted the filename-non-ascii-whitespace-test branch August 25, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RFC 5987 filename* encoding untested for non-ASCII and whitespace filenames

3 participants