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 { diff --git a/site/site_test.go b/site/site_test.go index 0348340acf986..de4e150618b8b 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" @@ -69,6 +70,59 @@ 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 httpmw.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, + + // No OAuth2 configs, refresh will fail. + OAuth2Configs: &httpmw.OAuth2Configs{ + Github: nil, + OIDC: nil, + }, + }) + + r := httptest.NewRequest("GET", "/", nil) + r.Header.Set(codersdk.SessionTokenHeader, token) + rw := httptest.NewRecorder() + + handler.ServeHTTP(rw, r) + + // Ensure we get a clean HTML response with no user data or errors + // from httpmw.ExtractAPIKey. + assert.Equal(t, http.StatusOK, rw.Code) + body := rw.Body.String() + assert.Equal(t, "", body) +} + func TestCaching(t *testing.T) { t.Parallel()