Add option to set success and error pages for when calling AcquireInt… #490

wayneforrest · 2024-06-12T10:18:52Z

Add option to set success and error web pages for when using AcquireTokenInteractive

For example :

 var successPage = []byte(`<!DOCTYPE html>
     <html>
          <body>A Success Page</body>
     </html>`)
     
 var errorPage = []byte(`<!DOCTYPE html>
     <html>
          <body>A Error Page</body>
     </html>`)
     
    
result, err = client.AcquireTokenInteractive(context.TODO(), scopes,
	public.WithSystemBrowserOptions(successPage, errorPage),
)
if err != nil {
	log.Fatalf("Failed to acquire token interactively: %v", err)
}

wayneforrest · 2024-06-12T10:43:43Z

@microsoft-github-policy-service agree

…

On 12 Jun 2024, at 10:19 PM, microsoft-github-policy-service[bot] ***@***.***> wrote: @microsoft-github-policy-service agree

apps/public/public.go

apps/internal/local/server.go

go.mod

localden · 2024-07-19T20:59:54Z

@bgavrilMS @chlowell @rayluo overall, the PR looks good to me, but to confirm, we're OK with arbitrary byte[] payloads being passed for error and success pages?

wayneforrest · 2024-08-02T01:18:07Z

@bgavrilMS @chlowell @rayluo overall, the PR looks good to me, but to confirm, we're OK with arbitrary byte[] payloads being passed for error and success pages?

Are there any more changes that you would like me to add to this PR?

cc @chlowell you still have a change request on this PR. Has this been resolved for you?

chlowell · 2024-08-05T21:32:33Z

There are still a few things to address here:

we need tests
we should copy any []byte from the application to make its content immutable
Server interpolates error details into failPage:

microsoft-authentication-library-for-go/apps/internal/local/server.go

Line 148 in 004301c

_, _ = w.Write([]byte(fmt.Sprintf(failPage, headerErr, desc)))

A few options for addressing item 3:

don't interpolate when the application specified an error page
document the formatting, maybe simplify by interpolating only one string
consider a different design such as taking callbacks returning []byte instead of plain old []byte; then we can pass optional stuff like error details to the application and it can decide whether and how to present them

Option 2 looks good to me; @bgavrilMS @localden @rayluo do you have a preference?

localden · 2024-08-05T23:17:48Z

There are still a few things to address here:

we need tests

we should copy any []byte from the application to make its content immutable

Server interpolates error details into failPage:

microsoft-authentication-library-for-go/apps/internal/local/server.go

Line 148 in 004301c

_, _ = w.Write([]byte(fmt.Sprintf(failPage, headerErr, desc)))

A few options for addressing item 3:

don't interpolate when the application specified an error page

document the formatting, maybe simplify by interpolating only one string

consider a different design such as taking callbacks returning []byte instead of plain old []byte; then we can pass optional stuff like error details to the application and it can decide whether and how to present them

Option 2 looks good to me; @bgavrilMS @localden @rayluo do you have a preference?

Option 2 sounds reasonable to me - formatting to be documented and interpolation simplified. Thanks for reviewing @chlowell!

rayluo · 2024-08-06T07:55:49Z

3. Server interpolates error details into failPage:

microsoft-authentication-library-for-go/apps/internal/local/server.go

Line 148 in 004301c

_, _ = w.Write([]byte(fmt.Sprintf(failPage, headerErr, desc)))

A few options for addressing item 3: ...

What are those 3 options trying to address? I believe, regardless of whether a default failPage or an application-specified s.optionErrorPage is being used, interpolation shall only accept escaped value (example here).

chlowell · 2024-08-06T18:08:03Z

What are those 3 options trying to address? I believe, regardless of whether a default failPage or an application-specified s.optionErrorPage is being used, interpolation shall only accept escaped value (example here).

