Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Authoritative Contained Resource Data #352
Authoritative Contained Resource Data #352
Changes from 18 commits
f07d1eede6eb1c0d102992e70f8f294245e5ea32a4630861cc0d8d4c91a77cbf599dd54734bbf98d2db44eaf345f2c8c8b30eee021727bb864d48410b6db9764e3738a1222a2746520dc33e3864ebdc4d29b7094File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This item in particular should be discussed in the security considerations section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you consider stat:size (I assume that's what your comment is referring to - GitHub UI trips me up sometimes) to more of a concern than the others?
Had this text earlier in the Note #contained-resource-metadata-considerations :
Removed it as per Kjetil's suggestion: #352 (comment)
Can revive and put it under #security-considerations but note that it may be equivalent / already captured by:
If you'd like more specific considerations that should be mentioned in there, we can do that as well. Have something in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided that a server can choose to omit this data (under the "inapplicability" clause), this is fine as-is.
Knowing the size of a file that one may not otherwise have access to read, can be extremely useful data when looking to exploit weaknesses in a server. I would have serious reservations about including that information on a Storage that holds sensitive personal data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe that since we do know that we cannot have a server check the authz of every child resource, then is inappropriate to leave it as an implementation detail, we'd need to specify a concrete mechanism. I think it would be better for this to be a SHOULD, as per RFC2119, but for now, it seems that it could be left out under the inapplicability clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that a server provides a
Last-Modifiedheader and assuming that we do not want to break how caching works for browsers, I do not see how this (dcterms:modifiedorstat:mtime) can be implemented without causing a cascade of updates to the root of a Storage.For example, consider a resource at
/lvl-1/lvl-2/foo.ttlwith the following response headers:The container at
/lvl-1/lvl-2/has the following headers:That container would include the following triples:
The container at
/lvl-1/has the following headers:And the following body:
Now consider that a client adds a triple to
/lvl-1/lvl-2/foo.ttl. The result will lead to theLast-Modifiedbeing updated on/lvl-1/lvl-2/foo.ttl. The content of that resource has changed, so the ETag header also changes.Because the
Last-Modifiedheader changes, the representation of/lvl-1/lvl-2/now also changes to reflect the new status of the contained resource. As a consequence, and in order to ensure that clients receive the latest version of/lvl-1/lvl-2/, theLast-ModifiedandETagheaders of this container resource must change.Because the
Last-Modifiedheader for/lvl-1/lvl-2/changes, the representation of/lvl-1/also needs to change, which leads to a further cascade of changes to the root of the storage.There are three ways around this, as I see it:
dcterms:modifiedorstat:mtimetriples (i.e. ignore this part of the spec)I prefer the third option because it makes it possible to include this data without breaking how browsers interact with HTTP resources, but absent that approach, the only route forward I see (while staying roughly in line with this spec) is to simply not include the modified time of contained resources (again, arguing "inapplicability").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @acoburn , I have been arguing the same, an auxiliary resource solves all problems, it can represent the entirety of the resource (and it makes sense to add data for each representation too if needed), avoids cascading problems, can be subject to separate authorization, can be used to track any changes and should be at least as easy to implement.
However, we haven't been able to find consensus around that for 0.9, so indeed, the result is that these data may be stale when conditional requests are used. I disagree with the conclusion, but as the RFC7232 is fairly vague at this point, I have chosen to accept it for 0.9, sincerely hoping we can revisit for 1.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the representation data of
/lvl-1/lvl-2/changes because of thedcterms:modifiedvalue of/lvl-1/lvl-2/foo.ttl, theLast-Modifiedheader of/lvl-1/lvl-2/need not change - nor prohibited - as there were no changes to the containment triples ( #server-container-last-modified ).A recommendation (or advisement) along the following lines may be necessary:
When the resource metadata of an existing contained resource changes, the server MUST send a weak
entity-tagwhen responding to container’s request URI.I want to mention again that the requirements introduced by this PR does not expect a container (
/lvl-1/lvl-2/) to include resource metadata about itself (</lvl-1/lvl-2/> dcterms:modified "2021-12-07T12:23:15Z"^^xsd:dateTime .). (I don't care if that's not the point - it easily introduces complications/assumptions to the discussion that we are better off without.) And again, if resource metadata about the container itself is desired - I may have missed the discussion that calls for it - the specification should needs to say so because both the generation and protection of those resource metadata needs to apply. On a related note:When a server plans to include resource metadata about the container in the response of the same container, the server should determine the
Last-Modifiedheader value as a regular change to the representation data. (It can send a strongentity-tagas usual.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does mean that apps/users will see stale data. From an impl perspective (given the choice), I would rather not provide the data than provide it in a way that confuses users.
I am really trying to be constructive here, but I don't see how this is implementable, given this text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stands, the Protocol uses the same requirement levels as in the RFC for Last-Modified and ETag headers. Servers might not include either Last-Modified or ETag headers, and so clients can't absolutely rely on them for being there.
That aside,
https://datatracker.ietf.org/doc/html/rfc7232#section-2.4 and https://datatracker.ietf.org/doc/html/rfc7232#section-6 suggests that headers with entity-tags will have higher precedence.
Server needs to enable the use of conditional requests so that client can eventually get a 200/304/412 or whatever.
Perhaps I'm missing something here but as mentioned in previous comment, I don't see a staleness issue around entity-tags.
If a client is only making decisions with Last-Modified/If-*-Since (when Last-Modified is provided by the server), then the representation metadata in the response will not help them to differentiate between a change to containment triples or any change to the representation. In this particular scenario, client has to make a GET request on the container.
We can certainly revisit that in 1.0.
For now, I suggest to add the following and go ahead with 0.9:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the entity tag changes from x to y, there is no issue, but that is orthogonal to the weakness indicator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good. Then in what case would it not change from x to y?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any time the representation changes, the ETag changes. There is no argument about that. I am only responding to the proposed requirement to use weak ETags. And to be clear, I am not opposed to using weak ETags, but we can't claim that weak ETags will solve this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weak entity-tag is suggested because strong entity-tag wouldn't be correct when only the resource metadata changes. All meanwhile allowing the client to make conditional requests. Right at this second, I don't have a strong opinion on adding that requirement because I do think it comes directly from the RFC but it may be reasonable to mention.
That aside, I acknowledge that there is (always) room to discuss/work this out further but it doesn't need to be a blocker for 0.9. (We mark handful of stuff to be revisited for 1.0, and I don't see why it can't be done here as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
746520d