oidc: harden reloadable confirmation flow

Updates #3365

(cherry picked from commit 10dd38fcef)

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
+		return "Your session has expired. Please try again."
-		// 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:
@@ -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
+	Code    int    // HTTP response code to send to client; 0 means 500
-	Msg  string // Response body to send to client
+	Msg     string // Response body to send to non-browser clients
-	Err  error  // Detailed error to log on the server
+	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(

hscontrol/handlers_test.go

@@ -135,9 +135,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),

hscontrol/oidc.go

@@ -96,7 +96,7 @@ func NewAuthProviderOIDC(
 		ClientID:     cfg.ClientID,
 		ClientSecret: cfg.ClientSecret,
 		Endpoint:     oidcProvider.Endpoint(),
-		RedirectURL:  strings.TrimSuffix(serverURL, "/") + "/oidc/callback",
+		RedirectURL:  oidcCallbackURL(serverURL),
 		Scopes:       cfg.Scope,
 	}
 
@@ -126,6 +126,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 fmt.Sprintf(
 		"%s/auth/%s",
@@ -171,10 +183,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,
@@ -274,8 +286,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)
+	a.clearOIDCCallbackCookie(writer, stateCookieName)
-	clearOIDCCallbackCookie(writer, nonceCookieName)
+	a.clearOIDCCallbackCookie(writer, nonceCookieName)
 
 	nodeExpiry := a.determineNodeExpiry(idToken.Expiry)
 
@@ -675,7 +687,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
@@ -936,7 +956,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
 		}
@@ -1043,27 +1068,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
+// clearOIDCCallbackCookie expires an OIDC callback cookie by name. Matching
-// path the cookie was set with is required for the browser to drop it.
+// the browser-facing path the cookie was set with is required for the browser
-func clearOIDCCallbackCookie(w http.ResponseWriter, name string) {
+// 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

hscontrol/oidc_callback_reload_test.go

@@ -1,337 +0,0 @@
-package hscontrol
-
-import (
-	"context"
-	"io"
-	"net/http"
-	"net/http/cookiejar"
-	"net/http/httptest"
-	"net/url"
-	"regexp"
-	"strings"
-	"testing"
-
-	grpcRuntime "github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
-	"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(grpcRuntime.NewServeMux())
-
-	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")
-}

hscontrol/oidc_test.go

@@ -1,15 +1,23 @@
 package hscontrol
 
 import (
+	"context"
+	"io"
 	"net/http"
+	"net/http/cookiejar"
 	"net/http/httptest"
+	"net/url"
+	"regexp"
 	"testing"
 	"time"
 
+	grpcRuntime "github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
 	"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 +193,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 +242,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 +261,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(grpcRuntime.NewServeMux())
+
+	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"))
+}