fix: security and reliability pass over cookies, token renewal and logout by me-cedric · Pull Request #290 · sevensolutions/traefik-oidc-auth

me-cedric · 2026-08-13T00:38:30Z

Summary

First off, thank you to everyone who has worked on this plugin. I use it in my
own stack every day and it just quietly does its job, which is the highest
compliment I can pay a piece of auth infrastructure. I went looking for ways to
give something back, so I spent some time reading the code with a critical eye.
Here's what I found.

They're independent of each other and each has its own commit, so anything you
disagree with can be dropped without touching the rest.

Security

The .Chunks cookie is client controlled and was used as a loop bound.
A request with TraefikOidcAuth.Session.Chunks: 2000000000 made
clearChunkedCookie write that many Set-Cookie headers, and every request
without a valid session reaches that code. The count is range checked now, the
writer refuses to produce more chunks than the reader accepts (that mismatch
would otherwise be a permanent login loop), and clearing works off the cookies
actually on the request so a client with a broken cookie can recover.
JWKS key lookups ran without the lock. JwksHandler has an RWMutex but
only the reload path took it, so validating a token while the cache was being
replaced was a data race. go test -race reproduces it with the added test.
Logout leaked the id token when the provider has no end_session_endpoint.
Parsing an empty endpoint gives an empty url, so the redirect became a relative
?client_id=...&id_token_hint=... pointing back at /logout. That loops, and
puts the id token in the address bar and the access logs. There's a local
logout fallback now, and the session cookie is cleared before leaving for the
IDP so an abandoned logout still ends the local session.
An unknown TokenValidation panicked. It was only noticed in the callback,
where it called .Error() on a nil error. Rejected at config load now.
The provider http client had no timeout, so an IDP that accepts a
connection and never answers holds a traefik goroutine indefinitely.

Reliability

