djpbessems/ContainerImage.Pinniped
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix form_post.js mistake from recent commit; Better CORS on callback

Browse Source
This commit is contained in:
Ryan Richard 2022-02-08 17:30:48 -08:00
parent f6f188565b
commit 5d79d4b9dc
5 changed files with 123 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
internal/oidc/provider/formposthtml/form_post.js
View File

@ -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) {
// 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');
} else {
// Got non-2XX http response status.
transitionToState('manual');
}
})
.catch(() => transitionToState('manual'));
};

4
internal/oidc/provider/formposthtml/formposthtml_test.go
View File

@ -30,7 +30,7 @@ var (
<head>
<meta charset="UTF-8">
<style>body{font-family:metropolis-light,Helvetica,sans-serif}h1{font-size:20px}.state{position:absolute;top:100px;left:50%;width:400px;height:80px;margin-top:-40px;margin-left:-200px;font-size:14px;line-height:24px}button{margin:-10px;padding:10px;text-align:left;width:100%;display:inline;border:none;background:0 0;cursor:pointer;transition:all .1s}button:hover{background-color:#eee;transform:scale(1.01)}button:active{background-color:#ddd;transform:scale(.99)}code{display:block;word-wrap:break-word;word-break:break-all;font-size:12px;font-family:monospace;color:#333}.copy-icon{float:left;width:36px;height:36px;margin-top:-3px;margin-right:10px;background-size:contain;background-repeat:no-repeat;background-image:url("data:image/svg+xml,%3Csvg width=%2236%22 height=%2236%22 viewBox=%220 0 36 36%22 xmlns=%22http://www.w3.org/2000/svg%22 xmlns:xlink=%22http://www.w3.org/1999/xlink%22%3E%3Ctitle%3Ecopy-to-clipboard-line%3C/title%3E%3Cpath d=%22M22.6 4H21.55a3.89 3.89.0 00-7.31.0H13.4A2.41 2.41.0 0011 6.4V10H25V6.4A2.41 2.41.0 0022.6 4zM23 8H13V6.25A.25.25.0 0113.25 6h2.69l.12-1.11A1.24 1.24.0 0116.61 4a2 2 0 013.15 1.18l.09.84h2.9a.25.25.0 01.25.25z%22 class=%22clr-i-outline clr-i-outline-path-1%22/%3E%3Cpath d=%22M33.25 18.06H21.33l2.84-2.83a1 1 0 10-1.42-1.42L17.5 19.06l5.25 5.25a1 1 0 00.71.29 1 1 0 00.71-1.7l-2.84-2.84H33.25a1 1 0 000-2z%22 class=%22clr-i-outline clr-i-outline-path-2%22/%3E%3Cpath d=%22M29 16h2V6.68A1.66 1.66.0 0029.35 5H27.08V7H29z%22 class=%22clr-i-outline clr-i-outline-path-3%22/%3E%3Cpath d=%22M29 31H7V7H9V5H6.64A1.66 1.66.0 005 6.67V31.32A1.66 1.66.0 006.65 33H29.36A1.66 1.66.0 0031 31.33V22.06H29z%22 class=%22clr-i-outline clr-i-outline-path-4%22/%3E%3Crect x=%220%22 y=%220%22 width=%2236%22 height=%2236%22 fill-opacity=%220%22/%3E%3C/svg%3E")}@keyframes loader{to{transform:rotate(360deg)}}#loading{content:'';box-sizing:border-box;width:80px;height:80px;margin-top:-40px;margin-left:-40px;border-radius:50%;border:2px solid #fff;border-top-color:#1b3951;animation:loader .6s linear infinite}</style>
<script>window.onload=()=>{const e=t=>{Array.from(document.querySelectorAll('.state')).forEach(e=>e.hidden=!0);const e=document.getElementById(t);e.hidden=!1,document.title=e.dataset.title,document.getElementById('favicon').setAttribute('href','data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 100 100%22><text y=%22.9em%22 font-size=%2290%22>'+e.dataset.favicon+'</text></svg>')};e('loading'),window.history.replaceState(null,'','./'),document.getElementById('manual-copy-button').onclick=()=>{const e=document.getElementById('manual-copy-button').innerText;navigator.clipboard.writeText(e).then(()=>console.info('copied authorization code '+e+' to clipboard')).catch(t=>console.error('failed to copy code '+e+' to clipboard: '+t))};const n=setTimeout(()=>e('manual'),2e3),t=document.forms[0].elements;fetch(t.redirect_uri.value,{method:'POST',mode:'no-cors',headers:{'Content-Type':'application/x-www-form-urlencoded;charset=UTF-8'},body:t.encoded_params.value}).then(t=>{clearTimeout(n),t.ok?e('success'):e('manual')}).catch(()=>e('manual'))}</script>
<script>window.onload=()=>{const e=t=>{Array.from(document.querySelectorAll('.state')).forEach(e=>e.hidden=!0);const e=document.getElementById(t);e.hidden=!1,document.title=e.dataset.title,document.getElementById('favicon').setAttribute('href','data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 100 100%22><text y=%22.9em%22 font-size=%2290%22>'+e.dataset.favicon+'</text></svg>')};e('loading'),window.history.replaceState(null,'','./'),document.getElementById('manual-copy-button').onclick=()=>{const e=document.getElementById('manual-copy-button').innerText;navigator.clipboard.writeText(e).then(()=>console.info('copied authorization code '+e+' to clipboard')).catch(t=>console.error('failed to copy code '+e+' to clipboard: '+t))};const n=setTimeout(()=>e('manual'),2e3),t=document.forms[0].elements;fetch(t.redirect_uri.value,{method:'POST',mode:'no-cors',headers:{'Content-Type':'application/x-www-form-urlencoded;charset=UTF-8'},body:t.encoded_params.value}).then(t=>{clearTimeout(n),e('success')}).catch(()=>e('manual'))}</script>
<link id="favicon" rel="icon"/>
</head>
<body>
@ -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 *; ` +

38
pkg/oidcclient/login.go
View File

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

63
pkg/oidcclient/login_test.go
View File

@ -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}
@ -2034,6 +2062,40 @@ func TestHandleAuthCodeCallback(t *testing.T) {
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))

19
test/integration/formposthtml_test.go
View File

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