I'm not worrying about escaping--maybe I should--but developer confusion and unexpected results. If we (try to) augment an application-provided error page with an error message, we should document that behavior or present the message via some API so developers expect the additional content.

rayluo · 2024-08-06T19:45:06Z

What are those 3 options trying to address? I believe, regardless of whether a default failPage or an application-specified s.optionErrorPage is being used, interpolation shall only accept escaped value (example here).

I'm not worrying about escaping--maybe I should--but developer confusion and unexpected results. If we (try to) augment an application-provided error page with an error message, we should document that behavior or present the message via some API so developers expect the additional content.

Great. Now we agree to escape the values that will be rendered on the error page. :-)

"Developer confusion and unexpected results" can be avoided by API documentation such as "the error page is treated as a template and its variable $Foo will be interpolated by blah blah". That is an intuitive concept and easily understandable.

wayneforrest · 2024-08-07T10:40:18Z

What are those 3 options trying to address? I believe, regardless of whether a default failPage or an application-specified s.optionErrorPage is being used, interpolation shall only accept escaped value (example here).

I'm not worrying about escaping--maybe I should--but developer confusion and unexpected results. If we (try to) augment an application-provided error page with an error message, we should document that behavior or present the message via some API so developers expect the additional content.

Great. Now we agree to escape the values that will be rendered on the error page. :-)

"Developer confusion and unexpected results" can be avoided by API documentation such as "the error page is treated as a template and its variable $Foo will be interpolated by blah blah". That is an intuitive concept and easily understandable.

Option 1 already solves my "personal" needs, for displaying end user friendly pages, where technical error codes / descriptions are not understood (unless you are a developer, useful for debugging).

Also are the messages multi lingual ? If not then Option 2 might not be an ideal solution for all?

Option 3 would allow for error code interpretation; allowing one to suggest a course of action to end users rather than error messages whose audience is more developer orientated. I am not sure how to control the "escaping" as the control falls away (user application space). Perhaps on the return path to the SDK before the server renders the page, the page can be sanitised using something like bluemonday : https://pkg.go.dev/github.com/microcosm-cc/bluemonday ?

wayneforrest · 2024-08-07T10:54:09Z

Hi @chlowell thanks for reviewing. Regarding the outstanding list of work:

