From 3ce5f428cf42b20c32144010baa781231affb492 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Tue, 3 Nov 2026 01:10:36 +0000 Subject: [PATCH] fix(session): keep old access token on failed store, cap incoming path, fold proto case SetAccessToken cleared the old token before it found that no request was available for the chunk cookies. The old token was lost. The stored post-login path had no length bound and could bloat every later Cookie header. It is now capped at 2048 bytes. DetermineScheme compared X-Forwarded-Proto case-sensitively while the cookie code did not, so HTTPS gave an http redirect_uri. --- internal/utils/utils.go | 2 +- session.go | 15 +++++++++ session_access_token_nilrequest_test.go | 36 ++++++++++++++++++++++ session_incoming_scheme_test.go | 41 +++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 session_access_token_nilrequest_test.go create mode 100644 session_incoming_scheme_test.go diff --git a/internal/utils/utils.go b/internal/utils/utils.go index f6c2a0a..f1f8cf0 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -142,7 +142,7 @@ func DetermineScheme(req *http.Request, forceHTTPS bool) string { // "X-Forwarded-Proto: ftp") would otherwise be emitted verbatim into // redirect_uri as a broken or weakened scheme. Invalid values fall // through to the TLS check (R103). - if scheme := req.Header.Get("X-Forwarded-Proto"); scheme == "https" || scheme == "http" { + if scheme := strings.ToLower(req.Header.Get("X-Forwarded-Proto")); scheme == "https" || scheme == "http" { return scheme } diff --git a/session.go b/session.go index 0fa3066..4ed64e3 100644 --- a/session.go +++ b/session.go @@ -2302,6 +2302,13 @@ func (sd *SessionData) SetAccessToken(token string) { return } + // Chunk cookies bind to the request; without one the store cannot + // complete, so keep the previous token. + if sd.request == nil { + sd.manager.logger.Error("SetAccessToken: sd.request is nil, cannot create chunk sessions") + return + } + // Validation passed: now it is safe to clear the existing token // before storing the new chunked one. sd.clearAccessTokenChunks() @@ -2937,6 +2944,9 @@ func (sd *SessionData) GetIncomingPath() string { return path } +// maxIncomingPathLen bounds the post-login redirect path kept in the session. +const maxIncomingPathLen = 2048 + // SetIncomingPath stores the original request URI for post-authentication redirect. // This allows the user to be redirected to their originally requested resource // after completing the authentication flow. @@ -2945,6 +2955,11 @@ func (sd *SessionData) GetIncomingPath() string { func (sd *SessionData) SetIncomingPath(path string) { sd.sessionMutex.Lock() defer sd.sessionMutex.Unlock() + // The path becomes cookie bytes on every later request; an oversized one + // would bloat the Cookie header or fail the save. Fall back to "/". + if len(path) > maxIncomingPathLen { + path = "" + } currentVal, _ := sd.mainSession.Values["incoming_path"].(string) if currentVal != path { sd.mainSession.Values["incoming_path"] = path diff --git a/session_access_token_nilrequest_test.go b/session_access_token_nilrequest_test.go new file mode 100644 index 0000000..e8f25f7 --- /dev/null +++ b/session_access_token_nilrequest_test.go @@ -0,0 +1,36 @@ +package traefikoidc + +import ( + "crypto/rand" + "encoding/base64" + "net/http/httptest" + "testing" +) + +// A chunked store that cannot run (no request to bind chunk cookies to) must +// leave the previous access token intact. +func TestSetAccessTokenNilRequestKeepsOldToken(t *testing.T) { + sm, err := NewSessionManager("test-encryption-key-32-characters", false, "", "", 0, NewLogger("error")) + if err != nil { + t.Fatal(err) + } + sess, err := sm.GetSession(httptest.NewRequest("GET", "/", nil)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(sess.returnToPoolSafely) + + const old = "smallaccesstokenvaluevaluevalue" + sess.SetAccessToken(old) + + blob := make([]byte, 3000) + if _, err := rand.Read(blob); err != nil { + t.Fatal(err) + } + sess.request = nil + sess.SetAccessToken(base64.StdEncoding.EncodeToString(blob)) + + if got := sess.GetAccessToken(); got != old { + t.Fatalf("old access token lost after a failed chunked store: %q", got) + } +} diff --git a/session_incoming_scheme_test.go b/session_incoming_scheme_test.go new file mode 100644 index 0000000..f9c8070 --- /dev/null +++ b/session_incoming_scheme_test.go @@ -0,0 +1,41 @@ +package traefikoidc + +import ( + "net/http/httptest" + "strings" + "testing" + + "github.com/lukaszraczylo/traefikoidc/internal/utils" +) + +// A deep link longer than the cap must not be persisted into the cookies. +func TestSetIncomingPathCapsLength(t *testing.T) { + sm, err := NewSessionManager("test-encryption-key-32-characters", false, "", "", 0, NewLogger("error")) + if err != nil { + t.Fatal(err) + } + sess, err := sm.GetSession(httptest.NewRequest("GET", "/", nil)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(sess.returnToPoolSafely) + + sess.SetIncomingPath("/ok?a=1") + if got := sess.GetIncomingPath(); got != "/ok?a=1" { + t.Fatalf("short path changed: %q", got) + } + sess.SetIncomingPath("/x?" + strings.Repeat("q", maxIncomingPathLen+1)) + if got := sess.GetIncomingPath(); got != "" { + t.Fatalf("oversized path stored (%d bytes)", len(got)) + } +} + +func TestDetermineSchemeCaseInsensitive(t *testing.T) { + for in, want := range map[string]string{"HTTPS": "https", "Https": "https", "HTTP": "http", "https": "https"} { + r := httptest.NewRequest("GET", "/", nil) + r.Header.Set("X-Forwarded-Proto", in) + if got := utils.DetermineScheme(r, false); got != want { + t.Errorf("X-Forwarded-Proto %q -> %q, want %q", in, got, want) + } + } +}