diff --git a/cmd/gotenberg.go b/cmd/gotenberg.go index d1786106..b7836751 100644 --- a/cmd/gotenberg.go +++ b/cmd/gotenberg.go @@ -80,7 +80,7 @@ func Run() { // Override their values if the corresponding environment variables are // set. fs.VisitAll(func(f *flag.Flag) { - envName := strings.ToUpper(strings.ReplaceAll(f.Name, "-", "_")) + envName := gotenberg.EnvVarName(f.Name) val, ok := os.LookupEnv(envName) if !ok { return diff --git a/pkg/gotenberg/allowlist.go b/pkg/gotenberg/allowlist.go new file mode 100644 index 00000000..8ffb6c8d --- /dev/null +++ b/pkg/gotenberg/allowlist.go @@ -0,0 +1,602 @@ +package gotenberg + +import ( + "strings" +) + +// AllowListRisk classifies why an allow-list pattern is dangerous. A URL that +// matches an allow-list skips the private and public IP checks, so a pattern +// that matches more than its author intended silently widens outbound access. +// See [AuditAllowList]. +type AllowListRisk string + +const ( + // AllowListRiskUnanchored marks a pattern with no leading "^". regexp2 + // searches rather than matches, so the pattern hits anywhere in the URL, + // including the query string. + AllowListRiskUnanchored AllowListRisk = "unanchored" + + // AllowListRiskUnanchoredBranch marks an alternation whose later branches + // have no leading "^". Anchoring only the first branch is a common slip. + AllowListRiskUnanchoredBranch AllowListRisk = "unanchored-branch" + + // AllowListRiskCatchAll marks a pattern with no literal prefix, such as + // ".+", which matches every URL and disables filtering entirely. + AllowListRiskCatchAll AllowListRisk = "catch-all" + + // AllowListRiskOpenHost marks a pattern whose host is not terminated, so + // it also matches attacker-chosen suffix hosts. For example + // "^https://trusted\.example\.com" matches + // "https://trusted.example.com.attacker.example/". + AllowListRiskOpenHost AllowListRisk = "open-host" +) + +// AllowListFinding reports one risky entry of an allow-list. +type AllowListFinding struct { + // Index is the zero-based position of the pattern within the flag value. + Index int + + // Pattern is the operator's pattern, verbatim. + Pattern string + + // Risk is why the pattern is dangerous. + Risk AllowListRisk +} + +// maxAuditedPatternLength bounds the patterns [AuditAllowList] inspects. A +// pathological pattern is not worth walking, and reporting nothing is better +// than reporting a partial verdict. +const maxAuditedPatternLength = 4096 + +// AuditAllowList reports the entries of an allow-list that match more URLs +// than their author is likely to intend. It is a lint over the pattern source, +// not a parser: it recognizes the shapes that are dangerous in practice and +// stays silent when it cannot be sure. +// +// Callers use the findings to warn operators. Never use them to reject a +// configuration: existing deployments rely on loose patterns, and a pattern +// this function does not flag is not thereby safe. +func AuditAllowList(patterns []string) []AllowListFinding { + var findings []AllowListFinding + + for i, pattern := range patterns { + if pattern == "" || len(pattern) > maxAuditedPatternLength { + continue + } + + risk, ok := auditPattern(pattern) + if ok { + findings = append(findings, AllowListFinding{Index: i, Pattern: pattern, Risk: risk}) + } + } + + return findings +} + +// auditPattern classifies a single pattern, reporting the first risk found. +func auditPattern(pattern string) (AllowListRisk, bool) { + body := trimInlineFlags(pattern) + + branches := splitTopLevelAlternation(body) + for i, branch := range branches { + branch = strings.TrimSpace(branch) + + anchored := hasStartAnchor(branch) + rest := strings.TrimPrefix(strings.TrimPrefix(branch, `\A`), "^") + + // Catch-all first: a pattern that constrains nothing matches every URL + // whether or not it is anchored, and saying so is more useful than + // telling the operator to anchor it. + if literalPrefix(rest) == "" { + return AllowListRiskCatchAll, true + } + + if !anchored { + if i == 0 { + return AllowListRiskUnanchored, true + } + return AllowListRiskUnanchoredBranch, true + } + + // A lookaround invalidates the token walk, so skip the host check for + // this branch rather than guess. The anchor and catch-all checks above + // still applied. + if containsLookaround(rest) { + continue + } + + if hostIsOpen(rest) { + return AllowListRiskOpenHost, true + } + } + + return "", false +} + +// trimInlineFlags removes a leading inline flag group such as "(?i)" so that +// the anchor check sees the pattern proper. +func trimInlineFlags(pattern string) string { + if !strings.HasPrefix(pattern, "(?") { + return pattern + } + + end := strings.Index(pattern, ")") + if end == -1 { + return pattern + } + + // Only a flag group qualifies. "(?:", "(?=", "(?!" and "(?<" open a real + // group and must stay. + flags := pattern[2:end] + if flags == "" || strings.ContainsAny(flags, ":=!<") { + return pattern + } + for _, r := range flags { + if !strings.ContainsRune("imsUx-", r) { + return pattern + } + } + + return pattern[end+1:] +} + +// hasStartAnchor reports whether branch begins with a start-of-input anchor. +func hasStartAnchor(branch string) bool { + return strings.HasPrefix(branch, "^") || strings.HasPrefix(branch, `\A`) +} + +// containsLookaround reports whether the pattern uses a lookaround, which the +// token walk in [hostIsOpen] cannot reason about. +func containsLookaround(s string) bool { + return strings.Contains(s, "(?=") || strings.Contains(s, "(?!") || strings.Contains(s, "(?<") +} + +// splitTopLevelAlternation splits on "|" at paren depth zero, honoring escapes +// and character classes. +func splitTopLevelAlternation(s string) []string { + var ( + parts []string + current strings.Builder + depth int + inClass bool + ) + + for i := 0; i < len(s); i++ { + c := s[i] + + switch { + case c == '\\' && i+1 < len(s): + current.WriteByte(c) + current.WriteByte(s[i+1]) + i++ + continue + case c == '[' && !inClass: + inClass = true + case c == ']' && inClass: + inClass = false + case c == '(' && !inClass: + depth++ + case c == ')' && !inClass: + depth-- + case c == '|' && !inClass && depth == 0: + parts = append(parts, current.String()) + current.Reset() + continue + } + + current.WriteByte(c) + } + + parts = append(parts, current.String()) + + return parts +} + +// literalPrefix returns the characters a matching URL must start with. It +// stops at the first optional or non-literal token, and descends one level +// into a leading mandatory group so that "^(https|http)://" is not mistaken +// for a catch-all. An empty result means the pattern constrains nothing. +func literalPrefix(s string) string { + var prefix strings.Builder + + for i := 0; i < len(s); { + // A group: descend once when it is mandatory, otherwise stop. + if s[i] == '(' { + end := matchingParen(s, i) + if end == -1 { + break + } + if isQuantified(s, end+1) { + break + } + + inner := trimInlineFlags(s[i+1 : end]) + branches := splitTopLevelAlternation(inner) + + common := literalPrefix(branches[0]) + for _, b := range branches[1:] { + common = commonPrefix(common, literalPrefix(b)) + } + prefix.WriteString(common) + + // Only the leading group is worth descending into. + break + } + + var token string + switch { + case s[i] == '\\' && i+1 < len(s): + token = s[i : i+2] + case s[i] == '[': + end := matchingBracket(s, i) + if end == -1 { + return prefix.String() + } + token = s[i : end+1] + default: + token = s[i : i+1] + } + + next := i + len(token) + if isQuantified(s, next) { + break + } + + // Only a plain literal or an escaped literal contributes. + switch { + case len(token) == 2 && token[0] == '\\' && !isEscapeClass(token[1]): + prefix.WriteByte(token[1]) + case len(token) == 1 && !strings.ContainsAny(token, `.[]()^$*+?{}|`): + prefix.WriteByte(token[0]) + default: + return prefix.String() + } + + i = next + } + + return prefix.String() +} + +// hostIsOpen reports whether the authority part of the pattern can be left +// without crossing a terminator, which means the pattern also matches +// attacker-chosen suffix hosts or userinfo. +// +// It walks the tokens after "://" and classifies each one. A terminator ends +// the authority, so the pattern is safe. A crosser can match "@", "?" or "#" +// and therefore lets a matching URL escape the authority, so the pattern is +// open. Reaching the end without a terminator is open too, which is the +// classic "^https://trusted\.example\.com" case. +func hostIsOpen(s string) bool { + _, after, ok := strings.Cut(s, "://") + if !ok { + // No authority to reason about, for example "^file:///tmp/". + return false + } + + rest := after + + for i := 0; i < len(rest); { + var token string + switch { + case rest[i] == '\\' && i+1 < len(rest): + token = rest[i : i+2] + case rest[i] == '[': + end := matchingBracket(rest, i) + if end == -1 { + return true + } + token = rest[i : end+1] + case rest[i] == '(': + end := matchingParen(rest, i) + if end == -1 { + return true + } + token = rest[i : end+1] + default: + token = rest[i : i+1] + } + + next := i + len(token) + optional := isOptionalQuantifier(rest, next) + + switch classifyHostToken(token) { + case hostTokenTerminator: + // An optional terminator does not end anything, since the URL may + // match without it. + if !optional { + return false + } + case hostTokenCrosser: + return true + case hostTokenNeutral: + // Part of the host itself, so keep walking. + } + + i = next + for i < len(rest) && isQuantifierByte(rest[i]) { + if rest[i] == '{' { + end := strings.IndexByte(rest[i:], '}') + if end == -1 { + return true + } + i += end + 1 + continue + } + i++ + } + } + + return true +} + +// hostTokenKind is how a token affects the walk in [hostIsOpen]. +type hostTokenKind int + +const ( + hostTokenNeutral hostTokenKind = iota + hostTokenTerminator + hostTokenCrosser +) + +// hostTerminators are the characters that end the authority of a URL. +const hostTerminators = "/:#?" + +// crosserClassChars are the characters that, if a class can match them, let a +// match escape the authority. +const crosserClassChars = "/@?#" + +// classifyHostToken classifies one token of the authority walk. +func classifyHostToken(token string) hostTokenKind { + switch { + case token == ".": + // The wildcard matches "@", "#" and "?", so a host built on it can be + // left without ever reaching a terminator. + return hostTokenCrosser + + case token == "$": + return hostTokenTerminator + + case len(token) == 1 && strings.Contains(hostTerminators, token): + return hostTokenTerminator + + case len(token) == 2 && token[0] == '\\': + switch token[1] { + case 'S', 'D', 'W': + return hostTokenCrosser + case 'd', 'w', 's': + return hostTokenNeutral + case 'p', 'P': + return hostTokenCrosser + } + if strings.Contains(hostTerminators, token[1:]) { + return hostTokenTerminator + } + return hostTokenNeutral + + case strings.HasPrefix(token, "["): + inner := strings.TrimSuffix(strings.TrimPrefix(token, "["), "]") + if strings.HasPrefix(inner, "^") { + // A negated class almost always admits "@". + return hostTokenCrosser + } + if classContainsAny(inner, crosserClassChars) { + return hostTokenCrosser + } + return hostTokenNeutral + + case strings.HasPrefix(token, "("): + return classifyGroup(token) + } + + return hostTokenNeutral +} + +// classifyGroup classifies a parenthesized group. A group whose every branch +// starts with a terminator ends the authority, which is what makes the +// idiomatic "(:|/|$)" safe. A group containing a crosser is a crosser. +func classifyGroup(token string) hostTokenKind { + inner := trimInlineFlags(strings.TrimSuffix(strings.TrimPrefix(token, "("), ")")) + inner = strings.TrimPrefix(inner, "?:") + + branches := splitTopLevelAlternation(inner) + + allTerminate := true + for _, branch := range branches { + if branch == "" { + allTerminate = false + continue + } + + kind := classifyHostToken(firstToken(branch)) + if kind == hostTokenCrosser { + return hostTokenCrosser + } + if kind != hostTokenTerminator { + allTerminate = false + } + + // A crosser anywhere inside the branch still escapes the authority. + if branchHasCrosser(branch) { + return hostTokenCrosser + } + } + + if allTerminate { + return hostTokenTerminator + } + + return hostTokenNeutral +} + +// branchHasCrosser reports whether any token of branch is a crosser. +func branchHasCrosser(branch string) bool { + for i := 0; i < len(branch); { + token := tokenAt(branch, i) + if token == "" { + return true + } + if classifyHostToken(token) == hostTokenCrosser { + return true + } + i += len(token) + } + + return false +} + +// firstToken returns the first regex token of s. +func firstToken(s string) string { + return tokenAt(s, 0) +} + +// tokenAt returns the regex token starting at index i, or "" if it is +// malformed. +func tokenAt(s string, i int) string { + if i >= len(s) { + return "" + } + + switch { + case s[i] == '\\' && i+1 < len(s): + return s[i : i+2] + case s[i] == '[': + end := matchingBracket(s, i) + if end == -1 { + return "" + } + return s[i : end+1] + case s[i] == '(': + end := matchingParen(s, i) + if end == -1 { + return "" + } + return s[i : end+1] + } + + return s[i : i+1] +} + +// classContainsAny reports whether a character class body can match any of the +// given characters, expanding simple ranges. +func classContainsAny(class, chars string) bool { + for i := 0; i < len(class); i++ { + if class[i] == '\\' && i+1 < len(class) { + // An escape class such as \S inside a class admits everything. + if strings.ContainsRune("SDW", rune(class[i+1])) { + return true + } + if strings.ContainsRune(chars, rune(class[i+1])) { + return true + } + i++ + continue + } + + if i+2 < len(class) && class[i+1] == '-' { + lo, hi := class[i], class[i+2] + for _, c := range []byte(chars) { + if c >= lo && c <= hi { + return true + } + } + i += 2 + continue + } + + if strings.ContainsRune(chars, rune(class[i])) { + return true + } + } + + return false +} + +// matchingParen returns the index of the ")" closing the "(" at start. +func matchingParen(s string, start int) int { + depth := 0 + inClass := false + + for i := start; i < len(s); i++ { + switch { + case s[i] == '\\' && i+1 < len(s): + i++ + case s[i] == '[' && !inClass: + inClass = true + case s[i] == ']' && inClass: + inClass = false + case s[i] == '(' && !inClass: + depth++ + case s[i] == ')' && !inClass: + depth-- + if depth == 0 { + return i + } + } + } + + return -1 +} + +// matchingBracket returns the index of the "]" closing the "[" at start. +func matchingBracket(s string, start int) int { + for i := start + 1; i < len(s); i++ { + switch { + case s[i] == '\\' && i+1 < len(s): + i++ + case s[i] == ']': + // A "]" immediately after "[" or "[^" is a literal. + if i == start+1 || (i == start+2 && s[start+1] == '^') { + continue + } + return i + } + } + + return -1 +} + +// isQuantifierByte reports whether c opens a quantifier. +func isQuantifierByte(c byte) bool { + return c == '?' || c == '*' || c == '+' || c == '{' +} + +// isQuantified reports whether a quantifier starts at index i. +func isQuantified(s string, i int) bool { + return i < len(s) && isQuantifierByte(s[i]) +} + +// isOptionalQuantifier reports whether the quantifier at index i lets the +// preceding token match nothing. +func isOptionalQuantifier(s string, i int) bool { + if i >= len(s) { + return false + } + + switch s[i] { + case '?', '*': + return true + case '{': + return strings.HasPrefix(s[i:], "{0") + } + + return false +} + +// isEscapeClass reports whether c after a backslash denotes a character class +// rather than a literal. +func isEscapeClass(c byte) bool { + return strings.ContainsRune("dDwWsSbBAzZpP", rune(c)) +} + +// commonPrefix returns the longest common prefix of a and b. +func commonPrefix(a, b string) string { + n := min(len(a), len(b)) + for i := range n { + if a[i] != b[i] { + return a[:i] + } + } + + return a[:n] +} diff --git a/pkg/gotenberg/allowlist_test.go b/pkg/gotenberg/allowlist_test.go new file mode 100644 index 00000000..96455be9 --- /dev/null +++ b/pkg/gotenberg/allowlist_test.go @@ -0,0 +1,177 @@ +package gotenberg + +import ( + "testing" + + "github.com/dlclark/regexp2" +) + +func TestAuditAllowList(t *testing.T) { + for _, tc := range []struct { + scenario string + pattern string + want AllowListRisk + }{ + // Safe: the host is terminated before anything can leave it. + {"idiomatic terminator group", `^https?://internal\.svc(:|/|$)`, ""}, + {"trailing slash", `^https://trusted\.example\.com/`, ""}, + {"optional port then terminator", `^https://example\.com(:[0-9]+)?(/|$)`, ""}, + {"positive class cannot leave authority", `^https://[a-z0-9.-]+\.s3\.amazonaws\.com/`, ""}, + {"leading mandatory group", `^(https|http)://a\.example\.com/`, ""}, + {"optional subdomain group", `^https://(www\.)?example\.com/`, ""}, + {"port terminator", `^https://example\.com:8443/`, ""}, + {"end anchor", `^https://example\.com$`, ""}, + {"alternation both anchored and terminated", `^https://a\.example/|^https://b\.example/`, ""}, + {"no authority to check", `^file:///tmp/`, ""}, + {"digit class in host", `^https://node\d+\.example\.com/`, ""}, + + // Unanchored: regexp2 searches, so these match anywhere in the URL. + {"no anchor", `trusted\.example\.com`, AllowListRiskUnanchored}, + {"no anchor with scheme", `https://trusted\.example\.com/`, AllowListRiskUnanchored}, + + // Only the first branch anchored. + {"second branch unanchored", `^http://a\.example/|http://b\.example/`, AllowListRiskUnanchoredBranch}, + + // Catch-all: matches every URL. + {"dot plus", `.+`, AllowListRiskCatchAll}, + {"dot star", `.*`, AllowListRiskCatchAll}, + {"anchored dot star", `^.*`, AllowListRiskCatchAll}, + {"anchored dot plus", `^.+`, AllowListRiskCatchAll}, + + // Open host: the reported vulnerability class. + {"advisory pattern", `^http://trusted\.example\.com`, AllowListRiskOpenHost}, + {"gotenberg.dev internet-facing recipe", `^https?://[^/]+\.internal\.example\.com`, AllowListRiskOpenHost}, + {"gotenberg.dev strict whitelist recipe", `^https://(api|cdn|images)\.internal\.example\.com`, AllowListRiskOpenHost}, + {"gotenberg.dev hooks recipe", `^https?://hooks\.internal\.example\.com`, AllowListRiskOpenHost}, + {"feature file pattern", `^https?://host.docker.internal.*`, AllowListRiskOpenHost}, + {"scheme only", `^https?://`, AllowListRiskOpenHost}, + {"wildcard subdomain", `^https://.+\.example\.com/`, AllowListRiskOpenHost}, + {"escaped dot is not a terminator", `^https?://example\.com\.`, AllowListRiskOpenHost}, + {"negated class in host", `^https://[^.]+\.example\.com/`, AllowListRiskOpenHost}, + } { + t.Run(tc.scenario, func(t *testing.T) { + findings := AuditAllowList([]string{tc.pattern}) + + if tc.want == "" { + if len(findings) != 0 { + t.Fatalf("AuditAllowList(%q) = %+v, want no finding", tc.pattern, findings) + } + return + } + + if len(findings) != 1 { + t.Fatalf("AuditAllowList(%q) returned %d findings, want 1", tc.pattern, len(findings)) + } + if findings[0].Risk != tc.want { + t.Fatalf("AuditAllowList(%q) risk = %q, want %q", tc.pattern, findings[0].Risk, tc.want) + } + }) + } +} + +// TestAuditAllowList_FlaggedPatternsAreActuallyExploitable proves the audit is +// not merely syntactic: every pattern it flags as open-host really does admit +// a host the operator did not intend. +func TestAuditAllowList_FlaggedPatternsAreActuallyExploitable(t *testing.T) { + for _, tc := range []struct { + pattern string + attack string + }{ + {`^http://trusted\.example\.com`, "http://trusted.example.com.attacker.example/"}, + {`^https?://[^/]+\.internal\.example\.com`, "http://a.internal.example.com.attacker.example/"}, + {`^https://(api|cdn|images)\.internal\.example\.com`, "https://api.internal.example.com.attacker.example/"}, + {`^https?://hooks\.internal\.example\.com`, "http://hooks.internal.example.com.attacker.example/"}, + {`^https?://host.docker.internal.*`, "http://host.docker.internal.attacker.example/"}, + {`^https://.+\.example\.com/`, "https://attacker.example/#x.example.com/"}, + {`^https?://example\.com\.`, "http://example.com.attacker.example/"}, + } { + t.Run(tc.pattern, func(t *testing.T) { + findings := AuditAllowList([]string{tc.pattern}) + if len(findings) == 0 { + t.Fatalf("pattern %q was not flagged", tc.pattern) + } + + ok, err := regexp2.MustCompile(tc.pattern, 0).MatchString(tc.attack) + if err != nil { + t.Fatalf("match %q: %v", tc.attack, err) + } + if !ok { + t.Fatalf("pattern %q does not match %q, so the finding is a false positive", tc.pattern, tc.attack) + } + }) + } +} + +// TestAuditAllowList_SafePatternsRejectTheAttacks is the converse: the shapes +// the audit stays silent about really do reject the same attacks. +func TestAuditAllowList_SafePatternsRejectTheAttacks(t *testing.T) { + safe := []string{ + `^https?://internal\.svc(:|/|$)`, + `^https://trusted\.example\.com/`, + `^https://example\.com(:[0-9]+)?(/|$)`, + `^https://[a-z0-9.-]+\.s3\.amazonaws\.com/`, + } + attacks := []string{ + "https://internal.svc.attacker.example/", + "https://trusted.example.com.attacker.example/", + "https://trusted.example.com@169.254.169.254/", + "https://example.com.attacker.example/", + "https://example.com@10.0.0.5/", + "https://bucket.s3.amazonaws.com.attacker.example/", + "https://bucket.s3.amazonaws.com@127.0.0.1/", + } + + for _, pattern := range safe { + t.Run(pattern, func(t *testing.T) { + if findings := AuditAllowList([]string{pattern}); len(findings) != 0 { + t.Fatalf("safe pattern %q was flagged as %q", pattern, findings[0].Risk) + } + + re := regexp2.MustCompile(pattern, 0) + for _, attack := range attacks { + ok, err := re.MatchString(attack) + if err != nil { + t.Fatalf("match %q: %v", attack, err) + } + if ok { + t.Fatalf("pattern %q matches attack %q but was not flagged", pattern, attack) + } + } + }) + } +} + +func TestAuditAllowList_SkipsEmptyAndOversized(t *testing.T) { + oversized := make([]byte, maxAuditedPatternLength+1) + for i := range oversized { + oversized[i] = 'a' + } + + findings := AuditAllowList([]string{"", string(oversized)}) + if len(findings) != 0 { + t.Fatalf("AuditAllowList returned %+v, want no finding", findings) + } +} + +func TestAuditAllowList_ReportsIndex(t *testing.T) { + findings := AuditAllowList([]string{ + `^https://ok\.example\.com/`, + `^https://open\.example\.com`, + }) + if len(findings) != 1 { + t.Fatalf("got %d findings, want 1", len(findings)) + } + if findings[0].Index != 1 { + t.Fatalf("findings[0].Index = %d, want 1", findings[0].Index) + } +} + +// TestAuditAllowList_ShippedChromiumDenyListIsNotAudited guards the rule that +// deny-lists are never audited. The shipped Chromium deny-list uses a +// lookaround and has no authority, so auditing it would produce noise. +func TestAuditAllowList_LookaroundIsNotFlaggedForHost(t *testing.T) { + findings := AuditAllowList([]string{`^file:(?!//\/tmp/).*`}) + if len(findings) != 0 { + t.Fatalf("AuditAllowList returned %+v, want no finding", findings) + } +} diff --git a/pkg/gotenberg/flags.go b/pkg/gotenberg/flags.go index 6e48499a..dcd7d360 100644 --- a/pkg/gotenberg/flags.go +++ b/pkg/gotenberg/flags.go @@ -1,11 +1,17 @@ package gotenberg import ( + "context" + "fmt" + "log/slog" + "strings" "time" "github.com/dlclark/regexp2" "github.com/labstack/gommon/bytes" flag "github.com/spf13/pflag" + + "github.com/gotenberg/gotenberg/v8/pkg/gotenberg/internal/log" ) // ParsedFlags wraps a [flag.FlagSet] so that retrieving the typed values is @@ -226,9 +232,14 @@ func (f *ParsedFlags) MustDeprecatedRegexp(deprecated string, newName string) *r // MustRegexpSlice returns a slice of compiled regular expressions from a // string-slice flag given by name. Empty strings are skipped. // It panics if an error occurs. +// +// Every allow-list and deny-list in Gotenberg is read through this method, so +// it is also where allow-list patterns are audited. See [AuditAllowList]. func (f *ParsedFlags) MustRegexpSlice(name string) []*regexp2.Regexp { vals := f.MustStringSlice(name) + f.warnRiskyAllowList(name, vals) + var regexps []*regexp2.Regexp for _, val := range vals { if val == "" { @@ -241,6 +252,114 @@ func (f *ParsedFlags) MustRegexpSlice(name string) []*regexp2.Regexp { return regexps } +// allowListFlagSuffix identifies the flags whose patterns grant an IP-check +// bypass. Deny-lists are never audited: they always apply, cannot be bypassed, +// and a loose deny-list is safe rather than dangerous. +const allowListFlagSuffix = "-allow-list" + +// warnRiskyAllowList logs one warning per allow-list entry that matches more +// URLs than its author is likely to intend. +// +// It warns and never fails: operators depend on loose patterns today, and +// rejecting them at startup would break running deployments. +func (f *ParsedFlags) warnRiskyAllowList(name string, vals []string) { + if !strings.HasSuffix(name, allowListFlagSuffix) { + return + } + + findings := AuditAllowList(vals) + if len(findings) == 0 { + return + } + + // The logger is nil until the entry point initializes it, which happens + // before any module is provisioned. Tests and embedders that call this + // method directly get no logger, and must not panic for it. + logger := log.Logger() + if logger == nil { + return + } + + for _, finding := range findings { + // Provision has no context.Context to propagate, so the trace-aware + // logging convention is satisfied with a background context. + logger.WarnContext( + context.Background(), + f.allowListWarning(name, finding), + slog.String("flag", "--"+name), + slog.String("env", EnvVarName(name)), + slog.Int("entry", finding.Index+1), + slog.String("reason", string(finding.Risk)), + ) + } +} + +// allowListWarning builds the operator-facing message for a finding. It names +// the flag and its environment variable, and, when they exist, the IP-check +// flags the entry silently disables. +func (f *ParsedFlags) allowListWarning(name string, finding AllowListFinding) string { + var b strings.Builder + + // Print the pattern raw rather than quoted: %q escapes every backslash, so + // the operator would not recognize the value they set. + fmt.Fprintf(&b, "--%s (%s) entry %d '%s' ", name, EnvVarName(name), finding.Index+1, finding.Pattern) + + switch finding.Risk { + case AllowListRiskUnanchored: + b.WriteString("is not anchored with ^, so it matches anywhere in the URL and a URL such as http://attacker.example/?u=trusted.example.com passes. ") + case AllowListRiskUnanchoredBranch: + b.WriteString("has an alternation branch that is not anchored with ^, and that branch matches anywhere in the URL. ") + case AllowListRiskCatchAll: + b.WriteString("matches every URL. ") + case AllowListRiskOpenHost: + b.WriteString("does not terminate the host, so it also matches suffix hosts such as http://trusted.example.com.attacker.example/. ") + } + + b.WriteString(f.bypassSentence(name)) + + switch finding.Risk { + case AllowListRiskUnanchored, AllowListRiskUnanchoredBranch: + b.WriteString("Anchor every branch with ^ and end the host with /, :, or $.") + case AllowListRiskCatchAll: + b.WriteString("Restrict the entry to the hosts you trust, or unset the flag.") + case AllowListRiskOpenHost: + b.WriteString("End the host with /, :, $, or a group such as (:|/|$).") + } + + return b.String() +} + +// bypassSentence names the IP-check flags an allow-list match skips, when the +// module registers them. +func (f *ParsedFlags) bypassSentence(name string) string { + prefix := strings.TrimSuffix(name, allowListFlagSuffix) + + private, public := prefix+"-deny-private-ips", prefix+"-deny-public-ips" + if f.Lookup(private) == nil || f.Lookup(public) == nil { + // A deprecated alias such as webhook-error-allow-list carries an extra + // segment that the IP-check flags do not have. + if i := strings.LastIndex(prefix, "-"); i != -1 { + private, public = prefix[:i]+"-deny-private-ips", prefix[:i]+"-deny-public-ips" + } + } + + if f.Lookup(private) == nil || f.Lookup(public) == nil { + return "A URL that matches the allow-list skips the private and public IP checks. " + } + + return fmt.Sprintf( + "A URL that matches the allow-list skips --%s (%s) and --%s (%s). ", + private, EnvVarName(private), public, EnvVarName(public), + ) +} + +// EnvVarName returns the environment variable that overrides the flag given by +// name. The entry point derives the same name when it applies environment +// overrides, so operator-facing messages can name both without drifting. +func EnvVarName(name string) string { + return strings.ToUpper(strings.ReplaceAll(name, "-", "_")) +} + // MustDeprecatedRegexpSlice returns the slice of compiled regular expressions // of a deprecated flag if it was explicitly set or the slice of the new flag. // It panics if an error occurs. diff --git a/pkg/gotenberg/flags_test.go b/pkg/gotenberg/flags_test.go index 887d75ef..4ffb64a9 100644 --- a/pkg/gotenberg/flags_test.go +++ b/pkg/gotenberg/flags_test.go @@ -3,6 +3,7 @@ package gotenberg import ( "reflect" "regexp" + "strings" "testing" "time" @@ -951,3 +952,105 @@ func TestParsedFlags_MustDeprecatedRegexpSlice(t *testing.T) { _ = regexp2.None // Keep import alive. } + +func TestParsedFlags_AllowListWarning(t *testing.T) { + fs := flag.NewFlagSet("tests", flag.ContinueOnError) + fs.StringSlice("chromium-allow-list", []string{}, "") + fs.Bool("chromium-deny-private-ips", false, "") + fs.Bool("chromium-deny-public-ips", false, "") + fs.StringSlice("standalone-allow-list", []string{}, "") + + parsedFlags := ParsedFlags{FlagSet: fs} + + for _, tc := range []struct { + scenario string + name string + finding AllowListFinding + contains []string + }{ + { + scenario: "open host names both IP-check flags and their env vars", + name: "chromium-allow-list", + finding: AllowListFinding{Index: 0, Pattern: `^https://trusted\.example\.com`, Risk: AllowListRiskOpenHost}, + contains: []string{ + "--chromium-allow-list (CHROMIUM_ALLOW_LIST)", + "entry 1", + `^https://trusted\.example\.com`, + "does not terminate the host", + "--chromium-deny-private-ips (CHROMIUM_DENY_PRIVATE_IPS)", + "--chromium-deny-public-ips (CHROMIUM_DENY_PUBLIC_IPS)", + "End the host with", + }, + }, + { + scenario: "catch-all tells the operator to restrict or unset", + name: "chromium-allow-list", + finding: AllowListFinding{Index: 2, Pattern: ".+", Risk: AllowListRiskCatchAll}, + contains: []string{"entry 3", "matches every URL", "Restrict the entry"}, + }, + { + scenario: "unanchored explains the search semantics", + name: "chromium-allow-list", + finding: AllowListFinding{Index: 0, Pattern: `trusted\.example\.com`, Risk: AllowListRiskUnanchored}, + contains: []string{"is not anchored with ^", "Anchor every branch with ^"}, + }, + { + scenario: "module without IP-check flags falls back to a generic sentence", + name: "standalone-allow-list", + finding: AllowListFinding{Index: 0, Pattern: `^https://a\.example\.com`, Risk: AllowListRiskOpenHost}, + contains: []string{"skips the private and public IP checks"}, + }, + } { + t.Run(tc.scenario, func(t *testing.T) { + msg := parsedFlags.allowListWarning(tc.name, tc.finding) + + for _, want := range tc.contains { + if !strings.Contains(msg, want) { + t.Fatalf("message %q does not contain %q", msg, want) + } + } + if strings.Contains(msg, "—") { + t.Fatalf("message must not contain an em dash: %q", msg) + } + }) + } +} + +func TestParsedFlags_WarnRiskyAllowList_SkipsDenyLists(t *testing.T) { + fs := flag.NewFlagSet("tests", flag.ContinueOnError) + fs.StringSlice("chromium-deny-list", []string{}, "") + + parsedFlags := ParsedFlags{FlagSet: fs} + + // A deny-list is never audited: it always applies and cannot be bypassed, + // so a loose one is safe. This must also not panic on a nil logger. + parsedFlags.warnRiskyAllowList("chromium-deny-list", []string{".+", `^file:(?!//\/tmp/).*`}) +} + +func TestParsedFlags_WarnRiskyAllowList_NilLoggerDoesNotPanic(t *testing.T) { + fs := flag.NewFlagSet("tests", flag.ContinueOnError) + fs.StringSlice("chromium-allow-list", []string{}, "") + + parsedFlags := ParsedFlags{FlagSet: fs} + + // Provision runs after the entry point initializes the logger, but tests + // and embedders reach this path with no logger at all. + parsedFlags.warnRiskyAllowList("chromium-allow-list", []string{".+"}) +} + +func TestEnvVarName(t *testing.T) { + for _, tc := range []struct { + name string + want string + }{ + {"chromium-allow-list", "CHROMIUM_ALLOW_LIST"}, + {"api-download-from-deny-private-ips", "API_DOWNLOAD_FROM_DENY_PRIVATE_IPS"}, + {"log-level", "LOG_LEVEL"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := EnvVarName(tc.name); got != tc.want { + t.Fatalf("EnvVarName(%q) = %q, want %q", tc.name, got, tc.want) + } + }) + } +}