Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gateway: fix erroneous Cache-Control: immutable on dir listings #3870

Merged
merged 1 commit into from
Apr 20, 2017

Conversation

ghost
Copy link

@ghost ghost commented Apr 19, 2017

We set the Cache-Control: immutable header on responses from the gateway's /ipfs route. That's fine, but we also set it on directory listings such as https://ipfs.io/ipfs/QmSwzyZ4zfHD9j4JZdXuzbWH4gXuDfZ7CdfMzpfqrMVcrz/go-libp2p-blankhost/ which effectively prevents us from changing e.g. the CSS on the listings, or really anything about the listing.

We should only ever set Cache-Control: immutable for responses that produce exactly the hash in the URL. This is only the case for unixfs files, not for the auto-generated directory listings.

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost ghost added kind/bug A bug in existing code (including security flaws) topic/gateway Topic gateway labels Apr 19, 2017
@ghost ghost added this to the Ipfs 0.4.9 milestone Apr 19, 2017
@ghost ghost added status/in-progress In progress labels Apr 19, 2017
@recmo
Copy link

recmo commented Apr 19, 2017

Immutability also affects ETags. In my PR I make them strongly validating, which is equivalent to saying byte-for-byte immutable. We can do the following:

  1. Turn them into weakly validating for dirs, implying binary differences but semantic sameness. The standard allows the implementer to decide what a "semantic change" is. We can argue that stylistic changes are not semantic. But browsers would not refresh the result afaik.
  2. Compute them on the fly by hashing the binary result.
  3. Add a version number to the dir ETag that we increment each time the dir generation code or its assets are changed.

I kind of like 3 because it is the most accurate and easy to implement. But it doesn't work if the generation code is runtime modifiable and adds a bit of maintenance burden.

@@ -23,6 +23,9 @@ import (
id "gx/ipfs/QmeWJwi61vii5g8zQUB9UGegfUbmhTKHgeDFP9XuSp5jZ4/go-libp2p/p2p/protocol/identify"
)

// `ipfs object new unixfs-dir`
var emptyDir = "/ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be better to do this:

diff --git a/core/corehttp/gateway_test.go b/core/corehttp/gateway_test.go
index 55c0b15ee..887f12ca5 100644
--- a/core/corehttp/gateway_test.go
+++ b/core/corehttp/gateway_test.go
@@ -18,13 +18,14 @@ import (
 	repo "github.com/ipfs/go-ipfs/repo"
 	config "github.com/ipfs/go-ipfs/repo/config"
 	testutil "github.com/ipfs/go-ipfs/thirdparty/testutil"
+	ufs "github.com/ipfs/go-ipfs/unixfs"
 
 	ci "gx/ipfs/QmPGxZ1DP2w45WcogpW1h43BvseXbfke9N91qotpoQcUeS/go-libp2p-crypto"
 	id "gx/ipfs/QmeWJwi61vii5g8zQUB9UGegfUbmhTKHgeDFP9XuSp5jZ4/go-libp2p/p2p/protocol/identify"
 )
 
 // `ipfs object new unixfs-dir`
-var emptyDir = "/ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn"
+var emptyDir = "/ipfs/" + ufs.EmptyDirNode().Cid().String()
 
 type mockNamesys map[string]path.Path
 

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in principle, but I'm against adding more dependencies on go-ipfs packages. The good thing about these hashes is that the underlying data won't change.

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@whyrusleeping whyrusleeping merged commit 87d07a9 into master Apr 20, 2017
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Apr 20, 2017
@dignifiedquire
Copy link
Member

@Kubuxu Kubuxu deleted the fix/directory-cache-control branch April 20, 2017 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants