From 4fdaed61f93bb11cb3bf047a4599d43cf50a4a09 Mon Sep 17 00:00:00 2001 From: wayne Date: Wed, 12 Jun 2024 21:57:40 +1200 Subject: [PATCH 01/31] Add option to set success and error pages for when calling AcquireInteractiveOption --- apps/internal/local/server.go | 16 ++++++++--- apps/public/public.go | 52 +++++++++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index fda5d7dd..cf1b7ec5 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -28,7 +28,7 @@ var okPage = []byte(` `) -const failPage = ` +var failPage = []byte(` @@ -40,7 +40,7 @@ const failPage = `

Error details: error %s error_description: %s

-` +`) // Result is the result from the redirect. type Result struct { @@ -60,7 +60,7 @@ type Server struct { } // New creates a local HTTP server and starts it. -func New(reqState string, port int) (*Server, error) { +func New(reqState string, port int, successPage []byte, errorPage []byte) (*Server, error) { var l net.Listener var err error var portStr string @@ -84,6 +84,14 @@ func New(reqState string, port int) (*Server, error) { return nil, err } + if len(successPage) > 0 { + okPage = successPage + } + + if len(errorPage) > 0 { + failPage = errorPage + } + serv := &Server{ Addr: fmt.Sprintf("http://localhost:%s", portStr), s: &http.Server{Addr: "localhost:0", ReadHeaderTimeout: time.Second}, @@ -145,7 +153,7 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { desc := html.EscapeString(q.Get("error_description")) // Note: It is a little weird we handle some errors by not going to the failPage. If they all should, // change this to s.error() and make s.error() write the failPage instead of an error code. - _, _ = w.Write([]byte(fmt.Sprintf(failPage, headerErr, desc))) + _, _ = w.Write([]byte(fmt.Sprintf(string(failPage), headerErr, desc))) s.putResult(Result{Err: fmt.Errorf(desc)}) return } diff --git a/apps/public/public.go b/apps/public/public.go index 392e5e43..9624c4f6 100644 --- a/apps/public/public.go +++ b/apps/public/public.go @@ -522,6 +522,8 @@ type interactiveAuthOptions struct { claims, domainHint, loginHint, redirectURI, tenantID string openURL func(url string) error authnScheme AuthenticationScheme + successPage []byte + errorPage []byte } // AcquireInteractiveOption is implemented by options for AcquireTokenInteractive @@ -529,6 +531,50 @@ type AcquireInteractiveOption interface { acquireInteractiveOption() } +func WithSuccessPage(successPage []byte) interface { + AcquireInteractiveOption + options.CallOption +} { + return struct { + AcquireInteractiveOption + options.CallOption + }{ + CallOption: options.NewCallOption( + func(a any) error { + switch t := a.(type) { + case *interactiveAuthOptions: + t.successPage = successPage + default: + return fmt.Errorf("unexpected options type %T", a) + } + return nil + }, + ), + } +} + +func WithErrorPage(errorPage []byte) interface { + AcquireInteractiveOption + options.CallOption +} { + return struct { + AcquireInteractiveOption + options.CallOption + }{ + CallOption: options.NewCallOption( + func(a any) error { + switch t := a.(type) { + case *interactiveAuthOptions: + t.errorPage = errorPage + default: + return fmt.Errorf("unexpected options type %T", a) + } + return nil + }, + ), + } +} + // WithLoginHint pre-populates the login prompt with a username. func WithLoginHint(username string) interface { AcquireInteractiveOption @@ -671,7 +717,7 @@ func (pca Client) AcquireTokenInteractive(ctx context.Context, scopes []string, if o.authnScheme != nil { authParams.AuthnScheme = o.authnScheme } - res, err := pca.browserLogin(ctx, redirectURL, authParams, o.openURL) + res, err := pca.browserLogin(ctx, redirectURL, authParams, o.openURL, o.successPage, o.successPage) if err != nil { return AuthResult{}, err } @@ -709,13 +755,13 @@ func parsePort(u *url.URL) (int, error) { } // browserLogin calls openURL and waits for a user to log in -func (pca Client) browserLogin(ctx context.Context, redirectURI *url.URL, params authority.AuthParams, openURL func(string) error) (interactiveAuthResult, error) { +func (pca Client) browserLogin(ctx context.Context, redirectURI *url.URL, params authority.AuthParams, openURL func(string) error, successPage []byte, errorPage []byte) (interactiveAuthResult, error) { // start local redirect server so login can call us back port, err := parsePort(redirectURI) if err != nil { return interactiveAuthResult{}, err } - srv, err := local.New(params.State, port) + srv, err := local.New(params.State, port, successPage, errorPage) if err != nil { return interactiveAuthResult{}, err } From 13193356a7983338f9d4579eec54f06daae0f5e6 Mon Sep 17 00:00:00 2001 From: wayne Date: Wed, 12 Jun 2024 22:24:50 +1200 Subject: [PATCH 02/31] fix: setting errorPage --- apps/public/public.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/public/public.go b/apps/public/public.go index 9624c4f6..fe24edd2 100644 --- a/apps/public/public.go +++ b/apps/public/public.go @@ -717,7 +717,7 @@ func (pca Client) AcquireTokenInteractive(ctx context.Context, scopes []string, if o.authnScheme != nil { authParams.AuthnScheme = o.authnScheme } - res, err := pca.browserLogin(ctx, redirectURL, authParams, o.openURL, o.successPage, o.successPage) + res, err := pca.browserLogin(ctx, redirectURL, authParams, o.openURL, o.successPage, o.errorPage) if err != nil { return AuthResult{}, err } From fa4a6be6ec5ce1377a513a5614ee22c86e07bcd5 Mon Sep 17 00:00:00 2001 From: wayne Date: Thu, 20 Jun 2024 22:41:19 +1200 Subject: [PATCH 03/31] Refactor using different function name WithSystemBrowserOptions to set error and success pages --- apps/public/public.go | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-) diff --git a/apps/public/public.go b/apps/public/public.go index fe24edd2..5b58f517 100644 --- a/apps/public/public.go +++ b/apps/public/public.go @@ -531,7 +531,7 @@ type AcquireInteractiveOption interface { acquireInteractiveOption() } -func WithSuccessPage(successPage []byte) interface { +func WithSystemBrowserOptions(successPage []byte, errorPage []byte) interface { AcquireInteractiveOption options.CallOption } { @@ -544,27 +544,6 @@ func WithSuccessPage(successPage []byte) interface { switch t := a.(type) { case *interactiveAuthOptions: t.successPage = successPage - default: - return fmt.Errorf("unexpected options type %T", a) - } - return nil - }, - ), - } -} - -func WithErrorPage(errorPage []byte) interface { - AcquireInteractiveOption - options.CallOption -} { - return struct { - AcquireInteractiveOption - options.CallOption - }{ - CallOption: options.NewCallOption( - func(a any) error { - switch t := a.(type) { - case *interactiveAuthOptions: t.errorPage = errorPage default: return fmt.Errorf("unexpected options type %T", a) From c110b76e6918fdc774b52b07cce480d1e02a8c54 Mon Sep 17 00:00:00 2001 From: wayne Date: Thu, 20 Jun 2024 23:48:23 +1200 Subject: [PATCH 04/31] update module name --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 74059617..affb7282 100644 --- a/go.mod +++ b/go.mod @@ -1,4 +1,4 @@ -module github.com/AzureAD/microsoft-authentication-library-for-go +module github.com/wayneforrest/microsoft-authentication-library-for-go go 1.18 From 587663fa0fa4e214b243438e241b6f27f65a2304 Mon Sep 17 00:00:00 2001 From: wayne Date: Fri, 21 Jun 2024 19:37:23 +1200 Subject: [PATCH 05/31] Revert failPage change, to be of type const string --- apps/internal/local/server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index cf1b7ec5..23cd6993 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -28,7 +28,7 @@ var okPage = []byte(` `) -var failPage = []byte(` +const failPage = ` @@ -40,7 +40,7 @@ var failPage = []byte(`

