@marcel91200 / caddy Mirror · read-only / commits / fbd65609
Fork 0

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS

Mirrored from https://github.com/Slazee/caddy.git

Synced Nov 14, 05:15 UTC

fileserver: Only redirect if filename not rewritten (fix #4205)

This is the more correct implementation of 23dadc0d86dd75dad7559c25f20c9641bc7bc30f (#4179)... I think. This commit effectively undoes the revert in 8848df9c5d372a559d01512b7a4ef00e38867b55, but with corrections to the logic. We *do* need to use the original request path (the path the browser knows) for redirects, since they are external, and rewrites are only internal. However, if the path was rewritten to a non-canonical path, we should not redirect to canonicalize that, since rewrites are intentional by the site owner. Canonicalizing the path involves modifying only the suffix (base element, or filename) of the path. Thus, if a rewrite involves only the prefix (like how handle_path strips a path prefix), then we can (hopefully!) safely redirect using the original URI since the filename was not rewritten. So basically, if rewrites modify the filename, we should not canonicalize those requests. If rewrites only modify another part of the path (commonly a prefix), we should be OK to redirect.

Matthew Holt committed Jun 17, 2021 at 09:55 UTC fbd6560976dc73052bd5d3277869912de68f6731
2 files changed +42 -14
modules/caddyhttp/fileserver/browse.go
+21 -8
@@ -42,15 +42,28 @@ func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter,
42 zap.String("path", dirPath),
43 zap.String("root", root))
44
45 - // navigation on the client-side gets messed up if the
46 - // URL doesn't end in a trailing slash because hrefs like
47 - // "/b/c" on a path like "/a" end up going to "/b/c" instead
45 + // Navigation on the client-side gets messed up if the
46 + // URL doesn't end in a trailing slash because hrefs to
47 + // "b/c" at path "/a" end up going to "/b/c" instead
48 // of "/a/b/c" - so we have to redirect in this case
49 - if !strings.HasSuffix(r.URL.Path, "/") {
50 - fsrv.logger.Debug("redirecting to trailing slash to preserve hrefs", zap.String("request_path", r.URL.Path))
51 - r.URL.Path += "/"
52 - http.Redirect(w, r, r.URL.String(), http.StatusMovedPermanently)
53 - return nil
49 + // so that the path is "/a/" and the client constructs
50 + // relative hrefs "b/c" to be "/a/b/c".
51 + //
52 + // Only redirect if the last element of the path (the filename) was not
53 + // rewritten; if the admin wanted to rewrite to the canonical path, they
54 + // would have, and we have to be very careful not to introduce unwanted
55 + // redirects and especially redirect loops! (Redirecting using the
56 + // original URI is necessary because that's the URI the browser knows,
57 + // we don't want to redirect from internally-rewritten URIs.)
58 + // See https://github.com/caddyserver/caddy/issues/4205.
59 + origReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
60 + if path.Base(origReq.URL.Path) == path.Base(r.URL.Path) {
61 + if !strings.HasSuffix(origReq.URL.Path, "/") {
62 + fsrv.logger.Debug("redirecting to trailing slash to preserve hrefs", zap.String("request_path", r.URL.Path))
63 + origReq.URL.Path += "/"
64 + http.Redirect(w, r, origReq.URL.String(), http.StatusMovedPermanently)
65 + return nil
66 + }
67 }
68
69 dir, err := fsrv.openFile(dirPath, w)
modules/caddyhttp/fileserver/staticfiles.go
+21 -6
@@ -20,6 +20,7 @@ import (
20 "mime"
21 "net/http"
22 "os"
23 + "path"
24 "path/filepath"
25 "strconv"
26 "strings"
@@ -240,12 +241,26 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
241 // trailing slash - not enforcing this can break relative hrefs
242 // in HTML (see https://github.com/caddyserver/caddy/issues/2741)
243 if fsrv.CanonicalURIs == nil || *fsrv.CanonicalURIs {
243 - if implicitIndexFile && !strings.HasSuffix(r.URL.Path, "/") {
244 - fsrv.logger.Debug("redirecting to canonical URI (adding trailing slash for directory)", zap.String("path", r.URL.Path))
245 - return redirect(w, r, r.URL.Path+"/")
246 - } else if !implicitIndexFile && strings.HasSuffix(r.URL.Path, "/") {
247 - fsrv.logger.Debug("redirecting to canonical URI (removing trailing slash for file)", zap.String("path", r.URL.Path))
248 - return redirect(w, r, r.URL.Path[:len(r.URL.Path)-1])
244 + // Only redirect if the last element of the path (the filename) was not
245 + // rewritten; if the admin wanted to rewrite to the canonical path, they
246 + // would have, and we have to be very careful not to introduce unwanted
247 + // redirects and especially redirect loops!
248 + // See https://github.com/caddyserver/caddy/issues/4205.
249 + origReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
250 + if path.Base(origReq.URL.Path) == path.Base(r.URL.Path) {
251 + if implicitIndexFile && !strings.HasSuffix(origReq.URL.Path, "/") {
252 + to := origReq.URL.Path + "/"
253 + fsrv.logger.Debug("redirecting to canonical URI (adding trailing slash for directory)",
254 + zap.String("from_path", origReq.URL.Path),
255 + zap.String("to_path", to))
256 + return redirect(w, r, to)
257 + } else if !implicitIndexFile && strings.HasSuffix(origReq.URL.Path, "/") {
258 + to := origReq.URL.Path[:len(origReq.URL.Path)-1]
259 + fsrv.logger.Debug("redirecting to canonical URI (removing trailing slash for file)",
260 + zap.String("from_path", origReq.URL.Path),
261 + zap.String("to_path", to))
262 + return redirect(w, r, to)
263 + }
264 }
265 }
266