signer: Avoid mutating response until signing (#291)

After successfully (200) fetching an AMP document, complete all
validation steps that could result in proxying the original response
before mutating any headers.

Author: mattwomple

packager/signer/signer.go

@@ -348,17 +348,16 @@ func (this *Signer) ServeHTTP(resp http.ResponseWriter, req *http.Request, param
 		proxy(resp, fetchResp, nil)
 		return
 	}
+	var act string
 	var transformVersion int64
 	if this.requireHeaders {
 		header_value := GetJoined(req.Header, "AMP-Cache-Transform")
-		var act string
 		act, transformVersion = amp_cache_transform.ShouldSendSXG(header_value)
 		if act == "" {
 			log.Println("Not packaging because AMP-Cache-Transform request header is invalid:", header_value)
 			proxy(resp, fetchResp, nil)
 			return
 		}
-		resp.Header().Set("AMP-Cache-Transform", act)
 	} else {
 		var err error
 		transformVersion, err = transformer.SelectVersion(nil)
@@ -387,17 +386,8 @@ func (this *Signer) ServeHTTP(resp http.ResponseWriter, req *http.Request, param
 				proxy(resp, fetchResp, nil)
 				return
 			}
-			fetchResp.Header.Del(header)
 		}
 
-		// Mutate the fetched CSP to make sure it cannot break AMP pages.
-		fetchResp.Header.Set(
-			"Content-Security-Policy",
-			MutateFetchedContentSecurityPolicy(
-				fetchResp.Header.Get("Content-Security-Policy")))
-
-		fetchResp.Header.Del("Link") // Ensure there are no privacy-violating Link:rel=preload headers.
-
 		if fetchResp.Header.Get("Variants") != "" || fetchResp.Header.Get("Variant-Key") != "" ||
 			// Include versioned headers per https://github.com/WICG/webpackage/pull/406.
 			fetchResp.Header.Get("Variants-04") != "" || fetchResp.Header.Get("Variant-Key-04") != "" {
@@ -408,7 +398,7 @@ func (this *Signer) ServeHTTP(resp http.ResponseWriter, req *http.Request, param
 			return
 		}
 
-		this.serveSignedExchange(resp, fetchResp, signURL, transformVersion)
+		this.serveSignedExchange(resp, fetchResp, signURL, act, transformVersion)
 
 	case 304:
 		// If fetchURL returns a 304, then also return a 304 with appropriate headers.
@@ -451,9 +441,7 @@ func formatLinkHeader(preloads []*rpb.Metadata_Preload) (string, error) {
 }
 
 // serveSignedExchange does the actual work of transforming, packaging and signed and writing to the response.
-func (this *Signer) serveSignedExchange(resp http.ResponseWriter, fetchResp *http.Response, signURL *url.URL, transformVersion int64) {
-	fetchResp.Header.Set("X-Content-Type-Options", "nosniff")
-
+func (this *Signer) serveSignedExchange(resp http.ResponseWriter, fetchResp *http.Response, signURL *url.URL, act string, transformVersion int64) {
 	// After this, fetchResp.Body is consumed, and attempts to read or proxy it will result in an empty body.
 	fetchBody, err := ioutil.ReadAll(io.LimitReader(fetchResp.Body, maxBodyLength))
 	if err != nil {
@@ -470,17 +458,43 @@ func (this *Signer) serveSignedExchange(resp http.ResponseWriter, fetchResp *htt
 		proxy(resp, fetchResp, fetchBody)
 		return
 	}
-	fetchResp.Header.Set("Content-Length", strconv.Itoa(len(transformed)))
+
+	// Validate and format Link header.
 	linkHeader, err := formatLinkHeader(metadata.Preloads)
 	if err != nil {
 		log.Println("Not packaging due to Link header error:", err)
 		proxy(resp, fetchResp, fetchBody)
 		return
 	}
+
+	// Begin mutations on original fetch response. From this point forward, do
+	// not fall-back to proxy().
+
+	// Remove stateful headers.
+	for header := range statefulResponseHeaders {
+		fetchResp.Header.Del(header)
+	}
+
+	// Set Link header if formatting returned a valid value, otherwise, delete
+	// it to ensure there are no privacy-violating Link:rel=preload headers.
 	if linkHeader != "" {
 		fetchResp.Header.Set("Link", linkHeader)
+	} else {
+		fetchResp.Header.Del("Link")
 	}
 
+	// Set content length.
+	fetchResp.Header.Set("Content-Length", strconv.Itoa(len(transformed)))
+
+	// Set general security headers.
+	fetchResp.Header.Set("X-Content-Type-Options", "nosniff")
+
+	// Mutate the fetched CSP to make sure it cannot break AMP pages.
+	fetchResp.Header.Set(
+		"Content-Security-Policy",
+		MutateFetchedContentSecurityPolicy(
+			fetchResp.Header.Get("Content-Security-Policy")))
+
 	exchange := signedexchange.NewExchange(
 		accept.SxgVersion, /*uri=*/signURL.String(), /*method=*/"GET",
 		http.Header{}, fetchResp.StatusCode, fetchResp.Header, []byte(transformed))
@@ -520,6 +534,13 @@ func (this *Signer) serveSignedExchange(resp http.ResponseWriter, fetchResp *htt
 		util.NewHTTPError(http.StatusInternalServerError, "Error serializing exchange: ", err).LogAndRespond(resp)
 	}
 
+	// If requireHeaders was true when constructing signer, the
+	// AMP-Cache-Transform outer response header is required (and has already
+	// been validated)
+	if (act != "") {
+		resp.Header().Set("AMP-Cache-Transform", act)
+	}
+
 	// TODO(twifkak): Add Cache-Control: public with expiry to match when we think the AMP Cache
 	// should fetch an update (half-way between signature date & expires).
 	resp.Header().Set("Content-Type", accept.SxgContentType)

packager/signer/signer_test.go

@@ -171,7 +171,6 @@ func (this *SignerSuite) TestSimple() {
 		[]string{"content-encoding", "content-length", "content-security-policy", "content-type", "date", "digest", "x-content-type-options"},
 		headerNames(exchange.ResponseHeaders))
 	this.Assert().Equal("text/html", exchange.ResponseHeaders.Get("Content-Type"))
-	this.Assert().Equal("text/html", exchange.ResponseHeaders.Get("Content-Type"))
 	this.Assert().Equal("nosniff", exchange.ResponseHeaders.Get("X-Content-Type-Options"))
 	this.Assert().Contains(exchange.SignatureHeaderValue, "validity-url=\""+this.httpSignURL()+"/amppkg/validity\"")
 	this.Assert().Contains(exchange.SignatureHeaderValue, "integrity=\"digest/mi-sha256-03\"")
@@ -462,6 +461,20 @@ func (this *SignerSuite) TestProxyUnsignedIfMissingAMPCacheTransformHeader() {
 	this.Assert().Equal(fakeBody, body, "incorrect body: %#v", resp)
 }
 
+func (this *SignerSuite) TestProxyUnsignedIfInvalidAMPCacheTransformHeader() {
+	urlSets := []util.URLSet{{
+		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},
+	}}
+	resp := pkgt.GetH(this.T(), this.new(urlSets), "/priv/doc?sign="+url.QueryEscape(this.httpsURL()+fakePath), http.Header{
+		"Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion},
+		"AMP-Cache-Transform": {"donotmatch"},
+	})
+	this.Assert().Equal(http.StatusOK, resp.StatusCode, "incorrect status: %#v", resp)
+	body, err := ioutil.ReadAll(resp.Body)
+	this.Require().NoError(err)
+	this.Assert().Equal(fakeBody, body, "incorrect body: %#v", resp)
+}
+
 func (this *SignerSuite) TestProxyUnsignedIfMissingAcceptHeader() {
 	urlSets := []util.URLSet{{
 		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},
@@ -609,6 +622,49 @@ func (this *SignerSuite) TestProxyTransformError() {
 	this.Assert().Equal(fakeBody, body, "incorrect body: %#v", resp)
 }
 
+func (this *SignerSuite) TestProxyHeadersUnaltered() {
+	urlSets := []util.URLSet{{
+		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},
+	}}
+
+	// "Perform local transformations" is close to the last opportunity that a
+	// response could be proxied instead of signed. Intentionally cause an error
+	// to occur so that we can verify the proxy response has not been altered.
+	getTransformerRequest = func(r *rtv.RTVCache, s, u string) *rpb.Request {
+		return &rpb.Request{Html: string(s), DocumentUrl: u, Config: rpb.Request_CUSTOM,
+			AllowedFormats: []rpb.Request_HtmlFormat{rpb.Request_AMP},
+			Transformers:   []string{"bogus"}}
+	}
+
+	originalHeaders := map[string]string {
+		"Content-Type": "text/html",
+		"Set-Cookie": "chocolate chip",
+		"Cache-Control": "max-age=31536000",
+		"Content-Length": fmt.Sprintf("%d", len(fakeBody)),
+	}
+
+	this.fakeHandler = func(resp http.ResponseWriter, req *http.Request) {
+		for key, value := range originalHeaders {
+			resp.Header().Set(key, value)
+		}
+		resp.Write(fakeBody)
+	}
+	resp := this.get(this.T(), this.new(urlSets), "/priv/doc?sign="+url.QueryEscape(this.httpsURL()+fakePath))
+	this.Assert().Equal(200, resp.StatusCode)
+
+	// Compare the final headers to the originals, removing each one after
+	// checking, so that we can finally verify that no additional headers were
+	// appended.
+	for key, value := range originalHeaders {
+		this.Assert().Equal([]string{value}, resp.Header[key])
+		resp.Header.Del(key)
+	}
+	this.Assert().Equal([]string{"Accept, AMP-Cache-Transform"}, resp.Header["Vary"])
+	resp.Header.Del("Vary")
+	resp.Header.Del("Date") // Date header is not tested; it may be updated.
+	this.Assert().Empty(resp.Header)
+}
+
 func TestSignerSuite(t *testing.T) {
 	suite.Run(t, new(SignerSuite))
 }