Error details: error %s error_description: %s

-`) +` // Result is the result from the redirect. type Result struct { @@ -153,7 +153,7 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { desc := html.EscapeString(q.Get("error_description")) // Note: It is a little weird we handle some errors by not going to the failPage. If they all should, // change this to s.error() and make s.error() write the failPage instead of an error code. - _, _ = w.Write([]byte(fmt.Sprintf(string(failPage), headerErr, desc))) + _, _ = w.Write([]byte(fmt.Sprintf(failPage, headerErr, desc))) s.putResult(Result{Err: fmt.Errorf(desc)}) return } From aa7dde869d6e1584f810827f0550786f9b559a9e Mon Sep 17 00:00:00 2001 From: wayne Date: Fri, 21 Jun 2024 19:47:09 +1200 Subject: [PATCH 06/31] Change success and error pages lifetime scope to be per "session", and remove the option error page string interpolation. --- apps/internal/local/server.go | 38 +++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index 23cd6993..b5879e90 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -53,10 +53,12 @@ type Result struct { // Server is an HTTP server. type Server struct { // Addr is the address the server is listening on. - Addr string - resultCh chan Result - s *http.Server - reqState string + Addr string + resultCh chan Result + s *http.Server + reqState string + optionSuccessPage []byte + optionErrorPage []byte } // New creates a local HTTP server and starts it. @@ -84,19 +86,13 @@ func New(reqState string, port int, successPage []byte, errorPage []byte) (*Serv return nil, err } - if len(successPage) > 0 { - okPage = successPage - } - - if len(errorPage) > 0 { - failPage = errorPage - } - serv := &Server{ - Addr: fmt.Sprintf("http://localhost:%s", portStr), - s: &http.Server{Addr: "localhost:0", ReadHeaderTimeout: time.Second}, - reqState: reqState, - resultCh: make(chan Result, 1), + Addr: fmt.Sprintf("http://localhost:%s", portStr), + s: &http.Server{Addr: "localhost:0", ReadHeaderTimeout: time.Second}, + reqState: reqState, + resultCh: make(chan Result, 1), + optionSuccessPage: successPage, + optionErrorPage: errorPage, } serv.s.Handler = http.HandlerFunc(serv.handler) @@ -153,7 +149,11 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { desc := html.EscapeString(q.Get("error_description")) // Note: It is a little weird we handle some errors by not going to the failPage. If they all should, // change this to s.error() and make s.error() write the failPage instead of an error code. + if len(s.optionErrorPage) > 0 { + _, _ = w.Write(s.optionErrorPage) + } else { _, _ = w.Write([]byte(fmt.Sprintf(failPage, headerErr, desc))) + } s.putResult(Result{Err: fmt.Errorf(desc)}) return } @@ -175,7 +175,11 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { return } - _, _ = w.Write(okPage) + if len(s.optionSuccessPage) > 0 { + _, _ = w.Write(s.optionSuccessPage) + } else { + _, _ = w.Write(okPage) + } s.putResult(Result{Code: code}) } From 4f65972e58dbdea5a11a01c22851e3d3df4a70ea Mon Sep 17 00:00:00 2001 From: wayne Date: Fri, 19 Jul 2024 07:21:03 +1200 Subject: [PATCH 07/31] Revert "update module name" This reverts commit 6a360744454a095b06b5dc62c79d5684ebfdeae0. --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index affb7282..74059617 100644 --- a/go.mod +++ b/go.mod @@ -1,4 +1,4 @@ -module github.com/wayneforrest/microsoft-authentication-library-for-go +module github.com/AzureAD/microsoft-authentication-library-for-go go 1.18 From 92a81c01710afae0a041a819fe217ec2a3b657dd Mon Sep 17 00:00:00 2001 From: wayne Date: Tue, 6 Aug 2024 22:23:16 +1200 Subject: [PATCH 08/31] Add tests for optional success and error pages --- apps/internal/local/server_test.go | 110 ++++++++++++++++++++--------- 1 file changed, 77 insertions(+), 33 deletions(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index 70af8b14..c649df17 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -20,53 +20,83 @@ func TestServer(t *testing.T) { defer cancel() tests := []struct { - desc string - reqState string - port int - q url.Values - failPage bool - statusCode int + desc string + reqState string + port int + q url.Values + failPage bool + statusCode int + successPage []byte + errorPage []byte }{ { - desc: "Error: Query Values has 'error' key", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"state"}, "error": []string{"error"}}, - statusCode: 200, - failPage: true, + desc: "Error: Query Values has 'error' key", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"state"}, "error": []string{"error"}}, + statusCode: 200, + failPage: true, + successPage: nil, + errorPage: nil, }, { - desc: "Error: Query Values missing 'state' key", - reqState: "state", - port: 0, - q: url.Values{"code": []string{"code"}}, - statusCode: http.StatusInternalServerError, + desc: "Error: Query Values missing 'state' key", + reqState: "state", + port: 0, + q: url.Values{"code": []string{"code"}}, + statusCode: http.StatusInternalServerError, + successPage: nil, + errorPage: nil, }, { - desc: "Error: Query Values missing had 'state' key value that was different that requested", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"etats"}, "code": []string{"code"}}, - statusCode: http.StatusInternalServerError, + desc: "Error: Query Values missing had 'state' key value that was different that requested", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"etats"}, "code": []string{"code"}}, + statusCode: http.StatusInternalServerError, + successPage: nil, + errorPage: nil, }, { - desc: "Error: Query Values missing 'code' key", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"state"}}, - statusCode: http.StatusInternalServerError, + desc: "Error: Query Values missing 'code' key", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"state"}}, + statusCode: http.StatusInternalServerError, + successPage: nil, + errorPage: nil, }, { - desc: "Success", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"state"}, "code": []string{"code"}}, - statusCode: 200, + desc: "Success", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"state"}, "code": []string{"code"}}, + statusCode: 200, + successPage: nil, + errorPage: nil, + }, + { + desc: "Success, with optional success page", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"state"}, "code": []string{"code"}}, + statusCode: 200, + successPage: []byte("test option success page"), + errorPage: nil, + }, + { + desc: "Error: Query Values missing 'state' key, and optional error page", + reqState: "state", + port: 0, + q: url.Values{"code": []string{"code"}}, + statusCode: http.StatusInternalServerError, + successPage: nil, + errorPage: []byte("test option error page"), }, } for _, test := range tests { - serv, err := New(test.reqState, test.port) + serv, err := New(test.reqState, test.port, test.successPage, test.errorPage) if err != nil { panic(err) } @@ -129,6 +159,20 @@ func TestServer(t *testing.T) { continue } + if len(test.successPage) > 0 { + if !strings.Contains(string(content), "test option success page") { + t.Errorf("TestServer(%s): -want/+got:\ntest option error page", test.desc) + } + continue + + } + if len(test.errorPage) > 0 { + if !strings.Contains(string(content), "test option error page") { + t.Errorf("TestServer(%s): -want/+got:\ntest option error page", test.desc) + } + continue + } + if !strings.Contains(string(content), "Authentication Complete") { t.Errorf("TestServer(%s): got failed page, okay page", test.desc) } From 9ac831a4b014aabc993b8c51d71e1aac2d0d3b0e Mon Sep 17 00:00:00 2001 From: wayne Date: Tue, 6 Aug 2024 22:39:19 +1200 Subject: [PATCH 09/31] make success and error pages immutable, isolate changes from user application --- apps/public/public.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/apps/public/public.go b/apps/public/public.go index 5b58f517..d4165cac 100644 --- a/apps/public/public.go +++ b/apps/public/public.go @@ -543,8 +543,11 @@ func WithSystemBrowserOptions(successPage []byte, errorPage []byte) interface { func(a any) error { switch t := a.(type) { case *interactiveAuthOptions: - t.successPage = successPage - t.errorPage = errorPage + t.successPage = make([]byte, len(successPage)) + copy(t.successPage, successPage) + + t.errorPage = make([]byte, len(errorPage)) + copy(t.errorPage, errorPage) default: return fmt.Errorf("unexpected options type %T", a) } From 552855ff0feddc060fb23ede14dae80246152e58 Mon Sep 17 00:00:00 2001 From: catinkayak Date: Sat, 10 Aug 2024 17:00:41 +1200 Subject: [PATCH 10/31] Update apps/internal/local/server_test.go Co-authored-by: Charles Lowell <10964656+chlowell@users.noreply.github.com> --- apps/internal/local/server_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index c649df17..35628b7f 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -36,8 +36,6 @@ func TestServer(t *testing.T) { q: url.Values{"state": []string{"state"}, "error": []string{"error"}}, statusCode: 200, failPage: true, - successPage: nil, - errorPage: nil, }, { desc: "Error: Query Values missing 'state' key", From fad8abf6decbd563efb48f09b25ac6c97480bd47 Mon Sep 17 00:00:00 2001 From: catinkayak Date: Sat, 10 Aug 2024 17:01:46 +1200 Subject: [PATCH 11/31] Update apps/internal/local/server_test.go using bytes.Equal, eliminating the cast Co-authored-by: Charles Lowell <10964656+chlowell@users.noreply.github.com> --- apps/internal/local/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index 35628b7f..a1541e01 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -158,7 +158,7 @@ func TestServer(t *testing.T) { } if len(test.successPage) > 0 { - if !strings.Contains(string(content), "test option success page") { + if !bytes.Equal(content, test.successPage) { t.Errorf("TestServer(%s): -want/+got:\ntest option error page", test.desc) } continue From f28cc478964880673b894e65eb26c0511650d031 Mon Sep 17 00:00:00 2001 From: wayne Date: Sat, 10 Aug 2024 17:03:42 +1200 Subject: [PATCH 12/31] Update server_test remove redundant setting success and error pages to nil --- apps/internal/local/server_test.go | 75 +++++++++++++----------------- 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index a1541e01..73bda202 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -4,6 +4,7 @@ package local import ( + "bytes" "context" "io" "net/http" @@ -30,48 +31,40 @@ func TestServer(t *testing.T) { errorPage []byte }{ { - desc: "Error: Query Values has 'error' key", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"state"}, "error": []string{"error"}}, - statusCode: 200, - failPage: true, + desc: "Error: Query Values has 'error' key", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"state"}, "error": []string{"error"}}, + statusCode: 200, + failPage: true, }, { - desc: "Error: Query Values missing 'state' key", - reqState: "state", - port: 0, - q: url.Values{"code": []string{"code"}}, - statusCode: http.StatusInternalServerError, - successPage: nil, - errorPage: nil, + desc: "Error: Query Values missing 'state' key", + reqState: "state", + port: 0, + q: url.Values{"code": []string{"code"}}, + statusCode: http.StatusInternalServerError, }, { - desc: "Error: Query Values missing had 'state' key value that was different that requested", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"etats"}, "code": []string{"code"}}, - statusCode: http.StatusInternalServerError, - successPage: nil, - errorPage: nil, + desc: "Error: Query Values missing had 'state' key value that was different that requested", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"etats"}, "code": []string{"code"}}, + statusCode: http.StatusInternalServerError, }, { - desc: "Error: Query Values missing 'code' key", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"state"}}, - statusCode: http.StatusInternalServerError, - successPage: nil, - errorPage: nil, + desc: "Error: Query Values missing 'code' key", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"state"}}, + statusCode: http.StatusInternalServerError, }, { - desc: "Success", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"state"}, "code": []string{"code"}}, - statusCode: 200, - successPage: nil, - errorPage: nil, + desc: "Success", + reqState: "state", + port: 0, + q: url.Values{"state": []string{"state"}, "code": []string{"code"}}, + statusCode: 200, }, { desc: "Success, with optional success page", @@ -80,16 +73,14 @@ func TestServer(t *testing.T) { q: url.Values{"state": []string{"state"}, "code": []string{"code"}}, statusCode: 200, successPage: []byte("test option success page"), - errorPage: nil, }, { - desc: "Error: Query Values missing 'state' key, and optional error page", - reqState: "state", - port: 0, - q: url.Values{"code": []string{"code"}}, - statusCode: http.StatusInternalServerError, - successPage: nil, - errorPage: []byte("test option error page"), + desc: "Error: Query Values missing 'state' key, and optional error page", + reqState: "state", + port: 0, + q: url.Values{"code": []string{"code"}}, + statusCode: http.StatusInternalServerError, + errorPage: []byte("test option error page"), }, } From 7b991a067a84f35d9eade40558d4772283d31439 Mon Sep 17 00:00:00 2001 From: wayne Date: Sat, 10 Aug 2024 17:19:42 +1200 Subject: [PATCH 13/31] Add error code and description to errpage template variables --- apps/internal/local/server.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index b5879e90..210148e6 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -37,11 +37,19 @@ const failPage = `

