Skip to content

fix(server): clarify the error when a login request is double-submitted - #4966

Open
pujitha24 wants to merge 1 commit into
dexidp:masterfrom
pujitha24:auto/issue-4451
Open

fix(server): clarify the error when a login request is double-submitted#4966
pujitha24 wants to merge 1 commit into
dexidp:masterfrom
pujitha24:auto/issue-4451

Conversation

@pujitha24

Copy link
Copy Markdown

Overview

Clarifies the error returned when a login or consent request is double-submitted (e.g. via double-click or a stale back-button resubmit), so the user sees a 400 with an explanatory message instead of an opaque 500 "Login error."

What this PR does / why we need it

Double-clicking the login/consent submit button, or resubmitting a stale page reached via the browser back button, sends a duplicate request for the same AuthRequest. The first submission completes normally and consumes the AuthRequest; by the time the duplicate reaches finalizeLogin, storage.UpdateAuthRequest fails with storage.ErrNotFound because the row is already gone. Previously, that error was wrapped with fmt.Errorf("...: %v", err), which discarded the error's identity, so every caller rendered the same generic 500 "Login error." regardless of cause.

Changes:

  • server/authflow/finalize.go: wrap the UpdateAuthRequest error with %w instead of %v so errors.Is(err, storage.ErrNotFound) works on the error finalizeLogin returns.
  • server/authflow/errors.go: add ErrMsgRequestAlreadyCompleted, a user-facing message explaining the request is no longer valid (already completed, or expired) rather than a generic failure.
  • server/authflow/callback.go and server/authflow/password.go (both call sites, including the SPNEGO branch): when finalizeLogin fails with errors.Is(err, storage.ErrNotFound), render 400 with the new message instead of 500 "Login error." This matches the existing convention in this package, where a missing AuthRequest is already treated as a 400 client-side condition elsewhere (e.g. callback.go's and password.go's GetAuthRequest checks, and consent.go's approval handler).
  • server/authflow/sessionlogin.go was deliberately left untouched: its UpdateAuthRequest failure path silently falls back rather than rendering a terminal error to the user, so it isn't affected by this failure mode.

User-visible behavior is otherwise unchanged: a double-submitted login still fails, and no session or auth request is recovered. The narrower, concrete improvement is that the response is now a 400 with an explanation the user can act on, instead of an opaque 500 that looks like a server-side database bug.

Closes #4451

Special notes for your reviewer

  • A maintainer confirmed this exact failure mode on the issue and noted that, short of the larger session-based rework tracked separately, clarifying the error would be worthwhile.
  • Reproduced the failure locally with a new unit test, TestFinalizeLoginAlreadyFinalized in server/authflow/finalize_test.go, which calls finalizeLogin against an AuthRequest that was never created in storage (simulating a duplicate submission after the first one already consumed it) and asserts the returned error still satisfies errors.Is(err, storage.ErrNotFound). Confirmed this test fails without the %v -> %w change and passes with it.
  • Ran go build ./..., go test ./server/... -race, and golangci-lint run ./server/authflow/.... All passed, including the full existing server/authflow suite (no regressions) and go vet.

AI assistance: this change was drafted with Claude Code.

Motivation:
Double-clicking the login/consent submit button, or resubmitting a
stale page reached via the browser back button, sends a duplicate
request for the same AuthRequest. The first submission completes
normally and consumes the AuthRequest; by the time the duplicate
reaches finalizeLogin, storage.UpdateAuthRequest fails with
storage.ErrNotFound because the row is already gone. Before this
change, that error was wrapped with fmt.Errorf("...: %v", err),
which discards the error's identity, so every caller rendered the
same generic 500 "Login error." regardless of cause. A maintainer
confirmed this exact failure mode on the issue and noted that,
short of the larger session-based rework tracked separately,
clarifying the error would be worthwhile.

Approach:
- server/authflow/finalize.go: wrap the UpdateAuthRequest error with
  %w instead of %v so errors.Is(err, storage.ErrNotFound) works on
  the error finalizeLogin returns.
- server/authflow/errors.go: add ErrMsgRequestAlreadyCompleted, a
  user-facing message explaining the request is no longer valid
  (already completed, or expired) rather than a generic failure.
- server/authflow/callback.go and server/authflow/password.go (both
  call sites, including the SPNEGO branch): when finalizeLogin fails
  with errors.Is(err, storage.ErrNotFound), render 400 with the new
  message instead of 500 "Login error." This matches the existing
  convention in this package, where a missing AuthRequest is already
  treated as a 400 client-side condition elsewhere (e.g. callback.go's
  and password.go's GetAuthRequest checks, and consent.go's approval
  handler).
- server/authflow/sessionlogin.go was deliberately left untouched: its
  UpdateAuthRequest failure path silently falls back rather than
  rendering a terminal error to the user, so it isn't affected by this
  failure mode.

User-visible behavior is otherwise unchanged: a double-submitted login
still fails, and no session or auth request is recovered. The
narrower, concrete improvement is that the response is now a 400 with
an explanation the user can act on, instead of an opaque 500 that
looks like a server-side database bug.

Validation:
Reproduced the failure locally with a new unit test,
TestFinalizeLoginAlreadyFinalized in server/authflow/finalize_test.go,
which calls finalizeLogin against an AuthRequest that was never
created in storage (simulating a duplicate submission after the first
one already consumed it) and asserts the returned error still
satisfies errors.Is(err, storage.ErrNotFound). Confirmed this test
fails without the %v -> %w change and passes with it.

Ran:
  go build ./...
  go test ./server/... -race
  golangci-lint run ./server/authflow/...
All passed, including the full existing server/authflow suite (no
regressions) and go vet.

Report: dexidp#4451
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@pujitha24

Copy link
Copy Markdown
Author

Just checking in — this is rebased on master and green on checks, let me know if there'''s anything I can do to make review easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Double-clicking the access button on the dex login confirmation authorization page results in an Internal Server Error Database error.

1 participant