EDM-2796: Redirect url is not calculated correctly#601
Merged
rawagner merged 1 commit intoflightctl:mainfrom Mar 31, 2026
Merged
EDM-2796: Redirect url is not calculated correctly#601rawagner merged 1 commit intoflightctl:mainfrom
rawagner merged 1 commit intoflightctl:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFrontend sends the UI origin as a URL-encoded redirect_base on login/logout requests. Backend resolves, validates, and persists explicit redirect URIs per OAuth state, constructs per-request OAuth clients using those redirect URIs, and adds forwarded-header trust controls for origin validation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Frontend
participant AuthGateway as "Auth Gateway"
participant Provider
participant CookieStore as "Cookie Store"
Client->>Frontend: Click "Login"
Frontend->>Frontend: compute redirect_base = origin
Frontend->>AuthGateway: GET /login?provider=...&redirect_base=...
AuthGateway->>AuthGateway: ResolveOAuthRedirectURI(redirect_base, request)
AuthGateway->>CookieStore: set oauth_redirect_uri_<state> = resolvedRedirectURI
AuthGateway->>Provider: GetLoginRedirectURL(state, codeChallenge, redirectURI)
Provider->>Provider: construct per-request OAuth client with redirectURI
Provider-->>AuthGateway: authorization URL
AuthGateway-->>Client: redirect to authorization URL
Client->>Provider: authorize (user)
Provider-->>Client: Redirect to callback with code
Client->>AuthGateway: POST /callback?code=...&state=...
AuthGateway->>CookieStore: get oauth_redirect_uri_<state>
AuthGateway->>Provider: Exchange code for token using created client
AuthGateway->>CookieStore: clear oauth_redirect_uri_<state>
AuthGateway-->>Client: Redirect to resolved callback URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
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.
🧹 Nitpick comments (5)
proxy/auth/openshift.go (1)
131-136: Consider logging the error when client creation fails.When
openshiftClientForRedirectreturns an error,GetLoginRedirectURLsilently returns an empty string. While the caller inauth.gohandles empty URLs by returning HTTP 500, adding a log statement here would aid debugging.🔧 Suggested improvement
func (o *OpenShiftAuthHandler) GetLoginRedirectURL(state string, codeChallenge string, redirectURI string) string { client, err := o.openshiftClientForRedirect(redirectURI) if err != nil { + log.GetLogger().WithError(err).Warn("Failed to create OpenShift OAuth client for redirect") return "" } return loginRedirect(client, state, codeChallenge) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/auth/openshift.go` around lines 131 - 136, GetLoginRedirectURL currently swallows the error from openshiftClientForRedirect and returns an empty string; update GetLoginRedirectURL to log the error before returning by calling your handler's logger (e.g. o.logger.Errorf or o.logger.Error with the error) or fallback to log.Printf if no logger exists, including contextual text and the error value, then return ""; ensure you still call loginRedirect(client, state, codeChallenge) only on success.proxy/auth/aap.go (1)
140-145: Consider logging client creation failures for debugging.Similar to the OpenShift handler, when
getAAPClientreturns an error, the method silently returns an empty string. Adding a log statement would help diagnose issues.🔧 Suggested improvement
func (a *AAPAuthHandler) GetLoginRedirectURL(state string, codeChallenge string, redirectURI string) string { client, err := getAAPClient(a.authURL, a.tokenURL, a.tlsConfig, a.clientId, redirectURI) if err != nil { + log.GetLogger().WithError(err).Warn("Failed to create AAP OAuth client for redirect") return "" } return loginRedirect(client, state, codeChallenge) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/auth/aap.go` around lines 140 - 145, GetLoginRedirectURL currently swallows errors from getAAPClient and returns an empty string; update it to log the error for debugging before returning. In the GetLoginRedirectURL method (receiver AAPAuthHandler) capture the err from getAAPClient and call the handler's logger (e.g., a.logger or a.log—use the existing logger field on AAPAuthHandler; if none exists, use the package logger) to emit a descriptive message that includes context ("failed to create AAP client for login redirect") and the err value, then return the empty string as before; leave the loginRedirect call unchanged.proxy/auth/oauth2.go (1)
86-91: Consider logging client creation failures for debugging.For consistency with other handlers and to aid debugging, consider logging when
oauth2ClientForRedirectreturns an error.🔧 Suggested improvement
func (o *OAuth2AuthHandler) GetLoginRedirectURL(state string, codeChallenge string, redirectURI string) string { client, err := o.oauth2ClientForRedirect(redirectURI) if err != nil { + // Consider importing log package and adding: + // log.GetLogger().WithError(err).Warn("Failed to create OAuth2 client for redirect") return "" } return loginRedirect(client, state, codeChallenge) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/auth/oauth2.go` around lines 86 - 91, GetLoginRedirectURL silently drops errors from oauth2ClientForRedirect; update OAuth2AuthHandler.GetLoginRedirectURL to log the returned error (including context like redirectURI and state) when oauth2ClientForRedirect fails before returning an empty string so failures are visible for debugging, using the handler's existing logger (e.g., o.logger / o.log) and including the error value in the log message.proxy/auth/redirect.go (2)
94-104: Silent empty-string return when cookie is missing.Returning an empty string when
http.ErrNoCookieoccurs may make it difficult for callers to distinguish "no cookie" from "empty redirect URI value." If this distinction matters upstream, consider returning a sentinel error or a boolean flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/auth/redirect.go` around lines 94 - 104, The function getOAuthRedirectURICookie currently returns an empty string when the cookie is missing, which conflates "no cookie" with an empty cookie value; change it to return a clear sentinel (e.g., a typed error like ErrNoOAuthRedirectCookie) or an additional boolean to indicate presence so callers can distinguish the two cases. Update getOAuthRedirectURICookie (and its callers) to use oauthRedirectURICookiePrefix + state (cookieName) as now, but on http.ErrNoCookie return the sentinel error (or value,false) instead of "", nil; leave other error paths unchanged. Ensure the new sentinel (ErrNoOAuthRedirectCookie) is declared near the top of the package or adjust call sites to handle the new (value, ok) boolean return.
80-92: Consider validatingstatebefore using it in cookie name.The
stateparameter is concatenated directly into the cookie name. While OAuth state values are typically alphanumeric, malicious or malformed state values could potentially cause issues with cookie parsing.🛡️ Optional: Add state validation
func setOAuthRedirectURICookie(w http.ResponseWriter, state, redirectURI string) { + // OAuth state should be alphanumeric; reject unexpected characters + for _, c := range state { + if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-' || c == '_') { + return // silently ignore invalid state + } + } cookieName := oauthRedirectURICookiePrefix + state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/auth/redirect.go` around lines 80 - 92, The cookie name is built by concatenating oauthRedirectURICookiePrefix with the raw state in setOAuthRedirectURICookie, which can produce invalid or unsafe cookie names; before using state in the cookie name validate/sanitize it (e.g., allow only a-zA-Z0-9-_ and reject/normalize others) or replace it with a deterministic safe token such as a hex or base64 (URL-safe) hash of the state; ensure the resulting name respects cookie length limits and update references that read this cookie to decode/compare using the same transformation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@proxy/auth/aap.go`:
- Around line 140-145: GetLoginRedirectURL currently swallows errors from
getAAPClient and returns an empty string; update it to log the error for
debugging before returning. In the GetLoginRedirectURL method (receiver
AAPAuthHandler) capture the err from getAAPClient and call the handler's logger
(e.g., a.logger or a.log—use the existing logger field on AAPAuthHandler; if
none exists, use the package logger) to emit a descriptive message that includes
context ("failed to create AAP client for login redirect") and the err value,
then return the empty string as before; leave the loginRedirect call unchanged.
In `@proxy/auth/oauth2.go`:
- Around line 86-91: GetLoginRedirectURL silently drops errors from
oauth2ClientForRedirect; update OAuth2AuthHandler.GetLoginRedirectURL to log the
returned error (including context like redirectURI and state) when
oauth2ClientForRedirect fails before returning an empty string so failures are
visible for debugging, using the handler's existing logger (e.g., o.logger /
o.log) and including the error value in the log message.
In `@proxy/auth/openshift.go`:
- Around line 131-136: GetLoginRedirectURL currently swallows the error from
openshiftClientForRedirect and returns an empty string; update
GetLoginRedirectURL to log the error before returning by calling your handler's
logger (e.g. o.logger.Errorf or o.logger.Error with the error) or fallback to
log.Printf if no logger exists, including contextual text and the error value,
then return ""; ensure you still call loginRedirect(client, state,
codeChallenge) only on success.
In `@proxy/auth/redirect.go`:
- Around line 94-104: The function getOAuthRedirectURICookie currently returns
an empty string when the cookie is missing, which conflates "no cookie" with an
empty cookie value; change it to return a clear sentinel (e.g., a typed error
like ErrNoOAuthRedirectCookie) or an additional boolean to indicate presence so
callers can distinguish the two cases. Update getOAuthRedirectURICookie (and its
callers) to use oauthRedirectURICookiePrefix + state (cookieName) as now, but on
http.ErrNoCookie return the sentinel error (or value,false) instead of "", nil;
leave other error paths unchanged. Ensure the new sentinel
(ErrNoOAuthRedirectCookie) is declared near the top of the package or adjust
call sites to handle the new (value, ok) boolean return.
- Around line 80-92: The cookie name is built by concatenating
oauthRedirectURICookiePrefix with the raw state in setOAuthRedirectURICookie,
which can produce invalid or unsafe cookie names; before using state in the
cookie name validate/sanitize it (e.g., allow only a-zA-Z0-9-_ and
reject/normalize others) or replace it with a deterministic safe token such as a
hex or base64 (URL-safe) hash of the state; ensure the resulting name respects
cookie length limits and update references that read this cookie to
decode/compare using the same transformation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60724d77-c531-452b-a5d0-bddf04b8491b
📒 Files selected for processing (10)
apps/standalone/src/app/components/Login/LoginPage.tsxapps/standalone/src/app/utils/apiCalls.tsproxy/auth/aap.goproxy/auth/auth.goproxy/auth/common.goproxy/auth/k8s.goproxy/auth/oauth2.goproxy/auth/oidc.goproxy/auth/openshift.goproxy/auth/redirect.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/auth/common.go`:
- Around line 46-47: Update the GetLoginRedirectURL signature to return (string,
error) in the auth interface and adjust its implementations: change
GetLoginRedirectURL(state, codeChallenge, redirectURI string) string to
GetLoginRedirectURL(state, codeChallenge, redirectURI string) (string, error) in
auth/common.go, then modify the implementations in proxy/auth/openshift.go and
proxy/auth/oidc.go to return the actual error from client construction or
resolution (instead of collapsing to ""), use ResolveOAuthRedirectURI as needed
and propagate any errors up, and update all call sites to handle the returned
error instead of relying on empty-string checks.
In `@proxy/auth/redirect.go`:
- Around line 15-18: The current ResolveOAuthRedirectURI (and
ResolveLogoutRedirectBase) logic validates a provided redirect_base as a bare
origin but then hardcodes origin + "/callback" (or discards configured UI path),
which drops any path component from config.BaseUiUrl; instead, parse
config.BaseUiUrl to extract its path and preserve it when building the final
callback/logout URL: validate that the origin portion of either redirect_base or
config.BaseUiUrl matches the incoming request (Host / X-Forwarded-*), then join
that origin with the preserved path from config.BaseUiUrl (e.g.,
"<origin><basePath>/callback" or "<origin><basePath>/logout") rather than using
just "/callback"; apply the same change to ResolveLogoutRedirectBase so both
redirects keep the UI base path while still enforcing origin validation.
- Around line 55-67: requestSchemeAndHost (and redirectBaseMatchesRequest)
currently treats X-Forwarded-Proto/Host as authoritative; change it so these
headers are only used when the request is known to originate from a trusted
proxy (or when a trusted-proxy normalization step has already run in
proxy/bridge/handler.go). Specifically: add a check in requestSchemeAndHost that
verifies the request is from a trusted source (e.g., known RemoteAddr/CIDR or a
flag/header explicitly set by proxy/bridge/handler.go) before reading
X-Forwarded-Proto/X-Forwarded-Host; otherwise fall back to r.TLS and r.Host.
Alternatively, ensure proxy/bridge/handler.go normalizes/overwrites those
headers and set a trusted marker that requestSchemeAndHost validates before
trusting forwarded values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7a98288-8fe7-46f5-8303-f660644b92b0
📒 Files selected for processing (10)
apps/standalone/src/app/components/Login/LoginPage.tsxapps/standalone/src/app/utils/apiCalls.tsproxy/auth/aap.goproxy/auth/auth.goproxy/auth/common.goproxy/auth/k8s.goproxy/auth/oauth2.goproxy/auth/oidc.goproxy/auth/openshift.goproxy/auth/redirect.go
✅ Files skipped from review due to trivial changes (1)
- proxy/auth/auth.go
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/standalone/src/app/components/Login/LoginPage.tsx
- apps/standalone/src/app/utils/apiCalls.ts
- proxy/auth/k8s.go
- proxy/auth/oidc.go
- proxy/auth/oauth2.go
- proxy/auth/aap.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
proxy/config/config_test.go (1)
9-19: Consider adding edge case tests for CIDR parsing.The tests cover basic cases but could benefit from testing:
- IPv6 addresses and CIDRs
- Invalid entries (to verify they're skipped)
- Empty string input
💡 Additional test cases
func TestParseTrustedProxyCIDRs_EdgeCases(t *testing.T) { t.Parallel() // Empty string nets := parseTrustedProxyCIDRs("") if len(nets) != 0 { t.Fatalf("expected 0 nets for empty string, got %d", len(nets)) } // Invalid entry is skipped nets = parseTrustedProxyCIDRs("invalid, 127.0.0.1") if len(nets) != 1 { t.Fatalf("expected 1 net (invalid skipped), got %d", len(nets)) } // IPv6 nets = parseTrustedProxyCIDRs("::1") if len(nets) != 1 || !nets[0].Contains(net.ParseIP("::1")) { t.Fatalf("expected single-host CIDR for ::1") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/config/config_test.go` around lines 9 - 19, Add a new table/combined test named TestParseTrustedProxyCIDRs_EdgeCases that calls parseTrustedProxyCIDRs to assert three edge behaviors: (1) an empty input string returns zero nets, (2) an input with an invalid token like "invalid, 127.0.0.1" skips the invalid entry and only returns the valid IPv4 net, and (3) an IPv6 input such as "::1" yields a single-host CIDR that Contains(net.ParseIP("::1")). Use t.Parallel() and the same assertion style as TestParseTrustedProxyCIDRs so the tests run consistently with the existing suite.proxy/auth/oidc.go (1)
88-99: Minor: Redundant variable assignment.Line 88
oidcForClient := oidcResponsecreates an unnecessary copy sinceoidcResponseis used directly. However, this may be intentional for clarity wheninternalAuthURLhandling modifies it below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/auth/oidc.go` around lines 88 - 99, The local variable oidcForClient is redundant: remove the line `oidcForClient := oidcResponse` and use oidcResponse directly when constructing the OIDCAuthHandler (set oidcDiscoveryForClient: oidcResponse); alternatively, if you intended to mutate a copy when handling internalAuthURL, keep the variable but document that purpose and perform the mutation on oidcForClient before passing it into the OIDCAuthHandler fields (e.g., oidcDiscoveryForClient). Ensure the change updates the OIDCAuthHandler construction (fields like oidcDiscoveryForClient, endSessionEndpoint, userInfoEndpoint, tokenEndpoint) accordingly.proxy/config/config.go (1)
56-82: Silent failure on invalid CIDR entries could mask configuration errors.When
net.ParseCIDRfails and the input isn't a valid IP either, the entry is silently skipped. Consider logging a warning so operators notice misconfiguredTRUSTED_PROXY_CIDRSentries.💡 Suggested improvement
func parseTrustedProxyCIDRs(s string) []*net.IPNet { var out []*net.IPNet for _, part := range strings.Split(s, ",") { part = strings.TrimSpace(part) if part == "" { continue } _, ipnet, err := net.ParseCIDR(part) if err != nil { // Single IP without mask: treat as /32 or /128 if ip := net.ParseIP(part); ip != nil { suffix := "/128" if ip.To4() != nil { suffix = "/32" } _, ipnet, err = net.ParseCIDR(ip.String() + suffix) if err != nil { continue } out = append(out, ipnet) - } + } else { + // Could add: log.Printf("warning: invalid TRUSTED_PROXY_CIDRS entry: %q", part) + } continue } out = append(out, ipnet) } return out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/config/config.go` around lines 56 - 82, In parseTrustedProxyCIDRs, invalid entries are silently skipped when net.ParseCIDR fails and net.ParseIP also returns nil; change this to emit a warning so operators see misconfigured TRUSTED_PROXY_CIDRS entries: after the second ParseCIDR attempt (the branch that checks ip := net.ParseIP(part)), if ip == nil (or the fallback ParseCIDR still errors) call the project’s logger (e.g., processLogger, logger, or fallback to log.Printf) to warn that the CIDR/IP string is invalid and being ignored, then continue; keep the rest of the parsing logic intact and include the offending part value in the warning message for easy debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/auth/redirect.go`:
- Around line 112-124: The Secure flag for the redirect-URI cookie is currently
determined only by config.TlsCertPath inside setOAuthRedirectURICookie; update
the API to accept the *http.Request (or call a helper that does) and compute the
effective client-facing scheme (use config.TlsCertPath, r.TLS, and trusted
X-Forwarded-Proto when config.ShouldTrustForwardedHeaders(r) is true) to decide
Secure. Modify setOAuthRedirectURICookie to take the *http.Request (or call
isSecureConnection(r)) and set cookie.Secure based on that result so cookies are
marked secure when the external connection is HTTPS even behind a
TLS-terminating proxy.
---
Nitpick comments:
In `@proxy/auth/oidc.go`:
- Around line 88-99: The local variable oidcForClient is redundant: remove the
line `oidcForClient := oidcResponse` and use oidcResponse directly when
constructing the OIDCAuthHandler (set oidcDiscoveryForClient: oidcResponse);
alternatively, if you intended to mutate a copy when handling internalAuthURL,
keep the variable but document that purpose and perform the mutation on
oidcForClient before passing it into the OIDCAuthHandler fields (e.g.,
oidcDiscoveryForClient). Ensure the change updates the OIDCAuthHandler
construction (fields like oidcDiscoveryForClient, endSessionEndpoint,
userInfoEndpoint, tokenEndpoint) accordingly.
In `@proxy/config/config_test.go`:
- Around line 9-19: Add a new table/combined test named
TestParseTrustedProxyCIDRs_EdgeCases that calls parseTrustedProxyCIDRs to assert
three edge behaviors: (1) an empty input string returns zero nets, (2) an input
with an invalid token like "invalid, 127.0.0.1" skips the invalid entry and only
returns the valid IPv4 net, and (3) an IPv6 input such as "::1" yields a
single-host CIDR that Contains(net.ParseIP("::1")). Use t.Parallel() and the
same assertion style as TestParseTrustedProxyCIDRs so the tests run consistently
with the existing suite.
In `@proxy/config/config.go`:
- Around line 56-82: In parseTrustedProxyCIDRs, invalid entries are silently
skipped when net.ParseCIDR fails and net.ParseIP also returns nil; change this
to emit a warning so operators see misconfigured TRUSTED_PROXY_CIDRS entries:
after the second ParseCIDR attempt (the branch that checks ip :=
net.ParseIP(part)), if ip == nil (or the fallback ParseCIDR still errors) call
the project’s logger (e.g., processLogger, logger, or fallback to log.Printf) to
warn that the CIDR/IP string is invalid and being ignored, then continue; keep
the rest of the parsing logic intact and include the offending part value in the
warning message for easy debugging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a042606-fa2a-494c-b8f4-b3f766adcbb4
📒 Files selected for processing (12)
apps/standalone/src/app/components/Login/LoginPage.tsxapps/standalone/src/app/utils/apiCalls.tsproxy/auth/aap.goproxy/auth/auth.goproxy/auth/common.goproxy/auth/k8s.goproxy/auth/oauth2.goproxy/auth/oidc.goproxy/auth/openshift.goproxy/auth/redirect.goproxy/config/config.goproxy/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/standalone/src/app/utils/apiCalls.ts
- apps/standalone/src/app/components/Login/LoginPage.tsx
- proxy/auth/k8s.go
- proxy/auth/aap.go
- proxy/auth/oauth2.go
- proxy/auth/auth.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
proxy/auth/redirect.go (1)
112-123:⚠️ Potential issue | 🟡 MinorDerive the redirect-URI cookie's
Secureflag from the effective request scheme.These helpers still key
Secureoffconfig.TlsCertPath, so proxied HTTPS deployments write this per-state cookie as non-Secure whenever TLS is terminated upstream. Please pass the request into these helpers and baseSecureonr.TLSplus trustedX-Forwarded-Proto.Also applies to: 138-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/auth/redirect.go` around lines 112 - 123, The setOAuthRedirectURICookie helper (and the companion cookie helper at lines 138-149) currently derives Secure from config.TlsCertPath; change their signatures to accept the *http.Request* (e.g., setOAuthRedirectURICookie(w http.ResponseWriter, r *http.Request, state, redirectURI string)) and compute Secure using the effective request scheme: treat the request as secure if r.TLS != nil or if a trusted X-Forwarded-Proto header equals "https" (reuse existing trusted-proxy logic if available), then set cookie.Secure based on that value before calling http.SetCookie; update all call sites to pass the request.
🧹 Nitpick comments (1)
proxy/auth/oauth2.go (1)
80-84: Consider using_for the unusedtokenparameter.The
tokenparameter is declared but never used in the function body. For consistency with the second parameter, consider using_to explicitly indicate it's unused (assuming this matches an interface signature).♻️ Suggested fix
-func (o *OAuth2AuthHandler) Logout(token string, _ string) (string, error) { +func (o *OAuth2AuthHandler) Logout(_ string, _ string) (string, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/auth/oauth2.go` around lines 80 - 84, The Logout method declares an unused parameter named token; update the function signature for OAuth2AuthHandler.Logout to explicitly mark it unused (e.g., change "token string" to "_ string") so it matches the existing unused second parameter and avoids unused-variable warnings while keeping the same behavior and interface compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/auth/redirect.go`:
- Around line 102-109: redirectBaseMatchesRequest currently compares raw
scheme://host strings so origins with implicit default ports (like
https://example.com vs https://example.com:443) mismatch; add a helper (e.g.,
normalizeOrigin) that lowercases/trims scheme and host, parses into a url.URL,
and strips the default port by returning scheme + "://" + Hostname() when
(scheme=="https" && port==443) or (scheme=="http" && port==80), otherwise return
scheme + "://" + Host; then use normalizeOrigin when building both candidate
(from u.Scheme/u.Host) and actual (from requestSchemeAndHost result) before
comparing in redirectBaseMatchesRequest.
---
Duplicate comments:
In `@proxy/auth/redirect.go`:
- Around line 112-123: The setOAuthRedirectURICookie helper (and the companion
cookie helper at lines 138-149) currently derives Secure from
config.TlsCertPath; change their signatures to accept the *http.Request* (e.g.,
setOAuthRedirectURICookie(w http.ResponseWriter, r *http.Request, state,
redirectURI string)) and compute Secure using the effective request scheme:
treat the request as secure if r.TLS != nil or if a trusted X-Forwarded-Proto
header equals "https" (reuse existing trusted-proxy logic if available), then
set cookie.Secure based on that value before calling http.SetCookie; update all
call sites to pass the request.
---
Nitpick comments:
In `@proxy/auth/oauth2.go`:
- Around line 80-84: The Logout method declares an unused parameter named token;
update the function signature for OAuth2AuthHandler.Logout to explicitly mark it
unused (e.g., change "token string" to "_ string") so it matches the existing
unused second parameter and avoids unused-variable warnings while keeping the
same behavior and interface compatibility.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b23c7356-63da-4522-9114-6693fc56e91b
📒 Files selected for processing (12)
apps/standalone/src/app/components/Login/LoginPage.tsxapps/standalone/src/app/utils/apiCalls.tsproxy/auth/aap.goproxy/auth/auth.goproxy/auth/common.goproxy/auth/k8s.goproxy/auth/oauth2.goproxy/auth/oidc.goproxy/auth/openshift.goproxy/auth/redirect.goproxy/config/config.goproxy/config/config_test.go
✅ Files skipped from review due to trivial changes (1)
- proxy/auth/openshift.go
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/standalone/src/app/components/Login/LoginPage.tsx
- proxy/auth/common.go
rawagner
reviewed
Mar 30, 2026
proxy/config/config.go
Outdated
Comment on lines
+27
to
+37
| TrustXForwardedHeaders = parseBoolEnv("TRUST_X_FORWARDED_HEADERS", false) | ||
| ) | ||
|
|
||
| // trustedProxyNets is parsed from TRUSTED_PROXY_CIDRS (comma-separated). When non-empty and | ||
| // TrustXForwardedHeaders is true, forwarded headers apply only when the immediate client IP | ||
| // (r.RemoteAddr) falls within one of these networks. When empty, all clients are accepted | ||
| // once TrustXForwardedHeaders is true (use only if the proxy is not reachable from untrusted clients). | ||
| var trustedProxyNets []*net.IPNet | ||
|
|
||
| func init() { | ||
| trustedProxyNets = parseTrustedProxyCIDRs(getEnvVar("TRUSTED_PROXY_CIDRS", "")) |
Collaborator
There was a problem hiding this comment.
we should also make possible to set these new env variables via helm chart/quadlet config.
We should also document these new variables in CONFIGURATION.md
Contributor
Author
There was a problem hiding this comment.
Setting via helm chart/quadlet config will be set in another PR on flightctl repo.
Added documentation
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONFIGURATION.md`:
- Around line 23-24: The doc entry for TRUST_X_FORWARDED_HEADERS /
TRUSTED_PROXY_CIDRS understates the security risk: update CONFIGURATION.md to
clearly state that when TRUST_X_FORWARDED_HEADERS=true and TRUSTED_PROXY_CIDRS
is empty the proxy (see proxy/config/config.go) will accept X-Forwarded-* from
any direct client (i.e. unrestricted trust), and also note that malformed CIDR
entries are ignored and may lead to an effectively-empty allowlist (resulting
either in fail-closed behavior or unrestricted trust depending on current
parsing), so advise enabling only behind a trusted load balancer and validating
CIDR syntax.
In `@proxy/auth/auth.go`:
- Around line 286-289: The current handler returns provider/client setup errors
(from provider.GetLoginRedirectURL and the similar branch at lines 333-336)
directly to unauthenticated callers; change these branches to log the full error
server-side (use the existing logger) and call respondWithError(w,
http.StatusInternalServerError, "provider initialization error") or another
generic message instead of exposing err.Error(); retain early returns after
responding and ensure the logged message includes the original err and context
(e.g., "provider.GetLoginRedirectURL failed" or "provider initialization
failed") to aid debugging.
In `@proxy/auth/oidc.go`:
- Around line 162-172: The Logout method in OIDCAuthHandler currently calls
url.Parse on o.endSessionEndpoint even when it's empty, producing a bogus query
string; update OIDCAuthHandler.Logout to short-circuit and return "" (nil error)
when o.endSessionEndpoint is empty or only whitespace before calling url.Parse,
so providers without an end-session endpoint fall back to UI-only logout and no
invalid URL is returned.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bb01b35-b498-4de6-92ea-fff0f64a44b6
📒 Files selected for processing (14)
CONFIGURATION.mdapps/standalone/src/app/components/Login/LoginPage.tsxapps/standalone/src/app/utils/apiCalls.tsproxy/auth/aap.goproxy/auth/auth.goproxy/auth/common.goproxy/auth/k8s.goproxy/auth/oauth2.goproxy/auth/oidc.goproxy/auth/openshift.goproxy/auth/redirect.goproxy/auth/redirect_test.goproxy/config/config.goproxy/config/config_test.go
✅ Files skipped from review due to trivial changes (1)
- apps/standalone/src/app/utils/apiCalls.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/standalone/src/app/components/Login/LoginPage.tsx
- proxy/auth/k8s.go
- proxy/auth/openshift.go
rawagner
approved these changes
Mar 31, 2026
rawagner
pushed a commit
to rawagner/flightctl-ui
that referenced
this pull request
Mar 31, 2026
rawagner
pushed a commit
that referenced
this pull request
Mar 31, 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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
New Features
Security
Documentation
Tests