From b58fe54c5022071622ee11ce3b1d3d98771fa8b1 Mon Sep 17 00:00:00 2001 From: Catherine Date: Sun, 7 Dec 2025 04:20:58 +0000 Subject: [PATCH] Report "dead" redirects as site issues. Using a non-forced redirect with a URL matching a manifest entry turns out to be a common and confusing mistake. --- src/manifest.go | 11 +++++--- src/pages.go | 2 +- src/redirects.go | 72 ++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 66 insertions(+), 19 deletions(-) diff --git a/src/manifest.go b/src/manifest.go index 93295d5..f6d7caa 100644 --- a/src/manifest.go +++ b/src/manifest.go @@ -257,24 +257,27 @@ func CompressFiles(ctx context.Context, manifest *Manifest) { // At the moment, there isn't a good way to report errors except to log them on the terminal. // (Perhaps in the future they could be exposed at `.git-pages/status.txt`?) func PrepareManifest(ctx context.Context, manifest *Manifest) error { - // Parse Netlify-style `_redirects` + // Parse Netlify-style `_redirects`. if err := ProcessRedirectsFile(manifest); err != nil { logc.Printf(ctx, "redirects err: %s\n", err) } else if len(manifest.Redirects) > 0 { logc.Printf(ctx, "redirects ok: %d rules\n", len(manifest.Redirects)) } - // Parse Netlify-style `_headers` + // Check if any redirects are unreachable. + LintRedirects(manifest) + + // Parse Netlify-style `_headers`. if err := ProcessHeadersFile(manifest); err != nil { logc.Printf(ctx, "headers err: %s\n", err) } else if len(manifest.Headers) > 0 { logc.Printf(ctx, "headers ok: %d rules\n", len(manifest.Headers)) } - // Sniff content type like `http.ServeContent` + // Sniff content type like `http.ServeContent`. DetectContentType(manifest) - // Opportunistically compress blobs (must be done last) + // Opportunistically compress blobs (must be done last). CompressFiles(ctx, manifest) return nil diff --git a/src/pages.go b/src/pages.go index 728d59b..fe97387 100644 --- a/src/pages.go +++ b/src/pages.go @@ -262,7 +262,7 @@ func getPage(w http.ResponseWriter, r *http.Request) error { redirectKind = RedirectForce } originalURL := (&url.URL{Host: r.Host}).ResolveReference(r.URL) - redirectURL, redirectStatus := ApplyRedirectRules(manifest, originalURL, redirectKind) + _, redirectURL, redirectStatus := ApplyRedirectRules(manifest, originalURL, redirectKind) if Is3xxHTTPStatus(redirectStatus) { writeRedirect(w, redirectStatus, redirectURL.String()) return nil diff --git a/src/redirects.go b/src/redirects.go index a0568e2..2670c33 100644 --- a/src/redirects.go +++ b/src/redirects.go @@ -13,7 +13,17 @@ import ( const RedirectsFileName string = "_redirects" -func unparseRule(rule redirects.Rule) string { +// Converts our Protobuf representation to tj/go-redirects. +func exportRedirectRule(rule *RedirectRule) *redirects.Rule { + return &redirects.Rule{ + From: rule.GetFrom(), + To: rule.GetTo(), + Status: int(rule.GetStatus()), + Force: rule.GetForce(), + } +} + +func unparseRedirectRule(rule *redirects.Rule) string { var statusPart string if rule.Force { statusPart = fmt.Sprintf("%d!", rule.Status) @@ -49,7 +59,7 @@ func Is3xxHTTPStatus(status int) bool { return status >= 300 && status <= 399 } -func validateRedirectRule(rule redirects.Rule) error { +func validateRedirectRule(rule *redirects.Rule) error { if len(rule.Params) > 0 { return fmt.Errorf("rules with parameters are not supported") } @@ -103,9 +113,9 @@ func ProcessRedirectsFile(manifest *Manifest) error { } for index, rule := range rules { - if err := validateRedirectRule(rule); err != nil { + if err := validateRedirectRule(&rule); err != nil { AddProblem(manifest, RedirectsFileName, - "rule #%d %q: %s", index+1, unparseRule(rule), err) + "rule #%d %q: %s", index+1, unparseRedirectRule(&rule), err) continue } manifest.Redirects = append(manifest.Redirects, &RedirectRule{ @@ -121,12 +131,7 @@ func ProcessRedirectsFile(manifest *Manifest) error { func CollectRedirectsFile(manifest *Manifest) string { var rules []string for _, rule := range manifest.GetRedirects() { - rules = append(rules, unparseRule(redirects.Rule{ - From: rule.GetFrom(), - To: rule.GetTo(), - Status: int(rule.GetStatus()), - Force: rule.GetForce(), - })+"\n") + rules = append(rules, unparseRedirectRule(exportRedirectRule(rule))+"\n") } return strings.Join(rules, "") } @@ -147,18 +152,22 @@ type RedirectKind int const ( RedirectAny RedirectKind = iota + RedirectNormal RedirectForce ) func ApplyRedirectRules( manifest *Manifest, fromURL *url.URL, kind RedirectKind, ) ( - toURL *url.URL, status int, + rule *RedirectRule, toURL *url.URL, status int, ) { fromSegments := pathSegments(fromURL.Path) next: - for _, rule := range manifest.Redirects { - if kind == RedirectForce && !*rule.Force { + for _, rule = range manifest.Redirects { + switch { + case kind == RedirectNormal && *rule.Force: + continue + case kind == RedirectForce && !*rule.Force: continue } // check if the rule matches fromURL @@ -205,8 +214,43 @@ next: RawQuery: fromURL.RawQuery, } status = int(*rule.Status) - break + return } // no redirect found + rule = nil return } + +func redirectHasSplat(rule *RedirectRule) bool { + ruleFromURL, _ := url.Parse(*rule.From) // pre-validated in `validateRedirectRule` + ruleFromSegments := pathSegments(ruleFromURL.Path) + return slices.Contains(ruleFromSegments, "*") +} + +func LintRedirects(manifest *Manifest) { + for name, entry := range manifest.GetContents() { + nameURL, err := url.Parse("/" + name) + if err != nil { + continue + } + + // Check if the entry URL would trigger a non-forced redirect if the entry didn't exist. + // If the redirect matches exactly one URL (i.e. has no splat) then it will never be + // triggered and an issue is reported; if the rule has a splat, it will always be possible + // to trigger it, as it matches an infinite number of URLs. + rule, _, _ := ApplyRedirectRules(manifest, nameURL, RedirectNormal) + if rule != nil && !redirectHasSplat(rule) { + entryDesc := "file" + if entry.GetType() == Type_Directory { + entryDesc = "directory" + } + AddProblem(manifest, name, + "%s shadows redirect %q; remove the %s or use a %d! forced redirect instead", + entryDesc, + unparseRedirectRule(exportRedirectRule(rule)), + entryDesc, + rule.GetStatus(), + ) + } + } +}