Merge pull request #325 from ampproject/master

Cherry-pick onto v2

Author: twifkak

CONTRIBUTING.md

@@ -29,41 +29,4 @@ Guidelines](https://opensource.google.com/conduct/).
 
 # Getting Started
 
-## Cloning the repository
-
-When cloning, use `git clone -b master`; the default branch is `releases` which
-is not meant for developing against.
-
-## Running a development server
-
-Copy `amppkg.example.toml` to `amppkg.toml` and modify it to suit your needs.
-You may use `testdata/b1/server.{cert,privkey}` to get started. However, you are
-at your own risk if you instruct your browser to trust `server.cert`. *NEVER*
-instruct your browser to trust `ca.cert`.
-
-## Presubmits
-
-- Run `go fmt` on the code you change. Don't run `go fmt ./...`; this affects
-  files that are mirrored from Google to GitHub, so we can't change them here.
-- Make sure `go test ./...` passes.
-- `golint` and `go vet` are optional.
-- Make sure PRs are sent to `master` and not `releases`, unless you're releasing
-  a new version of amppkg.
-
-## New dependencies
-
-Feel free to add dependencies on small code if it implements a feature that's
-hard to implement yourself. Try not to add large dependencies, or dependencies
-for the sake of minor development inconvenience (unless it's for test code
-only). They add risk by bringing in code of unknown provenance, and bloat the
-binary.
-
-If you need to add or upgrade dependencies, AMP Packager uses
-[dep](https://golang.github.io/dep/).
-
-# Suggestions for contributions
-
-Take a look at [good first
-issues](https://github.com/ampproject/amppackager/labels/good%20first%20issue),
-and please communicate early and often, to ensure we agree on the solution
-before you invest a lot of time into its implementation.
+See the [Contributing wiki](../../wiki/Contributing).

packager/certcache/certcache.go

@@ -169,7 +169,6 @@ func (this *CertCache) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
 			expiry = 0
 		}
 		resp.Header().Set("Cache-Control", "public, max-age="+strconv.Itoa(expiry))
-		resp.Header().Set("ETag", "\""+this.certName+"\"")
 		resp.Header().Set("X-Content-Type-Options", "nosniff")
 		cbor, err := this.createCertChainCBOR(ocsp)
 		if err != nil {

packager/signer/signer.go

@@ -504,10 +504,17 @@ func (this *Signer) serveSignedExchange(resp http.ResponseWriter, fetchResp *htt
 		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)
-	resp.Header().Set("Cache-Control", "no-transform")
+	// We set a zero freshness lifetime on the SXG, so that naive caching
+	// intermediaries won't inhibit the update of this resource on AMP
+	// caches. AMP caches are recommended to base their update strategies
+	// on a combination of inner and outer resource lifetime.
+	//
+	// If you change this code to set a Cache-Control based on the inner
+	// resource, you need to ensure that its max-age is no longer than the
+	// lifetime of the signature (6 days, per above). Maybe an even tighter
+	// bound than that, based on data about client clock skew.
+	resp.Header().Set("Cache-Control", "no-transform, max-age=0")
 	resp.Header().Set("X-Content-Type-Options", "nosniff")
 	if _, err := resp.Write(body.Bytes()); err != nil {
 		log.Println("Error writing response:", err)

packager/signer/validation_test.go

@@ -13,8 +13,6 @@ import (
 
 func urlFrom(url *url.URL, err *util.HTTPError) *url.URL { return url }
 
-func errorFrom(url *url.URL, err *util.HTTPError) *util.HTTPError { return err }
-
 func urlOrDie(spec string) *url.URL {
 	url, err := url.Parse(spec)
 	if err != nil {
@@ -24,6 +22,8 @@ func urlOrDie(spec string) *url.URL {
 }
 
 func TestParseURL(t *testing.T) {
+	errorFrom := func(url *url.URL, err *util.HTTPError) *util.HTTPError { return err }
+
 	assert.EqualError(t, errorFrom(parseURL("", "sign")), "sign URL is unspecified")
 	if err := errorFrom(parseURL("abc-@#79!%^/", "sign")); assert.NotNil(t, err) {
 		assert.Contains(t, err.Error(), "Error parsing sign URL")