mirror of
https://github.com/juanfont/headscale.git
synced 2026-07-08 00:50:20 +09:00
oidc: set SameSite=Lax on state/nonce CSRF cookies
setCSRFCookie set no SameSite attribute despite a nolint comment claiming it did, so the OIDC state/nonce cookies relied on the browser default. Set Lax explicitly (Strict would drop the cookie on the cross-site IdP->callback navigation and break login), hardening browsers that do not default to Lax against OIDC login CSRF.
This commit is contained in:
committed by
Kristoffer Dalby
parent
eb57a3a62b
commit
0fdff0c79b
+7
-1
@@ -935,7 +935,7 @@ func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string) (string,
|
||||
return val, err
|
||||
}
|
||||
|
||||
//nolint:gosec // G124: Secure set conditionally via r.TLS; HttpOnly + SameSite already set
|
||||
//nolint:gosec // G124: Secure set conditionally via r.TLS; HttpOnly + SameSite set below
|
||||
c := &http.Cookie{
|
||||
Path: "/oidc/callback",
|
||||
Name: getCookieName(name, val),
|
||||
@@ -943,6 +943,12 @@ func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string) (string,
|
||||
MaxAge: int(time.Hour.Seconds()),
|
||||
Secure: 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
|
||||
// would drop it and break login. Setting it explicitly also stops
|
||||
// pre-Lax-default browsers from sending it on other cross-site
|
||||
// requests.
|
||||
SameSite: http.SameSiteLaxMode,
|
||||
}
|
||||
http.SetCookie(w, c)
|
||||
|
||||
|
||||
@@ -1,9 +1,13 @@
|
||||
package hscontrol
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
|
||||
"github.com/juanfont/headscale/hscontrol/types"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestDoOIDCAuthorization(t *testing.T) {
|
||||
@@ -171,3 +175,22 @@ func TestDoOIDCAuthorization(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestSetCSRFCookieSameSite verifies the OIDC state/nonce CSRF cookies carry an
|
||||
// explicit SameSite=Lax attribute. Lax (not Strict) is required because the
|
||||
// OIDC callback is a cross-site top-level GET navigation from the IdP that must
|
||||
// still carry the cookie — Strict would drop it and break login. The cookie
|
||||
// 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) {
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest(http.MethodGet, "/auth/abcdef0123456789", nil)
|
||||
|
||||
_, err := setCSRFCookie(w, r, "state")
|
||||
require.NoError(t, err)
|
||||
|
||||
cookies := w.Result().Cookies()
|
||||
require.Len(t, cookies, 1)
|
||||
assert.Equal(t, http.SameSiteLaxMode, cookies[0].SameSite,
|
||||
"OIDC CSRF cookie must explicitly set SameSite=Lax")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user