-
Notifications
You must be signed in to change notification settings - Fork 1.2k
mod_dav: optionally set mtime against x-oc-mtime header #556
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
base: trunk
Are you sure you want to change the base?
Conversation
…av_hooks_repository` modules/dav/fs/repos.c: implement `dav_fs_set_mtime` Signed-off-by: Leo <[email protected]>
Signed-off-by: Leo <[email protected]>
Signed-off-by: Leo <[email protected]>
Signed-off-by: Leo <[email protected]>
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 is an interesting feature. Are you aware of any standardisation effort for that header?
modules/dav/fs/repos.c
Outdated
status = apr_file_mtime_set(resource->info->pathname, mtime, pool); | ||
|
||
if (status != APR_SUCCESS) { | ||
return dav_new_error(pool, HTTP_BAD_REQUEST, 0, status, "Could not set mtime."); |
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 we get this far this is likely not a client error so this should be mapped to something appropriate rather than a 400. Or default to 500?
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.
actually i was a bit unsure whether 4xx or 5xx should be returned in such case. despite most invalid patterns has already being sanitized in dav_parse_mtime
, including:
- non-digits, e.g.
nonsense bytes
- negatives, e.g.
-123123123
- non-decimal, e.g.
0xa0b1
- empty strings
- overflowing
while apr_file_mtime_set
may still fail, in most cases it is clear that a 500 should be returned, e.g. kernel VFS failed to respond utime
syscall. but i was wondering if there is any possibility that an invalid user input fails apr_file_mtime_set
where 400 should be returned.
as of now I would return a 500 in such cases instead of 400.
seems there is not much such efforts (i.e. RFC drafts) from what i know. it is an approach OwnCloud (predecessor of NextCloud) adopts to enable this feature is also implemented by rclone's given that this is a non-standard, i would implement it but keep it disabled by default to avoid any non-standard breaking change. users who prefer to use this feature should have it opted in manually with |
…mtime when `conf->honor_mtime_header == DAV_ENABLED_ON`, check `set_mtime` callback is available before invoking, log debug-level messages docs/log-message-tags: bumped with `docs/log-message-tags/update-log-msg-tags` Signed-off-by: Leo <[email protected]>
…e_set` fails, emit error-level log message also docs/log-message-tags: bumped with `docs/log-message-tags/update-log-msg-tags` Signed-off-by: Leo <[email protected]>
WebDAV RFCs did not specify a standard approach to set modification time for files or directory on the server, this feature enables better presevation of file metadata when uploading.
as a compensation, OwnCloud (now known as NextCloud) proposed a
X-OC-Mtime
header which contains UNIX timestamp of mtime with file-modifying methods (e.g.PUT
) which implements the feature above as a extension to the standard.this patch allows apache to optionally honor the
X-OC-Mtime
header and set mtime accordingly ifDavHonorMtimeHeader On
is set.