From 2dba4503266436a615f4c1c18e07aa36ec713498 Mon Sep 17 00:00:00 2001 From: David Laban Date: Mon, 10 Aug 2020 16:50:25 +0100 Subject: [PATCH] feat: use uri-reference for ui_url etc. to allow relative urls (#617) --- .schema/config.schema.json | 27 +++++++++++---------- x/http_secure_redirect_test.go | 43 ++++++++++++++++++++++++---------- 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/.schema/config.schema.json b/.schema/config.schema.json index 7680dd46aa3..4a23e1c0be5 100644 --- a/.schema/config.schema.json +++ b/.schema/config.schema.json @@ -13,10 +13,11 @@ "title": "Redirect browsers to set URL per default", "description": "ORY Kratos redirects to this URL per default on completion of self-service flows and other browser interaction. Read this [article for more information on browser redirects](https://www.ory.sh/kratos/docs/concepts/browser-redirect-flow-completion).", "type": "string", - "format": "uri", - "minLength": 6, + "format": "uri-reference", + "minLength": 1, "examples": [ - "https://my-app.com/dashboard" + "https://my-app.com/dashboard", + "/dashboard" ] }, "selfServiceSessionRevokerHook": { @@ -274,11 +275,12 @@ "type": "array", "items": { "type": "string", - "format": "uri" + "format": "uri-reference" }, "examples": [ [ "https://app.my-app.com/dashboard", + "/dashboard", "https://www.my-app.com/" ] ], @@ -296,7 +298,7 @@ "title": "URL of the Settings page.", "description": "URL where the Settings UI is hosted. Check the [reference implementation](https://github.com/ory/kratos-selfservice-ui-node).", "type": "string", - "format": "uri", + "format": "uri-reference", "examples": [ "https://my-app.com/user/settings" ], @@ -350,7 +352,7 @@ "title": "Registration UI URL", "description": "URL where the Registration UI is hosted. Check the [reference implementation](https://github.com/ory/kratos-selfservice-ui-node).", "type": "string", - "format": "uri", + "format": "uri-reference", "examples": [ "https://my-app.com/signup" ], @@ -379,7 +381,7 @@ "title": "Login UI URL", "description": "URL where the Login UI is hosted. Check the [reference implementation](https://github.com/ory/kratos-selfservice-ui-node).", "type": "string", - "format": "uri", + "format": "uri-reference", "examples": [ "https://my-app.com/login" ], @@ -415,7 +417,7 @@ "title": "Verify UI URL", "description": "URL where the ORY Verify UI is hosted. This is the page where users activate and / or verify their email or telephone number. Check the [reference implementation](https://github.com/ory/kratos-selfservice-ui-node).", "type": "string", - "format": "uri", + "format": "uri-reference", "examples": [ "https://my-app.com/verify" ], @@ -459,7 +461,7 @@ "title": "Recovery UI URL", "description": "URL where the ORY Recovery UI is hosted. This is the page where users request and complete account recovery. Check the [reference implementation](https://github.com/ory/kratos-selfservice-ui-node).", "type": "string", - "format": "uri", + "format": "uri-reference", "examples": [ "https://my-app.com/verify" ], @@ -496,7 +498,7 @@ "title": "ORY Kratos Error UI URL", "description": "URL where the ORY Kratos Error UI is hosted. Check the [reference implementation](https://github.com/ory/kratos-selfservice-ui-node).", "type": "string", - "format": "uri", + "format": "uri-reference", "examples": [ "https://my-app.com/kratos-error" ], @@ -671,9 +673,10 @@ "title": "Public Base URL", "description": "The URL where the public endpoint is exposed at.", "type": "string", - "format": "uri", + "format": "uri-reference", "examples": [ - "https://my-app.com/.ory/kratos/public" + "https://my-app.com/.ory/kratos/public", + "/.ory/kratos/public/" ] }, "host": { diff --git a/x/http_secure_redirect_test.go b/x/http_secure_redirect_test.go index 6120525465f..a341dd3cdbe 100644 --- a/x/http_secure_redirect_test.go +++ b/x/http_secure_redirect_test.go @@ -75,9 +75,8 @@ func TestSecureContentNegotiationRedirection(t *testing.T) { } func TestSecureRedirectTo(t *testing.T) { - const defaultReturnTo = "/default-return-to" - var newServer = func(t *testing.T, isTLS bool, expectErr bool, opts func(ts *httptest.Server) []x.SecureRedirectOption) *httptest.Server { + var newServer = func(t *testing.T, isTLS bool, isRelative bool, expectErr bool, opts func(ts *httptest.Server) []x.SecureRedirectOption) *httptest.Server { var ts *httptest.Server f := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if opts == nil { @@ -85,7 +84,11 @@ func TestSecureRedirectTo(t *testing.T) { return nil } } - ru, err := x.SecureRedirectTo(r, urlx.ParseOrPanic(ts.URL+defaultReturnTo), opts(ts)...) + defaultReturnTo := "/default-return-to" + if !isRelative { + defaultReturnTo = ts.URL+defaultReturnTo + } + ru, err := x.SecureRedirectTo(r, urlx.ParseOrPanic(defaultReturnTo), opts(ts)...) if expectErr { require.Error(t, err) _, _ = w.Write([]byte("error")) @@ -117,21 +120,37 @@ func TestSecureRedirectTo(t *testing.T) { } t.Run("case=return to default URL if nothing is allowed", func(t *testing.T) { - s := newServer(t, false, false, nil) + s := newServer(t, false, false, false, nil) _, body := makeRequest(t, s, "?return_to=/foo") assert.EqualValues(t, body, s.URL+"/default-return-to") }) t.Run("case=return to foo with server baseURL if allowed", func(t *testing.T) { - s := newServer(t, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { + s := newServer(t, false, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { return []x.SecureRedirectOption{x.SecureRedirectAllowURLs([]url.URL{*urlx.ParseOrPanic(ts.URL)})} }) _, body := makeRequest(t, s, "?return_to=/foo") assert.Equal(t, body, s.URL+"/foo") }) + t.Run("case=return to a relative path works", func(t *testing.T) { + s := newServer(t, false, true, false, func(ts *httptest.Server) []x.SecureRedirectOption { + return []x.SecureRedirectOption{x.SecureRedirectAllowURLs([]url.URL{*urlx.ParseOrPanic("/foo")})} + }) + _, body := makeRequest(t, s, "?return_to=/foo/kratos") + assert.Equal(t, body, "/foo/kratos") + }) + + t.Run("case=return to a fully qualified domain is forbidden if whitelist is relative", func(t *testing.T) { + s := newServer(t, false, true, true, func(ts *httptest.Server) []x.SecureRedirectOption { + return []x.SecureRedirectOption{x.SecureRedirectAllowURLs([]url.URL{*urlx.ParseOrPanic("/foo")})} + }) + _, body := makeRequest(t, s, "?return_to=https://www.ory.sh/foo/kratos") + assert.Equal(t, body, "error") + }) + t.Run("case=return to another domain works", func(t *testing.T) { - s := newServer(t, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { + s := newServer(t, false, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { return []x.SecureRedirectOption{x.SecureRedirectAllowURLs([]url.URL{*urlx.ParseOrPanic("https://www.ory.sh/foo")})} }) _, body := makeRequest(t, s, "?return_to=https://www.ory.sh/foo/kratos") @@ -139,7 +158,7 @@ func TestSecureRedirectTo(t *testing.T) { }) t.Run("case=return to another domain fails if host mismatches", func(t *testing.T) { - s := newServer(t, false, true, func(ts *httptest.Server) []x.SecureRedirectOption { + s := newServer(t, false, false, true, func(ts *httptest.Server) []x.SecureRedirectOption { return []x.SecureRedirectOption{x.SecureRedirectAllowURLs([]url.URL{*urlx.ParseOrPanic("https://www.not-ory.sh/")})} }) _, body := makeRequest(t, s, "?return_to=https://www.ory.sh/kratos") @@ -147,7 +166,7 @@ func TestSecureRedirectTo(t *testing.T) { }) t.Run("case=return to another domain fails if path mismatches", func(t *testing.T) { - s := newServer(t, false, true, func(ts *httptest.Server) []x.SecureRedirectOption { + s := newServer(t, false, false, true, func(ts *httptest.Server) []x.SecureRedirectOption { return []x.SecureRedirectOption{x.SecureRedirectAllowURLs([]url.URL{*urlx.ParseOrPanic("https://www.ory.sh/not-kratos")})} }) _, body := makeRequest(t, s, "?return_to=https://www.ory.sh/kratos") @@ -155,7 +174,7 @@ func TestSecureRedirectTo(t *testing.T) { }) t.Run("case=return to another domain fails if scheme mismatches", func(t *testing.T) { - s := newServer(t, false, true, func(ts *httptest.Server) []x.SecureRedirectOption { + s := newServer(t, false, false, true, func(ts *httptest.Server) []x.SecureRedirectOption { return []x.SecureRedirectOption{x.SecureRedirectAllowURLs([]url.URL{*urlx.ParseOrPanic("http://www.ory.sh/")})} }) _, body := makeRequest(t, s, "?return_to=https://www.ory.sh/kratos") @@ -163,7 +182,7 @@ func TestSecureRedirectTo(t *testing.T) { }) t.Run("case=should work with self-service modifier", func(t *testing.T) { - s := newServer(t, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { + s := newServer(t, false, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { return []x.SecureRedirectOption{x.SecureRedirectAllowSelfServiceURLs(urlx.ParseOrPanic(ts.URL))} }) _, body := makeRequest(t, s, "?return_to=/self-service/foo") @@ -171,7 +190,7 @@ func TestSecureRedirectTo(t *testing.T) { }) t.Run("case=should work with default return to", func(t *testing.T) { - s := newServer(t, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { + s := newServer(t, false, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { return []x.SecureRedirectOption{x.SecureRedirectOverrideDefaultReturnTo(urlx.ParseOrPanic(ts.URL + "/another-default"))} }) _, body := makeRequest(t, s, "") @@ -179,7 +198,7 @@ func TestSecureRedirectTo(t *testing.T) { }) t.Run("case=should override return_to", func(t *testing.T) { - s := newServer(t, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { + s := newServer(t, false, false, false, func(ts *httptest.Server) []x.SecureRedirectOption { return []x.SecureRedirectOption{ x.SecureRedirectAllowURLs([]url.URL{*urlx.ParseOrPanic(ts.URL)}), x.SecureRedirectUseSourceURL("https://foo/bar?return_to=/override"),