From 2e86f270cd0f36a7f1e6a5faf741c0f1b0a08788 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Michaud?= Date: Sat, 18 Jan 2020 15:57:42 +0100 Subject: [PATCH] Encode URL set to rd parameter. (#559) * Encode URL set to rd parameter. URL encoding that parameter solves PR #476. Some URL parameters set during redirection were magically disappearing after the redirection due to the authentication process. By using URL encoding, those parameters should not be stripped anymore. * Fix integration tests. --- internal/handlers/handler_verify.go | 2 +- internal/handlers/handler_verify_test.go | 19 ++++++ internal/suites/action_visit.go | 6 +- .../suites/scenario_redirection_url_test.go | 62 +++++++++++++++++++ internal/suites/suite_kubernetes_test.go | 4 ++ internal/suites/suite_standalone_test.go | 13 +++- internal/suites/suite_traefik_test.go | 4 ++ web/src/views/LoginPortal/LoginPortal.tsx | 2 +- 8 files changed, 105 insertions(+), 7 deletions(-) create mode 100644 internal/suites/scenario_redirection_url_test.go diff --git a/internal/handlers/handler_verify.go b/internal/handlers/handler_verify.go index 41eb49de1..1e20b5aa3 100644 --- a/internal/handlers/handler_verify.go +++ b/internal/handlers/handler_verify.go @@ -228,7 +228,7 @@ func VerifyGet(ctx *middlewares.AutheliaCtx) { // is computed from X-Fowarded-* headers or X-Original-URL. rd := string(ctx.QueryArgs().Peek("rd")) if rd != "" { - redirectionURL := fmt.Sprintf("%s?rd=%s", rd, targetURL.String()) + redirectionURL := fmt.Sprintf("%s?rd=%s", rd, url.QueryEscape(targetURL.String())) if strings.Contains(redirectionURL, "/%23/") { ctx.Logger.Warn("Characters /%23/ have been detected in redirection URL. This is not needed anymore, please strip it") } diff --git a/internal/handlers/handler_verify_test.go b/internal/handlers/handler_verify_test.go index b1e3f5078..035dbfe8d 100644 --- a/internal/handlers/handler_verify_test.go +++ b/internal/handlers/handler_verify_test.go @@ -503,3 +503,22 @@ func TestShouldKeepSessionWhenInactivityTimeoutHasNotBeenExceeded(t *testing.T) assert.Equal(t, "john", newUserSession.Username) assert.Equal(t, authentication.TwoFactor, newUserSession.AuthenticationLevel) } + +func TestShouldURLEncodeRedirectionURLParameter(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + userSession := mock.Ctx.GetSession() + userSession.Username = "john" + userSession.AuthenticationLevel = authentication.NotAuthenticated + mock.Ctx.SaveSession(userSession) + + mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com") + mock.Ctx.Request.SetHost("mydomain.com") + mock.Ctx.Request.SetRequestURI("/?rd=https://auth.mydomain.com") + + VerifyGet(mock.Ctx) + + assert.Equal(t, "Found. Redirecting to https://auth.mydomain.com?rd=https%3A%2F%2Ftwo-factor.example.com", + string(mock.Ctx.Response.Body())) +} diff --git a/internal/suites/action_visit.go b/internal/suites/action_visit.go index 76a4b1d50..e9be4f574 100644 --- a/internal/suites/action_visit.go +++ b/internal/suites/action_visit.go @@ -13,9 +13,9 @@ func (wds *WebDriverSession) doVisit(t *testing.T, url string) { assert.NoError(t, err) } -func (wds *WebDriverSession) doVisitAndVerifyURLIs(ctx context.Context, t *testing.T, url string) { +func (wds *WebDriverSession) doVisitAndVerifyOneFactorStep(ctx context.Context, t *testing.T, url string) { wds.doVisit(t, url) - wds.verifyURLIs(ctx, t, url) + wds.verifyIsFirstFactorPage(ctx, t) } func (wds *WebDriverSession) doVisitLoginPage(ctx context.Context, t *testing.T, targetURL string) { @@ -23,5 +23,5 @@ func (wds *WebDriverSession) doVisitLoginPage(ctx context.Context, t *testing.T, if targetURL != "" { suffix = fmt.Sprintf("?rd=%s", targetURL) } - wds.doVisitAndVerifyURLIs(ctx, t, fmt.Sprintf("%s/%s", LoginBaseURL, suffix)) + wds.doVisitAndVerifyOneFactorStep(ctx, t, fmt.Sprintf("%s/%s", LoginBaseURL, suffix)) } diff --git a/internal/suites/scenario_redirection_url_test.go b/internal/suites/scenario_redirection_url_test.go new file mode 100644 index 000000000..3f20f017d --- /dev/null +++ b/internal/suites/scenario_redirection_url_test.go @@ -0,0 +1,62 @@ +package suites + +import ( + "context" + "fmt" + "log" + "testing" + "time" + + "github.com/stretchr/testify/suite" +) + +type RedirectionURLScenario struct { + *SeleniumSuite +} + +func NewRedirectionURLScenario() *RedirectionURLScenario { + return &RedirectionURLScenario{ + SeleniumSuite: new(SeleniumSuite), + } +} + +func (rus *RedirectionURLScenario) SetupSuite() { + wds, err := StartWebDriver() + + if err != nil { + log.Fatal(err) + } + + rus.WebDriverSession = wds +} + +func (rus *RedirectionURLScenario) TearDownSuite() { + err := rus.WebDriverSession.Stop() + + if err != nil { + log.Fatal(err) + } +} + +func (rus *RedirectionURLScenario) SetupTest() { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + rus.doLogout(ctx, rus.T()) + rus.doVisit(rus.T(), HomeBaseURL) + rus.verifyIsHome(ctx, rus.T()) +} + +func (rus *RedirectionURLScenario) TestShouldVerifyCustomURLParametersArePropagatedAfterRedirection() { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + targetURL := fmt.Sprintf("%s/secret.html?myparam=test", SingleFactorBaseURL) + rus.doLoginOneFactor(ctx, rus.T(), "john", "password", false, targetURL) + rus.verifySecretAuthorized(ctx, rus.T()) + rus.verifyURLIs(ctx, rus.T(), targetURL) +} + +func TestRedirectionURLScenario(t *testing.T) { + suite.Run(t, NewRedirectionURLScenario()) +} diff --git a/internal/suites/suite_kubernetes_test.go b/internal/suites/suite_kubernetes_test.go index 8162e882c..4f74e5751 100644 --- a/internal/suites/suite_kubernetes_test.go +++ b/internal/suites/suite_kubernetes_test.go @@ -22,6 +22,10 @@ func (s *KubernetesSuite) TestTwoFactorScenario() { suite.Run(s.T(), NewTwoFactorScenario()) } +func (s *KubernetesSuite) TestRedirectionURLScenario() { + suite.Run(s.T(), NewRedirectionURLScenario()) +} + func TestKubernetesSuite(t *testing.T) { suite.Run(t, NewKubernetesSuite()) } diff --git a/internal/suites/suite_standalone_test.go b/internal/suites/suite_standalone_test.go index 9842e412f..4755c12ed 100644 --- a/internal/suites/suite_standalone_test.go +++ b/internal/suites/suite_standalone_test.go @@ -6,6 +6,7 @@ import ( "io/ioutil" "log" "net/http" + "net/url" "testing" "time" @@ -133,7 +134,9 @@ func (s *StandaloneSuite) TestShouldVerifyAPIVerifyRedirectFromXOriginalURL() { s.Assert().Equal(res.StatusCode, 302) body, err := ioutil.ReadAll(res.Body) s.Assert().NoError(err) - s.Assert().Equal(string(body), fmt.Sprintf("Found. Redirecting to %s?rd=%s", LoginBaseURL, AdminBaseURL)) + + urlEncodedAdminURL := url.QueryEscape(AdminBaseURL) + s.Assert().Equal(fmt.Sprintf("Found. Redirecting to %s?rd=%s", LoginBaseURL, urlEncodedAdminURL), string(body)) } func (s *StandaloneSuite) TestShouldVerifyAPIVerifyRedirectFromXOriginalHostURI() { @@ -149,7 +152,9 @@ func (s *StandaloneSuite) TestShouldVerifyAPIVerifyRedirectFromXOriginalHostURI( s.Assert().Equal(res.StatusCode, 302) body, err := ioutil.ReadAll(res.Body) s.Assert().NoError(err) - s.Assert().Equal(string(body), fmt.Sprintf("Found. Redirecting to %s?rd=https://secure.example.com:8080/", LoginBaseURL)) + + urlEncodedAdminURL := url.QueryEscape(SecureBaseURL + "/") + s.Assert().Equal(fmt.Sprintf("Found. Redirecting to %s?rd=%s", LoginBaseURL, urlEncodedAdminURL), string(body)) } func (s *StandaloneSuite) TestStandaloneWebDriverScenario() { @@ -180,6 +185,10 @@ func (s *StandaloneSuite) TestAvailableMethodsScenario() { suite.Run(s.T(), NewAvailableMethodsScenario([]string{"ONE-TIME PASSWORD"})) } +func (s *StandaloneSuite) TestRedirectionURLScenario() { + suite.Run(s.T(), NewRedirectionURLScenario()) +} + func TestStandaloneSuite(t *testing.T) { suite.Run(t, NewStandaloneSuite()) } diff --git a/internal/suites/suite_traefik_test.go b/internal/suites/suite_traefik_test.go index cc7505eea..d23b713bf 100644 --- a/internal/suites/suite_traefik_test.go +++ b/internal/suites/suite_traefik_test.go @@ -22,6 +22,10 @@ func (s *TraefikSuite) TestTwoFactorScenario() { suite.Run(s.T(), NewTwoFactorScenario()) } +func (s *TraefikSuite) TestRedirectionURLScenario() { + suite.Run(s.T(), NewRedirectionURLScenario()) +} + func TestTraefikSuite(t *testing.T) { suite.Run(t, NewTraefikSuite()) } diff --git a/web/src/views/LoginPortal/LoginPortal.tsx b/web/src/views/LoginPortal/LoginPortal.tsx index dfff8f25f..5be67d9f7 100644 --- a/web/src/views/LoginPortal/LoginPortal.tsx +++ b/web/src/views/LoginPortal/LoginPortal.tsx @@ -71,7 +71,7 @@ export default function () { useEffect(() => { if (state) { const redirectionSuffix = redirectionURL - ? `?rd=${encodeURI(redirectionURL)}` + ? `?rd=${encodeURIComponent(redirectionURL)}` : ''; if (state.authentication_level === AuthenticationLevel.Unauthenticated) {