rack_response ETAG is weakly generated

[prem-prakash]

The rack_response plugin ETAG response header is generated based on attributes that can remain the same even if the file content has been changed.

        # Value for the "ETag" header.
        def etag
          digest = Digest::SHA256.hexdigest("#{file.shrine_class}-#{file.storage_key}-#{file.id}")

          %(W/"#{digest.byteslice(0, 32)}")
        end

This may result in unwanted response caching.

[prem-prakash]

Proposed PR: #704

[gustavokloh]

👍 for this.

[jrochkind]

So, file.id is basically the path on disk, or key on other storage.

In general, in my experience with shrine, things are set up so the path on disk or in other storage, which is what file.id is, can not stay the same with content changing -- files on disk are treated as immutable, and changing an attachment will result in a new file on disk (or in other storage) with the old one deleted. The new file will use a new id (path on disk or key in other storage), which by default is randomly generated, with a big enough range that conflicts can be considered not happening.

This is implemented in default generate_location which ends up calilng SecureRandom.hex. I don't think you're talking about the possibilty of SecureRandom.hex having a collision...

I'm curious, have you over-ridden generate_location in a way that successive files with different content may share the same location, instead of using a unique-to-content location? I wonder if docs should discourage this more -- i wonder if there are other places in shrine, not just etag, that are built assuming unique non-colliding locations.

Or, are you using shrine in some way that actually manages to change the content on a file on disk/in storage instead of treating it as immutable? I also worry that is going to cause other problems.

I have no problem with allowing passing in an etag as in patch, but I worry the fact that it is necessary for the reasons stated here reveal other things likely to cause other problems!

[prem-prakash]

Yes, I overrode generate_location in a way that it is always the same for a single record, regardless of whether the attached file content changes or not. To monitor changes, I have an updated_at metadata field, which I query when needed.

[jrochkind]

Thanks @prem-prakash ! While this is possible to do, I'm going to PR a change to "changing location" docs to suggest that if you don't have a reason not to, it's best to keep unique immutable storage locations, because it makes many things easier!