fix(acls): auth bypass via forward auth - #1044
Conversation
dosubot
Bot
added
the
size:XS
📝 WalkthroughWalkthroughForward-auth now extracts parsed path components from forwarded and external-auth requests, then applies normalized prefix or delimited-regex ACL matching. Tests cover query-string mismatches, substring paths, root blocking, literal allows, and blocked-path configuration. ChangesForward-auth ACL validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Traefik
participant getForwardAuthContext
participant AuthEnabledRule
Traefik->>getForwardAuthContext: Send x-forwarded-uri
getForwardAuthContext->>getForwardAuthContext: Parse URI and extract Path
getForwardAuthContext->>AuthEnabledRule: Evaluate path ACL
AuthEnabledRule-->>Traefik: Return allow or deny
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/proxy_controller_test.go`:
- Around line 290-337: Update the ACL evaluator/matcher used for forwarded
requests to parse x-forwarded-uri and compare ACL path rules against the URL’s
exact path, excluding the query string and preventing substring matches.
Preserve allow matches only for the exact configured path and block only that
exact path so the four tests validate the intended behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8967dd13-0c82-4c6e-ba02-bd63804bfd00
📒 Files selected for processing (2)
internal/controller/proxy_controller_test.gointernal/test/test.go
There was a problem hiding this comment.
Pull request overview
This PR strengthens the forward-auth (Traefik) ACL regression suite to prevent authentication bypasses where path-based ACLs could be satisfied via forwarded URI query strings or unintended substring matches.
Changes:
- Adds a new
app_path_blocktest app configuration to exercise block-list path ACL behavior. - Adds forward-auth tests ensuring allow/block path ACLs don’t match via query-string injection or path substrings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/test/test.go | Adds a new test app (app_path_block) used by proxy/controller ACL tests. |
| internal/controller/proxy_controller_test.go | Adds forward-auth (Traefik) regression tests for query-string and substring path matching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4d35c25 to
be1fb1c
Compare
dosubot
Bot
added
size:M
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/access_controls_rules.go`:
- Around line 184-190: Update the configuredPath normalization in Evaluate to
preserve the root value "/" before trimming trailing slashes, ensuring a
path.block of "/" remains available to the root check and matches every request.
Keep empty non-root paths skipped while retaining the existing configuredPath
and path prefix matching behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96c36242-4ebb-452f-8583-175eb519eb34
📒 Files selected for processing (5)
internal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/service/access_controls_rules.gointernal/service/access_controls_rules_test.gointernal/test/test.go
💤 Files with no reviewable changes (1)
- internal/service/access_controls_rules_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/proxy_controller_test.go
- internal/test/test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
internal/service/access_controls_rules_test.go:533
TestAuthEnabledRuleno longer validates any of the path allow/block behaviors (match, non-match, allow override for blocked subpaths, etc.). Since the PR changes path matching semantics, this leaves the core ACL rule effectively untested at the service layer.
{
name: "denies when no path rules are configured",
ctx: &ACLContext{
ACLs: &model.App{},
Path: "/anything",
internal/controller/proxy_controller_test.go:340
- There’s no test asserting that a matching blocked path (e.g.
/blocked) is actually denied for the forward-auth flow; current additions only verify non-matches. A positive block assertion would catch regressions where block rules become no-ops.
},
{
description: "Ensure path allow ACL works on nginx auth request",
middlewares: []gin.HandlerFunc{},
be1fb1c to
3d5ab54
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
internal/service/access_controls_rules_test.go:536
- This test currently asserts that allow path "/public" matches request path "/publicity", which is a substring match (not a subpath match). If allow rules are meant to match exact paths and prefix subpaths (e.g. "/public" and "/public/..."), update this test case to use a real subpath like "/public/index" so it doesn't codify the substring behavior.
ACLs: &model.App{
Path: model.AppPath{Allow: "/public"},
},
Path: "/publicity",
},
internal/model/config.go:336
- The updated field descriptions don’t explain the exact matching semantics (exact path vs subpath) or how to write a regex unambiguously. Given the implementation treats any value that starts and ends with "/" as a regex, it would help to document that prefix matches are segment-aware ("/public" matches "/public" and "/public/...", not "/publicity") and that regexes must be wrapped with leading/trailing slashes (e.g. "/^/public-[0-9]+$/").
type AppPath struct {
Allow string `description:"Comma-separated list of allowed path prefixes or slash-delimited regular expressions." yaml:"allow,omitempty"`
Block string `description:"Comma-separated list of blocked path prefixes or slash-delimited regular expressions." yaml:"block,omitempty"`
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/access_controls_rules.go`:
- Around line 196-203: Update the path-matching logic around the regex
compilation branch in the access-control rule evaluator so trailing-slash
entries such as "/admin/" remain literal prefix rules rather than being
interpreted as regexes. Introduce and use a non-overlapping regex syntax for
explicitly marked expressions, while preserving regex anchors and adding
allow/block regression coverage for ambiguous literal prefixes and anchored
patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53485d81-e9e2-4b03-a32f-e6f669f400e3
📒 Files selected for processing (4)
internal/controller/proxy_controller.gointernal/model/config.gointernal/service/access_controls_rules.gointernal/service/access_controls_rules_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/proxy_controller.go
3a377ad to
cce0eaa
Compare
| func matchPathRule(paths, path string) (bool, error) { | ||
| for _, configuredPath := range strings.Split(paths, ",") { | ||
| configuredPath = strings.TrimSpace(configuredPath) | ||
|
|
||
| if configuredPath == "/" { | ||
| return true, nil | ||
| } | ||
|
|
||
| if configuredPath == "" { | ||
| continue | ||
| } | ||
|
|
||
| // only apply regex if the path starts and ends with a slash, e.g. /regex/ | ||
| if strings.HasPrefix(configuredPath, "/") && strings.HasSuffix(configuredPath, "/") { | ||
| regex, err := regexp.Compile(configuredPath[1 : len(configuredPath)-1]) | ||
| if err != nil { | ||
| return false, fmt.Errorf("invalid path regex %q: %w", configuredPath, err) | ||
| } | ||
|
|
||
| if regex.MatchString(path) { | ||
| return true, nil | ||
| } | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| if strings.HasPrefix(path, configuredPath) { | ||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| return false, nil | ||
| } |
There was a problem hiding this comment.
@scottmckendry we are changing behavior here and possibly breaking things. The way ACLs for paths should work is the following:
Normal whitelist
In case we have something like:
tinyauth.apps.foo.path.allow: /bar,/foo/bar,/hello
On every request we loop through the rules and check if the request URI has any prefix that matches. For example, /bar and all sub-paths such as /bar/foo/bar will be allowed. This is the current intended behavior.
Regex whitelist
For regex whitelist there is no looping, the check for regex happens before the comma split and we just check if the rule starts and ends with slashes (/). For example this example is allowed:
tinyauth.apps.foo.path.allow: /\/foo|bar/
This is not allowed:
tinyauth.apps.foo.path.allow /\/foo|bar/,/\/test/
There is no need to allow such thing since your comma-seperated whitelist can be recreated using regex and users using regex will know what they are doing. Now, as for how regex is treated, it's a simple match (not a prefix match). It's the user's responsibility to know how to configure regex matching correctly, not ours.
Hopefully this clears things out a bit on how the parsing of the whitelist should work.
There was a problem hiding this comment.
thanks, this is quite helpful actually. I think I've covered all scenarios now.
| parsedURI, err := url.ParseRequestURI(uri) | ||
|
|
||
| if err != nil { | ||
| return ProxyContext{}, fmt.Errorf("invalid x-forwarded-uri: %w", err) | ||
| } | ||
|
|
||
| if parsedURI.Path == "" { | ||
| parsedURI.Path = "/" | ||
| } |
There was a problem hiding this comment.
We should do this for all auth module types and also strip out the query parameters immediately. Tinyauth won't need them in any part of the authentication chain anyway.
There was a problem hiding this comment.
good call, done :)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
internal/service/access_controls_rules.go:195
matchPathRuletreats/.../as a regex unconditionally. This makes a normal directory-style rule like/public/behave like the regexpublic, which will match unrelated paths such as/admin/publicand can unintentionally allow/deny requests. Consider only enabling regex mode when the inner expression actually contains regex metacharacters (or use an explicit regex prefix) so trailing-slash paths remain literal prefixes.
if strings.HasPrefix(paths, "/") && strings.HasSuffix(paths, "/") {
regex, err := regexp.Compile(paths[1 : len(paths)-1])
if err != nil {
return false, fmt.Errorf("invalid path regex %q: %w", paths, err)
}
internal/controller/proxy_controller.go:359
getRequestPathcurrently treats a parsed URI with an emptyPathas/. That means relative references like?q=1(no path component) will be accepted and treated as root, which can change ACL behavior for ForwardAuth/ExtAuthz. It’s safer to reject URIs that have no scheme/host and no path, and to trim surrounding whitespace before parsing.
if parsedURI.Path == "" {
return "/", nil
}
internal/service/access_controls_rules_test.go:537
- There’s no regression test ensuring comma-separated path rules with empty items (e.g.
/foo,) don’t match everything, and that a literal trailing-slash path like/public/is treated as a prefix (not as a regex delimiter). Adding cases for these would help prevent future ACL bypass regressions.
{
name: "allows when path starts with allow path",
ctx: &ACLContext{
ACLs: &model.App{
Path: model.AppPath{Allow: "/public"},
},
Path: "/publicity",
},
expected: EffectAllow,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/service/access_controls_rules.go:188
AuthEnabledRulepreviously treatedPath.Allow/Path.Blockvalues as raw regex patterns (e.g. "^/admin"), butmatchPathRulenow only treats patterns wrapped in "/.../" as regex. Any existing deployments using the old regex style will silently stop matching and could unintentionally allow access (e.g. a block rule "^/admin" becomes a literal prefix and won’t match any request). Consider preserving backward compatibility for single (non-comma-separated) patterns that look like regex (anchors/metacharacters), or explicitly rejecting them with a clear error so misconfigurations fail closed.
func matchPathRule(paths, path string) (bool, error) {
paths = strings.TrimRight(strings.TrimSpace(paths), ",")
if paths == "/" {
return true, nil
internal/service/access_controls_rules_test.go:577
- The new
matchPathRulebehavior includes explicit error handling for invalid "/.../" regex patterns, butTestAuthEnabledRuleno longer covers the “regex fails to compile” cases. Adding tests for invalid allow/block regex patterns would help ensure malformed patterns continue to fail closed (deny).
{
name: "allows when path matches allow regex",
ctx: &ACLContext{
ACLs: &model.App{
Path: model.AppPath{Allow: "/^/public-[0-9]+$/"},
},
Path: "/public-42",
},
expected: EffectAllow,
},
{
name: "denies when comma-separated allow paths do not match",
ctx: &ACLContext{
ACLs: &model.App{
Path: model.AppPath{Allow: "/bar,/foo/bar,/hello"},
},
Path: "/private",
},
expected: EffectDeny,
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/proxy_controller_test.go (1)
350-359: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the new parser error and default branches.
These tests cover valid query-bearing paths only. Add cases for malformed
x-forwarded-uri, missing/empty Envoypath, and encoded separators so the new 400, root-default, and canonicalization behavior is regression-tested.Also applies to: 370-380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/proxy_controller_test.go` around lines 350 - 359, Extend the proxy controller test cases around the existing path-allow ACL scenario to cover malformed x-forwarded-uri input, missing or empty Envoy path values, and encoded path separators. Assert the new 400 response for parser errors, root-default behavior for absent paths, and canonicalized handling for encoded separators, using the existing router, recorder, and request setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/proxy_controller.go`:
- Around line 348-358: Update getRequestPath to return the same
canonical/escaped path representation used by proxy routing, rather than
parsedURI.Path, while preserving the root-path fallback and parse-error
behavior. Audit the ACL path-rule evaluation to ensure all prefix comparisons
use this normalized representation consistently, then add coverage for %2F and
double-encoded paths and document the representation targeted by path rules.
---
Nitpick comments:
In `@internal/controller/proxy_controller_test.go`:
- Around line 350-359: Extend the proxy controller test cases around the
existing path-allow ACL scenario to cover malformed x-forwarded-uri input,
missing or empty Envoy path values, and encoded path separators. Assert the new
400 response for parser errors, root-default behavior for absent paths, and
canonicalized handling for encoded separators, using the existing router,
recorder, and request setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b25d96cc-4785-4575-aa43-e60d1e3d22c6
📒 Files selected for processing (4)
internal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/service/access_controls_rules.gointernal/service/access_controls_rules_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/access_controls_rules.go
- internal/service/access_controls_rules_test.go
| func getRequestPath(uri string) (string, error) { | ||
| parsedURI, err := url.ParseRequestURI(uri) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if parsedURI.Path == "" { | ||
| return "/", nil | ||
| } | ||
|
|
||
| return parsedURI.Path, nil |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat >/tmp/check_url.go <<'EOF'
package main
import (
"fmt"
"net/url"
)
func main() {
for _, raw := range []string{"", "/allowed%2Fprivate"} {
u, err := url.ParseRequestURI(raw)
if err != nil {
fmt.Printf("%q: error=%v\n", raw, err)
continue
}
fmt.Printf("%q: Path=%q EscapedPath=%q\n", raw, u.Path, u.EscapedPath())
}
}
EOF
go run /tmp/check_url.goRepository: tinyauthapp/tinyauth
Length of output: 267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline internal/controller/proxy_controller.go || true
echo "== getRequestPath and related proxy/auth refs =="
rg -n "getRequestPath|ParseRequestURI|EscapedPath|Path|ACL|acl|allowed|auth-request|Forward|forward|ext-authz|Authorization" internal/controller/proxy_controller.go internal -S
echo "== proxy_controller.go relevant section =="
sed -n '320,380p' internal/controller/proxy_controller.go
echo "== search for path matching/acl handling in tests/code =="
rg -n "get|path|Path|ACL|ACL|allowed|permission|authorize|rbac|acl" . -g '*.go' -g '*.yaml' -g '*.yml' -S | head -n 200Repository: tinyauthapp/tinyauth
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== proxy_controller.go relevant context =="
sed -n '96,290p' internal/controller/proxy_controller.go
sed -n '380,486p' internal/controller/proxy_controller.go
sed -n '536,548p' internal/controller/proxy_controller.go
echo "== path ACL evaluation implementations =="
ast-grep outline internal/service/access_controls.go internal/service/access_controls_rules.go || true
sed -n '1,220p' internal/service/access_controls.go
sed -n '220,360p' internal/service/access_controls.go
sed -n '1,160p' internal/service/access_controls_rules.go
sed -n '160,320p' internal/service/access_controls_rules.go
echo "== ACL path tests for escaped/same-path =="
rg -n "Escaped|escaped|Path" internal/service/access_controls* internal/controller/proxy_controller* -S | head -n 200
echo "== Go URL path encoding semantics probe =="
cat >/tmp/probe.go <<'EOF'
package main
import (
"fmt"
"net/url"
)
func main() {
for _, raw := range []string{
"/allowed%2Fprivate",
"/foo%252Fbar",
clean:="",
} {}
// No-op compile-time probe for current shell tooling availability.
}
EOF
go run >/tmp/url_probe_output.txt <<'EOF' >/dev/null 2>&1
package main
import ("fmt"; "net/url")
func main() {
for _, raw := range []string{"/allowed%2Fprivate", "/foo%252Fbar"} {
u, err := url.ParseRequestRaw(raw)
if err != nil { fmt.Printf("%q error=%v\n", raw, err); continue }
fmt.Printf("%q Path=%q EscapedPath:%q RawPath:%q\n", raw, u.Path, u.EscapedPath(), u.RawPath)
}
}
EOF
cat /tmp/url_probe_output.txt
echo "== exact test cases around ACL assertions =="
sed -n '260,400p' internal/controller/proxy_controller_test.goRepository: tinyauthapp/tinyauth
Length of output: 10002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== access_controls_rules.go path matcher =="
sed -n '170,230p' internal/service/access_controls_rules.go
rg -n "PathPrefix|Match|path rule|matchPathRule" internal/service internal/controller -S
echo "== exact ACL path assertion tests =="
sed -n '260,400p' internal/controller/proxy_controller_test.go
echo "== policy engine and all path matcher lines =="
sed -n '1,220p' internal/service/policy_engine.go
rg -n "PathPrefix\(|Match\(|access-controls|path" internal/service/access_controls_rules.go internal/service -SRepository: tinyauthapp/tinyauth
Length of output: 19446
Align ACL path evaluation with proxy canonicalization.
url.ParseRequestURI returns /allowed%2Fprivate as Path="/allowed/private" while EscapedPath() still has /allowed%2Fprivate; ACL rule types all compare the decoded proxyCtx.Path, so path allow/block prefixes can classify escaped routes differently from a proxy routing on the canonical/escaped form. Normalize ACL input consistently, cover %2F/double-encoding in tests, and document which representation the path rules target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/proxy_controller.go` around lines 348 - 358, Update
getRequestPath to return the same canonical/escaped path representation used by
proxy routing, rather than parsedURI.Path, while preserving the root-path
fallback and parse-error behavior. Audit the ACL path-rule evaluation to ensure
all prefix comparisons use this normalized representation consistently, then add
coverage for %2F and double-encoded paths and document the representation
targeted by path rules.
Summary by CodeRabbit