From 72fc86164c38d23fbe5a7982a574bebfb4f58777 Mon Sep 17 00:00:00 2001 From: Livio Amstutz Date: Mon, 26 Apr 2021 14:31:26 +0200 Subject: [PATCH 1/2] fix: allow loopback redirect_uri for native apps --- pkg/op/authrequest.go | 65 ++++++++++++---- pkg/op/authrequest_test.go | 151 ++++++++++++++++++++++++++++++++++--- 2 files changed, 192 insertions(+), 24 deletions(-) diff --git a/pkg/op/authrequest.go b/pkg/op/authrequest.go index 3a79b9b..765027f 100644 --- a/pkg/op/authrequest.go +++ b/pkg/op/authrequest.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "net/url" "strings" "time" @@ -157,32 +158,66 @@ func ValidateAuthReqRedirectURI(client Client, uri string, responseType oidc.Res if uri == "" { return ErrInvalidRequestRedirectURI("The redirect_uri is missing in the request. Please ensure it is added to the request. If you have any questions, you may contact the administrator of the application.") } + if strings.HasPrefix(uri, "https://") { + if !utils.Contains(client.RedirectURIs(), uri) { + return ErrInvalidRequestRedirectURI("The requested redirect_uri is missing in the client configuration. If you have any questions, you may contact the administrator of the application.") + } + return nil + } + if client.ApplicationType() == ApplicationTypeNative { + return validateAuthReqRedirectURINative(client, uri, responseType) + } if !utils.Contains(client.RedirectURIs(), uri) { return ErrInvalidRequestRedirectURI("The requested redirect_uri is missing in the client configuration. If you have any questions, you may contact the administrator of the application.") } - if client.DevMode() { - return nil - } - if strings.HasPrefix(uri, "https://") { - return nil - } - if responseType == oidc.ResponseTypeCode { - if strings.HasPrefix(uri, "http://") && IsConfidentialType(client) { + if strings.HasPrefix(uri, "http://") { + if client.DevMode() { return nil } - if !strings.HasPrefix(uri, "http://") && client.ApplicationType() == ApplicationTypeNative { + if responseType == oidc.ResponseTypeCode && IsConfidentialType(client) { return nil } return ErrInvalidRequest("This client's redirect_uri is http and is not allowed. If you have any questions, you may contact the administrator of the application.") - } else { - if client.ApplicationType() != ApplicationTypeNative { - return ErrInvalidRequestRedirectURI("Http is only allowed for native applications. Please change your redirect uri try again. If you have any questions, you may contact the administrator of the application.") + } + return ErrInvalidRequest("This client's redirect_uri is using a custom schema and is not allowed. If you have any questions, you may contact the administrator of the application.") +} + +//ValidateAuthReqRedirectURINative validates the passed redirect_uri and response_type to the registered uris and client type +func validateAuthReqRedirectURINative(client Client, uri string, responseType oidc.ResponseType) error { + parsedURL, isLoopback := LoopbackOrLocalhost(uri) + isCustomSchema := !strings.HasPrefix(uri, "http://") + if utils.Contains(client.RedirectURIs(), uri) { + if isLoopback || isCustomSchema { + return nil } - if !(strings.HasPrefix(uri, "http://localhost:") || strings.HasPrefix(uri, "http://localhost/")) { - return ErrInvalidRequestRedirectURI("Http is only allowed for localhost uri. Please change your redirect uri try again. If you have any questions, you may contact the administrator of the application at:") + return ErrInvalidRequest("This client's redirect_uri is http and is not allowed. If you have any questions, you may contact the administrator of the application.") + } + if !isLoopback { + return ErrInvalidRequestRedirectURI("The requested redirect_uri is missing in the client configuration. If you have any questions, you may contact the administrator of the application.") + } + for _, uri := range client.RedirectURIs() { + redirectURI, ok := LoopbackOrLocalhost(uri) + if ok && equalURI(parsedURL, redirectURI) { + return nil } } - return nil + return ErrInvalidRequestRedirectURI("The requested redirect_uri is missing in the client configuration. If you have any questions, you may contact the administrator of the application.") +} + +func equalURI(url1, url2 *url.URL) bool { + return url1.Path == url2.Path && url1.RawQuery == url2.RawQuery +} + +func LoopbackOrLocalhost(rawurl string) (*url.URL, bool) { + parsedURL, err := url.Parse(rawurl) + if err != nil { + return nil, false + } + hostName := parsedURL.Hostname() + return parsedURL, parsedURL.Scheme == "http" && + hostName == "localhost" || + hostName == "127.0.0.1" || + hostName == "::1" } //ValidateAuthReqResponseType validates the passed response_type to the registered response types diff --git a/pkg/op/authrequest_test.go b/pkg/op/authrequest_test.go index 3856acd..8427079 100644 --- a/pkg/op/authrequest_test.go +++ b/pkg/op/authrequest_test.go @@ -274,28 +274,112 @@ func TestValidateAuthReqRedirectURI(t *testing.T) { true, }, { - "unregistered fails", + "unregistered https fails", args{"https://unregistered.com/callback", mock.NewClientWithConfig(t, []string{"https://registered.com/callback"}, op.ApplicationTypeWeb, nil, false), oidc.ResponseTypeCode}, true, }, { - "code flow registered http not confidential fails", - args{"http://registered.com/callback", - mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeUserAgent, nil, false), + "unregistered http fails", + args{"http://unregistered.com/callback", + mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeWeb, nil, false), oidc.ResponseTypeCode}, true, }, { - "code flow registered http confidential ok", + "code flow registered https web ok", + args{"https://registered.com/callback", + mock.NewClientWithConfig(t, []string{"https://registered.com/callback"}, op.ApplicationTypeWeb, nil, false), + oidc.ResponseTypeCode}, + false, + }, + { + "code flow registered https native ok", + args{"https://registered.com/callback", + mock.NewClientWithConfig(t, []string{"https://registered.com/callback"}, op.ApplicationTypeNative, nil, false), + oidc.ResponseTypeCode}, + false, + }, + { + "code flow registered https user agent ok", + args{"https://registered.com/callback", + mock.NewClientWithConfig(t, []string{"https://registered.com/callback"}, op.ApplicationTypeUserAgent, nil, false), + oidc.ResponseTypeCode}, + false, + }, + { + "code flow registered http confidential (web) ok", args{"http://registered.com/callback", mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeWeb, nil, false), oidc.ResponseTypeCode}, false, }, { - "code flow registered custom not native fails", + "code flow registered http not confidential (user agent) fails", + args{"http://registered.com/callback", + mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeUserAgent, nil, false), + oidc.ResponseTypeCode}, + true, + }, + { + "code flow registered http not confidential (native) fails", + args{"http://registered.com/callback", + mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeNative, nil, false), + oidc.ResponseTypeCode}, + true, + }, + { + "code flow registered http localhost native ok", + args{"http://localhost:4200/callback", + mock.NewClientWithConfig(t, []string{"http://localhost/callback"}, op.ApplicationTypeNative, nil, false), + oidc.ResponseTypeCode}, + false, + }, + { + "code flow registered http loopback v4 native ok", + args{"http://127.0.0.1:4200/callback", + mock.NewClientWithConfig(t, []string{"http://127.0.0.1/callback"}, op.ApplicationTypeNative, nil, false), + oidc.ResponseTypeCode}, + false, + }, + { + "code flow registered http localhost native ok", + args{"http://[::1]:4200/callback", + mock.NewClientWithConfig(t, []string{"http://[::1]/callback"}, op.ApplicationTypeNative, nil, false), + oidc.ResponseTypeCode}, + false, + }, + { + "code flow unregistered http native fails", + args{"http://unregistered.com/callback", + mock.NewClientWithConfig(t, []string{"http://locahost/callback"}, op.ApplicationTypeNative, nil, false), + oidc.ResponseTypeCode}, + true, + }, + { + "code flow unregistered custom native fails", + args{"unregistered://callback", + mock.NewClientWithConfig(t, []string{"registered://callback"}, op.ApplicationTypeNative, nil, false), + oidc.ResponseTypeCode}, + true, + }, + { + "code flow unregistered loopback native fails", + args{"http://[::1]:4200/unregistered", + mock.NewClientWithConfig(t, []string{"http://[::1]:4200/callback"}, op.ApplicationTypeNative, nil, false), + oidc.ResponseTypeCode}, + true, + }, + { + "code flow registered custom not native (web) fails", + args{"custom://callback", + mock.NewClientWithConfig(t, []string{"custom://callback"}, op.ApplicationTypeWeb, nil, false), + oidc.ResponseTypeCode}, + true, + }, + { + "code flow registered custom not native (user agent) fails", args{"custom://callback", mock.NewClientWithConfig(t, []string{"custom://callback"}, op.ApplicationTypeUserAgent, nil, false), oidc.ResponseTypeCode}, @@ -311,7 +395,7 @@ func TestValidateAuthReqRedirectURI(t *testing.T) { { "code flow dev mode http ok", args{"http://registered.com/callback", - mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeNative, nil, true), + mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeUserAgent, nil, true), oidc.ResponseTypeCode}, false, }, @@ -355,12 +439,12 @@ func TestValidateAuthReqRedirectURI(t *testing.T) { args{"custom://callback", mock.NewClientWithConfig(t, []string{"custom://callback"}, op.ApplicationTypeNative, nil, false), oidc.ResponseTypeIDToken}, - true, + false, }, { "implicit flow dev mode http ok", args{"http://registered.com/callback", - mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeNative, nil, true), + mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeUserAgent, nil, true), oidc.ResponseTypeIDToken}, false, }, @@ -481,3 +565,52 @@ func TestAuthResponse(t *testing.T) { }) } } + +func Test_LoopbackOrLocalhost(t *testing.T) { + type args struct { + url string + } + tests := []struct { + name string + args args + want bool + }{ + { + "v4 no port ok", + args{url: "http://127.0.0.1/test"}, + true, + }, + { + "v6 no port ok", + args{url: "http://[::1]/test"}, + true, + }, + { + "locahost no port ok", + args{url: "http://localhost/test"}, + true, + }, + { + "v4 with port ok", + args{url: "http://127.0.0.1:4200/test"}, + true, + }, + { + "v6 with port ok", + args{url: "http://[::1]:4200/test"}, + true, + }, + { + "localhost with port ok", + args{url: "http://localhost:4200/test"}, + true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if _, got := op.LoopbackOrLocalhost(tt.args.url); got != tt.want { + t.Errorf("loopbackOrLocalhost() = %v, want %v", got, tt.want) + } + }) + } +} From 540a7bd7be7237fb9cb703a0a1c5de36d7c54677 Mon Sep 17 00:00:00 2001 From: Livio Amstutz Date: Thu, 29 Apr 2021 12:43:21 +0200 Subject: [PATCH 2/2] improve Loopback check --- pkg/op/authrequest.go | 15 ++++++++------- pkg/op/authrequest_test.go | 33 +++++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/pkg/op/authrequest.go b/pkg/op/authrequest.go index 765027f..8e6d8eb 100644 --- a/pkg/op/authrequest.go +++ b/pkg/op/authrequest.go @@ -3,6 +3,7 @@ package op import ( "context" "fmt" + "net" "net/http" "net/url" "strings" @@ -184,7 +185,7 @@ func ValidateAuthReqRedirectURI(client Client, uri string, responseType oidc.Res //ValidateAuthReqRedirectURINative validates the passed redirect_uri and response_type to the registered uris and client type func validateAuthReqRedirectURINative(client Client, uri string, responseType oidc.ResponseType) error { - parsedURL, isLoopback := LoopbackOrLocalhost(uri) + parsedURL, isLoopback := HTTPLoopbackOrLocalhost(uri) isCustomSchema := !strings.HasPrefix(uri, "http://") if utils.Contains(client.RedirectURIs(), uri) { if isLoopback || isCustomSchema { @@ -196,7 +197,7 @@ func validateAuthReqRedirectURINative(client Client, uri string, responseType oi return ErrInvalidRequestRedirectURI("The requested redirect_uri is missing in the client configuration. If you have any questions, you may contact the administrator of the application.") } for _, uri := range client.RedirectURIs() { - redirectURI, ok := LoopbackOrLocalhost(uri) + redirectURI, ok := HTTPLoopbackOrLocalhost(uri) if ok && equalURI(parsedURL, redirectURI) { return nil } @@ -208,16 +209,16 @@ func equalURI(url1, url2 *url.URL) bool { return url1.Path == url2.Path && url1.RawQuery == url2.RawQuery } -func LoopbackOrLocalhost(rawurl string) (*url.URL, bool) { +func HTTPLoopbackOrLocalhost(rawurl string) (*url.URL, bool) { parsedURL, err := url.Parse(rawurl) if err != nil { return nil, false } + if parsedURL.Scheme != "http" { + return nil, false + } hostName := parsedURL.Hostname() - return parsedURL, parsedURL.Scheme == "http" && - hostName == "localhost" || - hostName == "127.0.0.1" || - hostName == "::1" + return parsedURL, hostName == "localhost" || net.ParseIP(hostName).IsLoopback() } //ValidateAuthReqResponseType validates the passed response_type to the registered response types diff --git a/pkg/op/authrequest_test.go b/pkg/op/authrequest_test.go index 8427079..9bec1e7 100644 --- a/pkg/op/authrequest_test.go +++ b/pkg/op/authrequest_test.go @@ -316,16 +316,16 @@ func TestValidateAuthReqRedirectURI(t *testing.T) { false, }, { - "code flow registered http not confidential (user agent) fails", + "code flow registered http not confidential (native) fails", args{"http://registered.com/callback", - mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeUserAgent, nil, false), + mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeNative, nil, false), oidc.ResponseTypeCode}, true, }, { - "code flow registered http not confidential (native) fails", + "code flow registered http not confidential (user agent) fails", args{"http://registered.com/callback", - mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeNative, nil, false), + mock.NewClientWithConfig(t, []string{"http://registered.com/callback"}, op.ApplicationTypeUserAgent, nil, false), oidc.ResponseTypeCode}, true, }, @@ -344,7 +344,7 @@ func TestValidateAuthReqRedirectURI(t *testing.T) { false, }, { - "code flow registered http localhost native ok", + "code flow registered http loopback v6 native ok", args{"http://[::1]:4200/callback", mock.NewClientWithConfig(t, []string{"http://[::1]/callback"}, op.ApplicationTypeNative, nil, false), oidc.ResponseTypeCode}, @@ -420,6 +420,13 @@ func TestValidateAuthReqRedirectURI(t *testing.T) { oidc.ResponseTypeIDToken}, false, }, + { + "implicit flow registered http localhost web fails", + args{"http://localhost:9999/callback", + mock.NewClientWithConfig(t, []string{"http://localhost:9999/callback"}, op.ApplicationTypeWeb, nil, false), + oidc.ResponseTypeIDToken}, + true, + }, { "implicit flow registered http localhost user agent fails", args{"http://localhost:9999/callback", @@ -581,10 +588,15 @@ func Test_LoopbackOrLocalhost(t *testing.T) { true, }, { - "v6 no port ok", + "v6 short no port ok", args{url: "http://[::1]/test"}, true, }, + { + "v6 long no port ok", + args{url: "http://[0:0:0:0:0:0:0:1]/test"}, + true, + }, { "locahost no port ok", args{url: "http://localhost/test"}, @@ -596,10 +608,15 @@ func Test_LoopbackOrLocalhost(t *testing.T) { true, }, { - "v6 with port ok", + "v6 short with port ok", args{url: "http://[::1]:4200/test"}, true, }, + { + "v6 long with port ok", + args{url: "http://[0:0:0:0:0:0:0:1]:4200/test"}, + true, + }, { "localhost with port ok", args{url: "http://localhost:4200/test"}, @@ -608,7 +625,7 @@ func Test_LoopbackOrLocalhost(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if _, got := op.LoopbackOrLocalhost(tt.args.url); got != tt.want { + if _, got := op.HTTPLoopbackOrLocalhost(tt.args.url); got != tt.want { t.Errorf("loopbackOrLocalhost() = %v, want %v", got, tt.want) } })