Authentication failed. You can return to the application. Feel free to close this browser tab.

-

Error details: error %s error_description: %s

+

Error details: error {{.Code}}, error description: {{.Err}}

` +// code is the html template variable name, +// which matches the Result Code variable +const code string = "Code" + +// err is the html template variable name +// which matches the Rest Err variable +const err string = "Err" + // Result is the result from the redirect. type Result struct { // Code is the code sent by the authority server. From 0f55b064739af0ddd2052d431f2c3cbbdae7c3c4 Mon Sep 17 00:00:00 2001 From: wayne Date: Sat, 10 Aug 2024 17:21:35 +1200 Subject: [PATCH 14/31] Update server to hold the error page html template --- apps/internal/local/server.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index 210148e6..1255d31c 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -67,6 +67,7 @@ type Server struct { reqState string optionSuccessPage []byte optionErrorPage []byte + errorPageTemplate string } // New creates a local HTTP server and starts it. @@ -101,6 +102,7 @@ func New(reqState string, port int, successPage []byte, errorPage []byte) (*Serv resultCh: make(chan Result, 1), optionSuccessPage: successPage, optionErrorPage: errorPage, + errorPageTemplate: failPage, // default error page } serv.s.Handler = http.HandlerFunc(serv.handler) From 037b7e7fb1fa06518809257fc23dab5d93c75761 Mon Sep 17 00:00:00 2001 From: wayne Date: Sat, 10 Aug 2024 17:23:30 +1200 Subject: [PATCH 15/31] Update golang doc for WithSystemBrowserOptions describing the usage of the html template variables --- apps/public/public.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/public/public.go b/apps/public/public.go index d4165cac..96de3db7 100644 --- a/apps/public/public.go +++ b/apps/public/public.go @@ -531,6 +531,9 @@ type AcquireInteractiveOption interface { acquireInteractiveOption() } +// WithSystemBrowserOptions sets the optional success and error pages. +// The error page supports two optional html template variables {{.Code}} and {{.Err}}, +// which will be replaced with the corresponding error code, and descriptions. func WithSystemBrowserOptions(successPage []byte, errorPage []byte) interface { AcquireInteractiveOption options.CallOption From 2dc114ff6fd5c273c91232fae3bab8455335fb08 Mon Sep 17 00:00:00 2001 From: wayne Date: Sun, 11 Aug 2024 16:53:36 +1200 Subject: [PATCH 16/31] Update failpage to use html templates --- apps/internal/local/server.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index 1255d31c..d105c3b2 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -7,7 +7,7 @@ package local import ( "context" "fmt" - "html" + "html/template" "net" "net/http" "strconv" @@ -154,17 +154,25 @@ func (s *Server) putResult(r Result) { func (s *Server) handler(w http.ResponseWriter, r *http.Request) { q := r.URL.Query() + if len(s.optionErrorPage) > 0 { + s.errorPageTemplate = string(s.optionErrorPage) + } + headerErr := q.Get("error") if headerErr != "" { - desc := html.EscapeString(q.Get("error_description")) // Note: It is a little weird we handle some errors by not going to the failPage. If they all should, // change this to s.error() and make s.error() write the failPage instead of an error code. - if len(s.optionErrorPage) > 0 { - _, _ = w.Write(s.optionErrorPage) - } else { - _, _ = w.Write([]byte(fmt.Sprintf(failPage, headerErr, desc))) + failPageTemplate, err := template.New("failPage").Parse(s.errorPageTemplate) + if err != nil { + s.error(w, http.StatusInternalServerError, "error parsing template") + } + + errDesc := fmt.Errorf(q.Get("error_description")) + err = failPageTemplate.Execute(w, Result{Code: headerErr, Err: errDesc}) + if err != nil { + s.error(w, http.StatusInternalServerError, "error rendering page") } - s.putResult(Result{Err: fmt.Errorf(desc)}) + s.putResult(Result{Err: errDesc}) return } From 11000f84eeb98d6aff4b7f861051be7420edad24 Mon Sep 17 00:00:00 2001 From: wayne Date: Sun, 11 Aug 2024 16:53:56 +1200 Subject: [PATCH 17/31] Add tests for html template fail page --- apps/internal/local/server_test.go | 85 +++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 13 deletions(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index 73bda202..97172ba8 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -21,14 +21,15 @@ func TestServer(t *testing.T) { defer cancel() tests := []struct { - desc string - reqState string - port int - q url.Values - failPage bool - statusCode int - successPage []byte - errorPage []byte + desc string + reqState string + port int + q url.Values + failPage bool + statusCode int + successPage []byte + errorPage []byte + testTemplate bool }{ { desc: "Error: Query Values has 'error' key", @@ -82,6 +83,42 @@ func TestServer(t *testing.T) { statusCode: http.StatusInternalServerError, errorPage: []byte("test option error page"), }, + { + desc: "Error: Query Values missing 'state' key, and optional error page, with template having code and error", + reqState: "state", + port: 0, + q: url.Values{"error": []string{"error_code"}, "error_description": []string{"error_description"}}, + statusCode: 200, + errorPage: []byte("test option error page {{.Code}} {{.Err}}"), + testTemplate: true, + }, + { + desc: "Error: Query Values missing 'state' key, and optional error page, with template having only code", + reqState: "state", + port: 0, + q: url.Values{"error": []string{"error_code"}, "error_description": []string{"error_description"}}, + statusCode: 200, + errorPage: []byte("test option error page {{.Code}}"), + testTemplate: true, + }, + { + desc: "Error: Query Values missing 'state' key, and optional error page, with template having only error", + reqState: "state", + port: 0, + q: url.Values{"error": []string{"error_code"}, "error_description": []string{"error_description"}}, + statusCode: 200, + errorPage: []byte("test option error page {{.Err}}"), + testTemplate: true, + }, + { + desc: "Error: Query Values missing 'state' key, and optional error page, having no code or error", + reqState: "state", + port: 0, + q: url.Values{"error": []string{"error_code"}, "error_description": []string{"error_description"}}, + statusCode: 200, + errorPage: []byte("test option error page"), + testTemplate: true, + }, } for _, test := range tests { @@ -153,13 +190,35 @@ func TestServer(t *testing.T) { t.Errorf("TestServer(%s): -want/+got:\ntest option error page", test.desc) } continue - } - if len(test.errorPage) > 0 { - if !strings.Contains(string(content), "test option error page") { - t.Errorf("TestServer(%s): -want/+got:\ntest option error page", test.desc) + + if test.testTemplate { + if len(test.errorPage) > 0 { + errCode := bytes.Contains(test.errorPage, []byte("{{.Code}}")) + errDescription := bytes.Contains(test.errorPage, []byte("{{.Err}}")) + + if !errCode && !errDescription { + if !strings.Contains(string(content), "test option error page") { + t.Errorf("TestServer(%s): -want/+got:\ntest option error page", test.desc) + } + } + if errCode && errDescription { + if !strings.Contains(string(content), "test option error page error_code error_description") { + t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code error_description", test.desc) + } + } + if errCode && !errDescription { + if !strings.Contains(string(content), "test option error page error_code") { + t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code", test.desc) + } + } + if !errCode && errDescription { + if !strings.Contains(string(content), "test option error page error_description") { + t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_description", test.desc) + } + } + continue } - continue } if !strings.Contains(string(content), "Authentication Complete") { From ac071caddf123ebb0cad3f24b7c624bf53302f31 Mon Sep 17 00:00:00 2001 From: wayne Date: Sun, 11 Aug 2024 17:42:57 +1200 Subject: [PATCH 18/31] Refactor by setting fail and success pages when New HTTP server is created --- apps/internal/local/server.go | 50 ++++++++++++++---------------- apps/internal/local/server_test.go | 16 ---------- 2 files changed, 24 insertions(+), 42 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index d105c3b2..da5f45d4 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -28,7 +28,7 @@ var okPage = []byte(` `) -const failPage = ` +var failPage = []byte(` @@ -40,7 +40,7 @@ const failPage = `

Error details: error {{.Code}}, error description: {{.Err}}

-` +`) // code is the html template variable name, // which matches the Result Code variable @@ -61,13 +61,12 @@ type Result struct { // Server is an HTTP server. type Server struct { // Addr is the address the server is listening on. - Addr string - resultCh chan Result - s *http.Server - reqState string - optionSuccessPage []byte - optionErrorPage []byte - errorPageTemplate string + Addr string + resultCh chan Result + s *http.Server + reqState string + successPage []byte + errorPage []byte } // New creates a local HTTP server and starts it. @@ -95,14 +94,21 @@ func New(reqState string, port int, successPage []byte, errorPage []byte) (*Serv return nil, err } + if len(successPage) == 0 { + successPage = okPage + } + + if len(errorPage) == 0 { + errorPage = failPage + } + serv := &Server{ - Addr: fmt.Sprintf("http://localhost:%s", portStr), - s: &http.Server{Addr: "localhost:0", ReadHeaderTimeout: time.Second}, - reqState: reqState, - resultCh: make(chan Result, 1), - optionSuccessPage: successPage, - optionErrorPage: errorPage, - errorPageTemplate: failPage, // default error page + Addr: fmt.Sprintf("http://localhost:%s", portStr), + s: &http.Server{Addr: "localhost:0", ReadHeaderTimeout: time.Second}, + reqState: reqState, + resultCh: make(chan Result, 1), + successPage: successPage, + errorPage: errorPage, } serv.s.Handler = http.HandlerFunc(serv.handler) @@ -154,15 +160,11 @@ func (s *Server) putResult(r Result) { func (s *Server) handler(w http.ResponseWriter, r *http.Request) { q := r.URL.Query() - if len(s.optionErrorPage) > 0 { - s.errorPageTemplate = string(s.optionErrorPage) - } - headerErr := q.Get("error") if headerErr != "" { // Note: It is a little weird we handle some errors by not going to the failPage. If they all should, // change this to s.error() and make s.error() write the failPage instead of an error code. - failPageTemplate, err := template.New("failPage").Parse(s.errorPageTemplate) + failPageTemplate, err := template.New("failPage").Parse(string(s.errorPage)) if err != nil { s.error(w, http.StatusInternalServerError, "error parsing template") } @@ -193,11 +195,7 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { return } - if len(s.optionSuccessPage) > 0 { - _, _ = w.Write(s.optionSuccessPage) - } else { - _, _ = w.Write(okPage) - } + _, _ = w.Write(s.successPage) s.putResult(Result{Code: code}) } diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index 97172ba8..045b7a79 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -67,22 +67,6 @@ func TestServer(t *testing.T) { q: url.Values{"state": []string{"state"}, "code": []string{"code"}}, statusCode: 200, }, - { - desc: "Success, with optional success page", - reqState: "state", - port: 0, - q: url.Values{"state": []string{"state"}, "code": []string{"code"}}, - statusCode: 200, - successPage: []byte("test option success page"), - }, - { - desc: "Error: Query Values missing 'state' key, and optional error page", - reqState: "state", - port: 0, - q: url.Values{"code": []string{"code"}}, - statusCode: http.StatusInternalServerError, - errorPage: []byte("test option error page"), - }, { desc: "Error: Query Values missing 'state' key, and optional error page, with template having code and error", reqState: "state", From 260faed6464ec0c19c17da9c0fe0674fc229cd5b Mon Sep 17 00:00:00 2001 From: wayne Date: Sun, 11 Aug 2024 21:34:47 +1200 Subject: [PATCH 19/31] Fix typo --- apps/internal/local/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index da5f45d4..627355f5 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -47,7 +47,7 @@ var failPage = []byte(` const code string = "Code" // err is the html template variable name -// which matches the Rest Err variable +// which matches the Result Err variable const err string = "Err" // Result is the result from the redirect. From 61f5cf153662b400c33d3f5ab47d6eb4e6ae2396 Mon Sep 17 00:00:00 2001 From: wayne Date: Sun, 11 Aug 2024 21:50:51 +1200 Subject: [PATCH 20/31] Add test for default fail error page --- apps/internal/local/server_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index 045b7a79..24f7f5b1 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -103,6 +103,14 @@ func TestServer(t *testing.T) { errorPage: []byte("test option error page"), testTemplate: true, }, + { + desc: "Error: Query Values missing 'state' key, using default fail error page", + reqState: "state", + port: 0, + q: url.Values{"error": []string{"error_code"}, "error_description": []string{"error_description"}}, + statusCode: 200, + testTemplate: true, + }, } for _, test := range tests { @@ -202,6 +210,11 @@ func TestServer(t *testing.T) { } } continue + } else { + if !strings.Contains(string(content), "

