Skip to content

Adding the option to use an fs.FS for the file server middleware - #68

Merged
kolkov merged 5 commits into
go-ozzo:masterfrom
zekroTJA:master
Oct 27, 2026
Merged

kolkov merged 5 commits into
go-ozzo:masterfrom
zekroTJA:master

Conversation

@zekroTJA

@zekroTJA zekroTJA commented May 8, 2022

Copy link
Copy Markdown
Contributor

This pull request adds the option FS to the ServerOptions of the file server middleware which allows passing an fs.FS as content root.

For example, this makes it possible to use static web files directly compiled into the binary using embed.FS.

When the FS field is specified, this overrides the set RootPath. Otherwise, when FS is not specified, the given (or default) RootPath is used via os.DirFS.

Additionally, I've added a .gitattribute which specifies the contents of file/testdata to be cloned with LF end of lines. Otherwise, all unit tests fail when cloned on a Windows system because files are converted to CLRF.

zekroTJA added 2 commits May 8, 2022 11:03
Add .gitattributes where EOL is set to LF for testdata in file package so that tests would not fail on Windows systems.
@kolkov kolkov added type: feature New capability or rule priority: medium Normal development priority area: middleware Built-in middleware (auth, cors, logging) effort: medium Half day to a day labels Jul 3, 2026
os.DirFS rejects the names http.Dir used to clean, so the default RootPath
path stopped serving directory URLs with a trailing slash and paths with
"//", "." or ".." segments. Keep http.Dir for RootPath and clean names
the same way before handing them to an fs.FS.

Join the index file name with path.Join: http.FileSystem names are
slash-separated, and filepath.Join made index requests for subdirectories
404 on Windows.

Add fstest.MapFS tests, containment tests with hostile names for RootPath
and a directory-backed FS, FS notes in the ServerOptions and PathMap docs,
and an embed.FS example in the README.
@kolkov

kolkov commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, and sorry it sat for four years. Serving an embed.FS straight from the file middleware is a good addition, and having FS take precedence over RootPath is the shape I'd want too. The .gitattributes fix is welcome as well: with core.autocrlf=true the testdata files check out with CRLF and TestContent / TestServer fail.

Before merging I compared behaviour with master and found two things:

  • The default path now goes through os.DirFS, and fs.FS rejects names that http.Dir used to clean. On master, /css//main.css, /./css/main.css and /css/../index.html are served, and so is /css/ with an IndexFile (on Linux; on Windows it runs into the second point). With os.DirFS they all return 404, on Linux as well as Windows.
  • filepath.Join for the index file produces backslashes on Windows. fs.FS paths are always slash-separated, so the joined name is never found (os.DirFS rejects it outright). Master has the same problem with http.Dir, so that one isn't yours.

I pushed a fix to your branch (8e509ad): RootPath keeps using http.Dir, names are cleaned the same way before they reach an FS, the index file is joined with path.Join, and there are fstest.MapFS tests plus an embed.FS example in the README. io/fs needs Go 1.16 while go.mod still says 1.13; the minimum Go version goes up before this is merged, so that's covered. If you'd like anything done differently, let me know.

Resolves file/server.go against the v2.4.1 fix: the PathMap target check
(withinTarget) and plain 404s now apply to the FS path too, next to
cleanFS. Adds TestServerFSStaysInMappedDir with the v2.4.1 escape cases
through an fstest.MapFS, and the CHANGELOG entry for ServerOptions.FS.
Brings in the gddo removal (go-ozzo#75) so the branch is up to date with master,
as the ruleset requires.
@kolkov
kolkov self-requested a review as a code owner October 27, 2026 14:12
@codecov

codecov Bot commented Oct 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.87%. Comparing base (c6c3e81) to head (bf39330).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   94.85%   94.87%   +0.02%     
==========================================
  Files          21       21              
  Lines        1010     1014       +4     
==========================================
+ Hits          958      962       +4     
  Misses         52       52              
Files with missing lines Coverage Δ
file/server.go 95.83% <100.00%> (+0.18%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kolkov kolkov 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.

I merged master into the branch, so it now has the path checks from v2.4.1, and they apply to FS as well (there are tests for that through fstest.MapFS). CI is green on Linux, Windows and macOS. Approving; this goes into v2.5.0. Thanks for your patience!

@kolkov
kolkov merged commit db85c44 into go-ozzo:master Oct 27, 2026
11 of 12 checks passed
@kolkov kolkov mentioned this pull request Oct 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: middleware Built-in middleware (auth, cors, logging) effort: medium Half day to a day priority: medium Normal development priority type: feature New capability or rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants