diff --git a/hscontrol/handlers.go b/hscontrol/handlers.go index 7693ad945..2a0157cd6 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 668f545a2..f1ed71a09 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 5737cda26..e36678495 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 335ef9f01..000000000 --- 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 04d331649..c6bbdd241 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")) +}