Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions IMPLEMENTATION_PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)

---

Expand Down
33 changes: 31 additions & 2 deletions cmd/redirector-sync/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"flag"
"fmt"
"io"
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
13 changes: 7 additions & 6 deletions docs/FEATURES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
30 changes: 30 additions & 0 deletions docs/MANAGEMENT_API.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading