cgi: improve Location response handling

Add local URI path support, which isn't as fringe
as I originally thought. (it's supported by Apache)

Send an implicit 302 status on redirects (not 200).

Fixes #1597

R=rsc, r
CC=golang-dev
https://golang.org/cl/4442089

3 files changed, 116 insertions, 9 deletions

diff --git a/src/pkg/http/cgi/host.go b/src/pkg/http/cgi/host.go
index 35fbde705a..136d4e4ee2 100644
--- a/src/pkg/http/cgi/host.go
+++ b/src/pkg/http/cgi/host.go
@@ -49,6 +49,16 @@ type Handler struct {
 	InheritEnv []string    // environment variables to inherit from host, as "key"
 	Logger     *log.Logger // optional log for errors or nil to use log.Print
 	Args       []string    // optional arguments to pass to child process
+
+	// PathLocationHandler specifies the root http Handler that
+	// should handle internal redirects when the CGI process
+	// returns a Location header value starting with a "/", as
+	// specified in RFC 3875 § 6.3.2. This will likely be
+	// http.DefaultServeMux.
+	//
+	// If nil, a CGI response with a local URI path is instead sent
+	// back to the client and not redirected internally.
+	PathLocationHandler http.Handler
 }
 
 func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
@@ -171,13 +181,13 @@ func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	}
 
 	linebody, _ := bufio.NewReaderSize(cmd.Stdout, 1024)
-	headers := rw.Header()
-	statusCode := http.StatusOK
+	headers := make(http.Header)
+	statusCode := 0
 	for {
 		line, isPrefix, err := linebody.ReadLine()
 		if isPrefix {
 			rw.WriteHeader(http.StatusInternalServerError)
-			h.printf("CGI: long header line from subprocess.")
+			h.printf("cgi: long header line from subprocess.")
 			return
 		}
 		if err == os.EOF {
@@ -185,7 +195,7 @@ func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		}
 		if err != nil {
 			rw.WriteHeader(http.StatusInternalServerError)
-			h.printf("CGI: error reading headers: %v", err)
+			h.printf("cgi: error reading headers: %v", err)
 			return
 		}
 		if len(line) == 0 {
@@ -193,7 +203,7 @@ func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		}
 		parts := strings.Split(string(line), ":", 2)
 		if len(parts) < 2 {
-			h.printf("CGI: bogus header line: %s", string(line))
+			h.printf("cgi: bogus header line: %s", string(line))
 			continue
 		}
 		header, val := parts[0], parts[1]
@@ -202,13 +212,13 @@ func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		switch {
 		case header == "Status":
 			if len(val) < 3 {
-				h.printf("CGI: bogus status (short): %q", val)
+				h.printf("cgi: bogus status (short): %q", val)
 				return
 			}
 			code, err := strconv.Atoi(val[0:3])
 			if err != nil {
-				h.printf("CGI: bogus status: %q", val)
-				h.printf("CGI: line was %q", line)
+				h.printf("cgi: bogus status: %q", val)
+				h.printf("cgi: line was %q", line)
 				return
 			}
 			statusCode = code
@@ -216,11 +226,35 @@ func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 			headers.Add(header, val)
 		}
 	}
+
+	if loc := headers.Get("Location"); loc != "" {
+		if strings.HasPrefix(loc, "/") && h.PathLocationHandler != nil {
+			h.handleInternalRedirect(rw, req, loc)
+			return
+		}
+		if statusCode == 0 {
+			statusCode = http.StatusFound
+		}
+	}
+
+	if statusCode == 0 {
+		statusCode = http.StatusOK
+	}
+
+	// Copy headers to rw's headers, after we've decided not to
+	// go into handleInternalRedirect, which won't want its rw
+	// headers to have been touched.
+	for k, vv := range headers {
+		for _, v := range vv {
+			rw.Header().Add(k, v)
+		}
+	}
+
 	rw.WriteHeader(statusCode)
 
 	_, err = io.Copy(rw, linebody)
 	if err != nil {
-		h.printf("CGI: copy error: %v", err)
+		h.printf("cgi: copy error: %v", err)
 	}
 }
 
@@ -232,6 +266,37 @@ func (h *Handler) printf(format string, v ...interface{}) {
 	}
 }
 
+func (h *Handler) handleInternalRedirect(rw http.ResponseWriter, req *http.Request, path string) {
+	url, err := req.URL.ParseURL(path)
+	if err != nil {
+		rw.WriteHeader(http.StatusInternalServerError)
+		h.printf("cgi: error resolving local URI path %q: %v", path, err)
+		return
+	}
+	// TODO: RFC 3875 isn't clear if only GET is supported, but it
+	// suggests so: "Note that any message-body attached to the
+	// request (such as for a POST request) may not be available
+	// to the resource that is the target of the redirect."  We
+	// should do some tests against Apache to see how it handles
+	// POST, HEAD, etc. Does the internal redirect get the same
+	// method or just GET? What about incoming headers?
+	// (e.g. Cookies) Which headers, if any, are copied into the
+	// second request?
+	newReq := &http.Request{
+		Method:     "GET",
+		URL:        url,
+		RawURL:     path,
+		Proto:      "HTTP/1.1",
+		ProtoMajor: 1,
+		ProtoMinor: 1,
+		Header:     make(http.Header),
+		Host:       url.Host,
+		RemoteAddr: req.RemoteAddr,
+		TLS:        req.TLS,
+	}
+	h.PathLocationHandler.ServeHTTP(rw, newReq)
+}
+
 func upperCaseAndUnderscore(rune int) int {
 	switch {
 	case rune >= 'a' && rune <= 'z':
diff --git a/src/pkg/http/cgi/host_test.go b/src/pkg/http/cgi/host_test.go
index e8084b1134..9ac085f2f3 100644
--- a/src/pkg/http/cgi/host_test.go
+++ b/src/pkg/http/cgi/host_test.go
@@ -271,3 +271,40 @@ Transfer-Encoding: chunked
 			expected, got)
 	}
 }
+
+func TestRedirect(t *testing.T) {
+	if skipTest(t) {
+		return
+	}
+	h := &Handler{
+		Path: "testdata/test.cgi",
+		Root: "/test.cgi",
+	}
+	rec := runCgiTest(t, h, "GET /test.cgi?loc=http://foo.com/ HTTP/1.0\nHost: example.com\n\n", nil)
+	if e, g := 302, rec.Code; e != g {
+		t.Errorf("expected status code %d; got %d", e, g)
+	}
+	if e, g := "http://foo.com/", rec.Header().Get("Location"); e != g {
+		t.Errorf("expected Location header of %q; got %q", e, g)
+	}
+}
+
+func TestInternalRedirect(t *testing.T) {
+	if skipTest(t) {
+		return
+	}
+	baseHandler := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		fmt.Fprintf(rw, "basepath=%s\n", req.URL.Path)
+		fmt.Fprintf(rw, "remoteaddr=%s\n", req.RemoteAddr)
+	})
+	h := &Handler{
+		Path:                "testdata/test.cgi",
+		Root:                "/test.cgi",
+		PathLocationHandler: baseHandler,
+	}
+	expectedMap := map[string]string{
+		"basepath":   "/foo",
+		"remoteaddr": "1.2.3.4",
+	}
+	runCgiTest(t, h, "GET /test.cgi?loc=/foo HTTP/1.0\nHost: example.com\n\n", expectedMap)
+}
diff --git a/src/pkg/http/cgi/testdata/test.cgi b/src/pkg/http/cgi/testdata/test.cgi
index 253589eed9..a1b2ff893d 100755
--- a/src/pkg/http/cgi/testdata/test.cgi
+++ b/src/pkg/http/cgi/testdata/test.cgi
@@ -11,6 +11,11 @@ use CGI;
 my $q = CGI->new;
 my $params = $q->Vars;
 
+if ($params->{"loc"}) {
+    print "Location: $params->{loc}\r\n\r\n";
+    exit(0);
+}
+
 my $NL = "\r\n";
 $NL = "\n" if $params->{mode} eq "NL";