Merge commit from fork

* fix: prevent reflected XSS in `/response-headers` endpoint

* link to security policy from README

* decompose/refactor a bit

* apply same fix to /base64 endpoint

* fmt

* switch to allowlist of safe content types, w/ config option for backwards compat

* readme styling

Author: mccutchen

README.md

@@ -110,6 +110,15 @@ variables (or a combination of the two):
 | `-srv-read-timeout` | `SRV_READ_TIMEOUT` | Value to use for the http.Server's ReadTimeout option | 5s |
 | `-use-real-hostname` | `USE_REAL_HOSTNAME` | Expose real hostname as reported by os.Hostname() in the /hostname endpoint | false |
 
+#### ⚠️ **HERE BE DRAGONS** ⚠️
+
+These configuration options are dangerous and/or deprecated and should be
+avoided unless backwards compatibility is absolutely required.
+
+| Argument| Env var | Documentation | Default |
+| - | - | - | - |
+| `-unsafe-allow-dangerous-responses` | `UNSAFE_ALLOW_DANGEROUS_RESPONSES` | Allow endpoints to return unescaped HTML when clients control response Content-Type (enables XSS attacks) | false |
+
 **Notes:**
 - Command line arguments take precedence over environment variables.
 - See [Production considerations] for recommendations around safe configuration
@@ -201,6 +210,10 @@ public internet, consider tuning it appropriately:
 
 See [DEVELOPMENT.md][].
 
+## Security
+
+See [SECURITY.md][].
+
 ## Motivation & prior art
 
 I've been a longtime user of [Kenneith Reitz][kr]'s original
@@ -240,4 +253,5 @@ Compared to [ahmetb/go-httpbin][ahmet]:
 [mccutchen/httpbingo.org]: https://github.com/mccutchen/httpbingo.org
 [Observer]: https://pkg.go.dev/github.com/mccutchen/go-httpbin/v2/httpbin#Observer
 [Production considerations]: #production-considerations
+[SECURITY.md]: ./SECURITY.md
 [zerolog]: https://github.com/rs/zerolog

httpbin/cmd/cmd.go

