net/http/fcgi: expose cgi env vars in request context

The current interface can't access all environment
variables directly or via cgi.RequestFromMap, which
only reads variables on its "white list" to be set on
the http.Request it returns. If an fcgi variable is
not on the "white list" - e.g. REMOTE_USER - the old
code has no access to its value.

This passes variables in the Request context that aren't
used to add data to the Request itself and adds a method
that parses those env vars from the Request's context.

Fixes #16546

Change-Id: Ibf933a768b677ece1bb93d7bf99a14cef36ec671
Reviewed-on: https://go-review.googlesource.com/40012
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

src/go/build/deps_test.go

@@ -410,7 +410,7 @@ var pkgDeps = map[string][]string{
 	"expvar":             {"L4", "OS", "encoding/json", "net/http"},
 	"net/http/cgi":       {"L4", "NET", "OS", "crypto/tls", "net/http", "regexp"},
 	"net/http/cookiejar": {"L4", "NET", "net/http"},
-	"net/http/fcgi":      {"L4", "NET", "OS", "net/http", "net/http/cgi"},
+	"net/http/fcgi":      {"L4", "NET", "OS", "context", "net/http", "net/http/cgi"},
 	"net/http/httptest":  {"L4", "NET", "OS", "crypto/tls", "flag", "net/http", "net/http/internal", "crypto/x509"},
 	"net/http/httputil":  {"L4", "NET", "OS", "context", "net/http", "net/http/internal"},
 	"net/http/pprof":     {"L4", "OS", "html/template", "net/http", "runtime/pprof", "runtime/trace"},

src/net/http/fcgi/child.go

@@ -7,6 +7,7 @@ package fcgi
 // This file implements FastCGI from the perspective of a child process.
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -31,6 +32,10 @@ type request struct {
 	keepConn  bool
 }
 
+// envVarsContextKey uniquely identifies a mapping of CGI
+// environment variables to their values in a request context
+type envVarsContextKey struct{}
+
 func newRequest(reqId uint16, flags uint8) *request {
 	r := &request{
 		reqId:    reqId,
@@ -259,6 +264,18 @@ func (c *child) handleRecord(rec *record) error {
 	}
 }
 
+// filterOutUsedEnvVars returns a new map of env vars without the
+// variables in the given envVars map that are read for creating each http.Request
+func filterOutUsedEnvVars(envVars map[string]string) map[string]string {
+	withoutUsedEnvVars := make(map[string]string)
+	for k, v := range envVars {
+		if addFastCGIEnvToContext(k) {
+			withoutUsedEnvVars[k] = v
+		}
+	}
+	return withoutUsedEnvVars
+}
+
 func (c *child) serveRequest(req *request, body io.ReadCloser) {
 	r := newResponse(c, req)
 	httpReq, err := cgi.RequestFromMap(req.params)
@@ -268,6 +285,9 @@ func (c *child) serveRequest(req *request, body io.ReadCloser) {
 		c.conn.writeRecord(typeStderr, req.reqId, []byte(err.Error()))
 	} else {
 		httpReq.Body = body
+		withoutUsedEnvVars := filterOutUsedEnvVars(req.params)
+		envVarCtx := context.WithValue(httpReq.Context(), envVarsContextKey{}, withoutUsedEnvVars)
+		httpReq = httpReq.WithContext(envVarCtx)
 		c.handler.ServeHTTP(r, httpReq)
 	}
 	r.Close()
@@ -329,3 +349,39 @@ func Serve(l net.Listener, handler http.Handler) error {
 		go c.serve()
 	}
 }
+
+// ProcessEnv returns FastCGI environment variables associated with the request r
+// for which no effort was made to be included in the request itself - the data
+// is hidden in the request's context. As an example, if REMOTE_USER is set for a
+// request, it will not be found anywhere in r, but it will be included in
+// ProcessEnv's response (via r's context).
+func ProcessEnv(r *http.Request) map[string]string {
+	env, _ := r.Context().Value(envVarsContextKey{}).(map[string]string)
+	return env
+}
+
+// addFastCGIEnvToContext reports whether to include the FastCGI environment variable s
+// in the http.Request.Context, accessible via ProcessEnv.
+func addFastCGIEnvToContext(s string) bool {
+	// Exclude things supported by net/http natively:
+	switch s {
+	case "CONTENT_LENGTH", "CONTENT_TYPE", "HTTPS",
+		"PATH_INFO", "QUERY_STRING", "REMOTE_ADDR",
+		"REMOTE_HOST", "REMOTE_PORT", "REQUEST_METHOD",
+		"REQUEST_URI", "SCRIPT_NAME", "SERVER_PROTOCOL":
+		return false
+	}
+	if strings.HasPrefix(s, "HTTP_") {
+		return false
+	}
+	// Explicitly include FastCGI-specific things.
+	// This list is redundant with the default "return true" below.
+	// Consider this documentation of the sorts of things we expect
+	// to maybe see.
+	switch s {
+	case "REMOTE_USER":
+		return true
+	}
+	// Unknown, so include it to be safe.
+	return true
+}

src/net/http/fcgi/fcgi.go

@@ -24,7 +24,7 @@ import (
 )
 
 // recType is a record type, as defined by
-// http://www.fastcgi.com/devkit/doc/fcgi-spec.html#S8
+// https://web.archive.org/web/20150420080736/http://www.fastcgi.com/drupal/node/6?q=node/22#S8
 type recType uint8
 
 const (

src/net/http/fcgi/fcgi_test.go

@@ -278,3 +278,69 @@ func TestMalformedParams(t *testing.T) {
 	c := newChild(rw, http.DefaultServeMux)
 	c.serve()
 }
+
+// a series of FastCGI records that start and end a request
+var streamFullRequestStdin = bytes.Join([][]byte{
+	// set up request
+	makeRecord(typeBeginRequest, 1,
+		[]byte{0, byte(roleResponder), 0, 0, 0, 0, 0, 0}),
+	// add required parameters
+	makeRecord(typeParams, 1, nameValuePair11("REQUEST_METHOD", "GET")),
+	makeRecord(typeParams, 1, nameValuePair11("SERVER_PROTOCOL", "HTTP/1.1")),
+	// set optional parameters
+	makeRecord(typeParams, 1, nameValuePair11("REMOTE_USER", "jane.doe")),
+	makeRecord(typeParams, 1, nameValuePair11("QUERY_STRING", "/foo/bar")),
+	makeRecord(typeParams, 1, nil),
+	// begin sending body of request
+	makeRecord(typeStdin, 1, []byte("0123456789abcdef")),
+	// end request
+	makeRecord(typeEndRequest, 1, nil),
+},
+	nil)
+
+var envVarTests = []struct {
+	input               []byte
+	envVar              string
+	expectedVal         string
+	expectedFilteredOut bool
+}{
+	{
+		streamFullRequestStdin,
+		"REMOTE_USER",
+		"jane.doe",
+		false,
+	},
+	{
+		streamFullRequestStdin,
+		"QUERY_STRING",
+		"",
+		true,
+	},
+}
+
+// Test that environment variables set for a request can be
+// read by a handler. Ensures that variables not set will not
+// be exposed to a handler.
+func TestChildServeReadsEnvVars(t *testing.T) {
+	for _, tt := range envVarTests {
+		input := make([]byte, len(tt.input))
+		copy(input, tt.input)
+		rc := nopWriteCloser{bytes.NewBuffer(input)}
+		done := make(chan bool)
+		c := newChild(rc, http.HandlerFunc(func(
+			w http.ResponseWriter,
+			r *http.Request,
+		) {
+			env := ProcessEnv(r)
+			if _, ok := env[tt.envVar]; ok && tt.expectedFilteredOut {
+				t.Errorf("Expected environment variable %s to not be set, but set to %s",
+					tt.envVar, env[tt.envVar])
+			} else if env[tt.envVar] != tt.expectedVal {
+				t.Errorf("Expected %s, got %s", tt.expectedVal, env[tt.envVar])
+			}
+			done <- true
+		}))
+		go c.serve()
+		<-done
+	}
+}