From 2c9a686868e52f8ac3c3bb1d81f1bab56fca8d19 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Fri, 8 May 2026 19:49:28 +0200 Subject: [PATCH] fix(provider): backport "from" redirect validator to v1 (sibling of #275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- auth.go | 121 ++++++++++++++++------------- middleware/auth_test.go | 2 +- provider/apple.go | 5 ++ provider/apple_test.go | 36 ++++++++- provider/oauth1.go | 5 ++ provider/oauth1_test.go | 38 ++++++++- provider/oauth2.go | 13 ++++ provider/oauth2_test.go | 83 +++++++++++++++++++- provider/redirect.go | 69 +++++++++++++++++ provider/redirect_test.go | 120 +++++++++++++++++++++++++++++ provider/sender/email_test.go | 2 +- provider/verify.go | 22 ++++++ provider/verify_test.go | 127 +++++++++++++++++++++++++++++++ token/jwt.go | 20 +++++ v2/middleware/auth_test.go | 2 +- v2/provider/redirect_test.go | 26 +++++++ v2/provider/sender/email_test.go | 2 +- v2/provider/verify.go | 7 ++ v2/provider/verify_test.go | 47 ++++++++++++ 19 files changed, 688 insertions(+), 59 deletions(-) create mode 100644 provider/redirect.go create mode 100644 provider/redirect_test.go diff --git a/auth.go b/auth.go index 6da2cc49..8ad28138 100644 --- a/auth.go +++ b/auth.go @@ -63,6 +63,15 @@ type Opts struct { URL string // root url for the rest service, i.e. http://blah.example.com, required Validator token.Validator // validator allows to reject some valid tokens with user-defined logic + // AllowedRedirectHosts lists hostnames accepted in the "from" query + // parameter of OAuth/verify login flows. Setting this field enables + // host validation: the host of URL is always implicit, and any other + // host must appear here. Nil (the default) disables validation and + // preserves legacy permissive behavior — any non-empty "from" value + // is honored. Hardening is opt-in; to restrict to the service host + // only, pass a getter returning an empty slice. + AllowedRedirectHosts token.AllowedHosts + AvatarStore avatar.Store // store to save/load avatars, required (use avatar.NoOp to disable avatars support) AvatarResizeLimit int // resize avatar's limit in pixels AvatarRoutePath string // avatar routing prefix, i.e. "/api/v1/avatar", default `/avatar` @@ -226,14 +235,15 @@ func (s *Service) Middleware() middleware.Authenticator { // AddProviderWithUserAttributes adds provider with user attributes mapping func (s *Service) AddProviderWithUserAttributes(name, cid, csecret string, userAttributes provider.UserAttributes) { p := provider.Params{ - URL: s.opts.URL, - JwtService: s.jwtService, - Issuer: s.issuer, - AvatarSaver: s.avatarProxy, - Cid: cid, - Csecret: csecret, - L: s.logger, - UserAttributes: userAttributes, + URL: s.opts.URL, + JwtService: s.jwtService, + Issuer: s.issuer, + AvatarSaver: s.avatarProxy, + Cid: cid, + Csecret: csecret, + L: s.logger, + UserAttributes: userAttributes, + AllowedRedirectHosts: s.opts.AllowedRedirectHosts, } s.addProviderByName(name, p) } @@ -308,14 +318,15 @@ func (s *Service) isValidProviderName(name string) bool { // AddProvider adds provider for given name func (s *Service) AddProvider(name, cid, csecret string) { p := provider.Params{ - URL: s.opts.URL, - JwtService: s.jwtService, - Issuer: s.issuer, - AvatarSaver: s.avatarProxy, - Cid: cid, - Csecret: csecret, - L: s.logger, - UserAttributes: map[string]string{}, + URL: s.opts.URL, + JwtService: s.jwtService, + Issuer: s.issuer, + AvatarSaver: s.avatarProxy, + Cid: cid, + Csecret: csecret, + L: s.logger, + UserAttributes: map[string]string{}, + AllowedRedirectHosts: s.opts.AllowedRedirectHosts, } s.addProviderByName(name, p) } @@ -326,15 +337,16 @@ func (s *Service) AddProvider(name, cid, csecret string) { // For advanced configuration (e.g., UserAttributes), construct provider.Params directly. func (s *Service) AddMicrosoftProvider(cid, csecret, tenant string) { p := provider.Params{ - URL: s.opts.URL, - JwtService: s.jwtService, - Issuer: s.issuer, - AvatarSaver: s.avatarProxy, - Cid: cid, - Csecret: csecret, - L: s.logger, - UserAttributes: map[string]string{}, - MicrosoftTenant: tenant, + URL: s.opts.URL, + JwtService: s.jwtService, + Issuer: s.issuer, + AvatarSaver: s.avatarProxy, + Cid: cid, + Csecret: csecret, + L: s.logger, + UserAttributes: map[string]string{}, + MicrosoftTenant: tenant, + AllowedRedirectHosts: s.opts.AllowedRedirectHosts, } s.addProvider(provider.NewMicrosoft(p)) } @@ -342,13 +354,14 @@ func (s *Service) AddMicrosoftProvider(cid, csecret, tenant string) { // AddDevProvider with a custom host and port func (s *Service) AddDevProvider(host string, port int) { p := provider.Params{ - URL: s.opts.URL, - JwtService: s.jwtService, - Issuer: s.issuer, - AvatarSaver: s.avatarProxy, - L: s.logger, - Port: port, - Host: host, + URL: s.opts.URL, + JwtService: s.jwtService, + Issuer: s.issuer, + AvatarSaver: s.avatarProxy, + L: s.logger, + Port: port, + Host: host, + AllowedRedirectHosts: s.opts.AllowedRedirectHosts, } s.addProvider(provider.NewDev(p)) } @@ -356,11 +369,12 @@ func (s *Service) AddDevProvider(host string, port int) { // AddAppleProvider allow SignIn with Apple ID func (s *Service) AddAppleProvider(appleConfig provider.AppleConfig, privKeyLoader provider.PrivateKeyLoaderInterface) error { p := provider.Params{ - URL: s.opts.URL, - JwtService: s.jwtService, - Issuer: s.issuer, - AvatarSaver: s.avatarProxy, - L: s.logger, + URL: s.opts.URL, + JwtService: s.jwtService, + Issuer: s.issuer, + AvatarSaver: s.avatarProxy, + L: s.logger, + AllowedRedirectHosts: s.opts.AllowedRedirectHosts, } // 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 // AddCustomProvider adds custom provider (e.g. https://gopkg.in/oauth2.v3) func (s *Service) AddCustomProvider(name string, client Client, copts provider.CustomHandlerOpt) { p := provider.Params{ - URL: s.opts.URL, - JwtService: s.jwtService, - Issuer: s.issuer, - AvatarSaver: s.avatarProxy, - Cid: client.Cid, - Csecret: client.Csecret, - L: s.logger, + URL: s.opts.URL, + JwtService: s.jwtService, + Issuer: s.issuer, + AvatarSaver: s.avatarProxy, + Cid: client.Cid, + Csecret: client.Csecret, + L: s.logger, + AllowedRedirectHosts: s.opts.AllowedRedirectHosts, } s.addProvider(provider.NewCustom(name, p, copts)) } @@ -420,14 +435,16 @@ func (s *Service) AddDirectProviderWithUserIDFunc(name string, credChecker provi // AddVerifProvider adds provider user's verification sent by sender func (s *Service) AddVerifProvider(name, msgTmpl string, sender provider.Sender) { dh := provider.VerifyHandler{ - L: s.logger, - ProviderName: name, - Issuer: s.issuer, - TokenService: s.jwtService, - AvatarSaver: s.avatarProxy, - Sender: sender, - Template: msgTmpl, - UseGravatar: s.useGravatar, + L: s.logger, + ProviderName: name, + Issuer: s.issuer, + TokenService: s.jwtService, + AvatarSaver: s.avatarProxy, + Sender: sender, + Template: msgTmpl, + UseGravatar: s.useGravatar, + URL: s.opts.URL, + AllowedRedirectHosts: s.opts.AllowedRedirectHosts, } s.addProvider(dh) } diff --git a/middleware/auth_test.go b/middleware/auth_test.go index 1acae6ed..44fc80b5 100644 --- a/middleware/auth_test.go +++ b/middleware/auth_test.go @@ -340,7 +340,7 @@ func TestBasicAdminUserDoesNotLogPassword(t *testing.T) { const secret = "super-secret-attempted-passwd" var buf strings.Builder a := makeTestAuth(t) - a.L = logger.Func(func(format string, args ...interface{}) { + a.L = logger.Func(func(format string, args ...any) { fmt.Fprintf(&buf, format, args...) }) diff --git a/provider/apple.go b/provider/apple.go index e7e47605..86ade331 100644 --- a/provider/apple.go +++ b/provider/apple.go @@ -393,6 +393,11 @@ func (ah AppleHandler) AuthHandler(w http.ResponseWriter, r *http.Request) { // redirect to back url if presented in login query params if oauthClaims.Handshake != nil && oauthClaims.Handshake.From != "" { + if !isAllowedRedirect(oauthClaims.Handshake.From, ah.URL, ah.AllowedRedirectHosts) { + ah.Logf("[WARN] rejected redirect to disallowed host: %s", redirectHostForLog(oauthClaims.Handshake.From)) + rest.RenderJSON(w, &u) + return + } http.Redirect(w, r, oauthClaims.Handshake.From, http.StatusTemporaryRedirect) return } diff --git a/provider/apple_test.go b/provider/apple_test.go index 49ac619d..351b0183 100644 --- a/provider/apple_test.go +++ b/provider/apple_test.go @@ -327,6 +327,37 @@ func TestAppleHandler_LoginHandler(t *testing.T) { } +// TestAppleHandler_LoginHandlerFromRejectsExternalHost is the regression +// test for the redirect validator on the apple login path: with an allowlist +// policy enabled, /login?from=https://evil.example.com must NOT 302 to evil. +func TestAppleHandler_LoginHandlerFromRejectsExternalHost(t *testing.T) { + enablePolicy := func(p *Params) { + p.AllowedRedirectHosts = token.AllowedHostsFunc(func() ([]string, error) { return nil, nil }) + } + teardown := prepareAppleOauthTest(t, 8987, 8988, nil, enablePolicy) + defer teardown() + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + client := &http.Client{Jar: jar, Timeout: 5 * time.Second, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if !strings.HasPrefix(req.URL.Host, "localhost") { + return fmt.Errorf("blocked external redirect to %s", req.URL) + } + return nil + }, + } + + resp, err := client.Get("http://localhost:8987/login?site=remark&from=https://evil.example.com/phish") + require.NoError(t, err, "no external redirect expected -- fix should reject evil host") + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.NotContains(t, string(body), "evil.example.com") +} + func TestAppleHandler_LogoutHandler(t *testing.T) { teardown := prepareAppleOauthTest(t, 8691, 8692, nil) @@ -457,7 +488,7 @@ func prepareAppleHandlerTest(responseMode string, scopes []string) (*AppleHandle return NewApple(p, aCfg, cl) } -func prepareAppleOauthTest(t *testing.T, loginPort, authPort int, testToken *string) func() { +func prepareAppleOauthTest(t *testing.T, loginPort, authPort int, testToken *string, paramOpts ...func(*Params)) func() { signKey, testJWK := createTestSignKeyPairs(t) provider, err := prepareAppleHandlerTest("", []string{}) assert.NoError(t, err) @@ -504,6 +535,9 @@ func prepareAppleOauthTest(t *testing.T, loginPort, authPort int, testToken *str params := Params{URL: "url", Cid: "cid", Csecret: "csecret", JwtService: jwtService, Issuer: "go-pkgz/auth", L: logger.Std} + for _, opt := range paramOpts { + opt(¶ms) + } provider.Params = params svc := Service{Provider: provider} diff --git a/provider/oauth1.go b/provider/oauth1.go index 6159ac5c..7f492c26 100644 --- a/provider/oauth1.go +++ b/provider/oauth1.go @@ -163,6 +163,11 @@ func (h Oauth1Handler) AuthHandler(w http.ResponseWriter, r *http.Request) { // redirect to back url if presented in login query params if oauthClaims.Handshake != nil && oauthClaims.Handshake.From != "" { + if !isAllowedRedirect(oauthClaims.Handshake.From, h.URL, h.AllowedRedirectHosts) { + h.Logf("[WARN] rejected redirect to disallowed host: %s", redirectHostForLog(oauthClaims.Handshake.From)) + rest.RenderJSON(w, &u) + return + } http.Redirect(w, r, oauthClaims.Handshake.From, http.StatusTemporaryRedirect) return } diff --git a/provider/oauth1_test.go b/provider/oauth1_test.go index e3fdf470..8668f8a1 100644 --- a/provider/oauth1_test.go +++ b/provider/oauth1_test.go @@ -184,7 +184,40 @@ func TestOauth1MakeRedirURL(t *testing.T) { } } -func prepOauth1Test(t *testing.T, loginPort, authPort int) func() { //nolint +// TestOauth1LoginFromRejectsExternalHost is the regression test for the +// redirect validator on the oauth1 path: with an allowlist policy enabled, +// /login?from=https://evil.example.com must NOT 302 to evil and must fall +// back to the user-info JSON response. +func TestOauth1LoginFromRejectsExternalHost(t *testing.T) { + enablePolicy := func(p *Params) { + p.AllowedRedirectHosts = token.AllowedHostsFunc(func() ([]string, error) { return nil, nil }) + } + teardown := prepOauth1Test(t, loginPort, authPort, enablePolicy) + defer teardown() + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + client := &http.Client{Jar: jar, Timeout: timeout * time.Second, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if !strings.HasPrefix(req.URL.Host, "localhost") { + return fmt.Errorf("blocked external redirect to %s", req.URL) + } + return nil + }, + } + + resp, err := client.Get(fmt.Sprintf("http://localhost:%d/login?site=remark&from=https://evil.example.com/phish", loginPort)) + require.NoError(t, err, "no external redirect expected -- fix should reject evil host") + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Contains(t, string(body), `"name":"blah"`) + assert.NotContains(t, string(body), "evil.example.com") +} + +func prepOauth1Test(t *testing.T, loginPort, authPort int, paramOpts ...func(*Params)) func() { //nolint provider := Oauth1Handler{ name: "mock", @@ -223,6 +256,9 @@ func prepOauth1Test(t *testing.T, loginPort, authPort int) func() { //nolint params := Params{URL: "url", Cid: "aFdj12348sdja", Csecret: "Dwehsq2387akss", JwtService: jwtService, Issuer: "remark42", AvatarSaver: &mockAvatarSaver{}, L: logger.Std} + for _, opt := range paramOpts { + opt(¶ms) + } provider = initOauth1Handler(params, provider) svc := Service{Provider: provider} diff --git a/provider/oauth2.go b/provider/oauth2.go index d9273178..837e211f 100644 --- a/provider/oauth2.go +++ b/provider/oauth2.go @@ -42,6 +42,14 @@ type Params struct { AvatarSaver AvatarSaver UserAttributes UserAttributes + // AllowedRedirectHosts lists hostnames accepted in the "from" query + // parameter. Setting this field enables host validation: the host of + // URL is always implicit, and any other host must appear here. Nil + // disables validation and preserves legacy permissive behavior — any + // non-empty "from" value is honored. See isAllowedRedirect for the + // full policy. + AllowedRedirectHosts token.AllowedHosts + Port int // relevant for providers supporting port customization, for example dev oauth2 Host string // relevant for providers supporting host customization, for example dev oauth2 @@ -239,6 +247,11 @@ func (p Oauth2Handler) AuthHandler(w http.ResponseWriter, r *http.Request) { // redirect to back url if presented in login query params if oauthClaims.Handshake != nil && oauthClaims.Handshake.From != "" { + if !isAllowedRedirect(oauthClaims.Handshake.From, p.URL, p.AllowedRedirectHosts) { + p.Logf("[WARN] rejected redirect to disallowed host: %s", redirectHostForLog(oauthClaims.Handshake.From)) + rest.RenderJSON(w, &u) + return + } http.Redirect(w, r, oauthClaims.Handshake.From, http.StatusTemporaryRedirect) return } diff --git a/provider/oauth2_test.go b/provider/oauth2_test.go index 1c160477..adadd9ee 100644 --- a/provider/oauth2_test.go +++ b/provider/oauth2_test.go @@ -241,6 +241,83 @@ func TestOauth2InvalidHandler(t *testing.T) { assert.Equal(t, 500, resp.StatusCode) } +// TestOauth2LoginFromRejectsExternalHost reproduces the open-redirect attack +// against the oauth2 login flow. +// +// Attack scenario (before this fix): +// +// 1. Attacker crafts a link to the legitimate site: +// https://comments.example.com/auth/github/login?from=https://evil.example.com/phish +// 2. Victim clicks the link, sees the real GitHub OAuth consent screen and approves. +// 3. After the OAuth dance completes, the AuthHandler executed +// `http.Redirect(w, r, oauthClaims.Handshake.From, http.StatusTemporaryRedirect)` +// with no validation on `From` — so the browser was bounced straight to +// https://evil.example.com/phish, carrying a fresh session cookie and the +// trust the user had in the legitimate brand. +// +// With the fix in place and host validation enabled (Opts.AllowedRedirectHosts +// non-nil), `from` must point at the service's own host or a host listed in +// the allowlist. Otherwise the handler renders the user-info JSON and logs a +// [WARN]. Same vector applies to oauth1, apple and verify providers — they +// all share the same redirect-from-handshake pattern. +func TestOauth2LoginFromRejectsExternalHost(t *testing.T) { + enablePolicy := func(p *Params) { + p.AllowedRedirectHosts = token.AllowedHostsFunc(func() ([]string, error) { return nil, nil }) + } + teardown := prepOauth2Test(t, 8981, 8982, nil, enablePolicy) + defer teardown() + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + client := &http.Client{Jar: jar, Timeout: 5 * time.Second, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if !strings.HasPrefix(req.URL.Host, "localhost") { + return fmt.Errorf("blocked external redirect to %s", req.URL) + } + return nil + }, + } + + resp, err := client.Get("http://localhost:8981/login?site=remark&from=https://evil.example.com/phish") + require.NoError(t, err, "no external redirect expected — fix should reject evil host") + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Contains(t, string(body), `"name":"blah"`, "must return user JSON, not redirect to evil") + assert.NotContains(t, string(body), "evil.example.com") +} + +func TestOauth2LoginFromAllowsAllowlistedHost(t *testing.T) { + enablePolicy := func(p *Params) { + p.AllowedRedirectHosts = token.AllowedHostsFunc(func() ([]string, error) { return []string{"trusted.example.com"}, nil }) + } + teardown := prepOauth2Test(t, 8985, 8986, nil, enablePolicy) + defer teardown() + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + var lastRedirect string + client := &http.Client{Jar: jar, Timeout: 5 * time.Second, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + lastRedirect = req.URL.String() + if strings.Contains(req.URL.Host, "trusted.example.com") { + return http.ErrUseLastResponse + } + return nil + }, + } + + resp, err := client.Get("http://localhost:8985/login?site=remark&from=https://trusted.example.com/back") + require.NoError(t, err) + defer resp.Body.Close() + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode, "must 307-redirect to trusted host") + assert.Equal(t, "https://trusted.example.com/back", resp.Header.Get("Location")) + assert.Contains(t, lastRedirect, "trusted.example.com") +} + func TestMakeRedirURL(t *testing.T) { cases := []struct{ rootURL, route, out string }{ {"localhost:8080/", "/my/auth/path/google", "localhost:8080/my/auth/path/callback"}, @@ -258,7 +335,7 @@ func TestMakeRedirURL(t *testing.T) { } } -func prepOauth2Test(t *testing.T, loginPort, authPort int, btHook BearerTokenHook) func() { +func prepOauth2Test(t *testing.T, loginPort, authPort int, btHook BearerTokenHook, paramOpts ...func(*Params)) func() { provider := Oauth2Handler{ name: "mock", @@ -299,6 +376,10 @@ func prepOauth2Test(t *testing.T, loginPort, authPort int, btHook BearerTokenHoo params := Params{URL: "url", Cid: "cid", Csecret: "csecret", JwtService: jwtService, Issuer: "remark42", AvatarSaver: &mockAvatarSaver{}, L: logger.Std} + for _, opt := range paramOpts { + opt(¶ms) + } + provider = initOauth2Handler(params, provider) svc := Service{Provider: provider} diff --git a/provider/redirect.go b/provider/redirect.go new file mode 100644 index 00000000..25a32d8d --- /dev/null +++ b/provider/redirect.go @@ -0,0 +1,69 @@ +package provider + +import ( + "net/url" + "strings" + + "github.com/go-pkgz/auth/token" +) + +// isAllowedRedirect reports whether the "from" URL is safe to redirect to +// after a successful auth handshake. +// +// The check is opt-in: when allowed is nil the function returns true for any +// non-empty input, preserving the behavior of versions before the redirect +// validator existed. This keeps a dependency bump from breaking existing +// consumers; hardening is enabled by setting Opts.AllowedRedirectHosts. +// +// When allowed is non-nil: +// - only http/https schemes are accepted +// - relative paths and unparseable URLs are rejected +// - the service's own host (derived from serviceURL) is always allowed +// - any other host must appear in the allowed list +// +// Hostname comparison is case-insensitive and ignores the port: +// https://app.example.com:443 and https://App.Example.Com are treated as the +// same host. Operators wanting strict port-aware checks should list each +// host:port form explicitly via AllowedHosts. +func isAllowedRedirect(from, serviceURL string, allowed token.AllowedHosts) bool { + // permissive default: no allowlist configured = legacy behavior. + // guard against typed-nil AllowedHostsFunc values (non-nil interface + // wrapping a nil func) to avoid panicking in Get(). + if allowed == nil { + return from != "" + } + if fn, ok := allowed.(token.AllowedHostsFunc); ok && fn == nil { + return from != "" + } + u, err := url.Parse(from) + if err != nil || u.Hostname() == "" { + return false + } + if u.Scheme != "http" && u.Scheme != "https" { + return false + } + fromHost := u.Hostname() + if svc, sErr := url.Parse(serviceURL); sErr == nil && svc.Hostname() != "" && strings.EqualFold(svc.Hostname(), fromHost) { + return true + } + hosts, hErr := allowed.Get() + if hErr != nil { + return false + } + for _, h := range hosts { + if strings.EqualFold(h, fromHost) || strings.EqualFold(h, u.Host) { + return true + } + } + return false +} + +// redirectHostForLog extracts just the hostname from a from-URL for logging +// on rejection, so attacker-supplied paths/queries do not leak into operator +// logs. Returns a sentinel if the URL cannot be parsed. +func redirectHostForLog(from string) string { + if u, err := url.Parse(from); err == nil && u.Hostname() != "" { + return u.Hostname() + } + return "" +} diff --git a/provider/redirect_test.go b/provider/redirect_test.go new file mode 100644 index 00000000..3f67b703 --- /dev/null +++ b/provider/redirect_test.go @@ -0,0 +1,120 @@ +package provider + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/go-pkgz/auth/token" +) + +func TestIsAllowedRedirect(t *testing.T) { + allowedFn := func(hosts ...string) token.AllowedHosts { + return token.AllowedHostsFunc(func() ([]string, error) { return hosts, nil }) + } + errFn := token.AllowedHostsFunc(func() ([]string, error) { return nil, errors.New("boom") }) + + tests := []struct { + name string + from string + serviceURL string + allowed token.AllowedHosts + want bool + }{ + // permissive default (nil allowlist) — preserves pre-feature behavior + {name: "nil allowlist allows arbitrary external host (legacy)", from: "https://evil.com/x", serviceURL: "https://app.example.com", want: true}, + {name: "nil allowlist allows relative path (legacy)", from: "/foo/bar", serviceURL: "https://app.example.com", want: true}, + {name: "nil allowlist rejects empty from", from: "", serviceURL: "https://app.example.com", want: false}, + {name: "typed-nil AllowedHostsFunc treated as nil allowlist", from: "https://evil.com/x", serviceURL: "https://app.example.com", + allowed: token.AllowedHostsFunc(nil), want: true}, + + // policy enabled (any non-nil allowlist) — sanity checks reject malformed input + {name: "policy on: empty from rejected", from: "", serviceURL: "https://app.example.com", allowed: allowedFn(), want: false}, + {name: "policy on: relative path rejected", from: "/foo/bar", serviceURL: "https://app.example.com", allowed: allowedFn(), want: false}, + {name: "policy on: unparseable url rejected", from: "://not a url", serviceURL: "https://app.example.com", allowed: allowedFn(), want: false}, + + // policy on, service URL host implicit + {name: "policy on: same host as service allowed", from: "https://app.example.com/back", serviceURL: "https://app.example.com", allowed: allowedFn(), want: true}, + {name: "policy on: same host different scheme allowed", from: "http://app.example.com/back", serviceURL: "https://app.example.com", allowed: allowedFn(), want: true}, + {name: "policy on: same host with port matches", from: "https://app.example.com:443/x", serviceURL: "https://app.example.com:443", allowed: allowedFn(), want: true}, + {name: "policy on: explicit https default port matches no-port service", from: "https://app.example.com:443/x", serviceURL: "https://app.example.com", allowed: allowedFn(), want: true}, + {name: "policy on: explicit http default port matches no-port service", from: "http://app.example.com:80/x", serviceURL: "http://app.example.com", allowed: allowedFn(), want: true}, + {name: "policy on: different non-default port still allowed (hostname compare)", from: "https://app.example.com:8080/x", serviceURL: "https://app.example.com", allowed: allowedFn(), want: true}, + {name: "policy on: subdomain not implicitly allowed", from: "https://evil.app.example.com/x", serviceURL: "https://app.example.com", allowed: allowedFn(), want: false}, + {name: "policy on: different host rejected", from: "https://evil.com/phish", serviceURL: "https://app.example.com", allowed: allowedFn(), want: false}, + {name: "policy on: same host case-insensitive", from: "https://App.Example.Com/back", serviceURL: "https://app.example.com", allowed: allowedFn(), want: true}, + + // non-http(s) schemes are rejected even if host looks sane + {name: "policy on: javascript scheme rejected", from: "javascript:alert(1)", serviceURL: "https://app.example.com", allowed: allowedFn(), want: false}, + {name: "policy on: ftp scheme rejected", from: "ftp://app.example.com/file", serviceURL: "https://app.example.com", allowed: allowedFn(), want: false}, + {name: "policy on: data scheme rejected", from: "data:text/html,", serviceURL: "https://app.example.com", allowed: allowedFn(), want: false}, + + // policy with explicit allowlist entries + {name: "host in allowlist accepted", from: "https://admin.example.com/back", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com", "other.example.com"), want: true}, + {name: "host in allowlist case-insensitive", from: "https://Admin.Example.Com/back", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: true}, + {name: "allowlist entry case-insensitive vs mixed-case from", from: "https://admin.example.com/back", serviceURL: "https://app.example.com", + allowed: allowedFn("ADMIN.EXAMPLE.COM"), want: true}, + {name: "host not in allowlist rejected", from: "https://evil.com/phish", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "allowlist getter error treated as not allowed", from: "https://admin.example.com", serviceURL: "https://app.example.com", + allowed: errFn, want: false}, + {name: "malformed service URL falls back to allowlist", from: "https://admin.example.com", serviceURL: "://bad", + allowed: allowedFn("admin.example.com"), want: true}, + {name: "malformed service URL with empty allowlist rejects", from: "https://admin.example.com", serviceURL: "://bad", + allowed: allowedFn(), want: false}, + + // bypass-category characterization. None of these URLs grant access to + // hosts outside the allowlist; the test pins that behavior so a future + // refactor of url.Parse usage doesn't silently weaken it. + {name: "scheme-relative URL rejected (no scheme)", from: "//evil.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "userinfo does not bypass: allowed-looking user, evil host", from: "https://admin.example.com@evil.com/x", + serviceURL: "https://app.example.com", allowed: allowedFn("admin.example.com"), want: false}, + {name: "userinfo does not change allowed host check (evil userinfo, allowed host)", from: "https://evil@admin.example.com/x", + serviceURL: "https://app.example.com", allowed: allowedFn("admin.example.com"), want: true}, + {name: "IPv6 literal not in allowlist rejected", from: "https://[::1]/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "IPv6 literal explicitly allowed", from: "https://[::1]/x", serviceURL: "https://app.example.com", + allowed: allowedFn("::1"), want: true}, + {name: "IDN homoglyph not equal to ASCII allowlist entry", from: "https://аpp.example.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("app.example.com"), want: false}, + {name: "punycode form of homoglyph not equal to ASCII allowlist entry", from: "https://xn--pp-7lc.example.com/x", + serviceURL: "https://app.example.com", allowed: allowedFn("app.example.com"), want: false}, + {name: "percent-encoded host parse-fails rejected", from: "https://%65vil.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "backslash userinfo trick parse-fails rejected", from: "https://evil.com\\@admin.example.com/x", + serviceURL: "https://app.example.com", allowed: allowedFn("admin.example.com"), want: false}, + {name: "scheme without // (opaque) rejected", from: "https:evil.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "triple-slash empties host rejected", from: "https:///evil.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isAllowedRedirect(tt.from, tt.serviceURL, tt.allowed)) + }) + } +} + +func TestRedirectHostForLog(t *testing.T) { + tests := []struct { + name string + from string + want string + }{ + {name: "https URL with path and query", from: "https://evil.example.com/phish?token=abc", want: "evil.example.com"}, + {name: "URL with port", from: "https://evil.example.com:8080/path", want: "evil.example.com"}, + {name: "empty string", from: "", want: ""}, + {name: "relative path has no host", from: "/local/path", want: ""}, + {name: "garbage", from: "://not a url", want: ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, redirectHostForLog(tt.from)) + }) + } +} diff --git a/provider/sender/email_test.go b/provider/sender/email_test.go index f02c162a..6a2ee5a8 100644 --- a/provider/sender/email_test.go +++ b/provider/sender/email_test.go @@ -59,7 +59,7 @@ func TestEmail_New(t *testing.T) { func TestEmail_SendDoesNotLogBody(t *testing.T) { const secretBody = "Confirmation token: super-secret-magic-link-XYZ" var buf strings.Builder - capturing := logger.Func(func(format string, args ...interface{}) { + capturing := logger.Func(func(format string, args ...any) { fmt.Fprintf(&buf, format, args...) }) p := EmailParams{Host: "127.0.0.2", Port: 25, From: "from@example.com", diff --git a/provider/verify.go b/provider/verify.go index 53dcd09b..f947e382 100644 --- a/provider/verify.go +++ b/provider/verify.go @@ -40,6 +40,16 @@ type VerifyHandler struct { Sender Sender Template string UseGravatar bool + + // URL is the service's own root URL; its host is always permitted as + // a "from" redirect target. Optional but recommended. + URL string + // AllowedRedirectHosts lists additional hostnames permitted as "from" + // redirect targets. Setting this field enables host validation: the + // host of URL is always implicit, and any other host must appear + // here. Nil disables validation and preserves legacy permissive + // behavior — any non-empty "from" value is honored. + AllowedRedirectHosts token.AllowedHosts } // Sender defines interface to send emails @@ -139,6 +149,11 @@ func (e VerifyHandler) LoginHandler(w http.ResponseWriter, r *http.Request) { return } if confClaims.Handshake != nil && confClaims.Handshake.From != "" { + if !isAllowedRedirect(confClaims.Handshake.From, e.URL, e.AllowedRedirectHosts) { + e.Logf("[WARN] rejected redirect to disallowed host: %s", redirectHostForLog(confClaims.Handshake.From)) + rest.RenderJSON(w, claims.User) + return + } http.Redirect(w, r, confClaims.Handshake.From, http.StatusTemporaryRedirect) return } @@ -159,6 +174,13 @@ func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request) Handshake: &token.Handshake{ State: "", ID: user + "::" + address, + // without copying "from" here the redirect validator at the + // other end has nothing to validate or to redirect to. The + // docs (and #275) advertise ?from= on the verify login + // path, but the original sendConfirmation never put it on + // the handshake JWT, so production verify flows could never + // honor from at all. + From: r.URL.Query().Get("from"), }, SessionOnly: r.URL.Query().Get("session") != "" && r.URL.Query().Get("session") != "0", StandardClaims: jwt.StandardClaims{ diff --git a/provider/verify_test.go b/provider/verify_test.go index 291c8928..43c9c71d 100644 --- a/provider/verify_test.go +++ b/provider/verify_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/golang-jwt/jwt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -132,6 +133,132 @@ func TestVerifyHandler_LoginAcceptConfirm(t *testing.T) { assert.Equal(t, true, claims.SessionOnly) } +// TestVerifyHandler_PublicFromFlow_RejectsExternalHost drives the public flow: +// /login?from=... -> sendConfirmation -> email-link click -> /login?token=... +// -> validator rejects. Catches regressions where sendConfirmation drops the +// from query param (the JWT then carries an empty Handshake.From, the +// validator never fires, and production silently ignores the redirect). +func TestVerifyHandler_PublicFromFlow_RejectsExternalHost(t *testing.T) { + emailer := &mockSender{} + jwtSvc := token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }) + e := VerifyHandler{ + ProviderName: "test", + TokenService: jwtSvc, + Issuer: "iss-test", + L: logger.Std, + Sender: SenderFunc(emailer.Send), + Template: "token:{{.Token}}", + AllowedRedirectHosts: token.AllowedHostsFunc(func() ([]string, error) { return nil, nil }), + } + + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", + "/login?address=blah@user.com&user=test123&site=remark42&from=https://evil.example.com/phish", + http.NoBody) + require.NoError(t, err) + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr, req) + require.Equal(t, http.StatusOK, rr.Code) + tknStr := strings.TrimPrefix(emailer.text, "token:") + require.NotEmpty(t, tknStr, "sendConfirmation must produce a token") + + // sanity: the produced token MUST carry from in the handshake claim -- + // otherwise the validator at the redemption side has nothing to act on + // and the redirect would silently no-op in production. + parsed, err := jwtSvc.Parse(tknStr) + require.NoError(t, err) + require.NotNil(t, parsed.Handshake) + require.Equal(t, "https://evil.example.com/phish", parsed.Handshake.From, + "sendConfirmation must propagate ?from to handshake JWT, otherwise the redirect validator never runs in production") + + // redeem and assert the validator rejects the external host + rr2 := httptest.NewRecorder() + req2, err := http.NewRequest("GET", "/login?token="+tknStr, http.NoBody) + require.NoError(t, err) + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr2, req2) + assert.Equal(t, http.StatusOK, rr2.Code, "must return user JSON, not 307 to evil") + assert.Equal(t, "", rr2.Header().Get("Location")) + assert.NotContains(t, rr2.Body.String(), "evil.example.com") +} + +func TestVerifyHandler_LoginAcceptConfirmFromRejectsExternalHost(t *testing.T) { + jwtSvc := token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }) + e := VerifyHandler{ + ProviderName: "test", + TokenService: jwtSvc, + Issuer: "iss-test", + L: logger.Std, + // non-nil empty allowlist enables the policy with no extra hosts + AllowedRedirectHosts: token.AllowedHostsFunc(func() ([]string, error) { return nil, nil }), + } + + confTok, err := jwtSvc.Token(token.Claims{ + Handshake: &token.Handshake{ + ID: "test123::blah@user.com", + From: "https://evil.example.com/phish", + }, + StandardClaims: jwt.StandardClaims{ + Audience: "remark42", + ExpiresAt: time.Now().Add(time.Hour).Unix(), + }, + }) + require.NoError(t, err) + + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", fmt.Sprintf("/login?token=%s", confTok), http.NoBody) + require.NoError(t, err) + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code, "must return user JSON, not 307 to evil") + assert.Equal(t, "", rr.Header().Get("Location")) + assert.Contains(t, rr.Body.String(), `"name":"test123"`) + assert.NotContains(t, rr.Body.String(), "evil.example.com") +} + +func TestVerifyHandler_LoginAcceptConfirmFromAllowsAllowlistedHost(t *testing.T) { + jwtSvc := token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }) + e := VerifyHandler{ + ProviderName: "test", + TokenService: jwtSvc, + Issuer: "iss-test", + L: logger.Std, + AllowedRedirectHosts: token.AllowedHostsFunc(func() ([]string, error) { + return []string{"trusted.example.com"}, nil + }), + } + + confTok, err := jwtSvc.Token(token.Claims{ + Handshake: &token.Handshake{ + ID: "test123::blah@user.com", + From: "https://trusted.example.com/back", + }, + StandardClaims: jwt.StandardClaims{ + Audience: "remark42", + ExpiresAt: time.Now().Add(time.Hour).Unix(), + }, + }) + require.NoError(t, err) + + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", fmt.Sprintf("/login?token=%s", confTok), http.NoBody) + require.NoError(t, err) + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr, req) + + assert.Equal(t, http.StatusTemporaryRedirect, rr.Code, "must 307-redirect to trusted host") + assert.Equal(t, "https://trusted.example.com/back", rr.Header().Get("Location")) +} + func TestVerifyHandler_LoginAcceptConfirmWithAvatar(t *testing.T) { e := VerifyHandler{ ProviderName: "test", diff --git a/token/jwt.go b/token/jwt.go index 62dc40d8..7507a4b9 100644 --- a/token/jwt.go +++ b/token/jwt.go @@ -423,3 +423,23 @@ type AudienceFunc func() ([]string, error) func (f AudienceFunc) Get() ([]string, error) { return f() } + +// AllowedHosts defines interface returning list of hostnames allowed in the +// "from" redirect parameter of OAuth and verify flows. The service's own host +// (derived from Opts.URL) is always allowed implicitly; this list is for +// additional hosts. +type AllowedHosts interface { + Get() ([]string, error) +} + +// AllowedHostsFunc adapter to allow ordinary functions to be used as AllowedHosts. +// Assigning a nil AllowedHostsFunc to an interface field (e.g. +// Opts.AllowedRedirectHosts) produces a typed-nil interface that would panic +// when Get is called; the provider-side validator recognizes this form and +// treats it as "no allowlist configured". +type AllowedHostsFunc func() ([]string, error) + +// Get calls f() +func (f AllowedHostsFunc) Get() ([]string, error) { + return f() +} diff --git a/v2/middleware/auth_test.go b/v2/middleware/auth_test.go index eaae31e1..3d5ba540 100644 --- a/v2/middleware/auth_test.go +++ b/v2/middleware/auth_test.go @@ -339,7 +339,7 @@ func TestBasicAdminUserDoesNotLogPassword(t *testing.T) { const secret = "super-secret-attempted-passwd" var buf strings.Builder a := makeTestAuth(t) - a.L = logger.Func(func(format string, args ...interface{}) { + a.L = logger.Func(func(format string, args ...any) { fmt.Fprintf(&buf, format, args...) }) diff --git a/v2/provider/redirect_test.go b/v2/provider/redirect_test.go index 7b455b1b..5a506371 100644 --- a/v2/provider/redirect_test.go +++ b/v2/provider/redirect_test.go @@ -65,6 +65,32 @@ func TestIsAllowedRedirect(t *testing.T) { allowed: allowedFn("admin.example.com"), want: true}, {name: "malformed service URL with empty allowlist rejects", from: "https://admin.example.com", serviceURL: "://bad", allowed: allowedFn(), want: false}, + + // bypass-category characterization. None of these URLs grant access to + // hosts outside the allowlist; the test pins that behavior so a future + // refactor of url.Parse usage doesn't silently weaken it. + {name: "scheme-relative URL rejected (no scheme)", from: "//evil.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "userinfo does not bypass: allowed-looking user, evil host", from: "https://admin.example.com@evil.com/x", + serviceURL: "https://app.example.com", allowed: allowedFn("admin.example.com"), want: false}, + {name: "userinfo does not change allowed host check (evil userinfo, allowed host)", from: "https://evil@admin.example.com/x", + serviceURL: "https://app.example.com", allowed: allowedFn("admin.example.com"), want: true}, + {name: "IPv6 literal not in allowlist rejected", from: "https://[::1]/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "IPv6 literal explicitly allowed", from: "https://[::1]/x", serviceURL: "https://app.example.com", + allowed: allowedFn("::1"), want: true}, + {name: "IDN homoglyph not equal to ASCII allowlist entry", from: "https://аpp.example.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("app.example.com"), want: false}, + {name: "punycode form of homoglyph not equal to ASCII allowlist entry", from: "https://xn--pp-7lc.example.com/x", + serviceURL: "https://app.example.com", allowed: allowedFn("app.example.com"), want: false}, + {name: "percent-encoded host parse-fails rejected", from: "https://%65vil.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "backslash userinfo trick parse-fails rejected", from: "https://evil.com\\@admin.example.com/x", + serviceURL: "https://app.example.com", allowed: allowedFn("admin.example.com"), want: false}, + {name: "scheme without // (opaque) rejected", from: "https:evil.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, + {name: "triple-slash empties host rejected", from: "https:///evil.com/x", serviceURL: "https://app.example.com", + allowed: allowedFn("admin.example.com"), want: false}, } for _, tt := range tests { diff --git a/v2/provider/sender/email_test.go b/v2/provider/sender/email_test.go index b888336d..112f1e13 100644 --- a/v2/provider/sender/email_test.go +++ b/v2/provider/sender/email_test.go @@ -59,7 +59,7 @@ func TestEmail_New(t *testing.T) { func TestEmail_SendDoesNotLogBody(t *testing.T) { const secretBody = "Confirmation token: super-secret-magic-link-XYZ" var buf strings.Builder - capturing := logger.Func(func(format string, args ...interface{}) { + capturing := logger.Func(func(format string, args ...any) { fmt.Fprintf(&buf, format, args...) }) p := EmailParams{Host: "127.0.0.2", Port: 25, From: "from@example.com", diff --git a/v2/provider/verify.go b/v2/provider/verify.go index 030007a0..737c60ba 100644 --- a/v2/provider/verify.go +++ b/v2/provider/verify.go @@ -174,6 +174,13 @@ func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request) Handshake: &token.Handshake{ State: "", ID: user + "::" + address, + // without copying "from" here the redirect validator at the + // other end has nothing to validate or to redirect to. The + // docs (and #275) advertise ?from= on the verify login + // path, but the original sendConfirmation never put it on + // the handshake JWT, so production verify flows could never + // honor from at all. + From: r.URL.Query().Get("from"), }, SessionOnly: r.URL.Query().Get("session") != "" && r.URL.Query().Get("session") != "0", RegisteredClaims: jwt.RegisteredClaims{ diff --git a/v2/provider/verify_test.go b/v2/provider/verify_test.go index b84df302..b9a4edd1 100644 --- a/v2/provider/verify_test.go +++ b/v2/provider/verify_test.go @@ -133,6 +133,53 @@ func TestVerifyHandler_LoginAcceptConfirm(t *testing.T) { assert.Equal(t, true, claims.SessionOnly) } +// TestVerifyHandler_PublicFromFlow_RejectsExternalHost drives the public flow: +// /login?from=... -> sendConfirmation -> email-link click -> /login?token=... +// -> validator rejects. Catches regressions where sendConfirmation drops the +// from query param (the JWT then carries an empty Handshake.From, the +// validator never fires, and production silently ignores the redirect). +func TestVerifyHandler_PublicFromFlow_RejectsExternalHost(t *testing.T) { + emailer := &mockSender{} + jwtSvc := token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }) + e := VerifyHandler{ + ProviderName: "test", + TokenService: jwtSvc, + Issuer: "iss-test", + L: logger.Std, + Sender: SenderFunc(emailer.Send), + Template: "token:{{.Token}}", + AllowedRedirectHosts: token.AllowedHostsFunc(func() ([]string, error) { return nil, nil }), + } + + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", + "/login?address=blah@user.com&user=test123&site=remark42&from=https://evil.example.com/phish", + http.NoBody) + require.NoError(t, err) + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr, req) + require.Equal(t, http.StatusOK, rr.Code) + tknStr := strings.TrimPrefix(emailer.text, "token:") + require.NotEmpty(t, tknStr, "sendConfirmation must produce a token") + + parsed, err := jwtSvc.Parse(tknStr) + require.NoError(t, err) + require.NotNil(t, parsed.Handshake) + require.Equal(t, "https://evil.example.com/phish", parsed.Handshake.From, + "sendConfirmation must propagate ?from to handshake JWT, otherwise the redirect validator never runs in production") + + rr2 := httptest.NewRecorder() + req2, err := http.NewRequest("GET", "/login?token="+tknStr, http.NoBody) + require.NoError(t, err) + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr2, req2) + assert.Equal(t, http.StatusOK, rr2.Code, "must return user JSON, not 307 to evil") + assert.Equal(t, "", rr2.Header().Get("Location")) + assert.NotContains(t, rr2.Body.String(), "evil.example.com") +} + func TestVerifyHandler_LoginAcceptConfirmFromRejectsExternalHost(t *testing.T) { jwtSvc := token.NewService(token.Opts{ SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }),