From 8056ced7fd5a68427e41d494088dde865dcb0e7a Mon Sep 17 00:00:00 2001 From: Sean Reifschneider Date: Tue, 1 Sep 2026 15:06:18 +0000 Subject: [PATCH 1/3] oidc: serve the registration confirmation page from a reloadable URL The interstitial was the body of /oidc/callback, the URL carrying the single-use code, so any reload re-entered the spent exchange. Redirect to GET /register/confirm/{auth_id}, also missing from the route table. --- hscontrol/app.go | 1 + hscontrol/handlers.go | 5 +- hscontrol/handlers_test.go | 2 +- hscontrol/oidc.go | 144 +++++++++-- hscontrol/oidc_callback_reload_test.go | 336 +++++++++++++++++++++++++ integration/scenario.go | 27 +- 6 files changed, 479 insertions(+), 36 deletions(-) create mode 100644 hscontrol/oidc_callback_reload_test.go diff --git a/hscontrol/app.go b/hscontrol/app.go index 12c89f6a11..ed693a24ef 100644 --- a/hscontrol/app.go +++ b/hscontrol/app.go @@ -475,6 +475,7 @@ func (h *Headscale) createRouter(apiV1Mux, apiV2Mux http.Handler) *chi.Mux { if provider, ok := h.authProvider.(*AuthProviderOIDC); ok { r.Get("/oidc/callback", provider.OIDCCallbackHandler) + r.Get("/register/confirm/{auth_id}", provider.RegisterConfirmGetHandler) r.Post("/register/confirm/{auth_id}", provider.RegisterConfirmHandler) } diff --git a/hscontrol/handlers.go b/hscontrol/handlers.go index 6ccfa6118b..7693ad9459 100644 --- a/hscontrol/handlers.go +++ b/hscontrol/handlers.go @@ -73,7 +73,10 @@ func userMessageForStatusCode(code int) string { case code == http.StatusUnauthorized || code == http.StatusForbidden: return "You are not authorized. Please contact your administrator." case code == http.StatusGone: - return "Your session has expired. Please try again." + // Overwhelmingly a reload or a back button on a link the user + // already used, not a failure, so lead with that. + return "This link has already been used or has expired. " + + "If your device is connected you are done; otherwise start the login again." case code >= 400 && code < 500: return "The request could not be processed. Please try again." default: diff --git a/hscontrol/handlers_test.go b/hscontrol/handlers_test.go index 9eaae41ee7..668f545a23 100644 --- a/hscontrol/handlers_test.go +++ b/hscontrol/handlers_test.go @@ -202,7 +202,7 @@ func TestHttpUserError(t *testing.T) { name: "gone_renders_session_expired", err: NewHTTPError(http.StatusGone, "login session expired, try again", nil), wantCode: http.StatusGone, - wantContains: "Your session has expired. Please try again.", + wantContains: "This link has already been used or has expired.", wantNotContain: "login session expired", }, { diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index fb2e6839c1..5737cda265 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "slices" "strings" "time" @@ -349,7 +350,7 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( // /register/{auth_id} could silently complete a registration when // the IdP allows silent SSO. if authInfo.Registration { - a.renderRegistrationConfirmInterstitial(writer, req, authInfo.AuthID, user, nodeExpiry) + a.beginRegistrationConfirmation(writer, req, authInfo.AuthID, user, nodeExpiry) return } @@ -667,34 +668,70 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim( // browser do not collide. const registerConfirmCSRFCookie = "headscale_register_confirm" +// registrationLinkSpentMsg is logged when a user returns to a +// registration link whose session is gone, which is usually a reload or a +// back button after they already confirmed. The page the user sees comes +// from [userMessageForStatusCode]. +const registrationLinkSpentMsg = "registration link already used or expired" + +var errRegistrationLinkSpent = NewHTTPError(http.StatusGone, registrationLinkSpentMsg, nil) + +// registerConfirmURL is the browser-facing URL of the confirmation page. +// It is built from server_url, like [AuthProviderOIDC.RegisterURL] and the +// OIDC redirect URI, so a Headscale that a reverse proxy serves under a +// path prefix hands the browser a URL that resolves. +func (a *AuthProviderOIDC) registerConfirmURL(authID types.AuthID) string { + return authPathURL(a.serverURL, "register/confirm", authID) +} + // setRegisterConfirmCookie writes the per-session register-confirm CSRF // cookie. Pass the CSRF token and authCacheExpiration seconds to set it; // pass ("", -1) to clear it after the registration is finalised. -func setRegisterConfirmCookie( +func (a *AuthProviderOIDC) setRegisterConfirmCookie( writer http.ResponseWriter, req *http.Request, authID types.AuthID, value string, maxAge int, - secure bool, ) { + // Scope the cookie to the browser-facing path, which carries the + // reverse proxy's prefix; the routed path does not. + path := "/register/confirm/" + authID.String() + if u, err := url.Parse(a.registerConfirmURL(authID)); err == nil { //nolint:noinlineerr + path = u.Path + } + //nolint:gosec // G124: Secure from server_url scheme or req.TLS; HttpOnly + SameSite already set http.SetCookie(writer, &http.Cookie{ Name: registerConfirmCSRFCookie, Value: value, - Path: "/register/confirm/" + authID.String(), + Path: path, MaxAge: maxAge, - Secure: secure || req.TLS != nil, + Secure: a.cookiesSecure() || req.TLS != nil, HttpOnly: true, - SameSite: http.SameSiteStrictMode, + // Lax, not Strict: the callback sets this cookie and immediately + // redirects to the confirmation page. That hop ends a redirect + // chain which began cross-site at the IdP, and Firefox evaluates + // the whole chain, so a Strict cookie is withheld and the + // confirmation page 403s. Lax still never rides a cross-site + // POST, so the confirm submission stays protected. + SameSite: http.SameSiteLaxMode, }) } -// renderRegistrationConfirmInterstitial captures the resolved OIDC -// identity and node expiry into the cached [types.AuthRequest], sets the CSRF -// cookie, and renders the confirmation page that the user must -// explicitly submit before the registration is finalised. -func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial( +// beginRegistrationConfirmation captures the resolved OIDC identity and +// node expiry into the cached [types.AuthRequest], sets the CSRF cookie, and +// redirects the browser to the confirmation page. +// +// The interstitial is served from its own URL rather than written inline +// here, because this request carries the single-use OAuth authorization +// code. A page rendered on this response leaves the browser parked on the +// code-bearing URL, and anything that reloads it — an extension calling +// window.location.reload(), the back button, pull-to-refresh, a prerender +// — re-enters the callback with a spent code and paints an error over the +// interstitial. Redirecting keeps the code exchange one-shot and makes the +// page the user waits on safe to reload. +func (a *AuthProviderOIDC) beginRegistrationConfirmation( writer http.ResponseWriter, req *http.Request, authID types.AuthID, @@ -726,14 +763,78 @@ func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial( CSRF: csrf, }) - setRegisterConfirmCookie(writer, req, authID, csrf, int(authCacheExpiration.Seconds()), a.cookiesSecure()) + a.setRegisterConfirmCookie(writer, req, authID, csrf, int(authCacheExpiration.Seconds())) + + // 303 See Other so the browser issues a fresh GET for the + // confirmation page and leaves the code-bearing URL behind as a + // transient hop rather than a history entry it can return to. + http.Redirect(writer, req, a.registerConfirmURL(authID), http.StatusSeeOther) +} + +// RegisterConfirmGetHandler renders the OIDC registration confirmation +// interstitial. It is reached via the redirect that +// [AuthProviderOIDC.beginRegistrationConfirmation] issues from the OIDC +// callback, and it is safe to reload: it only reads the pending +// confirmation captured on the cached [types.AuthRequest] and never touches +// the one-time code exchange. +// +// Listens in GET /register/confirm/:auth_id. +func (a *AuthProviderOIDC) RegisterConfirmGetHandler( + writer http.ResponseWriter, + req *http.Request, +) { + authID, err := authIDFromRequest(req) + if err != nil { + httpUserError(writer, err) + + return + } + + authReq, ok := a.h.state.GetAuthCacheEntry(authID) + if !ok { + httpUserError(writer, errRegistrationLinkSpent) + + return + } + + pending := authReq.PendingConfirmation() + if pending == nil { + httpUserError(writer, NewHTTPError(http.StatusForbidden, "registration not OIDC-authorized", nil)) + + return + } + + // Only the browser that completed the OIDC flow holds this cookie, and + // holding it is what authorises the confirm POST. Requiring it here too + // keeps the device details, and the token that finalises the + // registration, away from anyone who merely knows the auth ID — which + // the node being registered does. + cookie, err := req.Cookie(registerConfirmCSRFCookie) + if err != nil { + httpUserError(writer, NewHTTPError(http.StatusForbidden, "missing csrf cookie", err)) + + return + } + + if cookie.Value != pending.CSRF { + httpUserError(writer, NewHTTPError(http.StatusForbidden, "csrf token mismatch", nil)) + + return + } + + user, err := a.h.state.GetUserByID(types.UserID(pending.UserID)) + if err != nil { + httpUserError(writer, fmt.Errorf("looking up user: %w", err)) + + return + } regData := authReq.RegistrationData() info := templates.RegisterConfirmInfo{ - FormAction: "/register/confirm/" + authID.String(), + FormAction: a.registerConfirmURL(authID), CSRFTokenName: registerConfirmCSRFCookie, - CSRFToken: csrf, + CSRFToken: pending.CSRF, User: user.Display(), Hostname: regData.Hostname, MachineKey: regData.MachineKey.ShortString(), @@ -742,6 +843,9 @@ func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial( info.OS = regData.Hostinfo.OS } + // The page carries the token that finalises the registration, so no + // shared cache or history restore may serve it back. + writer.Header().Set("Cache-Control", "no-store") writer.Header().Set("Content-Type", "text/html; charset=utf-8") writer.WriteHeader(http.StatusOK) @@ -758,12 +862,6 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler( writer http.ResponseWriter, req *http.Request, ) { - if req.Method != http.MethodPost { - httpUserError(writer, errMethodNotAllowed) - - return - } - authID, err := authIDFromRequest(req) if err != nil { httpUserError(writer, err) @@ -804,7 +902,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler( authReq, ok := a.h.state.GetAuthCacheEntry(authID) if !ok { - httpUserError(writer, NewHTTPError(http.StatusGone, "registration session expired", nil)) + httpUserError(writer, errRegistrationLinkSpent) return } @@ -832,7 +930,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler( newNode, err := a.handleRegistration(user, authID, pending.NodeExpiry) if err != nil { if errors.Is(err, db.ErrNodeNotFoundRegistrationCache) { - httpUserError(writer, NewHTTPError(http.StatusGone, "registration session expired", err)) + httpUserError(writer, NewHTTPError(http.StatusGone, registrationLinkSpentMsg, err)) return } @@ -843,7 +941,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler( } // Clear the CSRF cookie now that the registration is final. - setRegisterConfirmCookie(writer, req, authID, "", -1, a.cookiesSecure()) + a.setRegisterConfirmCookie(writer, req, authID, "", -1) content := renderRegistrationSuccessTemplate(user, newNode) diff --git a/hscontrol/oidc_callback_reload_test.go b/hscontrol/oidc_callback_reload_test.go new file mode 100644 index 0000000000..335ef9f019 --- /dev/null +++ b/hscontrol/oidc_callback_reload_test.go @@ -0,0 +1,336 @@ +package hscontrol + +import ( + "context" + "io" + "net/http" + "net/http/cookiejar" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + + "github.com/juanfont/headscale/hscontrol/types" + "github.com/oauth2-proxy/mockoidc" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "tailscale.com/types/key" +) + +// oidcBrowser drives the interactive OIDC registration flow the way a +// browser does: over real HTTP against the real route table, through a +// cookie jar that honours Path and expiry, following redirects. +// +// Both halves matter for this bug. The route table is where the +// confirmation page's missing GET route lives, and cookie Path decides +// whether the state cookie the callback deletes is actually gone on the +// next hit. +type oidcBrowser struct { + app *Headscale + idp *mockoidc.MockOIDC + srv *httptest.Server + client *http.Client +} + +func newOIDCBrowser(t *testing.T) *oidcBrowser { + t.Helper() + + idp, err := mockoidc.Run() + require.NoError(t, err) + + t.Cleanup(func() { + _ = idp.Shutdown() + }) + + app := createTestApp(t) + + // The provider derives its OIDC redirect_uri from the server URL, and + // the server serves the router the provider is registered on, so bind + // the router late to break the cycle. + var router http.Handler + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + router.ServeHTTP(w, r) + })) + t.Cleanup(srv.Close) + + provider, err := NewAuthProviderOIDC( + context.Background(), + app, + srv.URL, + &types.OIDCConfig{ + Issuer: idp.Issuer(), + ClientID: idp.ClientID, + ClientSecret: idp.ClientSecret, + Scope: []string{"openid", "profile", "email"}, + }, + ) + require.NoError(t, err) + + app.authProvider = provider + router = app.createRouter(nil, nil) + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + return &oidcBrowser{ + app: app, + idp: idp, + srv: srv, + client: &http.Client{Jar: jar}, + } +} + +// get fetches a URL, follows redirects, and returns the status, the URL +// the browser ended up on, and the page body. +func (b *oidcBrowser) get(t *testing.T, rawURL string) (int, *url.URL, string) { + t.Helper() + + resp, err := b.client.Get(rawURL) //nolint:noctx,bodyclose // test client; closed below + require.NoError(t, err) + + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + return resp.StatusCode, resp.Request.URL, string(body) +} + +// pendingNode mints a pending node registration and returns the URL the +// tailscale client would print for the user to open. +func (b *oidcBrowser) pendingNode(t *testing.T) (types.AuthID, string) { + t.Helper() + + authID := types.MustAuthID() + b.app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: key.NewMachine().Public(), + NodeKey: key.NewNode().Public(), + Hostname: "reload-victim", + })) + + return authID, b.srv.URL + "/register/" + authID.String() +} + +var ( + csrfInputRe = regexp.MustCompile( + `name="` + registerConfirmCSRFCookie + `"[^>]*value="([^"]+)"`, + ) + formActionRe = regexp.MustCompile(`action="([^"]+)"`) +) + +// TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL reproduces +// https://github.com/juanfont/headscale/issues/3365. +// +// The interactive OIDC flow ends with the browser sitting on the +// confirmation interstitial, waiting for the user to click. Today that +// interstitial is written as the body of the /oidc/callback response, so +// the URL the browser is parked on is the one carrying the single-use +// OAuth authorization code. Reloading it re-enters the callback, which +// has already spent the code and deleted the state cookie, and the error +// page paints over the interstitial. The node is never registered. +// +// Adblock Plus is only the loudest trigger — it calls +// window.location.reload() to apply element-hiding filters. The back +// button, mobile pull-to-refresh and browser prerendering all aim at the +// same URL. +// +// The property asserted here is the one that fixes the whole class, +// stated without naming an implementation: wherever the flow leaves the +// browser, that URL must be free of the authorization code and safe to +// load again. +func TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL(t *testing.T) { + b := newOIDCBrowser(t) + + _, registerURL := b.pendingNode(t) + + status, landed, body := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status, "the login flow must reach a page") + require.Contains(t, body, "Confirm node registration", + "the flow must end on the confirmation interstitial") + + assert.Empty(t, landed.Query().Get("code"), + "the browser must not be left parked on the URL carrying the one-time "+ + "OAuth code; any reload of it re-enters the spent callback") + + reloadedStatus, _, reloadedBody := b.get(t, landed.String()) + + require.Equal(t, http.StatusOK, reloadedStatus, + "reloading the page the flow left the browser on must re-render it") + assert.Contains(t, reloadedBody, "Confirm node registration", + "the reload must show the confirmation interstitial, not an error page") +} + +// TestOIDCLoginCompletesAfterReload is the reporters' scenario end to +// end: the confirmation page is reloaded before the user clicks, and the +// registration must still complete. One deployment measured login +// completion falling from 100% to 61-73% across this exact step. +func TestOIDCLoginCompletesAfterReload(t *testing.T) { + b := newOIDCBrowser(t) + + _, registerURL := b.pendingNode(t) + + status, landed, _ := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status) + + // The spurious reload, on whatever URL the flow parked the browser on. + reloadedStatus, reloadedURL, body := b.get(t, landed.String()) + require.Equal(t, http.StatusOK, reloadedStatus, + "the page the user is sitting on must survive a reload") + + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2, "the reloaded page must still carry a usable confirm form") + + action := formActionRe.FindStringSubmatch(body) + require.Len(t, action, 2, "the reloaded page must still carry a form action") + + confirmURL, err := reloadedURL.Parse(action[1]) + require.NoError(t, err) + + //nolint:noctx,bodyclose // test client; closed below + confirmed, err := b.client.PostForm(confirmURL.String(), url.Values{ + registerConfirmCSRFCookie: {csrf[1]}, + }) + require.NoError(t, err) + + defer confirmed.Body.Close() + + confirmedBody, err := io.ReadAll(confirmed.Body) + require.NoError(t, err) + + require.Equal(t, http.StatusOK, confirmed.StatusCode, + "confirming after a reload must register the node") + assert.Contains(t, strings.ToLower(string(confirmedBody)), "registered", + "the user must get the registration success page") +} + +// TestRegisterConfirmGETIsNotADeadEnd covers the second, independent way +// a registration is lost, reported with no ad blocker involved: the +// confirmation endpoint is POST-only in the route table, so a user who +// refreshes or navigates back to it gets a bare 405 from the router with +// no way to recover, while the pending registration sits in the cache +// unreachable until it expires. +// +// This is a route-table gap, so it can only be observed through the real +// router — calling the handler directly cannot see it. +func TestRegisterConfirmGETIsNotADeadEnd(t *testing.T) { + b := newOIDCBrowser(t) + + authID, registerURL := b.pendingNode(t) + + // Complete the OIDC leg so there is a pending confirmation to render. + status, _, _ := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status) + + confirmStatus, _, body := b.get(t, b.srv.URL+"/register/confirm/"+authID.String()) + + require.NotEqual(t, http.StatusMethodNotAllowed, confirmStatus, + "GET on the confirmation URL must not be a dead end for a user who "+ + "refreshes or goes back") + require.Equal(t, http.StatusOK, confirmStatus, + "the confirmation page must be reachable by GET") + assert.Contains(t, body, "Confirm node registration") +} + +// TestRegisterConfirmNeedsTheCallbackCookie locks the reason the +// confirmation step exists. The node being registered knows its own auth +// ID, so the auth ID alone must never be enough to view the device +// details or to finalise the registration — only the browser that +// completed the OIDC login holds the cookie the callback set, and holding +// it is what authorises the confirm. +// +// Without this, an attacker could hand a victim a /register/{auth_id} +// link for the attacker's own node, let the victim's IdP silently sign +// in, and then confirm the registration themselves under the victim's +// identity. +func TestRegisterConfirmNeedsTheCallbackCookie(t *testing.T) { + b := newOIDCBrowser(t) + + authID, registerURL := b.pendingNode(t) + + status, _, body := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status) + + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2) + + // A second browser that knows the auth ID, and even the token from the + // rendered page, but never completed the OIDC login. + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + attacker := &http.Client{Jar: jar} + confirmURL := b.srv.URL + "/register/confirm/" + authID.String() + + //nolint:noctx,bodyclose // test client; closed below + viewed, err := attacker.Get(confirmURL) + require.NoError(t, err) + + defer viewed.Body.Close() + + assert.Equal(t, http.StatusForbidden, viewed.StatusCode, + "the confirmation page must not render without the callback cookie") + + //nolint:noctx,bodyclose // test client; closed below + submitted, err := attacker.PostForm(confirmURL, url.Values{ + registerConfirmCSRFCookie: {csrf[1]}, + }) + require.NoError(t, err) + + defer submitted.Body.Close() + + assert.Equal(t, http.StatusForbidden, submitted.StatusCode, + "the registration must not finalise without the callback cookie") + + cached, ok := b.app.state.GetAuthCacheEntry(authID) + require.True(t, ok, "the pending registration must survive the attempt") + assert.NotNil(t, cached.PendingConfirmation(), + "the pending registration must still be waiting for the real user") +} + +// TestSetRegisterConfirmCookieSameSite pins SameSite=Lax. Strict is +// withheld by browsers that evaluate the whole redirect chain, and this +// cookie now has to survive the callback's redirect to the confirmation +// page — a chain that begins cross-site at the identity provider. Lax is +// still never attached to a cross-site POST, so the confirm submission +// keeps its protection. +func TestSetRegisterConfirmCookieSameSite(t *testing.T) { + a := &AuthProviderOIDC{serverURL: "https://hs.example.com"} + authID := types.MustAuthID() + + rec := httptest.NewRecorder() + a.setRegisterConfirmCookie(rec, + httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil), + authID, "token", 900) + + cookies := rec.Result().Cookies() + require.Len(t, cookies, 1) + assert.Equal(t, http.SameSiteLaxMode, cookies[0].SameSite, + "the confirm cookie must survive the callback redirect") + assert.True(t, cookies[0].Secure, "https server_url must set Secure") + assert.Equal(t, "/register/confirm/"+authID.String(), cookies[0].Path) +} + +// TestRegisterConfirmURLFollowsServerURLPrefix covers the deployment +// where a reverse proxy serves Headscale under a path prefix. The +// redirect target, the form action and the cookie scope are all seen by +// the browser, so they carry the prefix even though the routed path does +// not. +func TestRegisterConfirmURLFollowsServerURLPrefix(t *testing.T) { + a := &AuthProviderOIDC{serverURL: "https://example.com/hs"} + authID := types.MustAuthID() + + assert.Equal(t, "https://example.com/hs/register/confirm/"+authID.String(), + a.registerConfirmURL(authID)) + + rec := httptest.NewRecorder() + a.setRegisterConfirmCookie(rec, + httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil), + authID, "token", 900) + + cookies := rec.Result().Cookies() + require.Len(t, cookies, 1) + assert.Equal(t, "/hs/register/confirm/"+authID.String(), cookies[0].Path, + "the cookie must be scoped to the path the browser sees") +} diff --git a/integration/scenario.go b/integration/scenario.go index 7af2816196..73eb73f2ac 100644 --- a/integration/scenario.go +++ b/integration/scenario.go @@ -1112,6 +1112,11 @@ func (j *debugJar) Dump(w io.Writer) { } } +// registerConfirmCSRFField is the name of both the hidden CSRF form field +// and the cookie on the OIDC registration confirmation interstitial. It +// mirrors registerConfirmCSRFCookie in hscontrol, which is unexported. +const registerConfirmCSRFField = "headscale_register_confirm" + func copyCookie(c *http.Cookie) *http.Cookie { cc := *c return &cc @@ -1225,10 +1230,11 @@ func doLoginURLWithClient(hostname string, loginURL *url.URL, hc *http.Client, f } } - // The OIDC registration flow now renders a confirmation interstitial - // (POST form) instead of completing immediately. Detect the form and + // The OIDC registration flow renders a confirmation interstitial + // (POST form) instead of completing immediately. Detect the form by its + // CSRF field, which does not move when the form action does, and // auto-submit it so integration tests behave like a real browser. - if followRedirects && strings.Contains(body, `action="/register/confirm/`) { + if followRedirects && strings.Contains(body, `name="`+registerConfirmCSRFField+`"`) { confirmBody, confirmURL, confirmErr := submitConfirmForm(hostname, body, resp, hc) if confirmErr != nil { return body, redirectURL, confirmErr @@ -1267,7 +1273,7 @@ func submitConfirmForm( // Extract hidden CSRF input value. The rendered has // attributes in name-type-value order so we grab the whole tag. - before, _, ok := strings.Cut(htmlBody, `name="headscale_register_confirm"`) + before, _, ok := strings.Cut(htmlBody, `name="`+registerConfirmCSRFField+`"`) if !ok { return "", nil, fmt.Errorf("%s confirm form: no CSRF input", hostname) //nolint:err113 } @@ -1293,18 +1299,17 @@ func submitConfirmForm( valEnd := strings.Index(inputTag[valStart:], `"`) csrfToken := inputTag[valStart : valStart+valEnd] - // Build the absolute POST URL from the response's request URL. - base := prevResp.Request.URL - confirmURL := &url.URL{ - Scheme: base.Scheme, - Host: base.Host, - Path: formAction, + // Resolve the form action against the page it was served from, so an + // absolute and a relative action both work. + confirmURL, err := prevResp.Request.URL.Parse(formAction) + if err != nil { + return "", nil, fmt.Errorf("%s confirm form: resolving action %q: %w", hostname, formAction, err) } log.Printf("%s auto-submitting confirm form: %s", hostname, confirmURL) formData := url.Values{ - "headscale_register_confirm": {csrfToken}, + registerConfirmCSRFField: {csrfToken}, } ctx := context.Background() From 242f91d5fc92974fe1d357ebaa3e5fd58baa071f Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 4 Sep 2026 13:29:59 +0000 Subject: [PATCH 2/3] oidc: harden reloadable confirmation flow Updates #3365 --- hscontrol/handlers.go | 23 +- hscontrol/handlers_test.go | 14 +- hscontrol/oidc.go | 58 +++- hscontrol/oidc_callback_reload_test.go | 336 ----------------------- hscontrol/oidc_test.go | 362 ++++++++++++++++++++++++- 5 files changed, 430 insertions(+), 363 deletions(-) delete mode 100644 hscontrol/oidc_callback_reload_test.go diff --git a/hscontrol/handlers.go b/hscontrol/handlers.go index 7693ad9459..2a0157cd6f 100644 --- a/hscontrol/handlers.go +++ b/hscontrol/handlers.go @@ -40,18 +40,23 @@ func httpError(w http.ResponseWriter, err error) { // an actionable message derived from the HTTP status code. func httpUserError(w http.ResponseWriter, err error) { code := http.StatusInternalServerError + userMsg := "" if herr, ok := errors.AsType[HTTPError](err); ok { if herr.Code != 0 { code = herr.Code } + userMsg = herr.UserMsg + log.Error().Err(herr.Err).Int("code", code).Msgf("user msg: %s", herr.Msg) } else { log.Error().Err(err).Int("code", code).Msg("http internal server error") } - userMsg := userMessageForStatusCode(code) + if userMsg == "" { + userMsg = userMessageForStatusCode(code) + } w.Header().Set("Content-Type", "text/html; charset=utf-8") w.WriteHeader(code) @@ -73,10 +78,7 @@ func userMessageForStatusCode(code int) string { case code == http.StatusUnauthorized || code == http.StatusForbidden: return "You are not authorized. Please contact your administrator." case code == http.StatusGone: - // Overwhelmingly a reload or a back button on a link the user - // already used, not a failure, so lead with that. - return "This link has already been used or has expired. " + - "If your device is connected you are done; otherwise start the login again." + return "Your session has expired. Please try again." case code >= 400 && code < 500: return "The request could not be processed. Please try again." default: @@ -86,9 +88,10 @@ func userMessageForStatusCode(code int) string { // HTTPError represents an error that is surfaced to the user via web. type HTTPError struct { - Code int // HTTP response code to send to client; 0 means 500 - Msg string // Response body to send to client - Err error // Detailed error to log on the server + Code int // HTTP response code to send to client; 0 means 500 + Msg string // Response body to send to non-browser clients + Err error // Detailed error to log on the server + UserMsg string // Optional safe message for browser-facing error pages } func (e HTTPError) Error() string { return fmt.Sprintf("http error[%d]: %s, %s", e.Code, e.Msg, e.Err) } @@ -99,6 +102,10 @@ func NewHTTPError(code int, msg string, err error) HTTPError { return HTTPError{Code: code, Msg: msg, Err: err} } +func newHTTPUserError(code int, msg, userMsg string, err error) HTTPError { + return HTTPError{Code: code, Msg: msg, Err: err, UserMsg: userMsg} +} + var errMethodNotAllowed = NewHTTPError(http.StatusMethodNotAllowed, "method not allowed", nil) var ErrRegisterMethodCLIDoesNotSupportExpire = errors.New( diff --git a/hscontrol/handlers_test.go b/hscontrol/handlers_test.go index 668f545a23..f1ed71a095 100644 --- a/hscontrol/handlers_test.go +++ b/hscontrol/handlers_test.go @@ -202,9 +202,21 @@ func TestHttpUserError(t *testing.T) { name: "gone_renders_session_expired", err: NewHTTPError(http.StatusGone, "login session expired, try again", nil), wantCode: http.StatusGone, - wantContains: "This link has already been used or has expired.", + wantContains: "Your session has expired. Please try again.", wantNotContain: "login session expired", }, + { + name: "gone_with_user_message_renders_specific_guidance", + err: newHTTPUserError( + http.StatusGone, + "registration link already used or expired", + "This link has already been used or has expired.", + nil, + ), + wantCode: http.StatusGone, + wantContains: "This link has already been used or has expired.", + wantNotContain: "registration link already used or expired", + }, { name: "bad_request_renders_generic_retry", err: NewHTTPError(http.StatusBadRequest, "state not found", nil), diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index 5737cda265..e366784954 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -97,7 +97,7 @@ func NewAuthProviderOIDC( ClientID: cfg.ClientID, ClientSecret: cfg.ClientSecret, Endpoint: oidcProvider.Endpoint(), - RedirectURL: strings.TrimSuffix(serverURL, "/") + "/oidc/callback", + RedirectURL: oidcCallbackURL(serverURL), Scopes: cfg.Scope, } @@ -127,6 +127,18 @@ func (a *AuthProviderOIDC) cookiesSecure() bool { return strings.HasPrefix(a.serverURL, "https://") } +func oidcCallbackURL(serverURL string) string { + return strings.TrimSuffix(serverURL, "/") + "/oidc/callback" +} + +func (a *AuthProviderOIDC) oidcCallbackPath() string { + if u, err := url.Parse(oidcCallbackURL(a.serverURL)); err == nil { //nolint:noinlineerr + return u.Path + } + + return "/oidc/callback" +} + func (a *AuthProviderOIDC) AuthURL(authID types.AuthID) string { return authPathURL(a.serverURL, "auth", authID) } @@ -166,10 +178,10 @@ func (a *AuthProviderOIDC) authHandler( } // Set the state and nonce cookies to protect against CSRF attacks - state := setCSRFCookie(writer, req, "state", a.cookiesSecure()) + state := a.setCSRFCookie(writer, req, "state") // Set the state and nonce cookies to protect against CSRF attacks - nonce := setCSRFCookie(writer, req, "nonce", a.cookiesSecure()) + nonce := a.setCSRFCookie(writer, req, "nonce") registrationInfo := AuthInfo{ AuthID: authID, @@ -275,8 +287,8 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( // The state/nonce cookies have served their CSRF purpose; clear them so a // single-use pair does not linger in the browser until MaxAge. - clearOIDCCallbackCookie(writer, stateCookieName) - clearOIDCCallbackCookie(writer, nonceCookieName) + a.clearOIDCCallbackCookie(writer, stateCookieName) + a.clearOIDCCallbackCookie(writer, nonceCookieName) nodeExpiry := a.determineNodeExpiry(idToken.Expiry) @@ -674,7 +686,15 @@ const registerConfirmCSRFCookie = "headscale_register_confirm" // from [userMessageForStatusCode]. const registrationLinkSpentMsg = "registration link already used or expired" -var errRegistrationLinkSpent = NewHTTPError(http.StatusGone, registrationLinkSpentMsg, nil) +const registrationLinkSpentUserMsg = "This link has already been used or has expired. " + + "If your device is connected you are done; otherwise start the login again." + +var errRegistrationLinkSpent = newHTTPUserError( + http.StatusGone, + registrationLinkSpentMsg, + registrationLinkSpentUserMsg, + nil, +) // registerConfirmURL is the browser-facing URL of the confirmation page. // It is built from server_url, like [AuthProviderOIDC.RegisterURL] and the @@ -930,7 +950,12 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler( newNode, err := a.handleRegistration(user, authID, pending.NodeExpiry) if err != nil { if errors.Is(err, db.ErrNodeNotFoundRegistrationCache) { - httpUserError(writer, NewHTTPError(http.StatusGone, registrationLinkSpentMsg, err)) + httpUserError(writer, newHTTPUserError( + http.StatusGone, + registrationLinkSpentMsg, + registrationLinkSpentUserMsg, + err, + )) return } @@ -1037,27 +1062,32 @@ func getCookieName(baseName, value string) string { return fmt.Sprintf("%s_%s", baseName, value[:n]) } -// clearOIDCCallbackCookie expires a /oidc/callback cookie by name. Matching the -// path the cookie was set with is required for the browser to drop it. -func clearOIDCCallbackCookie(w http.ResponseWriter, name string) { +// clearOIDCCallbackCookie expires an OIDC callback cookie by name. Matching +// the browser-facing path the cookie was set with is required for the browser +// to drop it. +func (a *AuthProviderOIDC) clearOIDCCallbackCookie(w http.ResponseWriter, name string) { //nolint:gosec // G124: a deletion cookie (empty value, MaxAge<0); security attributes are moot http.SetCookie(w, &http.Cookie{ Name: name, - Path: "/oidc/callback", + Path: a.oidcCallbackPath(), MaxAge: -1, }) } -func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string, secure bool) string { +func (a *AuthProviderOIDC) setCSRFCookie( + w http.ResponseWriter, + r *http.Request, + name string, +) string { val := rands.HexString(64) //nolint:gosec // G124: Secure from server_url scheme or req.TLS; HttpOnly + SameSite set below c := &http.Cookie{ - Path: "/oidc/callback", + Path: a.oidcCallbackPath(), Name: getCookieName(name, val), Value: val, MaxAge: int(time.Hour.Seconds()), - Secure: secure || r.TLS != nil, + Secure: a.cookiesSecure() || r.TLS != nil, HttpOnly: true, // Lax, not Strict: the OIDC callback is a cross-site top-level GET // redirect from the IdP that must still carry this cookie. Strict diff --git a/hscontrol/oidc_callback_reload_test.go b/hscontrol/oidc_callback_reload_test.go deleted file mode 100644 index 335ef9f019..0000000000 --- a/hscontrol/oidc_callback_reload_test.go +++ /dev/null @@ -1,336 +0,0 @@ -package hscontrol - -import ( - "context" - "io" - "net/http" - "net/http/cookiejar" - "net/http/httptest" - "net/url" - "regexp" - "strings" - "testing" - - "github.com/juanfont/headscale/hscontrol/types" - "github.com/oauth2-proxy/mockoidc" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "tailscale.com/types/key" -) - -// oidcBrowser drives the interactive OIDC registration flow the way a -// browser does: over real HTTP against the real route table, through a -// cookie jar that honours Path and expiry, following redirects. -// -// Both halves matter for this bug. The route table is where the -// confirmation page's missing GET route lives, and cookie Path decides -// whether the state cookie the callback deletes is actually gone on the -// next hit. -type oidcBrowser struct { - app *Headscale - idp *mockoidc.MockOIDC - srv *httptest.Server - client *http.Client -} - -func newOIDCBrowser(t *testing.T) *oidcBrowser { - t.Helper() - - idp, err := mockoidc.Run() - require.NoError(t, err) - - t.Cleanup(func() { - _ = idp.Shutdown() - }) - - app := createTestApp(t) - - // The provider derives its OIDC redirect_uri from the server URL, and - // the server serves the router the provider is registered on, so bind - // the router late to break the cycle. - var router http.Handler - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - router.ServeHTTP(w, r) - })) - t.Cleanup(srv.Close) - - provider, err := NewAuthProviderOIDC( - context.Background(), - app, - srv.URL, - &types.OIDCConfig{ - Issuer: idp.Issuer(), - ClientID: idp.ClientID, - ClientSecret: idp.ClientSecret, - Scope: []string{"openid", "profile", "email"}, - }, - ) - require.NoError(t, err) - - app.authProvider = provider - router = app.createRouter(nil, nil) - - jar, err := cookiejar.New(nil) - require.NoError(t, err) - - return &oidcBrowser{ - app: app, - idp: idp, - srv: srv, - client: &http.Client{Jar: jar}, - } -} - -// get fetches a URL, follows redirects, and returns the status, the URL -// the browser ended up on, and the page body. -func (b *oidcBrowser) get(t *testing.T, rawURL string) (int, *url.URL, string) { - t.Helper() - - resp, err := b.client.Get(rawURL) //nolint:noctx,bodyclose // test client; closed below - require.NoError(t, err) - - defer resp.Body.Close() - - body, err := io.ReadAll(resp.Body) - require.NoError(t, err) - - return resp.StatusCode, resp.Request.URL, string(body) -} - -// pendingNode mints a pending node registration and returns the URL the -// tailscale client would print for the user to open. -func (b *oidcBrowser) pendingNode(t *testing.T) (types.AuthID, string) { - t.Helper() - - authID := types.MustAuthID() - b.app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ - MachineKey: key.NewMachine().Public(), - NodeKey: key.NewNode().Public(), - Hostname: "reload-victim", - })) - - return authID, b.srv.URL + "/register/" + authID.String() -} - -var ( - csrfInputRe = regexp.MustCompile( - `name="` + registerConfirmCSRFCookie + `"[^>]*value="([^"]+)"`, - ) - formActionRe = regexp.MustCompile(`action="([^"]+)"`) -) - -// TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL reproduces -// https://github.com/juanfont/headscale/issues/3365. -// -// The interactive OIDC flow ends with the browser sitting on the -// confirmation interstitial, waiting for the user to click. Today that -// interstitial is written as the body of the /oidc/callback response, so -// the URL the browser is parked on is the one carrying the single-use -// OAuth authorization code. Reloading it re-enters the callback, which -// has already spent the code and deleted the state cookie, and the error -// page paints over the interstitial. The node is never registered. -// -// Adblock Plus is only the loudest trigger — it calls -// window.location.reload() to apply element-hiding filters. The back -// button, mobile pull-to-refresh and browser prerendering all aim at the -// same URL. -// -// The property asserted here is the one that fixes the whole class, -// stated without naming an implementation: wherever the flow leaves the -// browser, that URL must be free of the authorization code and safe to -// load again. -func TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL(t *testing.T) { - b := newOIDCBrowser(t) - - _, registerURL := b.pendingNode(t) - - status, landed, body := b.get(t, registerURL) - require.Equal(t, http.StatusOK, status, "the login flow must reach a page") - require.Contains(t, body, "Confirm node registration", - "the flow must end on the confirmation interstitial") - - assert.Empty(t, landed.Query().Get("code"), - "the browser must not be left parked on the URL carrying the one-time "+ - "OAuth code; any reload of it re-enters the spent callback") - - reloadedStatus, _, reloadedBody := b.get(t, landed.String()) - - require.Equal(t, http.StatusOK, reloadedStatus, - "reloading the page the flow left the browser on must re-render it") - assert.Contains(t, reloadedBody, "Confirm node registration", - "the reload must show the confirmation interstitial, not an error page") -} - -// TestOIDCLoginCompletesAfterReload is the reporters' scenario end to -// end: the confirmation page is reloaded before the user clicks, and the -// registration must still complete. One deployment measured login -// completion falling from 100% to 61-73% across this exact step. -func TestOIDCLoginCompletesAfterReload(t *testing.T) { - b := newOIDCBrowser(t) - - _, registerURL := b.pendingNode(t) - - status, landed, _ := b.get(t, registerURL) - require.Equal(t, http.StatusOK, status) - - // The spurious reload, on whatever URL the flow parked the browser on. - reloadedStatus, reloadedURL, body := b.get(t, landed.String()) - require.Equal(t, http.StatusOK, reloadedStatus, - "the page the user is sitting on must survive a reload") - - csrf := csrfInputRe.FindStringSubmatch(body) - require.Len(t, csrf, 2, "the reloaded page must still carry a usable confirm form") - - action := formActionRe.FindStringSubmatch(body) - require.Len(t, action, 2, "the reloaded page must still carry a form action") - - confirmURL, err := reloadedURL.Parse(action[1]) - require.NoError(t, err) - - //nolint:noctx,bodyclose // test client; closed below - confirmed, err := b.client.PostForm(confirmURL.String(), url.Values{ - registerConfirmCSRFCookie: {csrf[1]}, - }) - require.NoError(t, err) - - defer confirmed.Body.Close() - - confirmedBody, err := io.ReadAll(confirmed.Body) - require.NoError(t, err) - - require.Equal(t, http.StatusOK, confirmed.StatusCode, - "confirming after a reload must register the node") - assert.Contains(t, strings.ToLower(string(confirmedBody)), "registered", - "the user must get the registration success page") -} - -// TestRegisterConfirmGETIsNotADeadEnd covers the second, independent way -// a registration is lost, reported with no ad blocker involved: the -// confirmation endpoint is POST-only in the route table, so a user who -// refreshes or navigates back to it gets a bare 405 from the router with -// no way to recover, while the pending registration sits in the cache -// unreachable until it expires. -// -// This is a route-table gap, so it can only be observed through the real -// router — calling the handler directly cannot see it. -func TestRegisterConfirmGETIsNotADeadEnd(t *testing.T) { - b := newOIDCBrowser(t) - - authID, registerURL := b.pendingNode(t) - - // Complete the OIDC leg so there is a pending confirmation to render. - status, _, _ := b.get(t, registerURL) - require.Equal(t, http.StatusOK, status) - - confirmStatus, _, body := b.get(t, b.srv.URL+"/register/confirm/"+authID.String()) - - require.NotEqual(t, http.StatusMethodNotAllowed, confirmStatus, - "GET on the confirmation URL must not be a dead end for a user who "+ - "refreshes or goes back") - require.Equal(t, http.StatusOK, confirmStatus, - "the confirmation page must be reachable by GET") - assert.Contains(t, body, "Confirm node registration") -} - -// TestRegisterConfirmNeedsTheCallbackCookie locks the reason the -// confirmation step exists. The node being registered knows its own auth -// ID, so the auth ID alone must never be enough to view the device -// details or to finalise the registration — only the browser that -// completed the OIDC login holds the cookie the callback set, and holding -// it is what authorises the confirm. -// -// Without this, an attacker could hand a victim a /register/{auth_id} -// link for the attacker's own node, let the victim's IdP silently sign -// in, and then confirm the registration themselves under the victim's -// identity. -func TestRegisterConfirmNeedsTheCallbackCookie(t *testing.T) { - b := newOIDCBrowser(t) - - authID, registerURL := b.pendingNode(t) - - status, _, body := b.get(t, registerURL) - require.Equal(t, http.StatusOK, status) - - csrf := csrfInputRe.FindStringSubmatch(body) - require.Len(t, csrf, 2) - - // A second browser that knows the auth ID, and even the token from the - // rendered page, but never completed the OIDC login. - jar, err := cookiejar.New(nil) - require.NoError(t, err) - - attacker := &http.Client{Jar: jar} - confirmURL := b.srv.URL + "/register/confirm/" + authID.String() - - //nolint:noctx,bodyclose // test client; closed below - viewed, err := attacker.Get(confirmURL) - require.NoError(t, err) - - defer viewed.Body.Close() - - assert.Equal(t, http.StatusForbidden, viewed.StatusCode, - "the confirmation page must not render without the callback cookie") - - //nolint:noctx,bodyclose // test client; closed below - submitted, err := attacker.PostForm(confirmURL, url.Values{ - registerConfirmCSRFCookie: {csrf[1]}, - }) - require.NoError(t, err) - - defer submitted.Body.Close() - - assert.Equal(t, http.StatusForbidden, submitted.StatusCode, - "the registration must not finalise without the callback cookie") - - cached, ok := b.app.state.GetAuthCacheEntry(authID) - require.True(t, ok, "the pending registration must survive the attempt") - assert.NotNil(t, cached.PendingConfirmation(), - "the pending registration must still be waiting for the real user") -} - -// TestSetRegisterConfirmCookieSameSite pins SameSite=Lax. Strict is -// withheld by browsers that evaluate the whole redirect chain, and this -// cookie now has to survive the callback's redirect to the confirmation -// page — a chain that begins cross-site at the identity provider. Lax is -// still never attached to a cross-site POST, so the confirm submission -// keeps its protection. -func TestSetRegisterConfirmCookieSameSite(t *testing.T) { - a := &AuthProviderOIDC{serverURL: "https://hs.example.com"} - authID := types.MustAuthID() - - rec := httptest.NewRecorder() - a.setRegisterConfirmCookie(rec, - httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil), - authID, "token", 900) - - cookies := rec.Result().Cookies() - require.Len(t, cookies, 1) - assert.Equal(t, http.SameSiteLaxMode, cookies[0].SameSite, - "the confirm cookie must survive the callback redirect") - assert.True(t, cookies[0].Secure, "https server_url must set Secure") - assert.Equal(t, "/register/confirm/"+authID.String(), cookies[0].Path) -} - -// TestRegisterConfirmURLFollowsServerURLPrefix covers the deployment -// where a reverse proxy serves Headscale under a path prefix. The -// redirect target, the form action and the cookie scope are all seen by -// the browser, so they carry the prefix even though the routed path does -// not. -func TestRegisterConfirmURLFollowsServerURLPrefix(t *testing.T) { - a := &AuthProviderOIDC{serverURL: "https://example.com/hs"} - authID := types.MustAuthID() - - assert.Equal(t, "https://example.com/hs/register/confirm/"+authID.String(), - a.registerConfirmURL(authID)) - - rec := httptest.NewRecorder() - a.setRegisterConfirmCookie(rec, - httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil), - authID, "token", 900) - - cookies := rec.Result().Cookies() - require.Len(t, cookies, 1) - assert.Equal(t, "/hs/register/confirm/"+authID.String(), cookies[0].Path, - "the cookie must be scoped to the path the browser sees") -} diff --git a/hscontrol/oidc_test.go b/hscontrol/oidc_test.go index 04d3316498..c6bbdd2411 100644 --- a/hscontrol/oidc_test.go +++ b/hscontrol/oidc_test.go @@ -1,15 +1,22 @@ package hscontrol import ( + "context" + "io" "net/http" + "net/http/cookiejar" "net/http/httptest" + "net/url" + "regexp" "testing" "time" "github.com/hashicorp/golang-lru/v2/expirable" "github.com/juanfont/headscale/hscontrol/types" + "github.com/oauth2-proxy/mockoidc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "tailscale.com/types/key" ) func TestDoOIDCAuthorization(t *testing.T) { @@ -185,10 +192,11 @@ func TestDoOIDCAuthorization(t *testing.T) { // previously set no SameSite (despite a comment claiming it did), leaving // browsers that do not default to Lax sending it on cross-site requests. func TestSetCSRFCookieSameSite(t *testing.T) { + a := &AuthProviderOIDC{serverURL: "http://hs.example.com"} w := httptest.NewRecorder() r := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/auth/abcdef0123456789", nil) - setCSRFCookie(w, r, "state", false) + a.setCSRFCookie(w, r, "state") cookies := w.Result().Cookies() require.Len(t, cookies, 1) @@ -233,12 +241,14 @@ func TestGetAuthInfoFromStateSingleUse(t *testing.T) { // TestClearOIDCCallbackCookie asserts the cookie is expired (negative MaxAge) on // the same path it was set with, so the browser drops it. func TestClearOIDCCallbackCookie(t *testing.T) { + a := &AuthProviderOIDC{serverURL: "https://hs.example.com/prefix"} w := httptest.NewRecorder() - clearOIDCCallbackCookie(w, "state_abcdef") + a.clearOIDCCallbackCookie(w, "state_abcdef") cookies := w.Result().Cookies() require.Len(t, cookies, 1) assert.Equal(t, "state_abcdef", cookies[0].Name) + assert.Equal(t, "/prefix/oidc/callback", cookies[0].Path) assert.Negative(t, cookies[0].MaxAge, "deletion cookie must have negative MaxAge") } @@ -250,14 +260,358 @@ func TestSetCSRFCookieSecure(t *testing.T) { r := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/auth/abcdef0123456789", nil) secureRec := httptest.NewRecorder() - setCSRFCookie(secureRec, r, "state", true) + secureProvider := &AuthProviderOIDC{serverURL: "https://hs.example.com"} + secureProvider.setCSRFCookie(secureRec, r, "state") require.Len(t, secureRec.Result().Cookies(), 1) assert.True(t, secureRec.Result().Cookies()[0].Secure, "https server_url must set Secure even when req.TLS is nil (proxy case)") plainRec := httptest.NewRecorder() - setCSRFCookie(plainRec, r, "state", false) + plainProvider := &AuthProviderOIDC{serverURL: "http://hs.example.com"} + plainProvider.setCSRFCookie(plainRec, r, "state") require.Len(t, plainRec.Result().Cookies(), 1) assert.False(t, plainRec.Result().Cookies()[0].Secure, "plain-http server_url without req.TLS must not set Secure") } + +// oidcBrowser drives the interactive OIDC registration flow the way a +// browser does: over real HTTP against the real route table, through a +// cookie jar that honours Path and expiry, following redirects. +// +// Both halves matter for this bug. The route table is where the +// confirmation page's missing GET route lives, and cookie Path decides +// whether the state cookie the callback deletes is actually gone on the +// next hit. +type oidcBrowser struct { + app *Headscale + idp *mockoidc.MockOIDC + srv *httptest.Server + publicURL string + client *http.Client +} + +func newOIDCBrowser(t *testing.T) *oidcBrowser { + t.Helper() + + return newOIDCBrowserWithPrefix(t, "") +} + +func newOIDCBrowserWithPrefix(t *testing.T, prefix string) *oidcBrowser { + t.Helper() + + idp, err := mockoidc.Run() + require.NoError(t, err) + + t.Cleanup(func() { + _ = idp.Shutdown() + }) + + app := createTestApp(t) + + // The provider derives its OIDC redirect_uri from the server URL, and + // the server serves the router the provider is registered on, so bind + // the router late to break the cycle. + var router http.Handler + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if prefix == "" { + router.ServeHTTP(w, r) + return + } + + http.StripPrefix(prefix, router).ServeHTTP(w, r) + })) + t.Cleanup(srv.Close) + publicURL := srv.URL + prefix + + provider, err := NewAuthProviderOIDC( + context.Background(), + app, + publicURL, + &types.OIDCConfig{ + Issuer: idp.Issuer(), + ClientID: idp.ClientID, + ClientSecret: idp.ClientSecret, + Scope: []string{"openid", "profile", "email"}, + }, + ) + require.NoError(t, err) + + app.authProvider = provider + router = app.createRouter(nil, nil) + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + return &oidcBrowser{ + app: app, + idp: idp, + srv: srv, + publicURL: publicURL, + client: &http.Client{Jar: jar}, + } +} + +// get fetches a URL, follows redirects, and returns the status, the URL +// the browser ended up on, and the page body. +func (b *oidcBrowser) get(t *testing.T, rawURL string) (int, *url.URL, string) { + t.Helper() + + resp, err := b.client.Get(rawURL) //nolint:noctx,bodyclose // test client; closed below + require.NoError(t, err) + + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + return resp.StatusCode, resp.Request.URL, string(body) +} + +// pendingNode mints a pending node registration and returns the URL the +// tailscale client would print for the user to open. +func (b *oidcBrowser) pendingNode(t *testing.T) (types.AuthID, string) { + t.Helper() + + authID := types.MustAuthID() + b.app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: key.NewMachine().Public(), + NodeKey: key.NewNode().Public(), + Hostname: "reload-victim", + })) + + return authID, b.publicURL + "/register/" + authID.String() +} + +var ( + csrfInputRe = regexp.MustCompile( + `name="` + registerConfirmCSRFCookie + `"[^>]*value="([^"]+)"`, + ) + formActionRe = regexp.MustCompile(`action="([^"]+)"`) +) + +// TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL reproduces +// https://github.com/juanfont/headscale/issues/3365. +// +// Before this regression was fixed, the interactive OIDC flow wrote the +// confirmation interstitial as the body of the /oidc/callback response. +// The browser was therefore parked on the URL carrying the single-use +// OAuth authorization code. Reloading it re-entered the callback, which +// had already spent the code and deleted the state cookie, and the error +// page painted over the interstitial. The node was never registered. +// +// Adblock Plus is only the loudest trigger — it calls +// window.location.reload() to apply element-hiding filters. The back +// button, mobile pull-to-refresh and browser prerendering all aim at the +// same URL. +// +// The property asserted here is the one that fixes the whole class, +// stated without naming an implementation: wherever the flow leaves the +// browser, that URL must be free of the authorization code and safe to +// load again. +func TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL(t *testing.T) { + b := newOIDCBrowser(t) + + _, registerURL := b.pendingNode(t) + + status, landed, body := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status, "the login flow must reach a page") + require.Contains(t, body, "Confirm node registration", + "the flow must end on the confirmation interstitial") + + assert.Empty(t, landed.Query().Get("code"), + "the browser must not be left parked on the URL carrying the one-time "+ + "OAuth code; any reload of it re-enters the spent callback") + + reloadedStatus, _, reloadedBody := b.get(t, landed.String()) + + require.Equal(t, http.StatusOK, reloadedStatus, + "reloading the page the flow left the browser on must re-render it") + assert.Contains(t, reloadedBody, "Confirm node registration", + "the reload must show the confirmation interstitial, not an error page") +} + +// TestOIDCLoginCompletesAfterReload is the reporters' scenario end to +// end: the confirmation page is reloaded before the user clicks, and the +// registration must still complete. One deployment measured login +// completion falling from 100% to 61-73% across this exact step. +func TestOIDCLoginCompletesAfterReload(t *testing.T) { + b := newOIDCBrowser(t) + + authID, registerURL := b.pendingNode(t) + + status, landed, _ := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status) + + // The spurious reload, on whatever URL the flow parked the browser on. + reloadedStatus, reloadedURL, body := b.get(t, landed.String()) + require.Equal(t, http.StatusOK, reloadedStatus, + "the page the user is sitting on must survive a reload") + + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2, "the reloaded page must still carry a usable confirm form") + + action := formActionRe.FindStringSubmatch(body) + require.Len(t, action, 2, "the reloaded page must still carry a form action") + + confirmURL, err := reloadedURL.Parse(action[1]) + require.NoError(t, err) + + //nolint:noctx,bodyclose // test client; closed below + confirmed, err := b.client.PostForm(confirmURL.String(), url.Values{ + registerConfirmCSRFCookie: {csrf[1]}, + }) + require.NoError(t, err) + + defer confirmed.Body.Close() + + confirmedBody, err := io.ReadAll(confirmed.Body) + require.NoError(t, err) + + require.Equal(t, http.StatusOK, confirmed.StatusCode, + "confirming after a reload must register the node") + assert.Contains(t, string(confirmedBody), "Node registered", + "the user must get the registration success page") + + _, cached := b.app.state.GetAuthCacheEntry(authID) + assert.False(t, cached, "a completed registration must consume the auth session") + assert.True(t, b.app.state.ListNodes().ContainsFunc(func(node types.NodeView) bool { + return node.Hostname() == "reload-victim" + }), "the pending node must be persisted after confirmation") +} + +// TestRegisterConfirmGETIsNotADeadEnd covers the second, independent way +// a registration is lost, reported with no ad blocker involved: the +// confirmation endpoint used to be POST-only in the route table, so a +// user who refreshed or navigated back to it got a bare 405 from the +// router with no way to recover, while the pending registration sat in +// the cache unreachable until it expired. +// +// This is a route-table gap, so it can only be observed through the real +// router — calling the handler directly cannot see it. +func TestRegisterConfirmGETIsNotADeadEnd(t *testing.T) { + b := newOIDCBrowser(t) + + authID, registerURL := b.pendingNode(t) + + // Complete the OIDC leg so there is a pending confirmation to render. + status, _, _ := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status) + + confirmStatus, _, body := b.get(t, b.srv.URL+"/register/confirm/"+authID.String()) + + require.NotEqual(t, http.StatusMethodNotAllowed, confirmStatus, + "GET on the confirmation URL must not be a dead end for a user who "+ + "refreshes or goes back") + require.Equal(t, http.StatusOK, confirmStatus, + "the confirmation page must be reachable by GET") + assert.Contains(t, body, "Confirm node registration") +} + +// TestRegisterConfirmNeedsTheCallbackCookie locks the reason the +// confirmation step exists. The node being registered knows its own auth +// ID, so the auth ID alone must never be enough to view the device +// details or to finalise the registration — only the browser that +// completed the OIDC login holds the cookie the callback set, and holding +// it is what authorises the confirm. +// +// Without this, an attacker could hand a victim a /register/{auth_id} +// link for the attacker's own node, let the victim's IdP silently sign +// in, and then confirm the registration themselves under the victim's +// identity. +func TestRegisterConfirmNeedsTheCallbackCookie(t *testing.T) { + b := newOIDCBrowser(t) + + authID, registerURL := b.pendingNode(t) + + status, _, body := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status) + + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2) + + // A second browser that knows the auth ID, and even the token from the + // rendered page, but never completed the OIDC login. + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + attacker := &http.Client{Jar: jar} + confirmURL := b.srv.URL + "/register/confirm/" + authID.String() + + //nolint:noctx,bodyclose // test client; closed below + viewed, err := attacker.Get(confirmURL) + require.NoError(t, err) + + defer viewed.Body.Close() + + assert.Equal(t, http.StatusForbidden, viewed.StatusCode, + "the confirmation page must not render without the callback cookie") + + //nolint:noctx,bodyclose // test client; closed below + submitted, err := attacker.PostForm(confirmURL, url.Values{ + registerConfirmCSRFCookie: {csrf[1]}, + }) + require.NoError(t, err) + + defer submitted.Body.Close() + + assert.Equal(t, http.StatusForbidden, submitted.StatusCode, + "the registration must not finalise without the callback cookie") + + cached, ok := b.app.state.GetAuthCacheEntry(authID) + require.True(t, ok, "the pending registration must survive the attempt") + assert.NotNil(t, cached.PendingConfirmation(), + "the pending registration must still be waiting for the real user") +} + +// TestSetRegisterConfirmCookieSameSite pins SameSite=Lax. Strict is +// withheld by browsers that evaluate the whole redirect chain, and this +// cookie now has to survive the callback's redirect to the confirmation +// page — a chain that begins cross-site at the identity provider. Lax is +// still never attached to a cross-site POST, so the confirm submission +// keeps its protection. +func TestSetRegisterConfirmCookieSameSite(t *testing.T) { + a := &AuthProviderOIDC{serverURL: "https://hs.example.com"} + authID := types.MustAuthID() + + rec := httptest.NewRecorder() + a.setRegisterConfirmCookie(rec, + httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil), + authID, "token", 900) + + cookies := rec.Result().Cookies() + require.Len(t, cookies, 1) + assert.Equal(t, http.SameSiteLaxMode, cookies[0].SameSite, + "the confirm cookie must survive the callback redirect") + assert.True(t, cookies[0].Secure, "https server_url must set Secure") + assert.Equal(t, "/register/confirm/"+authID.String(), cookies[0].Path) +} + +// TestRegisterConfirmURLFollowsServerURLPrefix covers the deployment +// where a reverse proxy serves Headscale under a path prefix. The +// redirect target, the form action and the cookie scope are all seen by +// the browser, so they carry the prefix even though the routed path does +// not. +func TestRegisterConfirmURLFollowsServerURLPrefix(t *testing.T) { + b := newOIDCBrowserWithPrefix(t, "/hs") + authID, registerURL := b.pendingNode(t) + + status, landed, body := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status, + "the prefixed callback must receive its state and nonce cookies") + assert.Equal(t, "/hs/register/confirm/"+authID.String(), landed.Path) + assert.Contains(t, body, "Confirm node registration") +} + +func TestRouterMethodNotAllowedIncludesAllow(t *testing.T) { + b := newOIDCBrowser(t) + + //nolint:noctx,bodyclose // test client; closed below + resp, err := b.client.PostForm(b.srv.URL+"/health", nil) + require.NoError(t, err) + + defer resp.Body.Close() + + assert.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode) + assert.Equal(t, []string{http.MethodGet}, resp.Header.Values("Allow")) +} From 639da23237f961c914b9e849f540ff567dddb92c Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 4 Sep 2026 13:30:20 +0000 Subject: [PATCH 3/3] CHANGELOG: note OIDC confirmation reload fix Updates #3365 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 479b533358..fe3c3e17ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,10 @@ keys remain all-access. - Expiring or deleting a non-existent pre-auth key now returns an error instead of silently succeeding [#3324](https://github.com/juanfont/headscale/pull/3324) - Improve systemd service file hardening [#3341](https://github.com/juanfont/headscale/pull/3341) +- Fix interactive OIDC login when the confirmation page is reloaded by an ad + blocker, back navigation, or pull-to-refresh. The confirmation page now has + its own URL, keeping single-use authorization codes out of reloads + [#3448](https://github.com/juanfont/headscale/pull/3448) - Headscale now requires Go 1.27 to build - Fix extra-records filewatcher hanging on shutdown after the watched file is deleted, and leaking the watcher when setup fails [#3437](https://github.com/juanfont/headscale/pull/3437)