diff --git a/appview/state/login.go b/appview/state/login.go index c9174252..e923f091 100644 --- a/appview/state/login.go +++ b/appview/state/login.go @@ -106,7 +106,7 @@ func (s *State) Login(w http.ResponseWriter, r *http.Request) { } } - if err := s.oauth.SetAuthReturn(w, r, returnURL, addAccount); err != nil { + if err := s.oauth.SetAuthReturn(w, r, sanitizeReturnURL(returnURL), addAccount); err != nil { l.Error("failed to set auth return", "err", err) } @@ -125,6 +125,16 @@ func (s *State) Login(w http.ResponseWriter, r *http.Request) { } } +// sanitizeReturnURL ensures the return URL is a relative path on the same +// origin. Anything else — absolute URLs, protocol-relative URLs — is replaced +// with "/" to prevent open redirect after OAuth login. +func sanitizeReturnURL(s string) string { + if strings.HasPrefix(s, "/") && !strings.HasPrefix(s, "//") { + return s + } + return "/" +} + func (s *State) Logout(w http.ResponseWriter, r *http.Request) { l := s.logger.With("handler", "Logout") diff --git a/appview/state/login_test.go b/appview/state/login_test.go new file mode 100644 index 00000000..dd72c31c --- /dev/null +++ b/appview/state/login_test.go @@ -0,0 +1,28 @@ +package state + +import "testing" + +func TestSanitizeReturnURL(t *testing.T) { + cases := []struct { + input string + want string + }{ + {"/", "/"}, + {"/some/path", "/some/path"}, + {"/valid?query=1", "/valid?query=1"}, + {"/valid#anchor", "/valid#anchor"}, + // External URLs must be rejected. + {"https://evil.com", "/"}, + {"http://evil.com", "/"}, + // Protocol-relative URLs are treated as external by browsers. + {"//evil.com", "/"}, + {"//evil.com/phishing", "/"}, + // Empty string. + {"", "/"}, + } + for _, tc := range cases { + if got := sanitizeReturnURL(tc.input); got != tc.want { + t.Errorf("sanitizeReturnURL(%q) = %q, want %q", tc.input, got, tc.want) + } + } +}