fix(site): prevent ExtractAPIKey from dirtying the HTML output (#8450)

mafredri · web-flow · commit f6a8a5f7be39 · 2023-07-12T15:38:30.000+03:00
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
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()
 
