From 630a40ac4dd5ba0481678ac8003914f1992c7f6b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 12 Jul 2023 09:33:46 +0000 Subject: [PATCH 1/6] fix(site): prevent ExtractAPIKey from dirtying the HTML output If `httpmw.ExtractAPIKey` fails when we are rendering an HTML page, the HTML output will be dirtied with the error repsonse and the HTTP status will also be wrong. The use of this function in the `renderHTMLWithState` is additive, and failure means we simply can't embed static data. To fix this, we can simply pass a `http.ResponseWriter` that is no-op. Fixes #8351 --- site/site.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/site/site.go b/site/site.go index 6519e539e1458..2b8c93762101e 100644 --- a/site/site.go +++ b/site/site.go @@ -265,7 +265,7 @@ func ShouldCacheFile(reqFile string) bool { } func (h *Handler) serveHTML(resp http.ResponseWriter, request *http.Request, reqPath string, state htmlState) bool { - if data, err := h.renderHTMLWithState(resp, request, reqPath, state); err == nil { + if data, err := h.renderHTMLWithState(request, reqPath, state); err == nil { if reqPath == "" { // Pass "index.html" to the ServeContent so the ServeContent sets the right content headers. reqPath = "index.html" @@ -278,7 +278,7 @@ func (h *Handler) serveHTML(resp http.ResponseWriter, request *http.Request, req // renderWithState will render the file using the given nonce if the file exists // as a template. If it does not, it will return an error. -func (h *Handler) renderHTMLWithState(rw http.ResponseWriter, r *http.Request, filePath string, state htmlState) ([]byte, error) { +func (h *Handler) renderHTMLWithState(r *http.Request, filePath string, state htmlState) ([]byte, error) { var buf bytes.Buffer if filePath == "" { filePath = "index.html" @@ -290,7 +290,11 @@ func (h *Handler) renderHTMLWithState(rw http.ResponseWriter, r *http.Request, f // Cookies are sent when requesting HTML, so we can get the user // and pre-populate the state for the frontend to reduce requests. - apiKey, actor, _ := httpmw.ExtractAPIKey(rw, r, httpmw.ExtractAPIKeyConfig{ + // We use a noop response writer because we don't want to write + // anything to the response and break the HTML, an error means we + // simply don't pre-populate the state. + noopRW := noopResponseWriter{} + apiKey, actor, ok := httpmw.ExtractAPIKey(noopRW, r, httpmw.ExtractAPIKeyConfig{ Optional: true, DB: h.opts.Database, OAuth2Configs: h.opts.OAuth2Configs, @@ -300,7 +304,7 @@ func (h *Handler) renderHTMLWithState(rw http.ResponseWriter, r *http.Request, f RedirectToLogin: false, SessionTokenFunc: nil, }) - if apiKey != nil && actor != nil { + if ok && apiKey != nil && actor != nil { ctx := dbauthz.As(r.Context(), actor.Actor) var eg errgroup.Group @@ -392,6 +396,13 @@ func (h *Handler) renderHTMLWithState(rw http.ResponseWriter, r *http.Request, f return buf.Bytes(), nil } +// noopResponseWriter is a response writer that does nothing. +type noopResponseWriter struct{} + +func (noopResponseWriter) Header() http.Header { return http.Header{} } +func (noopResponseWriter) Write(p []byte) (int, error) { return len(p), nil } +func (noopResponseWriter) WriteHeader(int) {} + // secureHeaders is only needed for statically served files. We do not need this for api endpoints. // It adds various headers to enforce browser security features. func secureHeaders() *secure.Secure { From 54406d940f131d293451793b892a2acb84327470 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 12 Jul 2023 10:52:18 +0000 Subject: [PATCH 2/6] test(site): add test for injection with oauth error --- site/site_test.go | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/site/site_test.go b/site/site_test.go index 0348340acf986..26bf7f4eff5dd 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -69,6 +69,53 @@ func TestInjection(t *testing.T) { require.Equal(t, db2sdk.User(user, []uuid.UUID{}), got) } +func TestInjectionFailureProducesCleanHTML(t *testing.T) { + t.Parallel() + + db := dbfake.New() + + // Create an expired user with a refresh token, but provide no OAuth2 + // configuration so that refresh is impossible, this should result in + // an error when ExtractAPIKey is called. + user := dbgen.User(t, db, database.User{}) + _, token := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + LastUsed: database.Now().Add(-time.Hour), + ExpiresAt: database.Now().Add(-time.Second), + LoginType: database.LoginTypeGithub, + }) + _ = dbgen.UserLink(t, db, database.UserLink{ + UserID: user.ID, + LoginType: database.LoginTypeGithub, + OAuthRefreshToken: "hello", + OAuthExpiry: database.Now().Add(-time.Second), + }) + + binFs := http.FS(fstest.MapFS{}) + siteFS := fstest.MapFS{ + "index.html": &fstest.MapFile{ + Data: []byte("{{ .User }}"), + }, + } + handler := site.New(&site.Options{ + BinFS: binFs, + Database: db, + SiteFS: siteFS, + }) + + r := httptest.NewRequest("GET", "/", nil) + r.Header.Set(codersdk.SessionTokenHeader, token) + rw := httptest.NewRecorder() + + handler.ServeHTTP(rw, r) + + // Ensure we get a clean HTTML response with no user data or errors + // from httpmw.ExtractAPIKey. + require.Equal(t, http.StatusOK, rw.Code) + body := rw.Body.String() + require.Equal(t, "", body) +} + func TestCaching(t *testing.T) { t.Parallel() From 3180dbc20b5db5eea86f33f3e85d5a782fb2f480 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 12 Jul 2023 10:53:57 +0000 Subject: [PATCH 3/6] improve comment --- site/site_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/site_test.go b/site/site_test.go index 26bf7f4eff5dd..1757bbce6e4ac 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -76,7 +76,7 @@ func TestInjectionFailureProducesCleanHTML(t *testing.T) { // Create an expired user with a refresh token, but provide no OAuth2 // configuration so that refresh is impossible, this should result in - // an error when ExtractAPIKey is called. + // an error when httpmw.ExtractAPIKey is called. user := dbgen.User(t, db, database.User{}) _, token := dbgen.APIKey(t, db, database.APIKey{ UserID: user.ID, From 68f3aa93b8140424cd31c0c1196abddeed2c754d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 12 Jul 2023 10:54:28 +0000 Subject: [PATCH 4/6] typo --- site/site_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/site_test.go b/site/site_test.go index 1757bbce6e4ac..13512c7fb3b2f 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -109,7 +109,7 @@ func TestInjectionFailureProducesCleanHTML(t *testing.T) { handler.ServeHTTP(rw, r) - // Ensure we get a clean HTTML response with no user data or errors + // Ensure we get a clean HTML response with no user data or errors // from httpmw.ExtractAPIKey. require.Equal(t, http.StatusOK, rw.Code) body := rw.Body.String() From a3c408ebd24332bb5bc31aef8746bd8ec1f28a3a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 12 Jul 2023 11:08:40 +0000 Subject: [PATCH 5/6] be more explicit in test about skipped oauth configs --- site/site_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/site/site_test.go b/site/site_test.go index 13512c7fb3b2f..dd69a1b3f8ce4 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -26,6 +26,7 @@ import ( "github.com/coder/coder/coderd/database/db2sdk" "github.com/coder/coder/coderd/database/dbfake" "github.com/coder/coder/coderd/database/dbgen" + "github.com/coder/coder/coderd/httpmw" "github.com/coder/coder/codersdk" "github.com/coder/coder/site" "github.com/coder/coder/testutil" @@ -101,6 +102,12 @@ func TestInjectionFailureProducesCleanHTML(t *testing.T) { BinFS: binFs, Database: db, SiteFS: siteFS, + + // No OAuth2 configs, refresh will fail. + OAuth2Configs: &httpmw.OAuth2Configs{ + Github: nil, + OIDC: nil, + }, }) r := httptest.NewRequest("GET", "/", nil) From b70084d2165bdbfc35a522b4572df2673812db7b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 12 Jul 2023 11:10:22 +0000 Subject: [PATCH 6/6] s/require/assert/ to give more context --- site/site_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/site_test.go b/site/site_test.go index dd69a1b3f8ce4..de4e150618b8b 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -118,9 +118,9 @@ func TestInjectionFailureProducesCleanHTML(t *testing.T) { // Ensure we get a clean HTML response with no user data or errors // from httpmw.ExtractAPIKey. - require.Equal(t, http.StatusOK, rw.Code) + assert.Equal(t, http.StatusOK, rw.Code) body := rw.Body.String() - require.Equal(t, "", body) + assert.Equal(t, "", body) } func TestCaching(t *testing.T) {