1: I have added some tests, can you please review. (it's a bit hard to visualise the changes in GitHub)
2: I have also made a "copy" of success/error pages immutable once supplied to the SDK.
3: Interpolation - not done - I made some comments on this above.

rayluo · 2024-08-07T21:54:12Z

Option 1 already solves my "personal" needs, for displaying end user friendly pages, where technical error codes / descriptions are not understood (unless you are a developer, useful for debugging).

... not sure how to control the "escaping" as the control falls away (user application space) ...

I have also made a "copy" of success/error pages immutable once supplied to the SDK.

I'm not sure what the benefit is to make "a copy of success/error pages that is immutable". Is that an attack vector unique to Go?

But to be clear, my "escape/sanitize" suggestion is NOT for the app-provided success/error page. I was referring to the interpolation of error code and error description that already exist and is not fixed by this PR.

Technically, you could bypass the "not sure how to escape" issue, by removing that interpolation (even though it is NOT for app-provided error page). But that would risk a regression in the future. I would suggest we escape the error content properly this time.

wayneforrest · 2024-08-07T23:06:49Z

I'm not sure what the benefit is to make "a copy of success/error pages that is immutable". Is that an attack vector unique to Go?

I am not sure what the reasoning behind this is, it was suggested by @chlowell to make the []byte immutable. I also checked how golang passes values, and it's by default "value" / "copies" - What I did makes no sense to me now, it's already passing a copy. I think I should revert what I did, but will wait to hear more.

chlowell · 2024-08-09T17:44:36Z

What you reverted was correct--we should copy all slices provided by an application because slices are reference types. A slice value is a pointer to memory, not the content of that memory. We don't want to share a pointer with the application because the application can modify the data pointed to at any time (for example).

wayneforrest · 2024-08-09T20:43:28Z

What you reverted was correct--we should copy all slices provided by an application because slices are reference types. A slice value is a pointer to memory, not the content of that memory. We don't want to share a pointer with the application because the application can modify the data pointed to at any time (for example).

Thanks @chlowell ... And for the example! I can also confirm (dang GPT), I checked my Go PL Book page 51, and this commit on the Go Spec. (a slice has a pointer to a backing array)

apps/internal/local/server_test.go

apps/internal/local/server.go

wayneforrest · 2024-08-11T06:19:48Z

I have made some more changes, which might not be what's wanted yet?

1: I have used html templates, which is injection safe, and removed the Escape from earlier. https://pkg.go.dev/html/template
2: I have refactored the code as suggested by @chlowell (it makes it allot cleaner when checking lengths of optional pages)
3: I have added tests, to ensure the parsing works for when the template variables might and might not be present.
also removed the redundant nil initialisation of the error and success pages
4: I have altered default fail-page also to be a html template (using .Code and .Err template variables)
5: I have added doc comment to the public facing method WithSystemBrowserOptions - wording needs a review.

apps/internal/local/server.go

apps/internal/local/server_test.go

remove the option error page string interpolation.

This reverts commit 6a36074.

…lication

Co-authored-by: Charles Lowell <[email protected]>

using bytes.Equal, eliminating the cast Co-authored-by: Charles Lowell <[email protected]>

…o nil

…f the html template variables

…eated

….Err template fields

… template fields

…plication size, instead revert to using html.EscapeString

…iption

…test definitions itself

…cted value

sonarqubecloud · 2024-12-09T06:33:20Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

wayneforrest requested review from bgavrilMS and rayluo as code owners June 12, 2024 10:18

bgavrilMS requested a review from localden June 14, 2024 13:20

bgavrilMS reviewed Jun 14, 2024

View reviewed changes

apps/public/public.go Outdated Show resolved Hide resolved

bgavrilMS approved these changes Jun 20, 2024

View reviewed changes

chlowell requested changes Jun 20, 2024

View reviewed changes

apps/internal/local/server.go Outdated Show resolved Hide resolved

apps/internal/local/server.go Outdated Show resolved Hide resolved

wayneforrest force-pushed the feature/set-interactive-auth-success-and-error-pages branch from 13b8895 to 20f93ec Compare June 21, 2024 07:48

wayneforrest requested a review from chlowell June 27, 2024 20:20

rayluo reviewed Jul 18, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

localden approved these changes Aug 5, 2024

View reviewed changes

wayneforrest force-pushed the feature/set-interactive-auth-success-and-error-pages branch from 6169f27 to 213b935 Compare August 9, 2024 20:44

chlowell reviewed Aug 9, 2024

View reviewed changes

apps/internal/local/server_test.go Outdated Show resolved Hide resolved

apps/internal/local/server_test.go Outdated Show resolved Hide resolved

apps/internal/local/server.go Outdated Show resolved Hide resolved

apps/internal/local/server.go Outdated Show resolved Hide resolved

bgavrilMS approved these changes Aug 13, 2024

View reviewed changes

rayluo reviewed Aug 20, 2024

View reviewed changes

apps/internal/local/server.go Show resolved Hide resolved

rayluo reviewed Aug 28, 2024

View reviewed changes

apps/internal/local/server_test.go Outdated Show resolved Hide resolved

wayneforrest and others added 25 commits December 1, 2024 09:18

Revert failPage change, to be of type const string

587663f

Change success and error pages lifetime scope to be per "session", and

aa7dde8

remove the option error page string interpolation.

Revert "update module name"

4f65972

This reverts commit 6a36074.

Add tests for optional success and error pages

92a81c0

make success and error pages immutable, isolate changes from user app…

9ac831a

…lication

Update apps/internal/local/server_test.go

552855f

Co-authored-by: Charles Lowell <[email protected]>

Update apps/internal/local/server_test.go

fad8abf

using bytes.Equal, eliminating the cast Co-authored-by: Charles Lowell <[email protected]>

Update server_test remove redundant setting success and error pages t…

f28cc47

…o nil

Add error code and description to errpage template variables

7b991a0

Update server to hold the error page html template

0f55b06

Update golang doc for WithSystemBrowserOptions describing the usage o…

037b7e7

…f the html template variables

Update failpage to use html templates

2dc114f

Add tests for html template fail page

11000f8

Refactor by setting fail and success pages when New HTTP server is cr…

ac071ca

…eated

Fix typo

260faed

Add test for default fail error page

61f5cf1

add comments escaping html entities, protection against XSS

4b6ce29

add test for escaping html entities, when using the error page template

435203d

Update default error page XSS tests, to cover both the .Code and the …

3b04492

….Err template fields

Add option error page XSS tests, to cover both the .Code and the .Err…

0cde778

… template fields

Remove html template as it uses reflection, unnecessarily increase ap…

87372be

…plication size, instead revert to using html.EscapeString

Refactor, put variable closer to where it is being used

16fdca2

Update use function parameter list syntax

4834645

Update, use ReplaceAll for when replacing the erro code and err descr…

60f4ed1

…iption

Update error template tests, extracting the expected values into the …

5c38a70

…test definitions itself

wayneforrest force-pushed the feature/set-interactive-auth-success-and-error-pages branch from 7bc0694 to 5c38a70 Compare November 30, 2024 21:12

wayneforrest added 2 commits December 9, 2024 19:32

Update error page handling

c4e8578

Simplify tests by removing conditional logic, making use of the expte…

f419d6a

…cted value

wayneforrest force-pushed the feature/set-interactive-auth-success-and-error-pages branch from 27c44ef to f419d6a Compare December 9, 2024 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to set success and error pages for when calling AcquireInt… #490

Add option to set success and error pages for when calling AcquireInt… #490

wayneforrest commented Jun 12, 2024 •

edited

Loading

wayneforrest commented Jun 12, 2024 via email

localden commented Jul 19, 2024

wayneforrest commented Aug 2, 2024

chlowell commented Aug 5, 2024

localden commented Aug 5, 2024

rayluo commented Aug 6, 2024

chlowell commented Aug 6, 2024

rayluo commented Aug 6, 2024

wayneforrest commented Aug 7, 2024

wayneforrest commented Aug 7, 2024

rayluo commented Aug 7, 2024

wayneforrest commented Aug 7, 2024

chlowell commented Aug 9, 2024

wayneforrest commented Aug 9, 2024

wayneforrest commented Aug 11, 2024 •

edited

Loading

sonarqubecloud bot commented Dec 9, 2024

Add option to set success and error pages for when calling AcquireInt… #490

Are you sure you want to change the base?

Add option to set success and error pages for when calling AcquireInt… #490

Conversation

wayneforrest commented Jun 12, 2024 • edited Loading

wayneforrest commented Jun 12, 2024 via email

localden commented Jul 19, 2024

wayneforrest commented Aug 2, 2024

chlowell commented Aug 5, 2024

localden commented Aug 5, 2024

rayluo commented Aug 6, 2024

chlowell commented Aug 6, 2024

rayluo commented Aug 6, 2024

wayneforrest commented Aug 7, 2024

wayneforrest commented Aug 7, 2024

rayluo commented Aug 7, 2024

wayneforrest commented Aug 7, 2024

chlowell commented Aug 9, 2024

wayneforrest commented Aug 9, 2024

wayneforrest commented Aug 11, 2024 • edited Loading

sonarqubecloud bot commented Dec 9, 2024

Quality Gate passed

wayneforrest commented Jun 12, 2024 •

edited

Loading

wayneforrest commented Aug 11, 2024 •

edited

Loading