Skip to content

Allow access to unsanitized file name - #800

Open
j0hax wants to merge 1 commit into
gocolly:masterfrom
j0hax:master
Open

Allow access to unsanitized file name#800
j0hax wants to merge 1 commit into
gocolly:masterfrom
j0hax:master

Conversation

@j0hax

@j0hax j0hax commented Jan 4, 2024

Copy link
Copy Markdown

Hello there! I've come across a situation where I have to save a file with a "double" extension (*.kepub.epub), and the current implementation of (r *Response) FileName() purposefully breaks that by always calling SanitizeFileName().

I propose to provide direct access to an unsanitized file name with the method RawFileName(). This shouldn't affect compatibility and allows for accessing information similar to URL package's RawPath, RawQuery, etc.

@WGH-

WGH- commented Mar 14, 2024

Copy link
Copy Markdown
Collaborator

Ehm, perhaps, sanitizing double extension away was a mistake in the first place.

@StantonMatt StantonMatt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I checked this locally. The change is backwards-compatible for FileName() because it now just sanitizes RawFileName(), and the full Go suite passes on the PR branch.

What I ran:

GOCACHE=.codex-tmp/go-cache GOTMPDIR=.codex-tmp/go-tmp GOMODCACHE=.codex-tmp/go-mod go test ./... -count=1
git diff --check upstream/master...HEAD

Both passed. GitHub reports no checks for this branch.

I also used a local-only test with books/book.kepub.epub and a Content-Disposition filename like ../../unsafe.kepub.epub. That confirmed the tradeoff: RawFileName() preserves the exact header/path-derived name, including path separators and unsafe segments, while FileName() still returns a safe filename and still folds the double extension into names like books_book_kepub.epub.

So this does solve the caller need without changing existing FileName() behavior. The maintainer question above still seems important, though: if the double-extension folding is itself the bug, a smaller change may be to adjust SanitizeFileName()/FileName() so safe double extensions survive. If both APIs are desired, I’d add regression tests for RawFileName() and the existing sanitized behavior so the safety distinction is explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants