microsoft-authentication-library-for-go Add option to set success and error pages for when calling AcquireInt…

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)
}

Jun 12 '24 10:06 wayneforrest

@microsoft-github-policy-service agree

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

@microsoft-github-policy-service agree

Jun 12 '24 10:06 wayneforrest

@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?

Jul 19 '24 20:07 localden

@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?

Aug 02 '24 01:08 wayneforrest

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: https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/004301ceccb1206100a19cae1a6a4e4e37be8b20/apps/internal/local/server.go#L148

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?

Aug 05 '24 21:08 chlowell

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: https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/004301ceccb1206100a19cae1a6a4e4e37be8b20/apps/internal/local/server.go#L148

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!

Aug 05 '24 23:08 localden

3. Server interpolates error details into failPage: https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/004301ceccb1206100a19cae1a6a4e4e37be8b20/apps/internal/local/server.go#L148

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).

Aug 06 '24 07:08 rayluo

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.

Aug 06 '24 18:08 chlowell

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.

Aug 06 '24 19:08 rayluo

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 ?

Aug 07 '24 10:08 wayneforrest

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.

Aug 07 '24 10:08 wayneforrest

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.

Aug 07 '24 21:08 rayluo

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.

Aug 07 '24 23:08 wayneforrest

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).

Aug 09 '24 17:08 chlowell

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)

Aug 09 '24 20:08 wayneforrest

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.

Aug 11 '24 06:08 wayneforrest

I talked this over with SDK architects today and they pointed out that text/template uses reflection features that prevent the linker from identifying dead code (see this presentation for an explanation). Using that package in MSAL would therefore increase the binary size of dependent applications unnecessarily.

Simply continuing to interpolate details into the error page with fmt.Sprintf as I've suggested elsewhere doesn't work well either because fmt methods output warnings when arguments don't match verbs:

fmt.Sprintf("%s") // %!s(MISSING)
fmt.Sprintf("", "foo") // %!(EXTRA string=foo)

So, I'm thinking the best solution is to take a callback for customizing the error page: give applications a struct with error details and let them decide how to make HTML from it. We'd export the Result type (perhaps as public.AcquireTokenInteractiveResult?), and instead of a []byte for the error page, WithSystemBrowserOptions would take a func(ResultTypeNameTBD) ([]byte, error). Thoughts?

Sep 16 '24 23:09 chlowell

html/template unnecessarily increases application size 😞

Hi @chlowell, this comes as a big surprise, but also very interesting. Thank you for sharing the video.

I did a very rough test build, using this example as the SDK application:

package main

+import (
	"fmt"

	"github.com/AzureAD/microsoft-authentication-library-for-go/apps/public"
)

+func main() {
	_, _ = public.New("client_id", public.WithAuthority("https://login.microsoftonline.com/your_tenant"))
	fmt.Println("vim-go")
}

And got these results: go build ./... 4601314 main branch - #e331a85899 4867506 using html/template / reflection -rwxr-xr-x 1 wayneforrest staff 4.4M 21 Sep 22:32 microsoft-authentication-library-for-go -rwxr-xr-x 1 wayneforrest staff 4.6M 21 Sep 22:35 microsoft-authentication-library-for-go

diff = 260KB

With the callback solution suggestion, would the XSS protection be the responsibility of the SDK application?

An alternative would be to go back to using html.EscapeString and do manual interpolation. Here is an example application / approach I was thinking of:

https://go.dev/play/p/0qHtTBRn1Fy

Please let me know your thoughts. I am happy to implement either solution.

Sep 21 '24 11:09 wayneforrest

With the callback solution suggestion, would the XSS protection be the responsibility of the SDK application?

The stakes are high. SDK shall achieve that.

Sep 22 '24 06:09 rayluo

@rayluo / @chlowell Please see my last two commits: 1: I removed the html template, and reverted to using html.EscapeString as it was before. 2: Then also keeping the concept of a template, but instead doing the parsing / interpolation manually, circumventing the use of the html/template package.

I accidentally had a main.go file checked in, when I was comparing the binary sizes; I have removed the file, by removing the commit.

Oct 01 '24 22:10 wayneforrest