From 5d79d4b9dc3a2ce7918f581d178b5a763ec516ab Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 8 Feb 2022 17:30:48 -0800 Subject: [PATCH] Fix form_post.js mistake from recent commit; Better CORS on callback --- .../oidc/provider/formposthtml/form_post.js | 18 +++-- .../formposthtml/formposthtml_test.go | 4 +- pkg/oidcclient/login.go | 38 ++++++---- pkg/oidcclient/login_test.go | 71 +++++++++++++++++-- test/integration/formposthtml_test.go | 19 ++++- 5 files changed, 123 insertions(+), 27 deletions(-) diff --git a/internal/oidc/provider/formposthtml/form_post.js b/internal/oidc/provider/formposthtml/form_post.js index 57a18725..9e4b0203 100644 --- a/internal/oidc/provider/formposthtml/form_post.js +++ b/internal/oidc/provider/formposthtml/form_post.js @@ -44,18 +44,22 @@ window.onload = () => { responseParams['redirect_uri'].value, { method: 'POST', - mode: 'no-cors', + mode: 'no-cors', // in the future, we could change this to "cors" (see comment below) headers: {'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8'}, body: responseParams['encoded_params'].value, }) .then(response => { clearTimeout(timeout); - if (response.ok) { - transitionToState('success'); - } else { - // Got non-2XX http response status. - transitionToState('manual'); - } + // Requests made using "no-cors" mode will hide the real response.status by making it 0 + // and the real response.ok by making it false. + // If the real response was success, then we would like to show the success state. + // If the real response was an error, then we wish we could show the manual + // state, but we have no way to know that, as long as we are making "no-cors" requests. + // For now, show the success status for all responses. + // In the future, we could make this request in "cors" mode once old versions of our CLI + // which did not handle CORS are upgraded out by our users. That would allow us to use + // a conditional statement based on response.ok here to decide which state to transition into. + transitionToState('success'); }) .catch(() => transitionToState('manual')); }; diff --git a/internal/oidc/provider/formposthtml/formposthtml_test.go b/internal/oidc/provider/formposthtml/formposthtml_test.go index 0a6a30ec..afeb2755 100644 --- a/internal/oidc/provider/formposthtml/formposthtml_test.go +++ b/internal/oidc/provider/formposthtml/formposthtml_test.go @@ -30,7 +30,7 @@ var ( - + @@ -61,7 +61,7 @@ var ( // It's okay if this changes in the future, but this gives us a chance to eyeball the formatting. // Our browser-based integration tests should find any incompatibilities. testExpectedCSP = `default-src 'none'; ` + - `script-src 'sha256-Lon+X41NoXuVGPqi3LsAPmBqlDmwbu3lGhQii7/Zjrc='; ` + + `script-src 'sha256-P1dCaXS9frmkvGZ/cH/UljR70IOH963lmfptEgcn9j8='; ` + `style-src 'sha256-CtfkX7m8x2UdGYvGgDq+6b6yIAQsASW9pbQK+sG8fNA='; ` + `img-src data:; ` + `connect-src *; ` + diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index ca2335bc..afda042a 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -835,6 +835,20 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req var params url.Values if h.useFormPost { // nolint:nestif + // Return HTTP 405 for anything that's not a POST or an OPTIONS request. + if r.Method != http.MethodPost && r.Method != http.MethodOptions { + h.logger.V(debugLogLevel).Info("Pinniped: Got unexpected request on callback listener", "method", r.Method) + w.WriteHeader(http.StatusMethodNotAllowed) + return nil // keep listening for more requests + } + + // For POST and OPTIONS requests, calculate the allowed origin for CORS. + issuerURL, parseErr := url.Parse(h.issuer) + if parseErr != nil { + return httperr.Wrap(http.StatusInternalServerError, "invalid issuer url", parseErr) + } + allowOrigin := issuerURL.Scheme + "://" + issuerURL.Host + if r.Method == http.MethodOptions { // Google Chrome decided that it should do CORS preflight checks for this Javascript form submission POST request. // See https://developer.chrome.com/blog/private-network-access-preflight/ @@ -846,12 +860,9 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req return nil // keep listening for more requests } h.logger.V(debugLogLevel).Info("Pinniped: Got CORS preflight request from browser", "origin", origin) - issuerURL, parseErr := url.Parse(h.issuer) - if parseErr != nil { - return httperr.Wrap(http.StatusInternalServerError, "invalid issuer url", parseErr) - } // To tell the browser that it is okay to make the real POST request, return the following response. - w.Header().Set("Access-Control-Allow-Origin", issuerURL.Scheme+"://"+issuerURL.Host) + w.Header().Set("Access-Control-Allow-Origin", allowOrigin) + w.Header().Set("Vary", "*") // supposed to use Vary when Access-Control-Allow-Origin is a specific host w.Header().Set("Access-Control-Allow-Credentials", "false") w.Header().Set("Access-Control-Allow-Methods", "POST, OPTIONS") w.Header().Set("Access-Control-Allow-Private-Network", "true") @@ -864,20 +875,21 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req } w.WriteHeader(http.StatusNoContent) return nil // keep listening for more requests - } - - // Return HTTP 405 for anything that's not a POST. - if r.Method != http.MethodPost { - h.logger.V(debugLogLevel).Info("Pinniped: Got unexpected request on callback listener", "method", r.Method) - w.WriteHeader(http.StatusMethodNotAllowed) - return nil // keep listening for more requests - } + } // Otherwise, this is a POST request... // Parse and pull the response parameters from an application/x-www-form-urlencoded request body. if err := r.ParseForm(); err != nil { return httperr.Wrap(http.StatusBadRequest, "invalid form", err) } params = r.Form + + // Allow CORS requests for POST so in the future our Javascript code can be updated to use the fetch API's + // mode "cors", and still be compatible with older CLI versions starting with those that have this code + // for CORS headers. Updating to use CORS would allow our Javascript code (form_post.js) to see the true + // http response status from this endpoint. Note that the POST response does not need to set as many CORS + // headers as the OPTIONS preflight response. + w.Header().Set("Access-Control-Allow-Origin", allowOrigin) + w.Header().Set("Vary", "*") // supposed to use Vary when Access-Control-Allow-Origin is a specific host } else { // Return HTTP 405 for anything that's not a GET. if r.Method != http.MethodGet { diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index a7c765c8..fe6a8ce5 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -1885,6 +1885,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { method: http.MethodPost, query: "", wantNoCallbacks: true, + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusMethodNotAllowed, }, { @@ -1893,6 +1894,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { query: "", opt: withFormPostMode, wantNoCallbacks: true, + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusMethodNotAllowed, }, { @@ -1903,24 +1905,28 @@ func TestHandleAuthCodeCallback(t *testing.T) { body: []byte(`%`), opt: withFormPostMode, wantErr: `invalid form: invalid URL escape "%"`, + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusBadRequest, }, { name: "invalid state", query: "state=invalid", wantErr: "missing or invalid state parameter", + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusForbidden, }, { name: "error code from provider", query: "state=test-state&error=some_error", wantErr: `login failed with code "some_error"`, + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusBadRequest, }, { name: "error code with a description from provider", query: "state=test-state&error=some_error&error_description=optional%20error%20description", wantErr: `login failed with code "some_error": optional error description`, + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusBadRequest, }, { @@ -1929,6 +1935,23 @@ func TestHandleAuthCodeCallback(t *testing.T) { query: "", headers: map[string][]string{"Origin": {"https://some-origin.com"}}, wantErr: `invalid issuer url: parse "://bad-url": missing protocol scheme`, + wantHeaders: map[string][]string{}, + wantHTTPStatus: http.StatusInternalServerError, + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.useFormPost = true + h.issuer = "://bad-url" + return nil + } + }, + }, + { + name: "in form post mode, invalid issuer url config during POST request returns an error", + method: http.MethodPost, + query: "", + headers: map[string][]string{"Origin": {"https://some-origin.com"}}, + wantErr: `invalid issuer url: parse "://bad-url": missing protocol scheme`, + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusInternalServerError, opt: func(t *testing.T) Option { return func(h *handlerState) error { @@ -1944,6 +1967,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { query: "", opt: withFormPostMode, wantNoCallbacks: true, + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusBadRequest, }, { @@ -1957,6 +1981,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { "Access-Control-Allow-Credentials": {"false"}, "Access-Control-Allow-Methods": {"POST, OPTIONS"}, "Access-Control-Allow-Origin": {"https://valid-issuer.com"}, + "Vary": {"*"}, "Access-Control-Allow-Private-Network": {"true"}, }, opt: func(t *testing.T) Option { @@ -1981,6 +2006,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { "Access-Control-Allow-Credentials": {"false"}, "Access-Control-Allow-Methods": {"POST, OPTIONS"}, "Access-Control-Allow-Origin": {"https://valid-issuer.com"}, + "Vary": {"*"}, "Access-Control-Allow-Private-Network": {"true"}, "Access-Control-Allow-Headers": {"header1, header2, header3"}, }, @@ -1996,6 +2022,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { name: "invalid code", query: "state=test-state&code=invalid", wantErr: "could not complete code exchange: some exchange error", + wantHeaders: map[string][]string{}, wantHTTPStatus: http.StatusBadRequest, opt: func(t *testing.T) Option { return func(h *handlerState) error { @@ -2015,6 +2042,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { name: "valid", query: "state=test-state&code=valid", wantHTTPStatus: http.StatusOK, + wantHeaders: map[string][]string{"Content-Type": {"text/plain; charset=utf-8"}}, opt: func(t *testing.T) Option { return func(h *handlerState) error { h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} @@ -2030,10 +2058,44 @@ func TestHandleAuthCodeCallback(t *testing.T) { }, }, { - name: "valid form_post", - method: http.MethodPost, - headers: map[string][]string{"Content-Type": {"application/x-www-form-urlencoded"}}, - body: []byte(`state=test-state&code=valid`), + name: "valid form_post", + method: http.MethodPost, + headers: map[string][]string{"Content-Type": {"application/x-www-form-urlencoded"}}, + body: []byte(`state=test-state&code=valid`), + wantHeaders: map[string][]string{ + "Access-Control-Allow-Origin": {"https://valid-issuer.com"}, + "Vary": {"*"}, + "Content-Type": {"text/plain; charset=utf-8"}, + }, + wantHTTPStatus: http.StatusOK, + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.useFormPost = true + h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} + h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) provider.UpstreamOIDCIdentityProviderI { + mock := mockUpstream(t) + mock.EXPECT(). + ExchangeAuthcodeAndValidateTokens(gomock.Any(), "valid", pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), testRedirectURI). + Return(&oidctypes.Token{IDToken: &oidctypes.IDToken{Token: "test-id-token"}}, nil) + return mock + } + return nil + } + }, + }, + { + name: "valid form_post made with the same origin headers that would be used by a Javascript fetch client using mode=cors", + method: http.MethodPost, + headers: map[string][]string{ + "Content-Type": {"application/x-www-form-urlencoded"}, + "Origin": {"https://some-origin.com"}, + }, + body: []byte(`state=test-state&code=valid`), + wantHeaders: map[string][]string{ + "Access-Control-Allow-Origin": {"https://valid-issuer.com"}, + "Vary": {"*"}, + "Content-Type": {"text/plain; charset=utf-8"}, + }, wantHTTPStatus: http.StatusOK, opt: func(t *testing.T) Option { return func(h *handlerState) error { @@ -2062,6 +2124,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { pkce: pkce.Code("test-pkce"), nonce: nonce.Nonce("test-nonce"), logger: testlogger.New(t).Logger, + issuer: "https://valid-issuer.com/with/some/path", } if tt.opt != nil { require.NoError(t, tt.opt(t)(h)) diff --git a/test/integration/formposthtml_test.go b/test/integration/formposthtml_test.go index d845a60c..d891488e 100644 --- a/test/integration/formposthtml_test.go +++ b/test/integration/formposthtml_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -60,6 +60,10 @@ func TestFormPostHTML_Parallel(t *testing.T) { // // This case is fairly unlikely in practice, and if the CLI encounters // an error it can also expose it via stderr anyway. + // + // In the future, we could change the Javascript code to use mode 'cors' + // because we have upgraded our CLI callback endpoint to handle CORS, + // and then we could change this to formpostExpectManualState(). formpostExpectSuccessState(t, page) }) @@ -109,6 +113,19 @@ func formpostCallbackServer(t *testing.T) (string, func(*testing.T, url.Values)) results := make(chan url.Values) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // 404 for any other requests aside from POSTs. We do not need to support CORS preflight OPTIONS + // requests for this test because both the web page and the callback are on 127.0.0.1 (same origin). + if r.Method != http.MethodPost { + t.Logf("test callback server got unexpeted request method") + w.WriteHeader(http.StatusNotFound) + return + } + + // Allow CORS requests. This will be needed for this test in the future if we change + // the Javascript code from using mode 'no-cors' to instead use mode 'cors'. At the + // moment it should be ignored by the browser. + w.Header().Set("Access-Control-Allow-Origin", "*") + assert.NoError(t, r.ParseForm()) // Extract only the POST parameters (r.Form also contains URL query parameters).