Error details: error error_code, error description: error_description

") { + t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code error_description", test.desc) + } + continue } } From 4b6ce2992e1db91d440fcbce3d60f1f2f8962a17 Mon Sep 17 00:00:00 2001 From: wayne Date: Tue, 27 Aug 2024 20:15:31 +1200 Subject: [PATCH 21/31] add comments escaping html entities, protection against XSS --- apps/internal/local/server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index 627355f5..15ac1e97 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -7,7 +7,7 @@ package local import ( "context" "fmt" - "html/template" + "html/template" // must be html/template, and not text/template to have injection protection "net" "net/http" "strconv" @@ -164,13 +164,13 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { if headerErr != "" { // Note: It is a little weird we handle some errors by not going to the failPage. If they all should, // change this to s.error() and make s.error() write the failPage instead of an error code. - failPageTemplate, err := template.New("failPage").Parse(string(s.errorPage)) + failPageTemplate, err := template.New("failPage").Parse(string(s.errorPage)) // html template will be injection safe if err != nil { s.error(w, http.StatusInternalServerError, "error parsing template") } errDesc := fmt.Errorf(q.Get("error_description")) - err = failPageTemplate.Execute(w, Result{Code: headerErr, Err: errDesc}) + err = failPageTemplate.Execute(w, Result{Code: headerErr, Err: errDesc}) // escapes html entities if err != nil { s.error(w, http.StatusInternalServerError, "error rendering page") } From 435203db087d5918f35bb3fb47477d6980d0c846 Mon Sep 17 00:00:00 2001 From: wayne Date: Tue, 27 Aug 2024 20:19:57 +1200 Subject: [PATCH 22/31] add test for escaping html entities, when using the error page template --- apps/internal/local/server_test.go | 35 ++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index 24f7f5b1..22199481 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -21,15 +21,16 @@ func TestServer(t *testing.T) { defer cancel() tests := []struct { - desc string - reqState string - port int - q url.Values - failPage bool - statusCode int - successPage []byte - errorPage []byte - testTemplate bool + desc string + reqState string + port int + q url.Values + failPage bool + statusCode int + successPage []byte + errorPage []byte + testTemplate bool + testHTMLInjection bool }{ { desc: "Error: Query Values has 'error' key", @@ -111,6 +112,15 @@ func TestServer(t *testing.T) { statusCode: 200, testTemplate: true, }, + { + desc: "Error: Query Values missing 'state' key, using default fail error page - XSS test", + reqState: "state", + port: 0, + q: url.Values{"error": []string{""}, "error_description": []string{"error_description"}}, + statusCode: 200, + testTemplate: true, + testHTMLInjection: true, + }, } for _, test := range tests { @@ -185,6 +195,13 @@ func TestServer(t *testing.T) { } if test.testTemplate { + if test.testHTMLInjection { + if !strings.Contains(string(content), "<script>alert('this code snippet was executed')</script>") { + t.Errorf("TestServer(%s): want escaped html entities", test.desc) + } + continue + } + if len(test.errorPage) > 0 { errCode := bytes.Contains(test.errorPage, []byte("{{.Code}}")) errDescription := bytes.Contains(test.errorPage, []byte("{{.Err}}")) From 3b044922f36b235f5ca710a5e3975f83a8400385 Mon Sep 17 00:00:00 2001 From: wayne Date: Thu, 29 Aug 2024 08:19:44 +1200 Subject: [PATCH 23/31] Update default error page XSS tests, to cover both the .Code and the .Err template fields --- apps/internal/local/server_test.go | 46 ++++++++++++++++++------------ 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index 22199481..ad960d1c 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -21,16 +21,17 @@ func TestServer(t *testing.T) { defer cancel() tests := []struct { - desc string - reqState string - port int - q url.Values - failPage bool - statusCode int - successPage []byte - errorPage []byte - testTemplate bool - testHTMLInjection bool + desc string + reqState string + port int + q url.Values + failPage bool + statusCode int + successPage []byte + errorPage []byte + testTemplate bool + testErrCodeXSS bool + testErrDescriptionXSS bool }{ { desc: "Error: Query Values has 'error' key", @@ -113,13 +114,22 @@ func TestServer(t *testing.T) { testTemplate: true, }, { - desc: "Error: Query Values missing 'state' key, using default fail error page - XSS test", - reqState: "state", - port: 0, - q: url.Values{"error": []string{""}, "error_description": []string{"error_description"}}, - statusCode: 200, - testTemplate: true, - testHTMLInjection: true, + desc: "Error: Query Values missing 'state' key, using default fail error page - Error Code XSS test", + reqState: "state", + port: 0, + q: url.Values{"error": []string{""}, "error_description": []string{"error_description"}}, + statusCode: 200, + testTemplate: true, + testErrCodeXSS: true, + }, + { + desc: "Error: Query Values missing 'state' key, using default fail error page - Error Description XSS test", + reqState: "state", + port: 0, + q: url.Values{"error": []string{"error_code"}, "error_description": []string{""}}, + statusCode: 200, + testTemplate: true, + testErrDescriptionXSS: true, }, } @@ -195,7 +205,7 @@ func TestServer(t *testing.T) { } if test.testTemplate { - if test.testHTMLInjection { + if test.testErrCodeXSS || test.testErrDescriptionXSS { if !strings.Contains(string(content), "<script>alert('this code snippet was executed')</script>") { t.Errorf("TestServer(%s): want escaped html entities", test.desc) } From 0cde778b7e50761ba3519a370cc7202f36e8f2cb Mon Sep 17 00:00:00 2001 From: wayne Date: Thu, 29 Aug 2024 13:05:10 +1200 Subject: [PATCH 24/31] Add option error page XSS tests, to cover both the .Code and the .Err template fields --- apps/internal/local/server_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index ad960d1c..e1d9489b 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -131,6 +131,26 @@ func TestServer(t *testing.T) { testTemplate: true, testErrDescriptionXSS: true, }, + { + desc: "Error: Query Values missing 'state' key, using optional fail error page - Error Code XSS test", + reqState: "state", + port: 0, + q: url.Values{"error": []string{""}, "error_description": []string{"error_description"}}, + statusCode: 200, + errorPage: []byte("error: {{.Code}} error_description: {{.Err}}"), + testTemplate: true, + testErrCodeXSS: true, + }, + { + desc: "Error: Query Values missing 'state' key, using optional fail error page - Error Description XSS test", + reqState: "state", + port: 0, + q: url.Values{"error": []string{"error_code"}, "error_description": []string{""}}, + statusCode: 200, + errorPage: []byte("error: {{.Code}} error_description: {{.Err}}"), + testTemplate: true, + testErrDescriptionXSS: true, + }, } for _, test := range tests { @@ -212,6 +232,13 @@ func TestServer(t *testing.T) { continue } + if len(test.errorPage) > 0 && (test.testErrCodeXSS || test.testErrDescriptionXSS) { + if !strings.Contains(string(content), "<script>alert('this code snippet was executed')</script>") { + t.Errorf("TestServer(%s): want escaped html entities", test.desc) + } + continue + } + if len(test.errorPage) > 0 { errCode := bytes.Contains(test.errorPage, []byte("{{.Code}}")) errDescription := bytes.Contains(test.errorPage, []byte("{{.Err}}")) From 87372be4c06c33d57b7b639557a65f426ff8039e Mon Sep 17 00:00:00 2001 From: wayne Date: Fri, 27 Sep 2024 13:05:30 +1200 Subject: [PATCH 25/31] Remove html template as it uses reflection, unnecessarily increase application size, instead revert to using html.EscapeString --- apps/internal/local/server.go | 38 ++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index 15ac1e97..760c4604 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -5,9 +5,10 @@ package local import ( + "bytes" "context" "fmt" - "html/template" // must be html/template, and not text/template to have injection protection + "html" "net" "net/http" "strconv" @@ -42,13 +43,14 @@ var failPage = []byte(` `) -// code is the html template variable name, -// which matches the Result Code variable -const code string = "Code" - -// err is the html template variable name -// which matches the Result Err variable -const err string = "Err" +var ( + // code is the html template variable name, + // which matches the Result Code variable + code = []byte("{{.Code}}") + // err is the html template variable name + // which matches the Result Err variable + err = []byte("{{.Err}}") +) // Result is the result from the redirect. type Result struct { @@ -164,17 +166,21 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { if headerErr != "" { // Note: It is a little weird we handle some errors by not going to the failPage. If they all should, // change this to s.error() and make s.error() write the failPage instead of an error code. - failPageTemplate, err := template.New("failPage").Parse(string(s.errorPage)) // html template will be injection safe - if err != nil { - s.error(w, http.StatusInternalServerError, "error parsing template") + + errDesc := q.Get("error_description") + errorDesc := fmt.Errorf(errDesc) + + if bytes.Contains(s.errorPage, code) { + s.errorPage = bytes.Replace(s.errorPage, code, []byte(html.EscapeString(headerErr)), 1) // provides XSS protection } - errDesc := fmt.Errorf(q.Get("error_description")) - err = failPageTemplate.Execute(w, Result{Code: headerErr, Err: errDesc}) // escapes html entities - if err != nil { - s.error(w, http.StatusInternalServerError, "error rendering page") + if bytes.Contains(s.errorPage, err) { + s.errorPage = bytes.Replace(s.errorPage, err, []byte(html.EscapeString(errDesc)), 1) // provides XSS protection } - s.putResult(Result{Err: errDesc}) + + _, _ = w.Write(s.errorPage) + + s.putResult(Result{Err: errorDesc}) return } From 16fdca2ccb337086c17d6c2b3d3c324b4338c68a Mon Sep 17 00:00:00 2001 From: wayne Date: Fri, 27 Sep 2024 13:12:14 +1200 Subject: [PATCH 26/31] Refactor, put variable closer to where it is being used --- apps/internal/local/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index 760c4604..36050c69 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -168,7 +168,6 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { // change this to s.error() and make s.error() write the failPage instead of an error code. errDesc := q.Get("error_description") - errorDesc := fmt.Errorf(errDesc) if bytes.Contains(s.errorPage, code) { s.errorPage = bytes.Replace(s.errorPage, code, []byte(html.EscapeString(headerErr)), 1) // provides XSS protection @@ -180,6 +179,7 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { _, _ = w.Write(s.errorPage) + errorDesc := fmt.Errorf(errDesc) s.putResult(Result{Err: errorDesc}) return } From 48346452763b88c1c4bc68f88613aefaf775c569 Mon Sep 17 00:00:00 2001 From: wayne Date: Sun, 1 Dec 2024 09:15:13 +1300 Subject: [PATCH 27/31] Update use function parameter list syntax --- apps/public/public.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/public/public.go b/apps/public/public.go index 96de3db7..92f3fad5 100644 --- a/apps/public/public.go +++ b/apps/public/public.go @@ -534,7 +534,7 @@ type AcquireInteractiveOption interface { // WithSystemBrowserOptions sets the optional success and error pages. // The error page supports two optional html template variables {{.Code}} and {{.Err}}, // which will be replaced with the corresponding error code, and descriptions. -func WithSystemBrowserOptions(successPage []byte, errorPage []byte) interface { +func WithSystemBrowserOptions(successPage, errorPage []byte) interface { AcquireInteractiveOption options.CallOption } { From 60f4ed12e0c6eb25966d47f7383ef492e92a07a6 Mon Sep 17 00:00:00 2001 From: wayne Date: Sun, 1 Dec 2024 09:17:14 +1300 Subject: [PATCH 28/31] Update, use ReplaceAll for when replacing the erro code and err description --- apps/internal/local/server.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index 36050c69..f1cd2bfe 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -169,13 +169,8 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { errDesc := q.Get("error_description") - if bytes.Contains(s.errorPage, code) { - s.errorPage = bytes.Replace(s.errorPage, code, []byte(html.EscapeString(headerErr)), 1) // provides XSS protection - } - - if bytes.Contains(s.errorPage, err) { - s.errorPage = bytes.Replace(s.errorPage, err, []byte(html.EscapeString(errDesc)), 1) // provides XSS protection - } + s.errorPage = bytes.ReplaceAll(s.errorPage, code, []byte(html.EscapeString(headerErr))) // provides XSS protection + s.errorPage = bytes.ReplaceAll(s.errorPage, err, []byte(html.EscapeString(errDesc))) // provides XSS protection _, _ = w.Write(s.errorPage) From 5c38a707feb0c398cc84486ef5d7de1c27a689e7 Mon Sep 17 00:00:00 2001 From: wayne Date: Sun, 1 Dec 2024 09:18:23 +1300 Subject: [PATCH 29/31] Update error template tests, extracting the expected values into the test definitions itself --- apps/internal/local/server_test.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index e1d9489b..11ad73f6 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -32,6 +32,7 @@ func TestServer(t *testing.T) { testTemplate bool testErrCodeXSS bool testErrDescriptionXSS bool + expected string }{ { desc: "Error: Query Values has 'error' key", @@ -77,6 +78,7 @@ func TestServer(t *testing.T) { statusCode: 200, errorPage: []byte("test option error page {{.Code}} {{.Err}}"), testTemplate: true, + expected: "test option error page error_code error_description", }, { desc: "Error: Query Values missing 'state' key, and optional error page, with template having only code", @@ -86,6 +88,7 @@ func TestServer(t *testing.T) { statusCode: 200, errorPage: []byte("test option error page {{.Code}}"), testTemplate: true, + expected: "test option error page error_code", }, { desc: "Error: Query Values missing 'state' key, and optional error page, with template having only error", @@ -95,6 +98,7 @@ func TestServer(t *testing.T) { statusCode: 200, errorPage: []byte("test option error page {{.Err}}"), testTemplate: true, + expected: "test option error page error_description", }, { desc: "Error: Query Values missing 'state' key, and optional error page, having no code or error", @@ -104,6 +108,7 @@ func TestServer(t *testing.T) { statusCode: 200, errorPage: []byte("test option error page"), testTemplate: true, + expected: "test option error page", }, { desc: "Error: Query Values missing 'state' key, using default fail error page", @@ -112,6 +117,7 @@ func TestServer(t *testing.T) { q: url.Values{"error": []string{"error_code"}, "error_description": []string{"error_description"}}, statusCode: 200, testTemplate: true, + expected: "

Error details: error error_code, error description: error_description

", }, { desc: "Error: Query Values missing 'state' key, using default fail error page - Error Code XSS test", @@ -140,6 +146,7 @@ func TestServer(t *testing.T) { errorPage: []byte("error: {{.Code}} error_description: {{.Err}}"), testTemplate: true, testErrCodeXSS: true, + expected: "<script>alert('this code snippet was executed')</script>", }, { desc: "Error: Query Values missing 'state' key, using optional fail error page - Error Description XSS test", @@ -150,6 +157,7 @@ func TestServer(t *testing.T) { errorPage: []byte("error: {{.Code}} error_description: {{.Err}}"), testTemplate: true, testErrDescriptionXSS: true, + expected: "<script>alert('this code snippet was executed')</script>", }, } @@ -226,14 +234,14 @@ func TestServer(t *testing.T) { if test.testTemplate { if test.testErrCodeXSS || test.testErrDescriptionXSS { - if !strings.Contains(string(content), "<script>alert('this code snippet was executed')</script>") { + if !strings.Contains(string(content), test.expected) { t.Errorf("TestServer(%s): want escaped html entities", test.desc) } continue } if len(test.errorPage) > 0 && (test.testErrCodeXSS || test.testErrDescriptionXSS) { - if !strings.Contains(string(content), "<script>alert('this code snippet was executed')</script>") { + if !strings.Contains(string(content), test.expected) { t.Errorf("TestServer(%s): want escaped html entities", test.desc) } continue @@ -244,28 +252,28 @@ func TestServer(t *testing.T) { errDescription := bytes.Contains(test.errorPage, []byte("{{.Err}}")) if !errCode && !errDescription { - if !strings.Contains(string(content), "test option error page") { + if !strings.Contains(string(content), test.expected) { t.Errorf("TestServer(%s): -want/+got:\ntest option error page", test.desc) } } if errCode && errDescription { - if !strings.Contains(string(content), "test option error page error_code error_description") { + if !strings.Contains(string(content), test.expected) { t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code error_description", test.desc) } } if errCode && !errDescription { - if !strings.Contains(string(content), "test option error page error_code") { + if !strings.Contains(string(content), test.expected) { t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code", test.desc) } } if !errCode && errDescription { - if !strings.Contains(string(content), "test option error page error_description") { + if !strings.Contains(string(content), test.expected) { t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_description", test.desc) } } continue } else { - if !strings.Contains(string(content), "

Error details: error error_code, error description: error_description

") { + if !strings.Contains(string(content), test.expected) { t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code error_description", test.desc) } continue From c4e8578be75b27f480c2790f1a6cd27b93e416ac Mon Sep 17 00:00:00 2001 From: wayne Date: Mon, 9 Dec 2024 19:16:15 +1300 Subject: [PATCH 30/31] Update error page handling --- apps/internal/local/server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/internal/local/server.go b/apps/internal/local/server.go index f1cd2bfe..e6390829 100644 --- a/apps/internal/local/server.go +++ b/apps/internal/local/server.go @@ -169,10 +169,10 @@ func (s *Server) handler(w http.ResponseWriter, r *http.Request) { errDesc := q.Get("error_description") - s.errorPage = bytes.ReplaceAll(s.errorPage, code, []byte(html.EscapeString(headerErr))) // provides XSS protection - s.errorPage = bytes.ReplaceAll(s.errorPage, err, []byte(html.EscapeString(errDesc))) // provides XSS protection + errorPage := bytes.ReplaceAll(s.errorPage, code, []byte(html.EscapeString(headerErr))) // provides XSS protection + errorPage = bytes.ReplaceAll(errorPage, err, []byte(html.EscapeString(errDesc))) // provides XSS protection - _, _ = w.Write(s.errorPage) + _, _ = w.Write(errorPage) errorDesc := fmt.Errorf(errDesc) s.putResult(Result{Err: errorDesc}) From f419d6a836532a743060e4e664d85ccd23331f60 Mon Sep 17 00:00:00 2001 From: wayne Date: Mon, 9 Dec 2024 19:20:12 +1300 Subject: [PATCH 31/31] Simplify tests by removing conditional logic, making use of the exptected value --- apps/internal/local/server_test.go | 47 ++---------------------------- 1 file changed, 3 insertions(+), 44 deletions(-) diff --git a/apps/internal/local/server_test.go b/apps/internal/local/server_test.go index 11ad73f6..cafc223d 100644 --- a/apps/internal/local/server_test.go +++ b/apps/internal/local/server_test.go @@ -233,51 +233,10 @@ func TestServer(t *testing.T) { } if test.testTemplate { - if test.testErrCodeXSS || test.testErrDescriptionXSS { - if !strings.Contains(string(content), test.expected) { - t.Errorf("TestServer(%s): want escaped html entities", test.desc) - } - continue - } - - if len(test.errorPage) > 0 && (test.testErrCodeXSS || test.testErrDescriptionXSS) { - if !strings.Contains(string(content), test.expected) { - t.Errorf("TestServer(%s): want escaped html entities", test.desc) - } - continue - } - - if len(test.errorPage) > 0 { - errCode := bytes.Contains(test.errorPage, []byte("{{.Code}}")) - errDescription := bytes.Contains(test.errorPage, []byte("{{.Err}}")) - - if !errCode && !errDescription { - if !strings.Contains(string(content), test.expected) { - t.Errorf("TestServer(%s): -want/+got:\ntest option error page", test.desc) - } - } - if errCode && errDescription { - if !strings.Contains(string(content), test.expected) { - t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code error_description", test.desc) - } - } - if errCode && !errDescription { - if !strings.Contains(string(content), test.expected) { - t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code", test.desc) - } - } - if !errCode && errDescription { - if !strings.Contains(string(content), test.expected) { - t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_description", test.desc) - } - } - continue - } else { - if !strings.Contains(string(content), test.expected) { - t.Errorf("TestServer(%s): -want/+got:\ntest option error page error_code error_description", test.desc) - } - continue + if !strings.Contains(string(content), test.expected) { + t.Errorf("TestServer(%s): -want:%s got:%s ", test.desc, test.expected, string(content)) } + continue } if !strings.Contains(string(content), "Authentication Complete") {