Skip to content

fix(server/authn): Re-order auth flow to prevent side effects on handleCookieAuth #3923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gregfurman merged 2 commits into from
May 18, 2026
+23 −20
Merged

fix(server/authn): Re-order auth flow to prevent side effects on handleCookieAuth #3923

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3f4462
fix(server/authn): Reject missing cookie auth attempt
gregfurman May 15, 2026
5920247
fixup: re-order auth flow to prevent side effects on handleCookieAuth
gregfurman May 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/v1/server/authn/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ func (a *AuthN) handleCookieAuth(c echo.Context) error {

store := a.config.SessionStore

if _, err := c.Cookie(store.GetName()); err != nil {
return forbidden
}
Copy link
Copy Markdown
Contributor

@abelanger5 abelanger5 May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is correct? There are some endpoints where we would like to call Set-Cookie, for example when beginning an oauth flow, we need to store the oauth state parameter.

It's quite possible we don't hit this case on any such endpoints, in which case I wonder why we call SaveUnauthenticated later on (which we do a few lines later). Though it's possible that the error block is there so that we can invalidate a cookie which can no longer be decrypted.

Copy link
Copy Markdown
Collaborator Author

@gregfurman gregfurman May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is correct?

Not entirely 🫠 Since the bug here is that we shouldn't be creating a new UserSession entry when there is no session to be persisted -- which will always occur regardless of the auth type since we assume cookie auth whose fallback has the aforementioned side-effect of a new row being added.

I could rejig the logic here to instead start off assuming bearer auth (which has no side-effects) i.e from cookie -> bearer -> custom to bearer -> custom -> cookie

It's quite possible we don't hit this case on any such endpoints, in which case I wonder why we call SaveUnauthenticated later on (which we do a few lines later).

At a glance, I couldn't really figure out how to induce a fallback to SaveUnauthenticated which makes me think this handler logic deserves a revisit.

For this PR, let me not make any changes to the individual auth handler logic and instead try the aforementioned handler re-ordering. Wdyt?


session, err := store.Get(c.Request(), store.GetName())
ctx := c.Request().Context()
if err != nil {
Expand Down
Loading