@@ -91,6 +91,9 @@ func mainImpl(args []string, getEnvVal func(string) string, getEnviron func() []
 	if len(cfg.AllowedRedirectDomains) > 0 {
 		opts = append(opts, httpbin.WithAllowedRedirectDomains(cfg.AllowedRedirectDomains))
 	}
+	if cfg.UnsafeAllowDangerousResponses {
+		opts = append(opts, httpbin.WithUnsafeAllowDangerousResponses())
+	}
 	app := httpbin.New(opts...)
 
 	srv := &http.Server{
@@ -128,6 +131,14 @@ type config struct {
 	SrvReadHeaderTimeout   time.Duration
 	SrvReadTimeout         time.Duration
 
+	// If true, endpoints that allow clients to specify a response
+	// Conntent-Type will NOT escape HTML entities in the response body, which
+	// can enable (e.g.) reflected XSS attacks.
+	//
+	// This configuration is only supported for backwards compatibility if
+	// absolutely necessary.
+	UnsafeAllowDangerousResponses bool
+
 	// temporary placeholders for arguments that need extra processing
 	rawAllowedRedirectDomains string
 	rawUseRealHostname        bool
@@ -169,6 +180,10 @@ func loadConfig(args []string, getEnvVal func(string) string, getEnviron func()
 	fs.DurationVar(&cfg.SrvReadHeaderTimeout, "srv-read-header-timeout", defaultSrvReadHeaderTimeout, "Value to use for the http.Server's ReadHeaderTimeout option")
 	fs.DurationVar(&cfg.SrvReadTimeout, "srv-read-timeout", defaultSrvReadTimeout, "Value to use for the http.Server's ReadTimeout option")
 
+	// Here be dragons! This flag is only for backwards compatibility and
+	// should not be used in production.
+	fs.BoolVar(&cfg.UnsafeAllowDangerousResponses, "unsafe-allow-dangerous-responses", false, "Allow endpoints to return unescaped HTML when clients control response Content-Type (enables XSS attacks)")
+
 	// in order to fully control error output whether CLI arguments or env vars
 	// are used to configure the app, we need to take control away from the
 	// flag-set, which by defaults prints errors automatically.
@@ -258,10 +273,7 @@ func loadConfig(args []string, getEnvVal func(string) string, getEnviron func()
 		return nil, configErr(`invalid log format %q, must be "text" or "json"`, cfg.LogFormat)
 	}
 
-	// useRealHostname will be true if either the `-use-real-hostname`
-	// arg is given on the command line or if the USE_REAL_HOSTNAME env var
-	// is one of "1" or "true".
-	if useRealHostnameEnv := getEnvVal("USE_REAL_HOSTNAME"); useRealHostnameEnv == "1" || useRealHostnameEnv == "true" {
+	if getEnvBool(getEnvVal("USE_REAL_HOSTNAME")) {
 		cfg.rawUseRealHostname = true
 	}
 	if cfg.rawUseRealHostname {
@@ -301,6 +313,10 @@ func loadConfig(args []string, getEnvVal func(string) string, getEnviron func()
 		}
 	}
 
+	if getEnvBool(getEnvVal("UNSAFE_ALLOW_DANGEROUS_RESPONSES")) {
+		cfg.UnsafeAllowDangerousResponses = true
+	}
+
 	// reset temporary fields to their zero values
 	cfg.rawAllowedRedirectDomains = ""
 	cfg.rawUseRealHostname = false
@@ -319,6 +335,10 @@ func loadConfig(args []string, getEnvVal func(string) string, getEnviron func()
 	return cfg, nil
 }
 
+func getEnvBool(val string) bool {
+	return val == "1" || val == "true"
+}
+
 func listenAndServeGracefully(srv *http.Server, cfg *config, logger *slog.Logger) error {
 	doneCh := make(chan error, 1)
 

httpbin/cmd/cmd_test.go

@@ -46,6 +46,8 @@ const usage = `Usage of go-httpbin:
     	Value to use for the http.Server's ReadHeaderTimeout option (default 1s)
   -srv-read-timeout duration
     	Value to use for the http.Server's ReadTimeout option (default 5s)
+  -unsafe-allow-dangerous-responses
+    	Allow endpoints to return unescaped HTML when clients control response Content-Type (enables XSS attacks)
   -use-real-hostname
     	Expose value of os.Hostname() in the /hostname endpoint instead of dummy value
 `
@@ -475,6 +477,54 @@ func TestLoadConfig(t *testing.T) {
 				SrvReadTimeout: 1234 * time.Second,
 			}),
 		},
+
+		// unsafe-allow-dangerous-responses
+		"ok -unsafe-allow-dangerous-responses": {
+			args: []string{"-unsafe-allow-dangerous-responses"},
+			wantCfg: mergedConfig(defaultCfg, &config{
+				UnsafeAllowDangerousResponses: true,
+			}),
+		},
+		"ok -unsafe-allow-dangerous-responses=1": {
+			args: []string{"-unsafe-allow-dangerous-responses", "1"},
+			wantCfg: mergedConfig(defaultCfg, &config{
+				UnsafeAllowDangerousResponses: true,
+			}),
+		},
+		"ok -unsafe-allow-dangerous-responses=true": {
+			args: []string{"-unsafe-allow-dangerous-responses", "true"},
+			wantCfg: mergedConfig(defaultCfg, &config{
+				UnsafeAllowDangerousResponses: true,
+			}),
+		},
+		// any value for the argument is interpreted as true
+		"ok -unsafe-allow-dangerous-responses=0": {
+			args: []string{"-unsafe-allow-dangerous-responses", "0"},
+			wantCfg: mergedConfig(defaultCfg, &config{
+				UnsafeAllowDangerousResponses: true,
+			}),
+		},
+		"ok UNSAFE_ALLOW_DANGEROUS_RESPONSES=1": {
+			env: map[string]string{"UNSAFE_ALLOW_DANGEROUS_RESPONSES": "1"},
+			wantCfg: mergedConfig(defaultCfg, &config{
+				UnsafeAllowDangerousResponses: true,
+			}),
+		},
+		"ok UNSAFE_ALLOW_DANGEROUS_RESPONSES=true": {
+			env: map[string]string{"UNSAFE_ALLOW_DANGEROUS_RESPONSES": "true"},
+			wantCfg: mergedConfig(defaultCfg, &config{
+				UnsafeAllowDangerousResponses: true,
+			}),
+		},
+		// case sensitive
+		"ok UNSAFE_ALLOW_DANGEROUS_RESPONSES=TRUE": {
+			env:     map[string]string{"UNSAFE_ALLOW_DANGEROUS_RESPONSES": "TRUE"},
+			wantCfg: defaultCfg,
+		},
+		"ok UNSAFE_ALLOW_DANGEROUS_RESPONSES=false": {
+			env:     map[string]string{"UNSAFE_ALLOW_DANGEROUS_RESPONSES": "false"},
+			wantCfg: defaultCfg,
+		},
 	}
 
 	for name, tc := range testCases {

httpbin/handlers.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"html"
 	"io"
 	"net/http"
 	"net/http/httputil"
@@ -332,17 +333,40 @@ func (h *HTTPBin) Unstable(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(status)
 }
 
-// ResponseHeaders responds with a map of header values
+// ResponseHeaders sets every incoming query parameter as a response header and
+// returns the headers serialized as JSON.
+//
+// If the Content-Type query parameter is given and set to a "dangerous" value
+// (i.e. one that might be rendered as HTML in a web browser), the keys and
+// values in the JSON response body will be escaped.
 func (h *HTTPBin) ResponseHeaders(w http.ResponseWriter, r *http.Request) {
 	args := r.URL.Query()
+	contentType := args.Get("Content-Type")
+
+	// response headers are not escaped, regardless of content type
 	for k, vs := range args {
 		for _, v := range vs {
 			w.Header().Add(k, v)
 		}
 	}
-	if contentType := w.Header().Get("Content-Type"); contentType == "" {
+	// only set our own content type if one was not already set based on
+	// incoming request params
+	if contentType == "" {
 		w.Header().Set("Content-Type", jsonContentType)
 	}
+
+	// if response content type is dangrous, escape keys and values before
+	// serializing response body
+	if h.mustEscapeResponse(contentType) {
+		tmp := make(url.Values, len(args))
+		for k, vs := range args {
+			for _, v := range vs {
+				tmp.Add(html.EscapeString(k), html.EscapeString(v))
+			}
+		}
+		args = tmp
+	}
+
 	mustMarshalJSON(w, args)
 }
 
@@ -1103,6 +1127,10 @@ func (h *HTTPBin) Base64(w http.ResponseWriter, r *http.Request) {
 	if ct == "" {
 		ct = textContentType
 	}
+	// prevent XSS and other client side vulns if the content type is dangerous
+	if h.mustEscapeResponse(ct) {
+		result = []byte(html.EscapeString(string(result)))
+	}
 	writeResponse(w, http.StatusOK, ct, result)
 }
 

httpbin/handlers_test.go

@@ -9,6 +9,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
+	"html"
 	"io"
 	"log/slog"
 	"mime"
@@ -1256,6 +1257,92 @@ func TestResponseHeaders(t *testing.T) {
 		assert.StatusCode(t, resp, http.StatusOK)
 		assert.ContentType(t, resp, contentType)
 	})
+
+	t.Run("escaping HTML content", func(t *testing.T) {
+		dangerousString := "<img/src/onerror=alert('xss')>"
+
+		for _, tc := range []struct {
+			contentType  string
+			shouldEscape bool
+		}{
+			// a tiny number of content types are considered safe and do not
+			// require escaping (see isDangerousContentType)
+			{"application/json; charset=utf8", false},
+			{"text/plain", false},
+			{"application/octet-string", false},
+
+			// everything else requires escaping
+			{"", true},
+			{"application/xml", true},
+			{"image/png", true},
+			{"text/html; charset=utf8", true},
+			{"text/html", true},
+		} {
+			tc := tc
+			t.Run(tc.contentType, func(t *testing.T) {
+				t.Parallel()
+
+				params := url.Values{}
+				if tc.contentType != "" {
+					params.Set("Content-Type", tc.contentType)
+				}
+				// need to ensure dangerous strings are escaped as both keys
+				// and values
+				params.Set("xss", dangerousString)
+				params.Set(dangerousString, "xss")
+
+				req, _ := http.NewRequest("GET", fmt.Sprintf("%s/response-headers?%s", srv.URL, params.Encode()), nil)
+				resp := must.DoReq(t, client, req)
+
+				assert.StatusCode(t, resp, http.StatusOK)
+				if tc.contentType != "" {
+					assert.ContentType(t, resp, tc.contentType)
+				} else {
+					assert.ContentType(t, resp, jsonContentType)
+				}
+
+				gotParams := must.Unmarshal[url.Values](t, resp.Body)
+				for key, wantVals := range params {
+					if tc.shouldEscape {
+						key = html.EscapeString(key)
+					}
+					gotVals := gotParams[key]
+					assert.Equal(t, len(gotVals), len(wantVals), "unexpected number of values for key %q (escaped=%v)", key, tc.shouldEscape)
+					for i, wantVal := range wantVals {
+						gotVal := gotVals[i]
+						if tc.shouldEscape {
+							assert.Equal(t, gotVal, html.EscapeString(wantVal), "expected HTML-escaped value")
+						} else {
+							assert.Equal(t, gotVal, wantVal, "expected unescaped value")
+						}
+					}
+				}
+			})
+		}
+	})
+
+	t.Run("dangerously not escaping responses", func(t *testing.T) {
+		t.Parallel()
+
+		app := createApp(WithUnsafeAllowDangerousResponses())
+		srv := httptest.NewServer(app)
+		defer srv.Close()
+
+		dangerousString := "<img/src/onerror=alert('xss')>"
+
+		params := url.Values{}
+		params.Set("Content-Type", "text/html")
+		params.Set("xss", dangerousString)
+
+		req, _ := http.NewRequest("GET", fmt.Sprintf("%s/response-headers?%s", srv.URL, params.Encode()), nil)
+		resp := must.DoReq(t, client, req)
+
+		assert.StatusCode(t, resp, http.StatusOK)
+		assert.ContentType(t, resp, "text/html")
+
+		// dangerous string is not escaped
+		assert.BodyContains(t, resp, dangerousString)
+	})
 }
 
 func TestRedirects(t *testing.T) {
@@ -2992,6 +3079,12 @@ func TestBase64(t *testing.T) {
 			`{"server": "go-httpbin"}` + "\n",
 			"application/json",
 		},
+		{
+			// XSS prevention w/ dangerous content type
+			"/base64/PGltZy9zcmMvb25lcnJvcj1hbGVydCgneHNzJyk+?content-type=text/html",
+			html.EscapeString("<img/src/onerror=alert('xss')>"),
+			"text/html",
+		},
 	}
 
 	for _, test := range okTests {

httpbin/helpers.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io"
 	"math/rand"
+	"mime"
 	"mime/multipart"
 	"net/http"
 	"net/url"
@@ -587,3 +588,26 @@ func encodeServerTimings(timings []serverTiming) string {
 	}
 	return strings.Join(entries, ", ")
 }
+
+// The following content types are considered safe enough to skip HTML-escaping
+// response bodies.
+//
+// See [1] for an example of the wide variety of unsafe content types, which
+// varies by browser vendor and could change in the future.
+//
+// [1]: https://github.com/BlackFan/content-type-research/blob/4e4347254/XSS.md
+var safeContentTypes = map[string]bool{
+	"text/plain":               true,
+	"application/json":         true,
+	"application/octet-string": true,
+}
+
+// isDangerousContentType determines whether the given Content-Type header
+// value could be unsafe (e.g. at risk of XSS) when rendered by a web browser.
+func isDangerousContentType(ct string) bool {
+	mediatype, _, err := mime.ParseMediaType(ct)
+	if err != nil {
+		return true
+	}
+	return !safeContentTypes[mediatype]
+}