From cf9940dd92c53d6a8cdd38b185f1832886c95ecb Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 3 Aug 2026 02:24:52 +0200 Subject: [PATCH] fix(controlplane): validate the OIDC login callback URL (CP-N2) The `callback` query param of /auth/login was stored and later redirected to verbatim, so a crafted login link ended the OIDC dance at an attacker origin carrying the user JWT in the query string. The callback is now restricted to relative paths, loopback (CLI login) and the origins the deployment already declares: the control plane external URL, the login URL and ui_dashboard_url. The token redirect also opts out of caching and referrer leakage. Signed-off-by: Miguel Martinez Trivino --- app/controlplane/cmd/wire_gen.go | 4 +- app/controlplane/internal/service/auth.go | 90 +++++++++++++++++-- .../internal/service/auth_test.go | 53 +++++++++++ .../internal/service/auth_token_page.go | 9 +- 4 files changed, 147 insertions(+), 9 deletions(-) diff --git a/app/controlplane/cmd/wire_gen.go b/app/controlplane/cmd/wire_gen.go index 4e2151e9e..b10e0e01c 100644 --- a/app/controlplane/cmd/wire_gen.go +++ b/app/controlplane/cmd/wire_gen.go @@ -179,8 +179,7 @@ func wireApp(contextContext context.Context, bootstrap *conf.Bootstrap, readerWr projectUseCase := biz.NewProjectsUseCase(logger, projectsRepo, membershipRepo, auditorUseCase, groupUseCase, membershipUseCase, orgInvitationUseCase, orgInvitationRepo, authzUseCase) v5 := serviceOpts(logger, authzUseCase, projectUseCase, groupUseCase) workflowService := service.NewWorkflowService(workflowUseCase, workflowContractUseCase, projectUseCase, organizationUseCase, userUseCase, v5...) - confServer := bootstrap.Server - authService, err := service.NewAuthService(userUseCase, organizationUseCase, membershipUseCase, orgInvitationUseCase, auth, confServer, auditorUseCase, v5...) + authService, err := service.NewAuthService(userUseCase, organizationUseCase, membershipUseCase, orgInvitationUseCase, auth, bootstrap, auditorUseCase, v5...) if err != nil { cleanup3() cleanup2() @@ -317,6 +316,7 @@ func wireApp(contextContext context.Context, bootstrap *conf.Bootstrap, readerWr prometheusService := service.NewPrometheusService(organizationUseCase, prometheusUseCase, v5...) groupService := service.NewGroupService(groupUseCase, v5...) projectService := service.NewProjectService(v5...) + confServer := bootstrap.Server federatedAuthentication := bootstrap.FederatedAuthentication operationAuthorizationProvider := bootstrap.OperationAuthorizationProvider validator, err := newProtoValidator() diff --git a/app/controlplane/internal/service/auth.go b/app/controlplane/internal/service/auth.go index 263258a2d..bf66ae7ce 100644 --- a/app/controlplane/internal/service/auth.go +++ b/app/controlplane/internal/service/auth.go @@ -21,9 +21,12 @@ import ( "encoding/base64" "errors" "fmt" + "net" "net/http" "net/mail" "net/url" + "slices" + "strings" "time" pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" @@ -115,16 +118,18 @@ type AuthService struct { AuthURLs *AuthURLs auditorUseCase *biz.AuditorUseCase devMode bool + // scheme://host destinations the post-login redirect may target + allowedCallbackOrigins []string } -func NewAuthService(userUC *biz.UserUseCase, orgUC *biz.OrganizationUseCase, mUC *biz.MembershipUseCase, inviteUC *biz.OrgInvitationUseCase, authConfig *conf.Auth, serverConfig *conf.Server, auc *biz.AuditorUseCase, opts ...NewOpt) (*AuthService, error) { +func NewAuthService(userUC *biz.UserUseCase, orgUC *biz.OrganizationUseCase, mUC *biz.MembershipUseCase, inviteUC *biz.OrgInvitationUseCase, authConfig *conf.Auth, bootstrapConfig *conf.Bootstrap, auc *biz.AuditorUseCase, opts ...NewOpt) (*AuthService, error) { oidcConfig := authConfig.GetOidc() if oidcConfig == nil { return nil, errors.New("oauth configuration missing") } // Craft Auth related endpoints - authURLs, err := getAuthURLs(serverConfig.GetHttp(), authConfig.GetOidc().GetLoginUrlOverride()) + authURLs, err := getAuthURLs(bootstrapConfig.GetServer().GetHttp(), authConfig.GetOidc().GetLoginUrlOverride()) if err != nil { return nil, fmt.Errorf("failed to get auth URLs: %w", err) } @@ -152,9 +157,72 @@ func NewAuthService(userUC *biz.UserUseCase, orgUC *biz.OrganizationUseCase, mUC membershipUseCase: mUC, orgInvitesUseCase: inviteUC, auditorUseCase: auc, + allowedCallbackOrigins: originsOf(authURLs.Login, bootstrapConfig.GetServer().GetHttp().GetExternalUrl(), + bootstrapConfig.GetUiDashboardUrl()), }, nil } +var errInvalidCallback = errors.New("invalid callback URL") + +// originsOf extracts the origin of the given URLs, skipping empty or malformed ones +func originsOf(urls ...string) []string { + origins := make([]string, 0, len(urls)) + for _, raw := range urls { + u, err := url.Parse(raw) + if err != nil || u.Scheme == "" || u.Host == "" { + continue + } + + origins = append(origins, originOf(u)) + } + + return origins +} + +// originOf normalizes the URL down to scheme://host. Hosts are case insensitive, so a +// config typo like "https://App.Example.com" still matches the browser-sent origin +func originOf(u *url.URL) string { + return strings.ToLower(u.Scheme + "://" + u.Host) +} + +// callbackAllowed rejects post-login redirect targets that would hand the user JWT over to +// a third party. Relative paths (CAS download redirect) and loopback (CLI login) are always +// allowed, anything else must match a known origin. +func callbackAllowed(callback string, allowedOrigins []string) error { + if callback == "" { + return nil + } + + // Browsers fold "\" into "/", so "/\evil.example" would escape a seemingly relative path + if strings.Contains(callback, `\`) { + return errInvalidCallback + } + + u, err := url.Parse(callback) + if err != nil { + return errInvalidCallback + } + + // Relative path. Both parts must be empty, otherwise "//evil.example" would pass as a path + if u.Scheme == "" && u.Host == "" { + return nil + } + + if u.Scheme != "http" && u.Scheme != "https" { + return errInvalidCallback + } + + if host := u.Hostname(); host == "localhost" || net.ParseIP(host).IsLoopback() { + return nil + } + + if slices.Contains(allowedOrigins, originOf(u)) { + return nil + } + + return fmt.Errorf("callback URL not allowed: %s", originOf(u)) +} + type AuthURLs struct { Login, callback string loginIsOverridden bool @@ -221,6 +289,13 @@ func (h oauthHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } func loginHandler(svc *AuthService, w http.ResponseWriter, r *http.Request) *oauthResp { + // The final destination where the auth token will be pushed to, i.e the CLI. + // Rejected upfront so a crafted callback never starts the OIDC dance + callback := r.URL.Query().Get(oauth.QueryParamCallback) + if err := callbackAllowed(callback, svc.allowedCallbackOrigins); err != nil { + return newOauthResp(http.StatusBadRequest, err, true) + } + b := make([]byte, 16) _, err := rand.Read(b) if err != nil { @@ -231,8 +306,7 @@ func loginHandler(svc *AuthService, w http.ResponseWriter, r *http.Request) *oau state := base64.URLEncoding.EncodeToString(b) svc.setOauthCookie(w, cookieOauthStateName, state) - // Store the final destination where the auth token will be pushed to, i.e the CLI - svc.setOauthCookie(w, cookieCallback, r.URL.Query().Get(oauth.QueryParamCallback)) + svc.setOauthCookie(w, cookieCallback, callback) // Wether the token should be short lived or not svc.setOauthCookie(w, cookieLongLived, r.URL.Query().Get(oauth.QueryParamLongLived)) @@ -362,12 +436,18 @@ func callbackHandler(svc *AuthService, w http.ResponseWriter, r *http.Request) * return newOauthResp(http.StatusOK, nil, false) } - // Redirect to the callback URL + // Redirect to the callback URL. The cookie was validated at login time, re-check it here + // so a stale or tampered cookie can't turn this into an open redirect either + if err := callbackAllowed(callbackValue, svc.allowedCallbackOrigins); err != nil { + return newOauthResp(http.StatusBadRequest, err, true) + } + callbackURL, err := crafCallbackURL(callbackValue, userToken) if err != nil { return newOauthResp(http.StatusInternalServerError, fmt.Errorf("failed to craft callback URL: %w", err), false) } + setTokenLeakHeaders(w) http.Redirect(w, r, callbackURL, http.StatusFound) return newOauthResp(http.StatusTemporaryRedirect, nil, false) } diff --git a/app/controlplane/internal/service/auth_test.go b/app/controlplane/internal/service/auth_test.go index e74d939ac..a35bc20bf 100644 --- a/app/controlplane/internal/service/auth_test.go +++ b/app/controlplane/internal/service/auth_test.go @@ -16,6 +16,8 @@ package service import ( + "net/http" + "net/http/httptest" "testing" conf "github.com/chainloop-dev/chainloop/app/controlplane/internal/conf/controlplane/config/v1" @@ -124,3 +126,54 @@ func TestGetPreferredEmail(t *testing.T) { assert.Equal(t, tc.want, got) } } + +func TestCallbackAllowed(t *testing.T) { + // mixed case on purpose, origins are matched case insensitively + allowed := originsOf("https://app.chainloop.dev/login", "https://CP.Chainloop.dev", "https://app.chainloop.dev") + + testCases := []struct { + name string + callback string + wantErr bool + }{ + {name: "empty, token is rendered in a page", callback: ""}, + {name: "relative path, CAS download redirect", callback: "/download/sha256:deadbeef?foo=bar"}, + {name: "loopback with random port, CLI login", callback: "http://127.0.0.1:41337/auth/callback"}, + {name: "localhost with random port, CLI login", callback: "http://localhost:41337/auth/callback"}, + {name: "IPv6 loopback", callback: "http://[::1]:41337/auth/callback"}, + {name: "dashboard origin", callback: "https://app.chainloop.dev/login/callback?returnTo=%2Fprojects"}, + {name: "control plane origin, config declared it in a different case", callback: "https://cp.chainloop.dev/foo"}, + {name: "dashboard origin, browser sent a different case", callback: "https://APP.chainloop.dev/login/callback"}, + {name: "third party origin", callback: "https://evil.example/collect", wantErr: true}, + {name: "protocol relative", callback: "//evil.example/collect", wantErr: true}, + {name: "backslash escaping a relative path", callback: "/\\evil.example/collect", wantErr: true}, + {name: "non http scheme", callback: "javascript:alert(1)", wantErr: true}, + {name: "loopback lookalike host", callback: "http://localhost.evil.example/collect", wantErr: true}, + {name: "allowed host as a subdomain", callback: "https://app.chainloop.dev.evil.example/collect", wantErr: true}, + {name: "allowed host with a different scheme", callback: "http://app.chainloop.dev/collect", wantErr: true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := callbackAllowed(tc.callback, allowed) + if tc.wantErr { + assert.Error(t, err) + return + } + + assert.NoError(t, err) + }) + } +} + +// The callback cookie must not be set for a destination we would refuse to redirect to +func TestLoginHandlerRejectsForeignCallback(t *testing.T) { + svc := &AuthService{allowedCallbackOrigins: originsOf("https://app.chainloop.dev")} + + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/auth/login?callback=https%3A%2F%2Fevil.example%2Fcollect&long-lived=true", nil) + + resp := loginHandler(svc, w, r) + assert.Equal(t, http.StatusBadRequest, resp.code) + assert.Empty(t, w.Result().Cookies()) +} diff --git a/app/controlplane/internal/service/auth_token_page.go b/app/controlplane/internal/service/auth_token_page.go index 1e089c7d1..f25d9a52d 100644 --- a/app/controlplane/internal/service/auth_token_page.go +++ b/app/controlplane/internal/service/auth_token_page.go @@ -29,8 +29,7 @@ var tokenPageTemplate = template.Must(template.New("tokenPage").Parse(tokenPageH // leakage so the bearer token does not escape the page. func renderTokenPage(w http.ResponseWriter, token string) error { w.Header().Set("Content-Type", "text/html; charset=utf-8") - w.Header().Set("Cache-Control", "no-store") - w.Header().Set("Referrer-Policy", "no-referrer") + setTokenLeakHeaders(w) if err := tokenPageTemplate.Execute(w, struct{ Token string }{Token: token}); err != nil { return fmt.Errorf("failed to render token page: %w", err) @@ -38,6 +37,12 @@ func renderTokenPage(w http.ResponseWriter, token string) error { return nil } +// setTokenLeakHeaders keeps a response carrying a user token out of caches and Referer headers +func setTokenLeakHeaders(w http.ResponseWriter) { + w.Header().Set("Cache-Control", "no-store") + w.Header().Set("Referrer-Policy", "no-referrer") +} + // #nosec G101 -- HTML template, not a credential const tokenPageHTML = `