Skip to content

feat: for expired challenges return HTTP 410 and redirect url #3802

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+429 −10
Open

feat: for expired challenges return HTTP 410 and redirect url #3802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 42 additions & 0 deletions consent/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,13 @@ func (h *Handler) getOAuth2LoginRequest(w http.ResponseWriter, r *http.Request,

request, err := h.r.ConsentManager().GetLoginRequest(r.Context(), challenge)
if err != nil {
var challengeExpired *ErrChallengeExpired
if errors.As(err, &challengeExpired) {
h.r.Writer().WriteCode(w, r, http.StatusGone, &flow.OAuth2RedirectTo{
RedirectTo: challengeExpired.RedirectURL,
})
return
}
h.r.Writer().WriteError(w, r, err)
return
}
Expand Down Expand Up @@ -447,6 +454,13 @@ func (h *Handler) acceptOAuth2LoginRequest(w http.ResponseWriter, r *http.Reques
handledLoginRequest.ID = challenge
loginRequest, err := h.r.ConsentManager().GetLoginRequest(ctx, challenge)
if err != nil {
var challengeExpired *ErrChallengeExpired
if errors.As(err, &challengeExpired) {
h.r.Writer().WriteCode(w, r, http.StatusGone, &flow.OAuth2RedirectTo{
RedirectTo: challengeExpired.RedirectURL,
})
return
}
h.r.Writer().WriteError(w, r, err)
return
} else if loginRequest.Subject != "" && handledLoginRequest.Subject != loginRequest.Subject {
Expand Down Expand Up @@ -571,6 +585,13 @@ func (h *Handler) rejectOAuth2LoginRequest(w http.ResponseWriter, r *http.Reques
p.SetDefaults(flow.LoginRequestDeniedErrorName)
ar, err := h.r.ConsentManager().GetLoginRequest(ctx, challenge)
if err != nil {
var challengeExpired *ErrChallengeExpired
if errors.As(err, &challengeExpired) {
h.r.Writer().WriteCode(w, r, http.StatusGone, &flow.OAuth2RedirectTo{
RedirectTo: challengeExpired.RedirectURL,
})
return
}
h.r.Writer().WriteError(w, r, err)
return
}
Expand Down Expand Up @@ -661,6 +682,13 @@ func (h *Handler) getOAuth2ConsentRequest(w http.ResponseWriter, r *http.Request

request, err := h.r.ConsentManager().GetConsentRequest(r.Context(), challenge)
if err != nil {
var challengeExpired *ErrChallengeExpired
if errors.As(err, &challengeExpired) {
h.r.Writer().WriteCode(w, r, http.StatusGone, &flow.OAuth2RedirectTo{
RedirectTo: challengeExpired.RedirectURL,
})
return
}
h.r.Writer().WriteError(w, r, err)
return
}
Expand Down Expand Up @@ -753,6 +781,13 @@ func (h *Handler) acceptOAuth2ConsentRequest(w http.ResponseWriter, r *http.Requ

cr, err := h.r.ConsentManager().GetConsentRequest(ctx, challenge)
if err != nil {
var challengeExpired *ErrChallengeExpired
if errors.As(err, &challengeExpired) {
h.r.Writer().WriteCode(w, r, http.StatusGone, &flow.OAuth2RedirectTo{
RedirectTo: challengeExpired.RedirectURL,
})
return
}
h.r.Writer().WriteError(w, r, errorsx.WithStack(err))
return
}
Expand Down Expand Up @@ -864,6 +899,13 @@ func (h *Handler) rejectOAuth2ConsentRequest(w http.ResponseWriter, r *http.Requ
p.SetDefaults(flow.ConsentRequestDeniedErrorName)
hr, err := h.r.ConsentManager().GetConsentRequest(ctx, challenge)
if err != nil {
var challengeExpired *ErrChallengeExpired
if errors.As(err, &challengeExpired) {
h.r.Writer().WriteCode(w, r, http.StatusGone, &flow.OAuth2RedirectTo{
RedirectTo: challengeExpired.RedirectURL,
})
return
}
h.r.Writer().WriteError(w, r, errorsx.WithStack(err))
return
}
Expand Down
30 changes: 22 additions & 8 deletions consent/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
"testing"
"time"

"github.com/ory/hydra/v2/driver/config"

"github.com/stretchr/testify/require"

hydra "github.com/ory/hydra-client-go/v2"
Expand Down Expand Up @@ -85,11 +87,13 @@ func TestGetLoginRequest(t *testing.T) {
for k, tc := range []struct {
exists bool
handled bool
expired bool
status int
}{
{false, false, http.StatusNotFound},
{true, false, http.StatusOK},
{true, true, http.StatusGone},
{false, false, false, http.StatusNotFound},
{true, false, false, http.StatusOK},
{true, true, false, http.StatusGone},
{true, false, true, http.StatusGone},
} {
t.Run(fmt.Sprintf("exists=%v/handled=%v", tc.exists, tc.handled), func(t *testing.T) {
ctx := context.Background()
Expand All @@ -109,6 +113,10 @@ func TestGetLoginRequest(t *testing.T) {
RequestURL: requestURL,
RequestedAt: time.Now(),
})
if tc.expired {
require.NoError(t, conf.Set(ctx, config.KeyConsentRequestMaxAge, time.Millisecond))
time.Sleep(time.Millisecond * 5)
}
require.NoError(t, err)
challenge, err = f.ToLoginChallenge(ctx, reg)
require.NoError(t, err)
Expand All @@ -132,7 +140,7 @@ func TestGetLoginRequest(t *testing.T) {
require.NoError(t, err)
require.EqualValues(t, tc.status, resp.StatusCode)

if tc.handled {
if tc.handled || tc.expired {
var result flow.OAuth2RedirectTo
require.NoError(t, json.NewDecoder(resp.Body).Decode(&result))
require.Equal(t, requestURL, result.RedirectTo)
Expand All @@ -151,11 +159,13 @@ func TestGetConsentRequest(t *testing.T) {
for k, tc := range []struct {
exists bool
handled bool
expired bool
status int
}{
{false, false, http.StatusNotFound},
{true, false, http.StatusOK},
{true, true, http.StatusGone},
{false, false, false, http.StatusNotFound},
{true, false, false, http.StatusOK},
{true, true, false, http.StatusGone},
{true, false, true, http.StatusGone},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
ctx := context.Background()
Expand Down Expand Up @@ -192,6 +202,10 @@ func TestGetConsentRequest(t *testing.T) {
CSRF: challenge,
LoginChallenge: sqlxx.NullString(lr.ID),
}))
if tc.expired {
require.NoError(t, conf.Set(ctx, config.KeyConsentRequestMaxAge, time.Millisecond))
time.Sleep(time.Millisecond * 5)
}

if tc.handled {
_, err := reg.ConsentManager().HandleConsentRequest(ctx, f, &flow.AcceptOAuth2ConsentRequest{
Expand All @@ -217,7 +231,7 @@ func TestGetConsentRequest(t *testing.T) {
require.NoError(t, err)
require.EqualValues(t, tc.status, resp.StatusCode)

if tc.handled {
if tc.handled || tc.expired {
var result flow.OAuth2RedirectTo
require.NoError(t, json.NewDecoder(resp.Body).Decode(&result))
require.Equal(t, requestURL, result.RedirectTo)
Expand Down
8 changes: 8 additions & 0 deletions consent/strategy_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@ var ErrNoPreviousConsentFound = stderrs.New("no previous OAuth 2.0 Consent could
var ErrNoAuthenticationSessionFound = stderrs.New("no previous login session was found")
var ErrHintDoesNotMatchAuthentication = stderrs.New("subject from hint does not match subject from session")

type ErrChallengeExpired struct {
RedirectURL string
}

func (e *ErrChallengeExpired) Error() string {
return "challenge has expired"
}

func (s *DefaultStrategy) matchesValueFromSession(ctx context.Context, c fosite.Client, hintSubject string, sessionSubject string) error {
obfuscatedUserID, err := s.ObfuscateSubjectIdentifier(ctx, c, sessionSubject, "")
if err != nil {
Expand Down
Loading