Skip to content

Commit dde9063

Browse files
paskalumputun
authored andcommitted
fix(provider): backport "from" redirect validator to v1 (sibling of #275)
The "from" query parameter accepted by oauth1/oauth2/apple/verify login handlers was stored verbatim in the handshake JWT and used as the redirect target after a successful auth handshake with no validation. Any external URL passed as "from" became a 307 redirect after the user completed the real OAuth flow with the legitimate provider — usable for phishing and post-auth landing-page substitution. This is the same vulnerability fixed in v2 by #275; v1 was untouched. This PR ports the validator to v1 with the same opt-in policy: * token.AllowedHosts (interface) + AllowedHostsFunc (adapter), mirroring the existing token.Audience pattern. * Opts.AllowedRedirectHosts threaded through provider.Params, AppleHandler (via embedded Params) and VerifyHandler (own URL + AllowedRedirectHosts fields). * provider.isAllowedRedirect centralises the check; all four redirect call sites (oauth1.go:165, oauth2.go:241, apple.go:395, verify.go:141) gate on it and fall back to the existing JSON user-info response on rejection (with a [WARN] log via redirectHostForLog so attacker- supplied paths/queries do not leak into logs). Default (nil allowlist) is permissive — preserves pre-feature behaviour so existing consumers see no change. Hardening is enabled by setting Opts.AllowedRedirectHosts; passing an AllowedHostsFunc that returns nil restricts redirects to the service URL host only. Hostname comparison is case-insensitive and ignores the default port; non-http(s) schemes (javascript:, data:, ftp:) are rejected. Tests: * TestIsAllowedRedirect — 24 table cases covering permissive default, typed-nil guard, port equivalence, case-insensitivity, scheme rejection, allowlist matching. * TestRedirectHostForLog — 5 cases. * TestOauth2LoginFromRejectsExternalHost / TestOauth2LoginFromAllowsAllowlistedHost — integration coverage of the oauth2 path (negative + positive). * TestVerifyHandler_LoginAcceptConfirmFromRejectsExternalHost / TestVerifyHandler_LoginAcceptConfirmFromAllowsAllowlistedHost — integration coverage of the verify path (negative + positive). oauth1 and apple share the same code path through the embedded Params; the unit tests and shared isAllowedRedirect cover them.
1 parent 32d0c6e commit dde9063

19 files changed

Lines changed: 688 additions & 59 deletions

auth.go

Lines changed: 69 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,15 @@ type Opts struct {
6363
URL string // root url for the rest service, i.e. http://blah.example.com, required
6464
Validator token.Validator // validator allows to reject some valid tokens with user-defined logic
6565

66+
// AllowedRedirectHosts lists hostnames accepted in the "from" query
67+
// parameter of OAuth/verify login flows. Setting this field enables
68+
// host validation: the host of URL is always implicit, and any other
69+
// host must appear here. Nil (the default) disables validation and
70+
// preserves legacy permissive behavior — any non-empty "from" value
71+
// is honored. Hardening is opt-in; to restrict to the service host
72+
// only, pass a getter returning an empty slice.
73+
AllowedRedirectHosts token.AllowedHosts
74+
6675
AvatarStore avatar.Store // store to save/load avatars, required (use avatar.NoOp to disable avatars support)
6776
AvatarResizeLimit int // resize avatar's limit in pixels
6877
AvatarRoutePath string // avatar routing prefix, i.e. "/api/v1/avatar", default `/avatar`
@@ -226,14 +235,15 @@ func (s *Service) Middleware() middleware.Authenticator {
226235
// AddProviderWithUserAttributes adds provider with user attributes mapping
227236
func (s *Service) AddProviderWithUserAttributes(name, cid, csecret string, userAttributes provider.UserAttributes) {
228237
p := provider.Params{
229-
URL: s.opts.URL,
230-
JwtService: s.jwtService,
231-
Issuer: s.issuer,
232-
AvatarSaver: s.avatarProxy,
233-
Cid: cid,
234-
Csecret: csecret,
235-
L: s.logger,
236-
UserAttributes: userAttributes,
238+
URL: s.opts.URL,
239+
JwtService: s.jwtService,
240+
Issuer: s.issuer,
241+
AvatarSaver: s.avatarProxy,
242+
Cid: cid,
243+
Csecret: csecret,
244+
L: s.logger,
245+
UserAttributes: userAttributes,
246+
AllowedRedirectHosts: s.opts.AllowedRedirectHosts,
237247
}
238248
s.addProviderByName(name, p)
239249
}
@@ -308,14 +318,15 @@ func (s *Service) isValidProviderName(name string) bool {
308318
// AddProvider adds provider for given name
309319
func (s *Service) AddProvider(name, cid, csecret string) {
310320
p := provider.Params{
311-
URL: s.opts.URL,
312-
JwtService: s.jwtService,
313-
Issuer: s.issuer,
314-
AvatarSaver: s.avatarProxy,
315-
Cid: cid,
316-
Csecret: csecret,
317-
L: s.logger,
318-
UserAttributes: map[string]string{},
321+
URL: s.opts.URL,
322+
JwtService: s.jwtService,
323+
Issuer: s.issuer,
324+
AvatarSaver: s.avatarProxy,
325+
Cid: cid,
326+
Csecret: csecret,
327+
L: s.logger,
328+
UserAttributes: map[string]string{},
329+
AllowedRedirectHosts: s.opts.AllowedRedirectHosts,
319330
}
320331
s.addProviderByName(name, p)
321332
}
@@ -326,41 +337,44 @@ func (s *Service) AddProvider(name, cid, csecret string) {
326337
// For advanced configuration (e.g., UserAttributes), construct provider.Params directly.
327338
func (s *Service) AddMicrosoftProvider(cid, csecret, tenant string) {
328339
p := provider.Params{
329-
URL: s.opts.URL,
330-
JwtService: s.jwtService,
331-
Issuer: s.issuer,
332-
AvatarSaver: s.avatarProxy,
333-
Cid: cid,
334-
Csecret: csecret,
335-
L: s.logger,
336-
UserAttributes: map[string]string{},
337-
MicrosoftTenant: tenant,
340+
URL: s.opts.URL,
341+
JwtService: s.jwtService,
342+
Issuer: s.issuer,
343+
AvatarSaver: s.avatarProxy,
344+
Cid: cid,
345+
Csecret: csecret,
346+
L: s.logger,
347+
UserAttributes: map[string]string{},
348+
MicrosoftTenant: tenant,
349+
AllowedRedirectHosts: s.opts.AllowedRedirectHosts,
338350
}
339351
s.addProvider(provider.NewMicrosoft(p))
340352
}
341353

342354
// AddDevProvider with a custom host and port
343355
func (s *Service) AddDevProvider(host string, port int) {
344356
p := provider.Params{
345-
URL: s.opts.URL,
346-
JwtService: s.jwtService,
347-
Issuer: s.issuer,
348-
AvatarSaver: s.avatarProxy,
349-
L: s.logger,
350-
Port: port,
351-
Host: host,
357+
URL: s.opts.URL,
358+
JwtService: s.jwtService,
359+
Issuer: s.issuer,
360+
AvatarSaver: s.avatarProxy,
361+
L: s.logger,
362+
Port: port,
363+
Host: host,
364+
AllowedRedirectHosts: s.opts.AllowedRedirectHosts,
352365
}
353366
s.addProvider(provider.NewDev(p))
354367
}
355368

356369
// AddAppleProvider allow SignIn with Apple ID
357370
func (s *Service) AddAppleProvider(appleConfig provider.AppleConfig, privKeyLoader provider.PrivateKeyLoaderInterface) error {
358371
p := provider.Params{
359-
URL: s.opts.URL,
360-
JwtService: s.jwtService,
361-
Issuer: s.issuer,
362-
AvatarSaver: s.avatarProxy,
363-
L: s.logger,
372+
URL: s.opts.URL,
373+
JwtService: s.jwtService,
374+
Issuer: s.issuer,
375+
AvatarSaver: s.avatarProxy,
376+
L: s.logger,
377+
AllowedRedirectHosts: s.opts.AllowedRedirectHosts,
364378
}
365379

366380
// error checking at create need for catch one when apple private key init
@@ -376,13 +390,14 @@ func (s *Service) AddAppleProvider(appleConfig provider.AppleConfig, privKeyLoad
376390
// AddCustomProvider adds custom provider (e.g. https://gopkg.in/oauth2.v3)
377391
func (s *Service) AddCustomProvider(name string, client Client, copts provider.CustomHandlerOpt) {
378392
p := provider.Params{
379-
URL: s.opts.URL,
380-
JwtService: s.jwtService,
381-
Issuer: s.issuer,
382-
AvatarSaver: s.avatarProxy,
383-
Cid: client.Cid,
384-
Csecret: client.Csecret,
385-
L: s.logger,
393+
URL: s.opts.URL,
394+
JwtService: s.jwtService,
395+
Issuer: s.issuer,
396+
AvatarSaver: s.avatarProxy,
397+
Cid: client.Cid,
398+
Csecret: client.Csecret,
399+
L: s.logger,
400+
AllowedRedirectHosts: s.opts.AllowedRedirectHosts,
386401
}
387402
s.addProvider(provider.NewCustom(name, p, copts))
388403
}
@@ -420,14 +435,16 @@ func (s *Service) AddDirectProviderWithUserIDFunc(name string, credChecker provi
420435
// AddVerifProvider adds provider user's verification sent by sender
421436
func (s *Service) AddVerifProvider(name, msgTmpl string, sender provider.Sender) {
422437
dh := provider.VerifyHandler{
423-
L: s.logger,
424-
ProviderName: name,
425-
Issuer: s.issuer,
426-
TokenService: s.jwtService,
427-
AvatarSaver: s.avatarProxy,
428-
Sender: sender,
429-
Template: msgTmpl,
430-
UseGravatar: s.useGravatar,
438+
L: s.logger,
439+
ProviderName: name,
440+
Issuer: s.issuer,
441+
TokenService: s.jwtService,
442+
AvatarSaver: s.avatarProxy,
443+
Sender: sender,
444+
Template: msgTmpl,
445+
UseGravatar: s.useGravatar,
446+
URL: s.opts.URL,
447+
AllowedRedirectHosts: s.opts.AllowedRedirectHosts,
431448
}
432449
s.addProvider(dh)
433450
}

middleware/auth_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ func TestBasicAdminUserDoesNotLogPassword(t *testing.T) {
340340
const secret = "super-secret-attempted-passwd"
341341
var buf strings.Builder
342342
a := makeTestAuth(t)
343-
a.L = logger.Func(func(format string, args ...interface{}) {
343+
a.L = logger.Func(func(format string, args ...any) {
344344
fmt.Fprintf(&buf, format, args...)
345345
})
346346

provider/apple.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,11 @@ func (ah AppleHandler) AuthHandler(w http.ResponseWriter, r *http.Request) {
393393

394394
// redirect to back url if presented in login query params
395395
if oauthClaims.Handshake != nil && oauthClaims.Handshake.From != "" {
396+
if !isAllowedRedirect(oauthClaims.Handshake.From, ah.URL, ah.AllowedRedirectHosts) {
397+
ah.Logf("[WARN] rejected redirect to disallowed host: %s", redirectHostForLog(oauthClaims.Handshake.From))
398+
rest.RenderJSON(w, &u)
399+
return
400+
}
396401
http.Redirect(w, r, oauthClaims.Handshake.From, http.StatusTemporaryRedirect)
397402
return
398403
}

provider/apple_test.go

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,37 @@ func TestAppleHandler_LoginHandler(t *testing.T) {
327327

328328
}
329329

330+
// TestAppleHandler_LoginHandlerFromRejectsExternalHost is the regression
331+
// test for the redirect validator on the apple login path: with an allowlist
332+
// policy enabled, /login?from=https://evil.example.com must NOT 302 to evil.
333+
func TestAppleHandler_LoginHandlerFromRejectsExternalHost(t *testing.T) {
334+
enablePolicy := func(p *Params) {
335+
p.AllowedRedirectHosts = token.AllowedHostsFunc(func() ([]string, error) { return nil, nil })
336+
}
337+
teardown := prepareAppleOauthTest(t, 8987, 8988, nil, enablePolicy)
338+
defer teardown()
339+
340+
jar, err := cookiejar.New(nil)
341+
require.NoError(t, err)
342+
343+
client := &http.Client{Jar: jar, Timeout: 5 * time.Second,
344+
CheckRedirect: func(req *http.Request, via []*http.Request) error {
345+
if !strings.HasPrefix(req.URL.Host, "localhost") {
346+
return fmt.Errorf("blocked external redirect to %s", req.URL)
347+
}
348+
return nil
349+
},
350+
}
351+
352+
resp, err := client.Get("http://localhost:8987/login?site=remark&from=https://evil.example.com/phish")
353+
require.NoError(t, err, "no external redirect expected -- fix should reject evil host")
354+
defer resp.Body.Close()
355+
assert.Equal(t, http.StatusOK, resp.StatusCode)
356+
body, err := io.ReadAll(resp.Body)
357+
require.NoError(t, err)
358+
assert.NotContains(t, string(body), "evil.example.com")
359+
}
360+
330361
func TestAppleHandler_LogoutHandler(t *testing.T) {
331362

332363
teardown := prepareAppleOauthTest(t, 8691, 8692, nil)
@@ -457,7 +488,7 @@ func prepareAppleHandlerTest(responseMode string, scopes []string) (*AppleHandle
457488
return NewApple(p, aCfg, cl)
458489
}
459490

460-
func prepareAppleOauthTest(t *testing.T, loginPort, authPort int, testToken *string) func() {
491+
func prepareAppleOauthTest(t *testing.T, loginPort, authPort int, testToken *string, paramOpts ...func(*Params)) func() {
461492
signKey, testJWK := createTestSignKeyPairs(t)
462493
provider, err := prepareAppleHandlerTest("", []string{})
463494
assert.NoError(t, err)
@@ -504,6 +535,9 @@ func prepareAppleOauthTest(t *testing.T, loginPort, authPort int, testToken *str
504535

505536
params := Params{URL: "url", Cid: "cid", Csecret: "csecret", JwtService: jwtService,
506537
Issuer: "go-pkgz/auth", L: logger.Std}
538+
for _, opt := range paramOpts {
539+
opt(&params)
540+
}
507541
provider.Params = params
508542

509543
svc := Service{Provider: provider}

provider/oauth1.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,11 @@ func (h Oauth1Handler) AuthHandler(w http.ResponseWriter, r *http.Request) {
163163

164164
// redirect to back url if presented in login query params
165165
if oauthClaims.Handshake != nil && oauthClaims.Handshake.From != "" {
166+
if !isAllowedRedirect(oauthClaims.Handshake.From, h.URL, h.AllowedRedirectHosts) {
167+
h.Logf("[WARN] rejected redirect to disallowed host: %s", redirectHostForLog(oauthClaims.Handshake.From))
168+
rest.RenderJSON(w, &u)
169+
return
170+
}
166171
http.Redirect(w, r, oauthClaims.Handshake.From, http.StatusTemporaryRedirect)
167172
return
168173
}

provider/oauth1_test.go

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,40 @@ func TestOauth1MakeRedirURL(t *testing.T) {
184184
}
185185
}
186186

187-
func prepOauth1Test(t *testing.T, loginPort, authPort int) func() { //nolint
187+
// TestOauth1LoginFromRejectsExternalHost is the regression test for the
188+
// redirect validator on the oauth1 path: with an allowlist policy enabled,
189+
// /login?from=https://evil.example.com must NOT 302 to evil and must fall
190+
// back to the user-info JSON response.
191+
func TestOauth1LoginFromRejectsExternalHost(t *testing.T) {
192+
enablePolicy := func(p *Params) {
193+
p.AllowedRedirectHosts = token.AllowedHostsFunc(func() ([]string, error) { return nil, nil })
194+
}
195+
teardown := prepOauth1Test(t, loginPort, authPort, enablePolicy)
196+
defer teardown()
197+
198+
jar, err := cookiejar.New(nil)
199+
require.NoError(t, err)
200+
201+
client := &http.Client{Jar: jar, Timeout: timeout * time.Second,
202+
CheckRedirect: func(req *http.Request, via []*http.Request) error {
203+
if !strings.HasPrefix(req.URL.Host, "localhost") {
204+
return fmt.Errorf("blocked external redirect to %s", req.URL)
205+
}
206+
return nil
207+
},
208+
}
209+
210+
resp, err := client.Get(fmt.Sprintf("http://localhost:%d/login?site=remark&from=https://evil.example.com/phish", loginPort))
211+
require.NoError(t, err, "no external redirect expected -- fix should reject evil host")
212+
defer resp.Body.Close()
213+
assert.Equal(t, http.StatusOK, resp.StatusCode)
214+
body, err := io.ReadAll(resp.Body)
215+
require.NoError(t, err)
216+
assert.Contains(t, string(body), `"name":"blah"`)
217+
assert.NotContains(t, string(body), "evil.example.com")
218+
}
219+
220+
func prepOauth1Test(t *testing.T, loginPort, authPort int, paramOpts ...func(*Params)) func() { //nolint
188221

189222
provider := Oauth1Handler{
190223
name: "mock",
@@ -223,6 +256,9 @@ func prepOauth1Test(t *testing.T, loginPort, authPort int) func() { //nolint
223256

224257
params := Params{URL: "url", Cid: "aFdj12348sdja", Csecret: "Dwehsq2387akss", JwtService: jwtService,
225258
Issuer: "remark42", AvatarSaver: &mockAvatarSaver{}, L: logger.Std}
259+
for _, opt := range paramOpts {
260+
opt(&params)
261+
}
226262

227263
provider = initOauth1Handler(params, provider)
228264
svc := Service{Provider: provider}

provider/oauth2.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,14 @@ type Params struct {
4242
AvatarSaver AvatarSaver
4343
UserAttributes UserAttributes
4444

45+
// AllowedRedirectHosts lists hostnames accepted in the "from" query
46+
// parameter. Setting this field enables host validation: the host of
47+
// URL is always implicit, and any other host must appear here. Nil
48+
// disables validation and preserves legacy permissive behavior — any
49+
// non-empty "from" value is honored. See isAllowedRedirect for the
50+
// full policy.
51+
AllowedRedirectHosts token.AllowedHosts
52+
4553
Port int // relevant for providers supporting port customization, for example dev oauth2
4654
Host string // relevant for providers supporting host customization, for example dev oauth2
4755

@@ -239,6 +247,11 @@ func (p Oauth2Handler) AuthHandler(w http.ResponseWriter, r *http.Request) {
239247

240248
// redirect to back url if presented in login query params
241249
if oauthClaims.Handshake != nil && oauthClaims.Handshake.From != "" {
250+
if !isAllowedRedirect(oauthClaims.Handshake.From, p.URL, p.AllowedRedirectHosts) {
251+
p.Logf("[WARN] rejected redirect to disallowed host: %s", redirectHostForLog(oauthClaims.Handshake.From))
252+
rest.RenderJSON(w, &u)
253+
return
254+
}
242255
http.Redirect(w, r, oauthClaims.Handshake.From, http.StatusTemporaryRedirect)
243256
return
244257
}

0 commit comments

Comments
 (0)