coder · mafredri · Jul 12, 2023 · Jul 12, 2023 · Jul 12, 2023 · Jul 12, 2023
diff --git 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
@@ -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("<html>{{ .User }}</html>"),
+		},
+	}
+	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, "<html></html>", body)
+}
+
 func TestCaching(t *testing.T) {
 	t.Parallel()