Uh oh!
There was an error while loading. Please reload this page.
refactor: refactor proxy controller to handle proxy auth modules better - #714
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a ProxyContext type and AuthModuleType enum; centralizes auth-module-driven request context construction (ForwardAuth, ExtAuthz, AuthRequest); refactors error handling and routing to operate on the consolidated ProxyContext instead of ad-hoc request-specific extraction. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant ProxyCtrl as ProxyController
participant AuthMod as Auth Module
participant App as Protected App
Client->>ProxyCtrl: HTTP request
ProxyCtrl->>ProxyCtrl: determineAuthModules() / getProxyContext()
ProxyCtrl->>AuthMod: getContextFromAuthModule()
AuthMod-->>ProxyCtrl: ProxyContext (Host, Proto, Path, Method, Type, IsBrowser)
ProxyCtrl->>ProxyCtrl: useFriendlyError(proxyCtx)?
alt Auth passes
ProxyCtrl->>App: route/proxy request
App-->>Client: response
else Auth fails
ProxyCtrl->>ProxyCtrl: handleError(proxyCtx)
ProxyCtrl-->>Client: error/redirect
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
steveiliop56
marked this pull request as ready for review
March 14, 2026 11:27
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/controller/proxy_controller_test.go (1)
101-302: Add a browser redirect regression case.All of these requests omit
User-Agent, so the newuseFriendlyErrorbranch never executes. OneForwardAuth/ExtAuthzcase that asserts the 307/loginor/unauthorizedresponse would cover the main new behavior here, and it can double as a regression test for preservingx-original-urlquery strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/proxy_controller_test.go` around lines 101 - 302, Add a regression test in TestProxyHandler that exercises the new useFriendlyError branch by sending a logged-out browser-like request (set a common User-Agent header, e.g. "Mozilla/5.0") to one of the forward auth/ext_authz endpoints (use the existing "/api/auth/traefik" or "/api/auth/envoy" case), include the same upstream URL header(s) used in other tests (x-forwarded-host/x-forwarded-proto/x-forwarded-uri or Host + x-forwarded-proto) and assert the handler responds with a 307 redirect and that the Location header points to the friendly login/unauthorized path ("/login" or "/unauthorized") and preserves the original upstream URL as a query string (e.g., x-original-url or return_to). Place this new case near the logged-out tests so it runs with the same setup used by setupProxyController and refers to TestProxyHandler, setupProxyController, and the specific route "/api/auth/traefik" or "/api/auth/envoy".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/proxy_controller.go`:
- Around line 457-459: The Trace log is currently dumping c.Request.Header via
tlog.App.Trace().Interface("headers", c.Request.Header), which exposes sensitive
values; replace this with a sanitized headers map by either building an
allowlist of safe header names (e.g.
"Content-Type","User-Agent","Accept","Host","X-Request-ID") and only logging
those, or by copying c.Request.Header and redacting sensitive keys
("Authorization","Cookie","Set-Cookie","X-Forwarded-For","X-Auth-*" etc.) before
passing the map to tlog.App.Trace(); update the call site that invokes
tlog.App.Trace().Interface("headers", ...) in proxy_controller.go to use the
sanitized map instead.
- Around line 412-421: determineAuthModules currently defaults unknown proxy
names to ForwardAuth which masks typos; change determineAuthModules on
ProxyController to return an error for unsupported proxy names (e.g., return
([]AuthModuleType, error)) instead of a default slice, update the caller in
proxyHandler to check that error and respond with 400 when unsupported, and
apply the same change to the other proxy-name switch in this file (the other
function handling proxy-based decisions) so all unknown proxy names are rejected
rather than mapped to ForwardAuth.
- Around line 362-379: The parsed xOriginalUrl must be validated as an absolute
URL and its query string preserved: after calling url.Parse(xOriginalUrl) (in
getAuthRequestContext), check that parsed.Scheme and parsed.Host are non-empty
and return an error if they are missing (so relative URLs don't produce an empty
Host/Proto), and include parsed.RawQuery when constructing the ProxyContext.Path
or when later building RedirectURI so query parameters are preserved; update the
code that returns ProxyContext (fields Host, Proto, Path, Method,
Type=AuthRequest) to use parsed.Path + "?" + parsed.RawQuery (when RawQuery is
non-empty) or otherwise keep parsed.Path.
---
Nitpick comments:
In `@internal/controller/proxy_controller_test.go`:
- Around line 101-302: Add a regression test in TestProxyHandler that exercises
the new useFriendlyError branch by sending a logged-out browser-like request
(set a common User-Agent header, e.g. "Mozilla/5.0") to one of the forward
auth/ext_authz endpoints (use the existing "/api/auth/traefik" or
"/api/auth/envoy" case), include the same upstream URL header(s) used in other
tests (x-forwarded-host/x-forwarded-proto/x-forwarded-uri or Host +
x-forwarded-proto) and assert the handler responds with a 307 redirect and that
the Location header points to the friendly login/unauthorized path ("/login" or
"/unauthorized") and preserves the original upstream URL as a query string
(e.g., x-original-url or return_to). Place this new case near the logged-out
tests so it runs with the same setup used by setupProxyController and refers to
TestProxyHandler, setupProxyController, and the specific route
"/api/auth/traefik" or "/api/auth/envoy".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c65afbf0-c572-4518-ae08-b5688f78c753
📒 Files selected for processing (2)
internal/controller/proxy_controller.gointernal/controller/proxy_controller_test.go
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #714 +/- ##
==========================================
+ Coverage 15.20% 16.85% +1.65%
==========================================
Files 50 50 Lines 3748 3820 +72 ==========================================
+ Hits 570 644 +74 + Misses 3117 3112 -5 - Partials 61 64 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/proxy_controller.go (1)
132-134: Extract resource-name derivation into one helper.
strings.Split(proxyCtx.Host, ".")[0]is repeated in multiple branches. Centralizing this avoids drift and makes it easier to normalize host formats (e.g., host with port) in one place.Also applies to: 166-167, 177-178, 206-207, 217-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/proxy_controller.go` around lines 132 - 134, Repeated extraction of the resource name using strings.Split(proxyCtx.Host, ".")[0] should be centralized: add a helper function (e.g., deriveResourceFromHost(host string) string) that normalizes the host (trim whitespace, strip port via net.SplitHostPort or strings.Split on ':' when needed, handle empty host) and returns the left-most label; then replace all inline occurrences in proxy_controller.go (places building UnauthorizedQuery and any other branches referencing proxyCtx.Host) with calls to deriveResourceFromHost(proxyCtx.Host) so normalization and future changes live in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/proxy_controller.go`:
- Around line 353-388: getAuthRequestContext currently errors when
x-original-url is a path-only value; change it to accept path-only auth_request
flows by treating the header as a path if url.Parse yields no scheme/host: keep
extracting Path from the parsed URL (or use the raw header if parsing fails to
produce a Path), and avoid returning errors for empty host/proto for
AuthRequest; instead populate Host from c.Request.Host (or the Host header) and
Proto from X-Forwarded-Proto or c.Request.TLS (default "http"/"https") before
returning the ProxyContext (note function getAuthRequestContext and struct
fields Host, Proto, Path, Type (AuthRequest)).
---
Nitpick comments:
In `@internal/controller/proxy_controller.go`:
- Around line 132-134: Repeated extraction of the resource name using
strings.Split(proxyCtx.Host, ".")[0] should be centralized: add a helper
function (e.g., deriveResourceFromHost(host string) string) that normalizes the
host (trim whitespace, strip port via net.SplitHostPort or strings.Split on ':'
when needed, handle empty host) and returns the left-most label; then replace
all inline occurrences in proxy_controller.go (places building UnauthorizedQuery
and any other branches referencing proxyCtx.Host) with calls to
deriveResourceFromHost(proxyCtx.Host) so normalization and future changes live
in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67a7107d-c707-445e-b970-52811754e7ef
📒 Files selected for processing (1)
internal/controller/proxy_controller.go
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Merged
1 task
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Tests