renewToken had drifted from exchangeAuthCode: it sent resources instead
of resource (RFC 8707) and never sent the client assertion, so refreshing was
broken for ClientJwtPrivateKey setups.
Tokens are renewed at TokenRenewalThreshold of their lifetime, so the current
one is normally still valid. A failing refresh dropped the session anyway and
bounced the user to the IDP, which is what Issues with HomePage App #257 describes: the page breaks and
a reload logs you straight back in. A temporary failure now keeps the
session; a 4xx from the token endpoint still ends it immediately, so a revoked
refresh token doesn't buy extra time. The failed attempt is recorded on the
session so a struggling IDP doesn't get one POST per request.
New Provider.ClockSkewTolerance for the "token is not valid yet" errors in
🐛 Bug: "Returned token is not valid" #236. It defaults to 0, so nothing changes unless you set it, see the
question below.
When the provider doesn't return the token TokenValidation selects, the error
was token contains an invalid number of segments. It now names the missing
token and mentions the openid scope (🐛 Bug: Returned token is not valid with Pocket-ID >v2.9 #287).
A request with no session cookie logged an info line per request (also Issues with HomePage App #257).
That's debug now, but a truncated chunked cookie, which is the usual symptom
of hitting a cookie size limit, is reported separately instead of being lumped
in with it.

Smaller things

Header templates were re-parsed on every request for every header: the
Template cache was written to the range copy. They're parsed at config load
now, so a broken template also shows up at startup instead of being rendered
into an upstream header.
Introspection decoded the response body whatever the status code was.
The error page wrote the response twice when rendering failed.
CI ran go test in ./src, which only builds that one package, so the tests
under src/oidc, src/rules and src/utils were never executed. Now
go test -race ./....

Related: #236, #257, #287. I don't think any of them should be auto-closed.
#236 needs the reporter to confirm the new option helps, and #287 also asked for
a docs note that you've already added.

Worth your opinion

ClockSkewTolerance default. I set it to 0 on purpose: shipping a
relaxed default inside a hardening change would loosen token validation for
everyone who upgrades without asking for it. Most OIDC libraries default to
30-60s though, and 🐛 Bug: "Returned token is not valid" #236 only gets fixed for people who read the docs. Happy to
flip it to 30 if you'd rather.
Two upgrade notes, if you keep a changelog: refresh requests now carry
resource when RequestedResources is set (a provider that allows it on the
auth code grant but not the refresh grant would start rejecting refreshes), and
an invalid TokenValidation now fails middleware construction instead of every
request, which takes the router to 503 for anyone currently misconfigured.

Not included

I left clearLegacyCodeVerifierCookies alone (🚀 FR: Remove clearLegacyCodeVerifierCookies #288). It hasn't shipped in a
release yet, so removing it now would defeat the migration it was added for.
The OIDC state isn't bound to the browser, so a login CSRF is possible. The
fix needs a cookie, which cuts against the direction of fix(pkce): store encrypted verifier in OIDC state #283, so it seemed like
your call rather than something to slip into this PR.

Test plan

cd src && go test -race ./...
go vet ./..., gofmt -l .
govulncheck ./..., no vulnerabilities, dependencies are current
task test:e2e, 19/19 passing against Keycloak with this branch loaded
into traefik, so it's gone through Yaegi and not just the Go compiler
A real login/logout round trip against a provider with no
end_session_endpoint. Keycloak has one, so the fallback branch is only
covered by unit tests.

AI usage

Per AI_POLICY.md: a significant portion of this was written with Claude
Opus 5 (Claude Code, "ultracode" mode). What most affected quality and what was
actually found: two independent adversarial review passes over the diff, one
general code review and one security review. That's where the
renewal-after-revocation problem, the write-side chunk bound, the upstream header
regression and the CI gap came from, and those are the last six commits.
Verification throughout was go vet, gofmt, go test -race ./...,
govulncheck and the e2e suite. Every line has been read and is defensible; the
two open questions above are the places where I'd genuinely like your judgement
rather than mine.

The Chunks-cookie is client controlled and its value was used directly as a loop bound, so a request carrying `TraefikOidcAuth.Session.Chunks` with a huge number made clearChunkedCookie write that many Set-Cookie headers. Any unauthenticated request hits that path. The count is now range checked when reading, and clearing no longer trusts it at all: it expires the chunk cookies that are actually on the request, so a client that already has a broken cookie can still recover.

JwksHandler already carries an RWMutex but only the reload path ever took it. Key lookups walked RsaKeys/EcdsaKeys unlocked, so a token validation running while the JWKS cache was being replaced was a data race. Reproducible with `go test -race` via the new test.

renewToken had drifted apart from exchangeAuthCode in two ways: - it sent `resources`, while RFC 8707 (and the auth code exchange) use `resource`, so RequestedResources were dropped on every refresh and the renewed token could come back with the wrong audience - it never sent the client assertion, so a provider configured with ClientJwtPrivateKey rejected every refresh

An unknown TokenValidation value used to slip through until a user came back from the IDP, and the callback then dereferenced a nil error while reporting it, so the whole request panicked instead of failing. Checking the value while the config is loaded surfaces the typo in the traefik logs right away. The callback keeps a proper error path for it.

Parsing an empty end_session_endpoint yields an empty url, so the logout redirect became a relative "?client_id=...&id_token_hint=..." pointing back at /logout. That loops, and it puts the id token in the browser address bar and in every access log on the way. Logout now clears the session cookie before leaving, and falls back to that local logout plus the post logout redirect when the provider has no such endpoint. Clearing up front also means a logout that never comes back from the IDP still ends the local session.

Tokens are renewed once they reach TokenRenewalThreshold of their lifetime, so the access token is normally still valid at that point. A failing refresh dropped the session anyway and sent the user back to the IDP, which is what sevensolutionsGH-257 describes: the page breaks, a reload logs you straight back in. If the current token still validates, a failed renewal is now logged and the request continues with it. A refresh that fails on an already invalid token is still an error.

Traefik and the IDP rarely agree on the exact second, and a token issued a moment "in the future" was rejected with "token is not valid yet" (sevensolutionsGH-236). The same happens the other way around for a token that just expired. Tokens are now validated with 30 seconds of leeway on exp, nbf and iat, adjustable through the new Provider.ClockSkewTolerance option.

When a provider leaves out the token that TokenValidation selects, the only hint was "token is malformed: token contains an invalid number of segments", which sends people looking in the wrong place. sevensolutionsGH-287 ran into it after Pocket ID started requiring the openid scope for an id_token. The log now names the missing token and points at the scope.

A request that simply carries no session cookie yet is the normal case for anything public, and logging one "Verifying token: unable to read session cookie: named cookie not present" line per request drowns out the rest of the log (mentioned in sevensolutionsGH-257). It's a debug message now. The wrapping also ran the message through strings.TrimLeft with a cutset, which eats leading characters rather than the "http: " prefix it was meant to strip, so that's gone too.

HeaderConfig has a Template field to cache the parsed template, but attachHeaders ranged over the headers by value, so the parsed template was written to a copy and thrown away. Every configured header was re-parsed on every single request. They're parsed while the config is loaded now, which also means a broken template shows up in the traefik log at startup instead of being rendered into the upstream header. The Values-branch no longer overwrites a template error with the json error that follows it either.

The response body was decoded whatever the status code was, so an error payload from the provider was read as an introspection result. A body that happens to carry "active" would then decide the outcome.

WriteError logged the render failure, wrote a 500 and then carried on to write the status code and an empty body on top of it, which makes go log a superfluous WriteHeader call.

It can't fail anymore since it stopped reading the chunk count, and no caller ever looked at it.

Keeping the session on any failed renewal was too generous. renewToken returns the same opaque error for a 502 and for a 400 invalid_grant, so a refresh token that the provider deliberately rejected -- revoked, logged out at the IDP, account disabled -- kept the old tokens working until they expired, up to 25% of their lifetime. A 4xx from the token endpoint is now a definitive rejection and ends the session as before. Only a transient failure keeps the session, and the attempt is recorded so the next requests don't hammer the token endpoint for the rest of the token's lifetime. Also returns an error instead of a nil session when the session storage has nothing for a ticket, which the cookie storage never does but an external one could, and the caller dereferences the error.

The http client talking to the IDP had no timeout, so a provider that accepts a connection and then never answers holds on to a traefik goroutine for as long as it likes. Discovery, token exchange, refresh, introspection and userinfo all go through it.

Two loose ends from the chunk count fix. The reader rejects a chunk count above the maximum but the writer had no such limit, so a session ticket over 32 chunks would be written at login and then rejected on every request after it: a login loop where it used to work. Storing it now fails with a clear message instead. Clearing walked every cookie on the request, which is attacker controlled too. A megabyte of chunk shaped cookies produced tens of thousands of Set-Cookie headers on a request that never reaches the upstream service.

A Values-template that fails to render, or renders something that isn't a json array, used to end up setting the header and then deleting it again. My earlier change made the error text stick, so the upstream service saw eg. "X-Roles: invalid character 'n' looking for beginning of value". The header is dropped and the reason logged instead. Also guards against a missing parsed template rather than trusting New to have filled it in, and reports a truncated chunked session cookie as such: it was returning the same "no cookie" sentinel as a request with no session at all, which now hides the usual symptom of hitting a cookie size limit behind debug logging.

`go test` in ./src only builds the src package, so the tests under src/oidc, src/rules and src/utils were never executed by CI or by `task test:unit`. The JWKS locking test added here also only shows the problem it guards against under -race.

Defaulting the tolerance to 30s would have relaxed token validation for everyone who upgrades without asking for it, which doesn't belong in this change. It defaults to 0 now, so nothing changes unless you set it. Also dropped `iat` from the docs, the parser never validates it. The logout diagram still showed the cookie being cleared at the callback, and said nothing about a provider without an end_session_endpoint. Tests: the logout tests now use a chunked session cookie and check it is gone on both paths, and cover the redirect_uri validation branch.

sevensolutions · 2026-08-14T09:35:51Z

Hi @me-cedric,
first of all, thx for this PR and for the compliments ❤️.
I really wish you would have separated this into multiple smaller PRs which would make it much simpler to review it.
I've read through the description and some of the things you (or AI :P) has found look very interesting and should definitely be addressed.
I will try to review it will take some time...

Copilot

Pull request overview

This PR is a security and reliability hardening pass over the traefik-oidc-auth plugin, focused on chunked session cookies, token renewal behavior, and the logout flow. It addresses several reported issues (#236, #257, #287) by making token validation more tolerant of clock skew, keeping sessions alive through transient refresh failures, and fixing a logout path that could leak the id token when a provider lacks an end_session_endpoint.

Changes:

Hardens chunked-cookie handling: range-checks the client-controlled .Chunks count, bounds the writer/reader/clear paths, and clears cookies based on what's actually on the request.
Reworks token renewal to survive temporary IDP failures (with a per-session retry throttle), rejects sessions on 4xx, and aligns renewToken with exchangeAuthCode (resource, client assertion); adds ClockSkewTolerance.
Fixes logout to clear the local session first and fall back gracefully without an end_session_endpoint; adds a JWKS read lock, parses header templates at config load, and fixes CI to run go test -race ./....

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`src/cookie.go`	Bounds chunk counts, rejects oversized values, clears only present chunks.
`src/session.go`	Adds renewal retry throttle, sentinel errors, missing-token error, cookie-store error handling.
`src/oidc.go`	Aligns `renewToken` with auth-code exchange, adds clock-skew leeway, checks introspection status, adds refresh-rejected sentinel.
`src/main.go`	Header-template safety net, logout local fallback, callback token validation switch, quieter no-session logging.
`src/config.go`	Validates `TokenValidation`/`ClockSkewTolerance`, pre-parses header templates, sets HTTP client timeout.
`src/config/config.go`	Adds `ClockSkewTolerance` field.
`src/session/sessionStorage.go`	Adds `RenewalFailedAt` to session state.
`src/oidc/jwks.go`	Takes read lock in `getRsaKey`/`getEcdsaKey`.
`src/errorPages/errorPage.go`	Returns after error to avoid double write.
`src/*_test.go`, `src/config_test.go`, `src/oidc/jwks_test.go`	New tests covering the above behaviors.
`taskfile.yml`, `.github/workflows/testing.yaml`	Run `go test -race ./...` across all packages.
`website/docs/*.md`	Documents `ClockSkewTolerance` and the logout fallback.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+		if err == errNoSessionCookie || err == errNoSession {
+			toa.logger.Log(logging.LevelDebug, "No session is present for the request.")
+		} else {
+			toa.logger.Log(logging.LevelInfo, "Verifying token: %s", err.Error())
+		}


me-cedric · 2026-08-14T10:50:50Z

hey, thanks for the reply. yeah it's mostly IA with a relevant subset of skills. not great I know.
I can split this into sub PRs if you prefer, with more specific subjects in mind !

sevensolutions · 2026-08-14T18:10:45Z

Hey,

I can split this into sub PRs if you prefer, with more specific subjects in mind !

Yes please. Splitting it would make the review process much easier ♥️.
You can mark the current PR as draft and keep it open for reference for now if you want.

me-cedric · 2026-08-15T10:23:52Z

Split up as you asked, and marked this one as a draft for reference. Six PRs, each cut from main and independent of the others, so they can be reviewed, merged, deferred or rejected in any order:

PR	What
#291	Session cookie chunking. `.Chunks` is client controlled and was used as a loop bound; range checked on read, bounded on write, and clearing works off the cookies actually present.
#292	Logout without an `end_session_endpoint`. Parsing an empty endpoint produced a relative redirect back to `/logout` carrying the id token. Local logout fallback, and the session cookie is cleared before leaving for the IDP.
#293	Requests to the provider. JWKS lookups took no read lock (data race), the http client had no timeout, introspection ignored the status code, and `renewToken` had drifted from `exchangeAuthCode`. Includes the CI change, since `go test` in `./src` never built `src/oidc` at all.
#294	Token renewal and lifetime. A transient refresh failure no longer drops a still-valid session, a 4xx still ends it immediately, and `ClockSkewTolerance` is added opt-in. Related: #257, #236.
#295	Configuration checked at load. Unknown `TokenValidation` (was a panic on the callback), header templates parsed once instead of per request, and a template error dropped instead of forwarded upstream. Related: #287.
#296	Log noise. A request with no session is a debug line now, with a truncated cookie and a missing stored session told apart from it rather than swept in.

Notes:

The two upgrade notes from the description above travel with the PR they belong to: resource on refresh is in fix: harden the requests to the provider and the JWKS cache #293, TokenValidation failing middleware construction is in fix: validate TokenValidation and the header templates when the config is loaded #295.
The open question about the ClockSkewTolerance default is in fix: survive a transient token renewal failure, add an opt-in ClockSkewTolerance #294. It still ships at 0.
@Copilot's review comment on this PR was right — the errNoSession branch was unreachable because the sentinel got re-wrapped through fmt.Errorf before it reached the check. Fixed in fix: report a request without a session at debug level #296, with a test.
Whichever lands first will make the others need a rebase; the overlaps are in src/session_test.go, src/config.go's New() and renewToken, and they're all mechanical. I'll keep them rebased.
Nothing was dropped in the split: I merged all six back together and diffed the result against this branch, and the only differences are the Copilot fix above and its test.

me-cedric added 19 commits August 13, 2026 01:56

fix: check the status code of the introspection response

a4ce7b8

The response body was decoded whatever the status code was, so an error payload from the provider was read as an introspection result. A body that happens to carry "active" would then decide the outcome.

fix: stop writing the error page twice when rendering fails

a10d24d

WriteError logged the render failure, wrote a 500 and then carried on to write the status code and an empty body on top of it, which makes go log a superfluous WriteHeader call.

chore: drop the unused error return of clearChunkedCookie

940e186

It can't fail anymore since it stopped reading the chunk count, and no caller ever looked at it.

sevensolutions requested a balanced review from Copilot August 14, 2026 09:37

Copilot started reviewing on behalf of sevensolutions August 14, 2026 09:37 View session

Copilot AI reviewed Aug 14, 2026

View reviewed changes

Comment thread src/main.go

Comment on lines +182 to +186

if err == errNoSessionCookie || err == errNoSession {

toa.logger.Log(logging.LevelDebug, "No session is present for the request.")

} else {

toa.logger.Log(logging.LevelInfo, "Verifying token: %s", err.Error())

}

me-cedric marked this pull request as draft August 15, 2026 09:51

This was referenced Aug 15, 2026

fix: validate TokenValidation and the header templates when the config is loaded #295

Merged

fix: report a request without a session at debug level #296

Merged

me-cedric mentioned this pull request Aug 27, 2026

feat: add an opt-in ClockSkewTolerance for token validation #300

Open

Navigation Menu

Uh oh!

fix: security and reliability pass over cookies, token renewal and logout - #290

fix: security and reliability pass over cookies, token renewal and logout#290
me-cedric wants to merge 19 commits into
sevensolutions:mainfrom
me-cedric:fix/hardening

me-cedric commented Aug 13, 2026

Uh oh!

sevensolutions commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

me-cedric commented Aug 14, 2026

Uh oh!

sevensolutions commented Aug 14, 2026

Uh oh!

me-cedric commented Aug 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

me-cedric commented Aug 13, 2026

Summary

Security

Reliability

Smaller things

Worth your opinion

Not included

Test plan

AI usage

Uh oh!

sevensolutions commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

me-cedric commented Aug 14, 2026

Uh oh!

sevensolutions commented Aug 14, 2026

Uh oh!

me-cedric commented Aug 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants