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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 27, 2021
Merged

be defensive about wrongly quoted etags #1870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/unreleased/more-robust-etag-handling.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Enhancement: Be defensive about wrongly quoted etags

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.

https://github.com/cs3org/reva/pull/1870
12 changes: 10 additions & 2 deletions internal/http/services/owncloud/ocdav/propfind.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide
// etags must be enclosed in double quotes and cannot contain them.
// See https://tools.ietf.org/html/rfc7232#section-2.3 for details
// TODO(jfd) handle weak tags that start with 'W/'
propstatOK.Prop = append(propstatOK.Prop, s.newProp("d:getetag", md.Etag))
propstatOK.Prop = append(propstatOK.Prop, s.newProp("d:getetag", quoteEtag(md.Etag)))
}

if md.PermissionSet != nil {
Expand Down Expand Up @@ -712,7 +712,7 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide
switch pf.Prop[i].Local {
case "getetag": // both
if md.Etag != "" {
propstatOK.Prop = append(propstatOK.Prop, s.newProp("d:getetag", md.Etag))
propstatOK.Prop = append(propstatOK.Prop, s.newProp("d:getetag", quoteEtag(md.Etag)))
} else {
propstatNotFound.Prop = append(propstatNotFound.Prop, s.newProp("d:getetag", ""))
}
Expand Down Expand Up @@ -816,6 +816,14 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide
return &response, nil
}

// be defensive about wrong encoded etags
func quoteEtag(etag string) string {
if strings.HasPrefix(etag, "W/") {
return `W/"` + strings.Trim(etag[2:], `"`) + `"`
Copy link
Member

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.

Copy link
Contributor Author

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/") {

}
return `"` + strings.Trim(etag, `"`) + `"`
Copy link
Contributor

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/

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Member

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.

}

// a file is only yours if you are the owner
func isCurrentUserOwner(ctx context.Context, owner *userv1beta1.UserId) bool {
contextUser, ok := ctxuser.ContextGetUser(ctx)
Expand Down