-
Notifications
You must be signed in to change notification settings - Fork 113
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
be defensive about wrongly quoted etags #1870
Conversation
75f4331
to
18fc5a4
Compare
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
if strings.HasPrefix(etag, "W/") { | ||
return `W/"` + strings.Trim(etag[2:], `"`) + `"` | ||
} | ||
return `"` + strings.Trim(etag, `"`) + `"` |
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.
@butonic is this not handled by the file systems? I think it should be added there so that other clients also don't face this issue. Also, what is the use case for W/
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.
It should be handled by storage drivers, not filesystems (if the filesystem gives you an etag that is great, but POSIX eg. does not have them, so the driver needs to provide an etag). In oc10 we internally omitted the quotes, but the etag spec says they are a quoted string, so we have to enclose the etag in quotes in webdav responses. The PR is called defensive to protect against sloppy driver implementations.
The W/
indicates a so called weak etag. It is only relevant for range requests. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag for a good explanation.
That being said all reva storage drivers I am aware of currently only produce strong etags, BUT in the future there might be a storage driver that fetches resources from another web server via whatever HTTP based protocol ... and that MIGHT use a weak etag ... which is why we have to properly deal with a weak etag here.
I propose to internally or even on the CS3 api omit the "
. It is far more readable when debugging and in logs.
@labkode opinions?
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.
Yep, I meant storage drivers only, sorry about that.
I agree with removing the quotes in the driver implementations and the CS3APIs. We can add a comment there https://cs3org.github.io/cs3apis/#cs3.storage.provider.v1beta1.ResourceInfo and remove those from the drivers. I think we can wait for @labkode to respond 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.
I'm in favour as well to keep the storage drivers implementation cleaner to remove quoting/encoding.
The decoration should happen in the ocdav layer as it only concerns owncloud clients.
// be defensive about wrong encoded etags | ||
func quoteEtag(etag string) string { | ||
if strings.HasPrefix(etag, "W/") { | ||
return `W/"` + strings.Trim(etag[2:], `"`) + `"` |
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 will panic when etag is empty.
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.
no, see line above: if strings.HasPrefix(etag, "W/") {
When ocdav renders etags it will now try to correct them to the definition as quoted strings which do not contain
"
. This prevents double or triple quoted etags on the webdav api.