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

ocdav: Fix parsing of PROPPATCH XML properties #743

Merged
merged 1 commit into from
May 15, 2020
Merged

ocdav: Fix parsing of PROPPATCH XML properties #743

merged 1 commit into from
May 15, 2020

Conversation

PVince81
Copy link
Contributor

Use the parent struct when parsing to make sure the decoder finds the
hint about "innerxml" parsing mode for proppatch properties,
and prevents mangling of said values when they contain an "xmlns"
attribute.

Fixes owncloud/ocis-reva#203

@butonic please double check the fix as I didn't understand the lang-related code and seemingly could not create a test case that has any difference before and after the fix.

If I proppatch a value of <foo xml:lang="en" xmlns="http://bar"/> it will now also appear as <foo xml:lang="en" xmlns="http://bar"/> in the extended attribute and propfind result.

@PVince81 PVince81 requested a review from labkode as a code owner May 14, 2020 14:51
@PVince81 PVince81 changed the title ocdav: Fix parsing of PROPPATCH properties ocdav: Fix parsing of PROPPATCH XML properties May 14, 2020
@PVince81
Copy link
Contributor Author

very strange, the linter says this is unused but it is used... I can't remove it without causing compilation errors:

internal/http/services/owncloud/ocdav/proppatch.go:216:6: type `xmlValue` is unused (unused)

@PVince81
Copy link
Contributor Author

Use the parent struct when parsing to make sure the decoder finds the
hint about "innerxml" parsing mode for proppatch properties,
and prevents mangling of said values when they contain an "xmlns"
attribute.
@PVince81
Copy link
Contributor Author

ok, got it, it's the whole type/struct/class that is unused (me still learning Golang...)

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

AFAIU the previous code tried to deal with cases where the proppatch value was reusing an existing namespace from outside the innerxml. I think it makes more sense to treat it as an opaque value. Maybe the existing code tries to do a little too much magic. Getting rid of it is a win.

@PVince81
Copy link
Contributor Author

cases where the proppatch value was reusing an existing namespace from outside the innerxml

strange, I seemed to remember seeing a comment somewhere that said that it's anyway illegal to reuse namespaces from the parent but cannot find it anywhere in the spec (not present in http://www.webdav.org/specs/rfc4918.html#property_values)

in any case, I think there isn't much benefit in supporting this as the custom properties we'll be setting will have their own namespaces. And for known properties (like the DAV: ones) they have no complex XML values.

I'm not even sure whether Sabre DAV's XML supports this scenario of namespace reuse.

@labkode labkode merged commit a51ce34 into cs3org:master May 15, 2020
@PVince81 PVince81 deleted the fix-proppatchprops-parsing branch May 15, 2020 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PROPPATCH mangles XML values
3 participants