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.

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

fileserver: Define request vars for tracking content served #5556

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 34 additions & 2 deletions modules/caddyhttp/fileserver/staticfiles.go
Expand Up @@ -71,10 +71,15 @@ func init() {
// modified the last component of the path (the filename).
//
// This handler sets the Etag and Last-Modified headers for static files.
// Files with an mtime of 1 second past epoch do not get an Etag.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Files with an mtime of 1 second past epoch do not get an Etag.
// Etag is omitted for files with a Unix epoch mod time of 0 or 1.

// It does not perform MIME sniffing to determine Content-Type based on
// contents, but does use the extension (if known); see the Go docs for
// details: https://pkg.go.dev/mime#TypeByExtension
//
// Variables are set on request context to store the path of the file being
// served (if serving a precompressed file, its name will be here), and a
// fs.FileInfo struct with stat data.
//
Comment on lines +79 to +82
Copy link
Member

Choose a reason for hiding this comment

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

Might be useful to describe what the variables are actually named -- and since this is a terminal handler (it writes the response and does not call a next middleware), the variable can only be used in response wrappers (i.e. returning back "up" the middleware chain). Not sure how to word that exactly but I can offer a suggestion if you'd like :D

// The file server properly handles requests with If-Match,
// If-Unmodified-Since, If-Modified-Since, If-None-Match, Range, and
// If-Range headers. It includes the file's modification time in the
Expand Down Expand Up @@ -231,7 +236,8 @@ func (fsrv *FileServer) Provision(ctx caddy.Context) error {
}

func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
ctx := r.Context()
repl := ctx.Value(caddy.ReplacerCtxKey).(*caddy.Replacer)

if runtime.GOOS == "windows" {
// reject paths with Alternate Data Streams (ADS)
Expand Down Expand Up @@ -302,6 +308,9 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
filename = indexPath
implicitIndexFile = true
fsrv.logger.Debug("located index file", zap.String("filename", filename))

caddyhttp.SetVar(ctx, StaticFileInfoVarKey, indexInfo)
caddyhttp.SetVar(ctx, StaticFilePathVarKey, indexPath)
break
}
}
Expand Down Expand Up @@ -394,6 +403,8 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
etag = calculateEtag(compressedInfo)
}

caddyhttp.SetVar(ctx, StaticFileInfoVarKey, compressedInfo)
caddyhttp.SetVar(ctx, StaticFilePathVarKey, compressedFilename)
break
}

Expand All @@ -415,6 +426,9 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
if etag == "" {
etag = calculateEtag(info)
}

caddyhttp.SetVar(ctx, StaticFileInfoVarKey, info)
caddyhttp.SetVar(ctx, StaticFilePathVarKey, filename)
}

// at this point, we're serving a file; Go std lib supports only
Expand Down Expand Up @@ -470,11 +484,23 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
w = statusOverrideResponseWriter{ResponseWriter: w, code: statusCodeOverride}
}

// Nix uses modtime=1 for files in its store, and it's useless to set
// the Last-Modified header in that case, so we need to check if the
// header was set by anything else first (i.e. user config or another
// module) so that we can later check if http.ServeContent set the header.
modTime := info.ModTime()
beforeLastModified := w.Header().Get("Last-Modified")

// let the standard library do what it does best; note, however,
// that errors generated by ServeContent are written immediately
// to the response, so we cannot handle them (but errors there
// are rare)
http.ServeContent(w, r, info.Name(), info.ModTime(), file.(io.ReadSeeker))
http.ServeContent(w, r, info.Name(), modTime, file.(io.ReadSeeker))

// Delete the Last-Modified header if it was changed and modtime=1 (i.e. Nix store)
if beforeLastModified != w.Header().Get("Last-Modified") && modTime.Unix() == 1 {
w.Header().Del("Last-Modified")
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? When http.ServeContent() returns, the response is already written.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good point, I'm not sure. I didn't try it.

Copy link

Choose a reason for hiding this comment

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

This is something we could handle with the same ResponseWriterWrapper trick, right? So maybe the mtime=1 special casing doesn't need to be upstream at all.

Copy link
Member

Choose a reason for hiding this comment

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

Can you just move these lines up to before ServeContent is called?

Copy link
Member

Choose a reason for hiding this comment

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

No because ServeContent is what is setting Last-Modified. That's what we're trying to intercept in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, duh.

Yeah, a ResponseWriter wrapper is probably best then.

}

return nil
}
Expand Down Expand Up @@ -674,6 +700,12 @@ var defaultIndexNames = []string{"index.html", "index.txt"}
const (
minBackoff, maxBackoff = 2, 5
separator = string(filepath.Separator)

// fs.FileInfo -- note that this contains only a basename, not a full path
StaticFileInfoVarKey = "handlers.file_server.info"

// string -- actual path used by caddy
StaticFilePathVarKey = "handlers.file_server.path"
)

// Interface guards
Expand Down