diff --git a/IMPLEMENTATION_PLAN.md b/IMPLEMENTATION_PLAN.md index 9c900e7..1ad84de 100644 --- a/IMPLEMENTATION_PLAN.md +++ b/IMPLEMENTATION_PLAN.md @@ -622,6 +622,99 @@ This document outlines the phased implementation approach for building The Redir --- +## Phase 9: Circular Redirect Detection & Mitigation +**Goal**: Detect redirect loops at config-time and protect against them at runtime +**Status**: Not Started + +### Stage 9.1: Static Cycle Detection (Lint Check) +**Goal**: Detect circular redirect chains at lint/config-load time +**Status**: Complete ✓ +**Success Criteria**: Lint catches direct cycles (A→B→A), transitive cycles (A→B→C→A), and self-loops for exact and prefix rules. Regex/glob cycles reported as warnings. + +**Implementation**: `internal/lint/lint.go` — new `checkCircularRedirects()` method + +**Approach**: Build a directed graph from redirect rules and detect cycles with DFS (three-color marking: white→gray→black). Each rule is an edge from its match path to its redirect destination. + +**Edge Construction by Match Type**: +- **Exact**: Edge from `rule.Match.Path` → parsed path of `rule.Redirect.GetLocation()` +- **Prefix with PreservePath**: Edge from `rule.Match.Path` → parsed path of `rule.Redirect.GetLocation()` + check if destination falls within the prefix's match space (self-loop detection) +- **Regex/Glob**: Generate 3-5 sample paths using heuristics (e.g., replace `(\d+)` with `123`, `(.*)` with `test`), trace each sample through all rules. Report as `SeverityWarning` (best-effort, not provably correct) + +**Graph Node Normalization**: +- Strip scheme and host from destinations that point back to the same service (detect via `match.host` or relative URLs) +- Normalize trailing slashes for comparison +- Only consider redirect rules (3xx status), skip non-redirect responses + +**Cycle Detection Algorithm**: +1. Build adjacency list from redirect rules +2. DFS with three colors: unvisited, in-progress, done +3. When an in-progress node is revisited → cycle found +4. Track the full chain for error messages (e.g., "Circular redirect: rule-A → rule-B → rule-C → rule-A") + +**Tasks**: +- [ ] Add `checkCircularRedirects()` method to `Linter` +- [ ] Implement `buildRedirectGraph()` helper — returns adjacency list of rule ID → destination rule IDs +- [ ] Implement `extractDestinationPath()` — parse redirect URL, normalize, return local path (or "" if external) +- [ ] Implement `findCycles()` — DFS cycle detection returning chains +- [ ] Handle `PreservePath` prefix self-loops (destination prefix matches source prefix) +- [ ] Best-effort regex/glob sample tracing (generate sample URLs, trace through rules) +- [ ] Register check in `Lint()` method +- [ ] Write tests: direct cycle, transitive cycle, self-loop, prefix self-loop, no-cycle (clean config), cross-host cycle, external destination (no cycle), regex sample-based warning +- [ ] Multi-source cycle detection: add `checkCrossSourceCycles()` to `MultiSourceLinter` + +**Tests**: `internal/lint/lint_test.go` + +### Stage 9.2: Runtime Loop Protection +**Goal**: Break redirect loops at request time via hop counter header +**Status**: Not Started +**Success Criteria**: Requests that bounce through the redirector more than N times (default: 10) receive 508 Loop Detected instead of another redirect. Zero performance impact on non-looping requests (single header read). + +**Implementation**: `internal/server/server.go` — modify `handleRedirect()` + +**Approach**: +1. On incoming request, read `X-Redirect-Count` header (integer, default 0) +2. If count >= max (configurable, default 10), return **508 Loop Detected** with diagnostic body +3. If count < max, set `X-Redirect-Count: count+1` on the redirect response +4. Add `redirector_loop_detected_total` counter metric + +**Why X-Redirect-Count**: +- Only works when the redirector redirects back to itself (the most dangerous case) +- Zero cost on first-hop requests (just a header read) +- RFC 8586 CDN-Loop is designed for multi-CDN chains — this is simpler and fits the single-service case + +**Configuration**: +```yaml +server: + max_redirect_hops: 10 # default, 0 = disabled +``` + +**Tasks**: +- [ ] Add `MaxRedirectHops` field to `ServerConfig` (default: 10) +- [ ] Read `X-Redirect-Count` header in `handleRedirect()`, before rule matching +- [ ] If count >= max: return 508, increment metric, log warning with request path and chain length +- [ ] If redirect: set `X-Redirect-Count: count+1` on response +- [ ] Add `LoopDetectedTotal` counter to `Metrics` struct +- [ ] Wire metric in server +- [ ] Write tests: no header (first hop), header at max (508 response), header below max (incremented), disabled (max=0 bypasses check), non-redirect response (no header added) +- [ ] Update docs: MANAGEMENT_API.md (new metric), CONFIGURATION.md (new field), README.md (mention in DDoS section) + +**Tests**: `internal/server/server_test.go` + +### Stage 9.3: Lint CLI Output for Cycles +**Goal**: Clear, actionable lint output for circular redirect findings +**Status**: Not Started +**Success Criteria**: `redirector-sync --lint` shows circular redirect chains with visual arrows and suggested fixes + +**Tasks**: +- [ ] Format cycle chains as: `⟳ Circular redirect detected: rule-A → rule-B → rule-C → rule-A` +- [ ] Include rule IDs, match paths, and destinations in the cycle report +- [ ] Suggest fixes: "Remove one rule from the chain or change the destination to break the cycle" +- [ ] JSON output includes cycle details for CI integration + +**Tests**: `internal/lint/lint_test.go` (output formatting) + +--- + ## Remaining Work ### Priority 1 (Recommended) @@ -632,6 +725,7 @@ This document outlines the phased implementation approach for building The Redir - [x] AWS Secrets Manager (Phase 5.3) ✓ - [x] Multi-cloud providers (Phase 5.4) ✓ - [ ] Multi-Tenancy (Phase 8.1) +- [ ] Circular Redirect Detection (Phase 9) --- diff --git a/cmd/redirector-sync/main.go b/cmd/redirector-sync/main.go index 6077c73..bf962bd 100644 --- a/cmd/redirector-sync/main.go +++ b/cmd/redirector-sync/main.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "flag" "fmt" "io" @@ -310,12 +311,27 @@ func startWebhookServer(ctx context.Context, port int, secret string, syncer *Sy if err := syncer.SyncOnce(syncCtx, false); err != nil { log.Error().Err(err).Msg("Webhook-triggered sync failed") + + // Return lint issues in the response body when available + var lintErr *LintError + if errors.As(err, &lintErr) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnprocessableEntity) + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "lint_failed", + "source": lintErr.Source, + "message": lintErr.Error(), + "issues": lintErr.Result.Issues, + }) + return + } + http.Error(w, "Sync failed", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) + _ = json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) }) mux.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) { @@ -390,6 +406,19 @@ func loadSyncerConfig(path string) (*SyncerConfig, error) { return &cfg, nil } +// LintError is returned when a sync fails due to lint issues. +// It carries the full lint result so callers (like webhook handlers) can +// surface the actual issues to the user instead of a generic "sync failed". +type LintError struct { + Source string + Result *lint.Result +} + +func (e *LintError) Error() string { + return fmt.Sprintf("lint errors found in config from source %s (%d errors)", + e.Source, len(e.Result.Errors())) +} + // Syncer handles config synchronization. type Syncer struct { cfg *SyncerConfig @@ -723,7 +752,7 @@ func (s *Syncer) SyncOnce(ctx context.Context, dryRun bool) error { s.mu.Lock() s.syncErrors++ s.mu.Unlock() - return fmt.Errorf("lint errors found in config from source %s", src.Name()) + return &LintError{Source: src.Name(), Result: lintResult} } for _, issue := range lintResult.Warnings() { diff --git a/docs/FEATURES.md b/docs/FEATURES.md index dca7bd8..73f39d9 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -428,12 +428,13 @@ SUGGESTIONS: ### Checks -1. **Duplicate IDs** - Same rule ID in multiple files -2. **Overlapping patterns** - Rules that match same paths -3. **Greedy patterns** - Patterns that may shadow other rules -4. **Invalid regex** - Syntax errors in patterns -5. **Performance suggestions** - Regex optimization hints -6. **Unreachable rules** - Rules that will never match +1. **Circular redirects** - Detects redirect loops via graph cycle detection (exact/prefix as errors, regex/glob as warnings). Catches direct cycles (A→B→A), transitive chains, prefix self-loops with `preserve_path`, and cross-host cycles +2. **Duplicate IDs** - Same rule ID in multiple files +3. **Overlapping patterns** - Rules that match same paths +4. **Greedy patterns** - Patterns that may shadow other rules +5. **Invalid regex** - Syntax errors in patterns +6. **Performance suggestions** - Regex optimization hints +7. **Unreachable rules** - Rules that will never match ### Implementation diff --git a/docs/MANAGEMENT_API.md b/docs/MANAGEMENT_API.md index 1628ddb..c7342d8 100644 --- a/docs/MANAGEMENT_API.md +++ b/docs/MANAGEMENT_API.md @@ -112,6 +112,36 @@ The syncer exposes metrics at `/metrics` on its webhook server port: | `redirector_sync_rules_fetched` | Gauge | — | Rules from last successful fetch | | `redirector_sync_lint_errors_total` | Counter | source | Lint errors during sync | +### Webhook Response (redirector-sync) + +When the syncer receives a webhook and triggers a sync, the response includes actionable details: + +**Success:** +```json +{"status": "ok"} +``` + +**Lint failure (HTTP 422):** +```json +{ + "status": "lint_failed", + "source": "github-primary", + "message": "lint errors found in config from source github-primary (2 errors)", + "issues": [ + { + "severity": "error", + "rule_id": "rule-a", + "message": "Circular redirect detected: rule-a -> rule-b -> rule-a", + "suggestion": "Remove one rule from the chain or change a destination to break the cycle" + } + ] +} +``` + +**Other failure (HTTP 500):** Plain text `"Sync failed"`. + +Use the `issues` array in CI pipelines to provide specific feedback when config changes introduce problems like circular redirects, duplicate IDs, or overlapping patterns. + --- ## Authentication diff --git a/docs/SYNCER.md b/docs/SYNCER.md index 7d8b7ca..1014777 100644 --- a/docs/SYNCER.md +++ b/docs/SYNCER.md @@ -129,6 +129,7 @@ Validate configuration and detect issues before deployment. Lint is integrated i ``` **Checks performed:** +- **Circular redirect detection** — cycles, self-loops, and regex/glob sample tracing - Duplicate rule IDs - Overlapping patterns (rules that match same paths) - Greedy patterns without negative priority @@ -139,22 +140,24 @@ Validate configuration and detect issues before deployment. Lint is integrated i Example output: ``` The Redirector - Config Linter -━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Loaded 25 rules -✗ ERRORS (1) -───────────────────────────────── +✗ ERRORS (2) +───────────────────────────────────────────────────────────────── [rule-5] Duplicate rule ID 'homepage' (first seen at index 0) + [rule-a] Circular redirect detected: rule-a -> rule-b -> rule-a + → Remove one rule from the chain or change a destination to break the cycle ⚠ WARNINGS (2) -───────────────────────────────── +───────────────────────────────────────────────────────────────── [api-v1] Rule 'api-v1' may overlap with 'api-all': Prefix '/api/' is contained in '/api/v1/' → Consider setting different priorities to control matching order [catch-all] Greedy glob pattern '/**' will match many paths → Set a negative priority (e.g., -100) to ensure it's evaluated last -━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ -Found: 1 errors, 2 warnings, 0 suggestions +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +Found: 2 errors, 2 warnings, 0 suggestions ``` ### Multi-Team Conflict Detection @@ -200,6 +203,209 @@ Recommendation: Teams should coordinate on conflicting paths or use different path prefixes to avoid runtime conflicts. ``` +### Circular Redirect Detection + +The linter detects redirect loops that would cause infinite request cycles. This is one of the most common issues with large redirect rule sets, especially on content sites where teams independently manage rules. + +#### How It Works + +The linter builds a **directed graph** from all redirect rules and runs **depth-first search (DFS)** cycle detection: + +``` + ┌──────────┐ ┌──────────┐ ┌──────────┐ + │ rule-a │────▶│ rule-b │────▶│ rule-c │ + │ /page-a │ │ /page-b │ │ /page-c │ + │ → /page-b│ │ → /page-c│ │ → /page-a│ ◀── cycle! + └──────────┘ └──────────┘ └──────────┘ + ▲ │ + └──────────────────────────────────┘ +``` + +Each rule is an edge: from the path it matches to the path it redirects to. When the graph has a cycle, the linter traces the full chain and reports it as an error. + +#### What It Detects + +| Type | Severity | Example | Description | +|------|----------|---------|-------------| +| **Direct cycle** | Error | `/a → /b → /a` | Two rules redirecting to each other | +| **Transitive cycle** | Error | `/a → /b → /c → /a` | Chain of 3+ rules forming a loop | +| **Prefix cross-cycle** | Error | `/foo/ → /bar/`, `/bar/ → /foo/` | Prefix rules bouncing between each other | +| **Cross-host cycle** | Error | `example.com/p1 → example.com/p2 → example.com/p1` | Cycle via absolute URLs pointing back to local hosts | +| **Prefix self-loop** | Error | `/old/ → /old/new/` with `preserve_path` | A prefix rule that redirects back into its own match space, causing expanding paths: `/old/x → /old/new/x → /old/new/new/x → ...` | +| **Regex self-match** | Warning | `^/api/v1/(.*) → /api/v1/v2/$1` | Regex destination still matches the same rule (best-effort, sample-based) | +| **Glob loop** | Warning | `/docs/* → /docs/archive/sample` | Glob destination falls within the same glob match pattern (best-effort) | + +#### Example: Direct Cycle + +Config with a circular redirect: +```yaml +rules: + - id: old-home + match: + type: exact + path: /old-home + redirect: + to: /new-home + status: 301 + + - id: new-home + match: + type: exact + path: /new-home + redirect: + to: /old-home # Oops — this creates a loop! + status: 301 +``` + +Lint output: +``` +The Redirector - Config Linter +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +Loaded 2 rules + +✗ ERRORS (1) +───────────────────────────────────────────────────────────────── + [old-home] Circular redirect detected: old-home -> new-home -> old-home + → Remove one rule from the chain or change a destination to break the cycle + +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +Found: 1 errors, 0 warnings, 0 suggestions +``` + +#### Example: Prefix Self-Loop with preserve_path + +This is a subtle bug — a prefix rule with `preserve_path: true` redirecting to a destination that starts with the same prefix: + +```yaml +rules: + - id: migrate-docs + match: + type: prefix + path: /docs/ + redirect: + to: /docs/v2/ + preserve_path: true + status: 301 +``` + +What happens at runtime: `/docs/api` → `/docs/v2/api` → `/docs/v2/v2/api` → `/docs/v2/v2/v2/api` → ... (infinite expanding path). + +Lint output: +``` +✗ ERRORS (1) +───────────────────────────────────────────────────────────────── + [migrate-docs] Prefix rule 'migrate-docs' with preserve_path creates a self-loop: + match '/docs/' redirects to '/docs/v2/' which is within the same prefix + → Change the destination to a path outside the match prefix, or disable preserve_path +``` + +**Fix:** Change the destination to a path outside the match prefix: +```yaml + - id: migrate-docs + match: + type: prefix + path: /docs/ + redirect: + to: /documentation/v2/ # Outside /docs/ — no loop + preserve_path: true + status: 301 +``` + +#### Example: Regex Warning (Best-Effort) + +Regex rules can't always be statically analyzed (regex intersection is undecidable), so the linter generates sample paths and traces them through the rules: + +```yaml +rules: + - id: api-rewrite + match: + type: regex + pattern: "^/api/v1/(.*)" + redirect: + to: /api/v1/v2/$1 # Destination still matches ^/api/v1/(.*)! + status: 302 +``` + +Lint output: +``` +⚠ WARNINGS (1) +───────────────────────────────────────────────────────────────── + [api-rewrite] Potential circular redirect: rule 'api-rewrite' destination '/api/v1/v2/$1' + may match the same rule (sample path: '/api/v1/sample' -> '/api/v1/v2/sample') + → Verify the destination does not fall within the rule's match pattern +``` + +#### Example: JSON Output for CI + +```bash +./redirector-sync --lint --lint-config config.yaml --lint-json +``` + +```json +{ + "issues": [ + { + "severity": "error", + "rule_id": "old-home", + "message": "Circular redirect detected: old-home -> new-home -> old-home", + "suggestion": "Remove one rule from the chain or change a destination to break the cycle" + }, + { + "severity": "warning", + "rule_id": "api-rewrite", + "message": "Potential circular redirect: rule 'api-rewrite' destination '/api/v1/v2/$1' may match the same rule (sample path: '/api/v1/sample' -> '/api/v1/v2/sample')", + "suggestion": "Verify the destination does not fall within the rule's match pattern" + } + ], + "rules_count": 5, + "files_count": 0 +} +``` + +Use this in CI to gate deployments: +```bash +# In your config repo's CI pipeline +if ! ./redirector-sync --lint --lint-config config.yaml; then + echo "Config validation failed — check for circular redirects" + exit 1 +fi +``` + +#### Webhook Response + +When the syncer receives a webhook and the fetched config has lint errors, the response includes the full issue list (instead of a generic "Sync failed"): + +``` +POST /webhook → HTTP 422 Unprocessable Entity +``` +```json +{ + "status": "lint_failed", + "source": "github-primary", + "message": "lint errors found in config from source github-primary (1 errors)", + "issues": [ + { + "severity": "error", + "rule_id": "old-home", + "message": "Circular redirect detected: old-home -> new-home -> old-home", + "suggestion": "Remove one rule from the chain or change a destination to break the cycle" + } + ] +} +``` + +This lets CI pipelines that trigger syncs via webhook inspect the response and provide specific feedback to the user. + +#### Detection Limitations + +| Rule Type | Detection | Notes | +|-----------|-----------|-------| +| Exact | Deterministic | Full cycle detection via graph analysis | +| Prefix | Deterministic | Includes `preserve_path` self-loop detection | +| Regex | Best-effort (samples) | Generates representative paths and traces them; reported as warnings | +| Glob | Best-effort (samples) | Same sample-based approach as regex; reported as warnings | +| External URLs | Skipped | Destinations pointing to hosts not in `match.host` are assumed external | + --- ## Debug Logging diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 0674b7d..3739a5a 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -3,6 +3,7 @@ package lint import ( "fmt" + "net/url" "regexp" "sort" "strings" @@ -91,6 +92,7 @@ func (l *Linter) Lint() *Result { result.Issues = append(result.Issues, l.checkRegexPerformance()...) result.Issues = append(result.Issues, l.checkUnreachableRules()...) result.Issues = append(result.Issues, l.checkMissingDefaults()...) + result.Issues = append(result.Issues, l.checkCircularRedirects()...) // Sort issues by severity sort.SliceStable(result.Issues, func(i, j int) bool { @@ -389,6 +391,439 @@ func (l *Linter) checkMissingDefaults() []Issue { return issues } +// checkCircularRedirects detects redirect loops by building a directed graph +// and running DFS cycle detection. Exact and prefix rules produce error-severity +// findings; regex/glob rules produce warnings (best-effort sample tracing). +func (l *Linter) checkCircularRedirects() []Issue { + var issues []Issue + + // Only consider redirect rules (3xx status) + redirectRules := make([]config.Rule, 0, len(l.cfg.Rules)) + for _, rule := range l.cfg.Rules { + if rule.Redirect.IsRedirect() { + redirectRules = append(redirectRules, rule) + } + } + + if len(redirectRules) == 0 { + return nil + } + + // Collect all hosts from rules to determine which destinations are "local" + localHosts := l.collectLocalHosts() + + // Build adjacency list: rule index -> list of rule indices it redirects to + adj := l.buildRedirectGraph(redirectRules, localHosts) + + // DFS cycle detection with three-color marking + chains := l.findCycles(redirectRules, adj) + + for _, chain := range chains { + ids := make([]string, len(chain)) + for i, idx := range chain { + ids[i] = redirectRules[idx].ID + } + chainStr := strings.Join(ids, " -> ") + + issues = append(issues, Issue{ + Severity: SeverityError, + RuleID: ids[0], + Message: fmt.Sprintf("Circular redirect detected: %s", chainStr), + Suggestion: "Remove one rule from the chain or change a destination to break the cycle", + }) + } + + // Check prefix self-loops (PreservePath redirecting back into own match space) + issues = append(issues, l.checkPrefixSelfLoops(redirectRules, localHosts)...) + + // Best-effort regex/glob sample tracing + issues = append(issues, l.checkRegexGlobCycles(redirectRules, localHosts)...) + + return issues +} + +// collectLocalHosts returns the set of hosts defined in rules' match.host fields. +// Destinations pointing to these hosts are considered "local" (could loop back). +// If no rules specify a host, all relative-path destinations are local. +func (l *Linter) collectLocalHosts() map[string]bool { + hosts := make(map[string]bool) + for _, rule := range l.cfg.Rules { + if rule.Match.Host != "" { + hosts[rule.Match.Host] = true + } + } + return hosts +} + +// extractDestinationPath parses a redirect destination URL and returns the +// local path if the destination points back to this service, or "" if external. +func (l *Linter) extractDestinationPath(dest string, localHosts map[string]bool) string { + // Relative paths are always local + if strings.HasPrefix(dest, "/") { + return dest + } + + parsed, err := url.Parse(dest) + if err != nil { + return "" + } + + // If we have no local hosts defined, we can't determine locality from absolute URLs + if len(localHosts) == 0 { + return "" + } + + // Check if the destination host is one of our local hosts + if localHosts[parsed.Hostname()] { + path := parsed.Path + if path == "" { + path = "/" + } + return path + } + + return "" +} + +// buildRedirectGraph creates an adjacency list mapping each rule index to +// the indices of rules that would match the redirect destination. +func (l *Linter) buildRedirectGraph(rules []config.Rule, localHosts map[string]bool) map[int][]int { + adj := make(map[int][]int) + + for i, rule := range rules { + dest := rule.Redirect.GetLocation() + destPath := l.extractDestinationPath(dest, localHosts) + if destPath == "" { + continue + } + + // Find which rules would match this destination path + for j, target := range rules { + if i == j { + continue // Self-loops handled separately in checkPrefixSelfLoops + } + + if l.pathMatchesRule(destPath, target) { + adj[i] = append(adj[i], j) + } + } + } + + return adj +} + +// pathMatchesRule checks if a given path would be matched by a rule. +func (l *Linter) pathMatchesRule(path string, rule config.Rule) bool { + switch rule.Match.Type { + case config.MatchTypeExact: + return path == rule.Match.Path + case config.MatchTypePrefix: + return strings.HasPrefix(path, rule.Match.Path) + case config.MatchTypeRegex: + if re := rule.CompiledRegex(); re != nil { + return re.MatchString(path) + } + // Try compiling the pattern for lint-time checking + re, err := regexp.Compile(rule.Match.Pattern) + if err != nil { + return false + } + return re.MatchString(path) + case config.MatchTypeGlob: + return l.globMatchesPath(rule.Match.Pattern, path) + } + return false +} + +// globMatchesPath does a simple glob match for lint purposes. +func (l *Linter) globMatchesPath(pattern, path string) bool { + // Handle common glob patterns + if pattern == "/**" || pattern == "/*" { + return true + } + if strings.HasSuffix(pattern, "/**") { + prefix := strings.TrimSuffix(pattern, "/**") + return strings.HasPrefix(path, prefix) + } + if strings.HasSuffix(pattern, "/*") { + prefix := strings.TrimSuffix(pattern, "/*") + return strings.HasPrefix(path, prefix+"/") + } + return false +} + +// findCycles runs DFS on the redirect graph and returns all cycles found. +// Each cycle is a slice of rule indices forming the loop. +func (l *Linter) findCycles(rules []config.Rule, adj map[int][]int) [][]int { + const ( + white = 0 // unvisited + gray = 1 // in progress (on current DFS stack) + black = 2 // done + ) + + color := make([]int, len(rules)) + parent := make([]int, len(rules)) + for i := range parent { + parent[i] = -1 + } + + var cycles [][]int + seen := make(map[string]bool) // Deduplicate cycles + + var dfs func(u int, stack []int) + dfs = func(u int, stack []int) { + color[u] = gray + stack = append(stack, u) + + for _, v := range adj[u] { + switch color[v] { + case gray: + // Found a cycle — extract the cycle from the stack + cycle := extractCycle(stack, v) + if cycle != nil { + key := cycleKey(cycle, rules) + if !seen[key] { + seen[key] = true + cycles = append(cycles, cycle) + } + } + case white: + dfs(v, stack) + } + } + + color[u] = black + } + + for i := range rules { + if color[i] == white { + dfs(i, nil) + } + } + + return cycles +} + +// extractCycle extracts the cycle portion from a DFS stack. +// The cycle starts at the node 'start' and includes everything +// after it on the stack, plus 'start' again to close the loop. +func extractCycle(stack []int, start int) []int { + for i, node := range stack { + if node == start { + cycle := make([]int, len(stack)-i+1) + copy(cycle, stack[i:]) + cycle[len(cycle)-1] = start // Close the loop + return cycle + } + } + return nil +} + +// cycleKey produces a canonical string key for deduplication. +// Rotates the cycle so the smallest rule ID comes first. +func cycleKey(cycle []int, rules []config.Rule) string { + if len(cycle) <= 1 { + return "" + } + // Exclude the closing element (duplicate of first) + nodes := cycle[:len(cycle)-1] + + // Find the minimum ID position + minIdx := 0 + for i := 1; i < len(nodes); i++ { + if rules[nodes[i]].ID < rules[nodes[minIdx]].ID { + minIdx = i + } + } + + // Rotate to start at minIdx + rotated := make([]string, len(nodes)) + for i := range nodes { + rotated[i] = rules[nodes[(i+minIdx)%len(nodes)]].ID + } + return strings.Join(rotated, "->") +} + +// checkPrefixSelfLoops detects prefix rules with PreservePath that redirect +// back into their own match space, creating implicit self-loops. +func (l *Linter) checkPrefixSelfLoops(rules []config.Rule, localHosts map[string]bool) []Issue { + var issues []Issue + + for _, rule := range rules { + if rule.Match.Type != config.MatchTypePrefix || !rule.Redirect.PreservePath { + continue + } + + dest := rule.Redirect.GetLocation() + destPath := l.extractDestinationPath(dest, localHosts) + if destPath == "" { + continue + } + + // A prefix rule with PreservePath creates a self-loop when the + // destination path starts with (or equals) the match prefix. + // Example: match "/old/" with PreservePath, redirect to "/old/new/" + // Request for /old/foo → /old/new/foo → /old/new/new/foo → ... + if strings.HasPrefix(destPath, rule.Match.Path) { + issues = append(issues, Issue{ + Severity: SeverityError, + RuleID: rule.ID, + Message: fmt.Sprintf( + "Prefix rule '%s' with preserve_path creates a self-loop: "+ + "match '%s' redirects to '%s' which is within the same prefix", + rule.ID, rule.Match.Path, destPath), + Suggestion: "Change the destination to a path outside the match prefix, or disable preserve_path", + }) + } + } + + return issues +} + +// checkRegexGlobCycles uses sample URLs to detect potential cycles involving +// regex and glob rules. These are reported as warnings since they're best-effort. +func (l *Linter) checkRegexGlobCycles(rules []config.Rule, localHosts map[string]bool) []Issue { + var issues []Issue + + for _, rule := range rules { + if rule.Match.Type != config.MatchTypeRegex && rule.Match.Type != config.MatchTypeGlob { + continue + } + + // Generate sample paths for this rule + samples := l.generateSamplePaths(rule) + dest := rule.Redirect.GetLocation() + + for _, sample := range samples { + // Simulate what the destination would be for this sample + resolvedDest := dest + if rule.Match.Type == config.MatchTypeRegex { + re := rule.CompiledRegex() + if re == nil { + var err error + re, err = regexp.Compile(rule.Match.Pattern) + if err != nil { + continue + } + } + if re.MatchString(sample) { + resolvedDest = re.ReplaceAllString(sample, dest) + } + } + + destPath := l.extractDestinationPath(resolvedDest, localHosts) + if destPath == "" { + continue + } + + // Check if the resolved destination would match the same rule + if l.pathMatchesRule(destPath, rule) { + issues = append(issues, Issue{ + Severity: SeverityWarning, + RuleID: rule.ID, + Message: fmt.Sprintf( + "Potential circular redirect: rule '%s' destination '%s' "+ + "may match the same rule (sample path: '%s' -> '%s')", + rule.ID, dest, sample, destPath), + Suggestion: "Verify the destination does not fall within the rule's match pattern", + }) + break // One warning per rule is enough + } + + // Check if destination matches any other regex/glob rule that + // could redirect back + for _, other := range rules { + if other.ID == rule.ID { + continue + } + if l.pathMatchesRule(destPath, other) { + otherDest := other.Redirect.GetLocation() + otherDestPath := l.extractDestinationPath(otherDest, localHosts) + if otherDestPath != "" && l.pathMatchesRule(otherDestPath, rule) { + issues = append(issues, Issue{ + Severity: SeverityWarning, + RuleID: rule.ID, + Message: fmt.Sprintf( + "Potential circular redirect: '%s' -> '%s' -> '%s' "+ + "(sample: '%s' -> '%s' -> '%s')", + rule.ID, other.ID, rule.ID, + sample, destPath, otherDestPath), + Suggestion: "Verify these rules don't create a redirect loop", + }) + break + } + } + } + } + } + + return issues +} + +// generateSamplePaths creates representative sample paths for a regex or glob rule. +func (l *Linter) generateSamplePaths(rule config.Rule) []string { + switch rule.Match.Type { + case config.MatchTypeRegex: + return generateRegexSamples(rule.Match.Pattern) + case config.MatchTypeGlob: + return generateGlobSamples(rule.Match.Pattern) + } + return nil +} + +// generateRegexSamples produces sample paths from a regex pattern by +// replacing common capture groups with representative values. +func generateRegexSamples(pattern string) []string { + // Strip anchors for replacement + p := strings.TrimPrefix(pattern, "^") + p = strings.TrimSuffix(p, "$") + + // Replace common capture group patterns with sample values + replacements := []struct { + re *regexp.Regexp + repl string + }{ + {regexp.MustCompile(`\(\\d\+\)`), "123"}, + {regexp.MustCompile(`\(\[^/\]\+\)`), "sample"}, + {regexp.MustCompile(`\(\[^/\]\*\)`), "sample"}, + {regexp.MustCompile(`\(\.\+\)`), "test/path"}, + {regexp.MustCompile(`\(\.\*\)`), "test"}, + {regexp.MustCompile(`\(\.\+\?\)`), "t"}, + {regexp.MustCompile(`\(\.\*\?\)`), "t"}, + {regexp.MustCompile(`\\d\+`), "123"}, + {regexp.MustCompile(`\[^/\]\+`), "sample"}, + {regexp.MustCompile(`\[^/\]\*`), "sample"}, + {regexp.MustCompile(`\.\+`), "test/path"}, + {regexp.MustCompile(`\.\*`), "test"}, + } + + sample := p + for _, r := range replacements { + sample = r.re.ReplaceAllString(sample, r.repl) + } + + // Ensure it starts with / + if !strings.HasPrefix(sample, "/") { + sample = "/" + sample + } + + return []string{sample} +} + +// generateGlobSamples produces sample paths from a glob pattern. +func generateGlobSamples(pattern string) []string { + sample := pattern + sample = strings.ReplaceAll(sample, "**", "sub/path") + sample = strings.ReplaceAll(sample, "*", "sample") + sample = strings.ReplaceAll(sample, "?", "x") + + if !strings.HasPrefix(sample, "/") { + sample = "/" + sample + } + + return []string{sample} +} + // SourceInput represents a config source for multi-source linting. type SourceInput struct { Name string // e.g., "marketing", "engineering" diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index f2278c5..0042615 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -1,6 +1,7 @@ package lint import ( + "strings" "testing" "github.com/jamengual/the-redirector/internal/config" @@ -398,6 +399,339 @@ func TestMultiSourceLinter_RulesPerSource(t *testing.T) { } } +// --- Circular Redirect Detection Tests --- + +func TestLinter_CheckCircularRedirects_DirectCycle(t *testing.T) { + // A -> B -> A (exact rules redirecting to relative paths) + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "rule-a", + Match: config.Match{Type: config.MatchTypeExact, Path: "/page-a"}, + Redirect: config.Redirect{Status: 301, To: "/page-b"}, + }, + { + ID: "rule-b", + Match: config.Match{Type: config.MatchTypeExact, Path: "/page-b"}, + Redirect: config.Redirect{Status: 301, To: "/page-a"}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + errors := result.Errors() + found := false + for _, e := range errors { + if e.RuleID == "rule-a" || e.RuleID == "rule-b" { + if contains(e.Message, "Circular redirect detected") { + found = true + } + } + } + if !found { + t.Error("Expected error for direct circular redirect (A -> B -> A)") + for _, e := range errors { + t.Logf(" Error: %s (rule: %s)", e.Message, e.RuleID) + } + } +} + +func TestLinter_CheckCircularRedirects_TransitiveCycle(t *testing.T) { + // A -> B -> C -> A + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "rule-a", + Match: config.Match{Type: config.MatchTypeExact, Path: "/a"}, + Redirect: config.Redirect{Status: 301, To: "/b"}, + }, + { + ID: "rule-b", + Match: config.Match{Type: config.MatchTypeExact, Path: "/b"}, + Redirect: config.Redirect{Status: 302, To: "/c"}, + }, + { + ID: "rule-c", + Match: config.Match{Type: config.MatchTypeExact, Path: "/c"}, + Redirect: config.Redirect{Status: 301, To: "/a"}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + errors := result.Errors() + found := false + for _, e := range errors { + if contains(e.Message, "Circular redirect detected") && + contains(e.Message, "rule-a") && + contains(e.Message, "rule-b") && + contains(e.Message, "rule-c") { + found = true + } + } + if !found { + t.Error("Expected error for transitive circular redirect (A -> B -> C -> A)") + for _, e := range errors { + t.Logf(" Error: %s (rule: %s)", e.Message, e.RuleID) + } + } +} + +func TestLinter_CheckCircularRedirects_NoCycle(t *testing.T) { + // A -> B, C -> D — no cycles + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "rule-a", + Match: config.Match{Type: config.MatchTypeExact, Path: "/a"}, + Redirect: config.Redirect{Status: 301, To: "/b"}, + }, + { + ID: "rule-b", + Match: config.Match{Type: config.MatchTypeExact, Path: "/b"}, + Redirect: config.Redirect{Status: 301, To: "/final"}, + }, + { + ID: "rule-c", + Match: config.Match{Type: config.MatchTypeExact, Path: "/c"}, + Redirect: config.Redirect{Status: 301, To: "/d"}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + for _, e := range result.Errors() { + if contains(e.Message, "Circular redirect") { + t.Errorf("Expected no circular redirect errors, got: %s", e.Message) + } + } +} + +func TestLinter_CheckCircularRedirects_ExternalDestination(t *testing.T) { + // Rules redirect to external hosts — should not be flagged as cycles + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "to-google", + Match: config.Match{Type: config.MatchTypeExact, Path: "/search"}, + Redirect: config.Redirect{Status: 301, To: "https://google.com/search"}, + }, + { + ID: "to-github", + Match: config.Match{Type: config.MatchTypeExact, Path: "/code"}, + Redirect: config.Redirect{Status: 301, To: "https://github.com/"}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + for _, e := range result.Errors() { + if contains(e.Message, "Circular redirect") { + t.Errorf("Expected no circular redirect errors for external destinations, got: %s", e.Message) + } + } +} + +func TestLinter_CheckCircularRedirects_CrossHostCycle(t *testing.T) { + // Rules with match.host — redirect to the same host creating a cycle + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "host-a", + Match: config.Match{Type: config.MatchTypeExact, Path: "/page1", Host: "example.com"}, + Redirect: config.Redirect{Status: 301, To: "https://example.com/page2"}, + }, + { + ID: "host-b", + Match: config.Match{Type: config.MatchTypeExact, Path: "/page2", Host: "example.com"}, + Redirect: config.Redirect{Status: 301, To: "https://example.com/page1"}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + errors := result.Errors() + found := false + for _, e := range errors { + if contains(e.Message, "Circular redirect detected") { + found = true + } + } + if !found { + t.Error("Expected error for cross-host circular redirect") + for _, e := range errors { + t.Logf(" Error: %s (rule: %s)", e.Message, e.RuleID) + } + } +} + +func TestLinter_CheckCircularRedirects_PrefixSelfLoop(t *testing.T) { + // Prefix rule with preserve_path redirecting into its own match space + // /old/ -> /old/new/ with preserve_path + // Request for /old/foo -> /old/new/foo -> /old/new/new/foo -> infinite + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "self-loop", + Match: config.Match{Type: config.MatchTypePrefix, Path: "/old/"}, + Redirect: config.Redirect{Status: 301, To: "/old/new/", PreservePath: true}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + errors := result.Errors() + found := false + for _, e := range errors { + if e.RuleID == "self-loop" && contains(e.Message, "self-loop") { + found = true + } + } + if !found { + t.Error("Expected error for prefix self-loop with preserve_path") + for _, e := range errors { + t.Logf(" Error: %s (rule: %s)", e.Message, e.RuleID) + } + } +} + +func TestLinter_CheckCircularRedirects_PrefixNoSelfLoop(t *testing.T) { + // Prefix rule with preserve_path but destination is OUTSIDE the match space + // /old/ -> /new/ with preserve_path — safe, no loop + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "safe-prefix", + Match: config.Match{Type: config.MatchTypePrefix, Path: "/old/"}, + Redirect: config.Redirect{Status: 301, To: "/new/", PreservePath: true}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + for _, e := range result.Errors() { + if e.RuleID == "safe-prefix" && contains(e.Message, "self-loop") { + t.Errorf("Expected no self-loop error for safe prefix rule, got: %s", e.Message) + } + } +} + +func TestLinter_CheckCircularRedirects_NonRedirectSkipped(t *testing.T) { + // Non-redirect rules (404, 503) should be completely skipped + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "not-found", + Match: config.Match{Type: config.MatchTypeExact, Path: "/missing"}, + Redirect: config.Redirect{Status: 404, Body: "Not Found"}, + }, + { + ID: "maintenance", + Match: config.Match{Type: config.MatchTypeExact, Path: "/api"}, + Redirect: config.Redirect{Status: 503, Body: "Maintenance"}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + for _, e := range result.Errors() { + if contains(e.Message, "Circular redirect") { + t.Errorf("Expected no circular redirect errors for non-redirect rules, got: %s", e.Message) + } + } +} + +func TestLinter_CheckCircularRedirects_PrefixCrossCycle(t *testing.T) { + // Two prefix rules creating a cross-cycle + // /foo/ -> /bar/ and /bar/ -> /foo/ + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "foo-to-bar", + Match: config.Match{Type: config.MatchTypePrefix, Path: "/foo/"}, + Redirect: config.Redirect{Status: 301, To: "/bar/"}, + }, + { + ID: "bar-to-foo", + Match: config.Match{Type: config.MatchTypePrefix, Path: "/bar/"}, + Redirect: config.Redirect{Status: 301, To: "/foo/"}, + }, + }, + } + + linter := New(cfg) + result := linter.Lint() + + errors := result.Errors() + found := false + for _, e := range errors { + if contains(e.Message, "Circular redirect detected") { + found = true + } + } + if !found { + t.Error("Expected error for prefix cross-cycle (/foo/ -> /bar/ -> /foo/)") + for _, e := range errors { + t.Logf(" Error: %s (rule: %s)", e.Message, e.RuleID) + } + } +} + +func TestLinter_CheckCircularRedirects_RegexSelfCycle(t *testing.T) { + // Regex rule that redirects back into its own match pattern + // ^/api/v1/(.*) -> /api/v1/v2/$1 + // This destination /api/v1/v2/... still matches ^/api/v1/(.*) + cfg := &config.Config{ + Rules: []config.Rule{ + { + ID: "api-loop", + Match: config.Match{Type: config.MatchTypeRegex, Pattern: "^/api/v1/(.*)"}, + Redirect: config.Redirect{Status: 301, To: "/api/v1/v2/$1"}, + }, + }, + } + + _ = cfg.Validate() // Compile regex + + linter := New(cfg) + result := linter.Lint() + + warnings := result.Warnings() + found := false + for _, w := range warnings { + if w.RuleID == "api-loop" && contains(w.Message, "Potential circular redirect") { + found = true + } + } + if !found { + t.Error("Expected warning for regex self-cycle") + for _, w := range warnings { + t.Logf(" Warning: %s (rule: %s)", w.Message, w.RuleID) + } + } +} + +// contains checks if a string contains a substring (test helper). +func contains(s, substr string) bool { + return strings.Contains(s, substr) +} + func TestMultiSourceResult_HasErrors(t *testing.T) { // With conflicts result := &MultiSourceResult{