cjw/Oxicloud
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

Merge pull request #473 from EdouardVanbelle/fix/nextcloud+webdav

Browse Source 
fix(nextcloud+webdav) fix bugs found via end to end tests
This commit is contained in:
Dionisio Pozo
2026-06-17 12:42:31 +02:00
committed by GitHub
parent e5059eee51 619c24e1e9
commit b8a0018785
17 changed files with 1081 additions and 703 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/interfaces/api/handlers/webdav_handler.rs
+338 -378
View File
@@ -658,6 +658,21 @@ async fn handle_proppatch(
) -> Result<Response<Body>, AppError> {
let _user = extract_user(&req)?;
// Active-lock guard (RFC 4918 §9.10.4): PROPPATCH writes properties,
// so a lock on the target must release them via `If:`. Captured
// before the body is consumed below so a rejected request doesn't
// even parse the XML.
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
if let Some(resp) =
enforce_native_lock(&state.webdav_lock_store, if_header_owned.as_deref(), &path)
{
return Ok(resp);
}
// Resolve the target resource type BEFORE consuming the body so
// we can pick the correct href shape in the multi-status
// response. RFC 4918 §5.2 + strict WebDAV-client parser rules
@@ -895,6 +910,111 @@ async fn handle_head(
.unwrap())
}
/// Resolve `path` to a user-owned resource using the optimized
/// PathResolver first, falling back to the legacy `get_folder_by_path` /
/// `get_file_by_path` lookups (the same ones GET uses) when the
/// optimized resolver returns NotFound.
///
/// **Why the fallback exists**: the optimized resolver and the read-side
/// `get_*_by_path` repositories don't always agree on what a "path"
/// looks like. The drive-refactor migration rewrote the `path` column
/// to strip the `My Folder - <user>/` prefix that the WebDAV dispatcher
/// (`resolve_webdav_path`) still prepends — leaving an inconsistency
/// where files PUT through the WebDAV surface stay reachable by GET
/// (legacy lookup) but invisible to the optimized resolver (strict
/// path-match). MOVE / DELETE / COPY previously 404'd on every
/// root-level file because they only used the optimized resolver.
///
/// Ownership is enforced in both branches: the optimized resolver
/// includes `user_id = $4` in its SQL; the fallback runs `assert_owner`
/// explicitly so a foreign-owned hit can't leak through.
async fn resolve_or_legacy(
state: &Arc<AppState>,
path: &str,
user_id: Uuid,
) -> Option<ResolvedResource> {
if let Some(resolver) = &state.path_resolver
&& let Ok(r) = resolver.resolve_path_for_user(path, user_id).await
{
return Some(r);
}
let user_id_str = user_id.to_string();
let folder_service = &state.applications.folder_service;
if let Ok(folder) = folder_service.get_folder_by_path(path).await
&& folder.owner_id.as_deref() == Some(&user_id_str)
{
return Some(ResolvedResource::Folder(folder));
}
let file_retrieval = &state.applications.file_retrieval_service;
if let Ok(file) = file_retrieval.get_file_by_path(path).await
&& file.owner_id.as_deref() == Some(&user_id_str)
{
return Some(ResolvedResource::File(file));
}
None
}
/// Extract every `<...>` token from a WebDAV `If:` header value.
///
/// RFC 4918 §10.4 defines a richer grammar (tagged-list / no-tag-list of
/// `(Condition)` items), but for our purposes the only thing that matters
/// is what lock tokens the caller is claiming to hold. Forgivingly scoop
/// every angle-bracketed value and let the caller compare against the
/// active lock token(s).
fn extract_if_header_tokens(if_header: &str) -> Vec<String> {
let mut out = Vec::new();
let mut current = String::new();
let mut inside = false;
for c in if_header.chars() {
match (inside, c) {
(false, '<') => {
inside = true;
current.clear();
}
(true, '>') => {
inside = false;
if !current.is_empty() {
out.push(std::mem::take(&mut current));
}
}
(true, c) => current.push(c),
_ => {}
}
}
out
}
/// RFC 4918 §9.10.4 — if `path` is locked, every mutating request MUST
/// carry the lock's token in its `If:` header. Returns `Some(Response)`
/// with a 423 Locked response when the request must be rejected; `None`
/// when the path is unlocked or the caller's `If:` header carries the
/// matching token (the cheap-and-cheerful submission check).
///
/// Shared by `handle_put` now and will be reused by `handle_delete`,
/// `handle_move`, `handle_copy`, and `handle_proppatch` when each of
/// those gets the same enforcement.
fn enforce_native_lock(
lock_store: &crate::infrastructure::services::webdav_lock_service::WebDavLockStore,
if_header: Option<&str>,
path: &str,
) -> Option<Response<Body>> {
let entry = lock_store.get_by_path(path)?;
if let Some(h) = if_header
&& extract_if_header_tokens(h)
.iter()
.any(|t| t == &entry.info.token)
{
return None;
}
Some(
Response::builder()
.status(StatusCode::LOCKED)
.body(Body::empty())
.unwrap(),
)
}
/**
* Handles PUT requests to create or update files.
*
@@ -926,6 +1046,21 @@ async fn handle_put(
return Err(AppError::bad_request("Cannot PUT to root folder"));
}
// ── Active-lock guard (RFC 4918 §9.10.4) ──────────────────────────
// Reject a write that targets a locked resource unless the request
// carries the lock token in `If:`. Captured before we consume the
// body into the CDC ingester — a 423 mustn't waste any bandwidth.
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
if let Some(resp) =
enforce_native_lock(&state.webdav_lock_store, if_header_owned.as_deref(), &path)
{
return Ok(resp);
}
// ── Ownership guard ────────────────────────────────────────
// Verify that the user owns the target file (update) or the
// parent folder (create). Without this check a user could
@@ -1117,6 +1252,18 @@ async fn handle_delete(
) -> Result<Response<Body>, AppError> {
let user = extract_user(&req)?;
// Active-lock guard (RFC 4918 §9.10.4).
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
if let Some(resp) =
enforce_native_lock(&state.webdav_lock_store, if_header_owned.as_deref(), &path)
{
return Ok(resp);
}
// Get services from state
let file_retrieval_service = &state.applications.file_retrieval_service;
let file_management_service = &state.applications.file_management_service;
@@ -1127,49 +1274,25 @@ async fn handle_delete(
return Err(AppError::forbidden("Cannot delete root folder"));
}
// Single-query path resolution (user-scoped)
if let Some(resolver) = &state.path_resolver {
match resolver.resolve_path_for_user(&path, user.id).await {
Ok(ResolvedResource::Folder(folder)) => {
folder_service
.delete_folder_with_perms(&folder.id, user.id)
.await
.map_err(|e| {
AppError::internal_error(format!("Failed to delete folder: {}", e))
})?;
}
Ok(ResolvedResource::File(file)) => {
file_management_service
.delete_file_with_perms(&file.id, user.id)
.await
.map_err(|e| {
AppError::internal_error(format!("Failed to delete file: {}", e))
})?;
}
Err(_) => return Err(AppError::not_found(format!("Resource not found: {}", path))),
}
} else {
// Fallback: legacy double-query path (with ownership check)
let folder_result = folder_service.get_folder_by_path(&path).await;
if let Ok(folder) = folder_result {
assert_owner(folder.owner_id.as_deref(), &user.id.to_string(), &path)?;
// Resolve via optimized resolver, falling back to the legacy
// double-query lookup (the one GET uses). Necessary because the
// optimized resolver and the read repositories disagree on path
// shape for some files; see `resolve_or_legacy` docs.
let _ = file_retrieval_service; // present for legacy fallback if needed elsewhere
match resolve_or_legacy(&state, &path, user.id).await {
Some(ResolvedResource::Folder(folder)) => {
folder_service
.delete_folder_with_perms(&folder.id, user.id)
.await
.map_err(|e| AppError::internal_error(format!("Failed to delete folder: {}", e)))?;
} else {
let file = file_retrieval_service
.get_file_by_path(&path)
.await
.map_err(|_e| AppError::not_found(format!("Resource not found: {}", path)))?;
assert_owner(file.owner_id.as_deref(), &user.id.to_string(), &path)?;
}
Some(ResolvedResource::File(file)) => {
file_management_service
.delete_file_with_perms(&file.id, user.id)
.await
.map_err(|e| AppError::internal_error(format!("Failed to delete file: {}", e)))?;
}
None => return Err(AppError::not_found(format!("Resource not found: {}", path))),
}
Ok(Response::builder()
@@ -1197,6 +1320,23 @@ async fn handle_move(
let user = extract_user(&req)?;
let source_path = path;
// Captured up front so a rejected MOVE doesn't run any DB work.
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
// Active-lock guard on the SOURCE (RFC 4918 §9.10.4): the move
// removes the source resource, which counts as modifying it.
if let Some(resp) = enforce_native_lock(
&state.webdav_lock_store,
if_header_owned.as_deref(),
&source_path,
) {
return Ok(resp);
}
// Get destination from Destination header
let destination = req
.headers()
@@ -1225,6 +1365,28 @@ async fn handle_move(
// SECURITY: reject path-traversal in destination
reject_path_traversal(&destination_path)?;
// Normalize destination through the SAME path-prefixing that
// `resolve_webdav_path` applied to `source_path` during dispatch.
// Without this, comparing source_parent_path (already prefixed with
// the user's home folder name) against dest_parent_path (raw from
// the URL, no prefix) always reports "different parent" — even for a
// pure rename at the same level — and breaks the move/rename branch
// selection below.
let destination_path = resolve_webdav_path(&state, user.id, &destination_path)
.await
.unwrap_or(destination_path);
// Destination lock guard: MOVE also creates/replaces a resource at
// the destination. If that path is locked, the same If: header must
// satisfy it.
if let Some(resp) = enforce_native_lock(
&state.webdav_lock_store,
if_header_owned.as_deref(),
&destination_path,
) {
return Ok(resp);
}
// Get services from state
let file_retrieval_service = &state.applications.file_retrieval_service;
let file_management_service = &state.applications.file_management_service;
@@ -1254,197 +1416,96 @@ async fn handle_move(
}
}
// Resolve source: single-query when PathResolver is available (user-scoped)
if let Some(resolver) = &state.path_resolver {
match resolver.resolve_path_for_user(&source_path, user.id).await {
Ok(ResolvedResource::Folder(folder)) => {
let dest_folder_name = destination_path
.split('/')
.next_back()
.unwrap_or(&destination_path);
let dest_parent_path = if let Some(idx) = destination_path.rfind('/') {
&destination_path[..idx]
} else {
""
};
// Resolve source via optimized resolver with legacy fallback (see
// `resolve_or_legacy` for the rationale). Single match collapses the
// two near-identical branches that the resolver-only + legacy-only
// versions used to keep.
let _ = file_retrieval_service; // referenced via resolve_or_legacy
let resolved = resolve_or_legacy(&state, &source_path, user.id)
.await
.ok_or_else(|| AppError::not_found(format!("Resource not found: {}", source_path)))?;
let move_dto = crate::application::dtos::folder_dto::MoveFolderDto {
parent_id: if dest_parent_path.is_empty() {
None
} else {
match folder_service.get_folder_by_path(dest_parent_path).await {
Ok(parent) => {
// SECURITY: verify destination parent belongs to caller (V-08)
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
Some(parent.id)
}
Err(_) => None,
}
},
};
folder_service
.move_folder_with_perms(&folder.id, move_dto, user.id)
.await
.map_err(AppError::from)?;
if folder.name != dest_folder_name {
let rename_dto = crate::application::dtos::folder_dto::RenameFolderDto {
name: dest_folder_name.to_string(),
};
folder_service
.rename_folder_with_perms(&folder.id, rename_dto, user.id)
.await
.map_err(AppError::from)?;
}
}
Ok(ResolvedResource::File(file)) => {
let dest_filename = destination_path
.split('/')
.next_back()
.unwrap_or(&destination_path);
let dest_parent_path = if let Some(idx) = destination_path.rfind('/') {
&destination_path[..idx]
} else {
""
};
let source_parent_path = if let Some(idx) = source_path.rfind('/') {
&source_path[..idx]
} else {
""
};
if source_parent_path != dest_parent_path {
// SECURITY: verify destination parent belongs to caller (V-08)
if !dest_parent_path.is_empty()
&& let Ok(parent) =
folder_service.get_folder_by_path(dest_parent_path).await
{
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
}
file_management_service
.move_file_with_perms(&file.id, user.id, Some(dest_parent_path.to_string()))
.await
.map_err(AppError::from)?;
}
if file.name != dest_filename {
file_management_service
.rename_file_with_perms(&file.id, user.id, dest_filename)
.await
.map_err(AppError::from)?;
}
}
Err(_) => {
return Err(AppError::not_found(format!(
"Resource not found: {}",
source_path
)));
}
}
} else {
// Fallback: legacy double-query path (with ownership check)
let folder_result = folder_service.get_folder_by_path(&source_path).await;
if let Ok(folder) = folder_result {
assert_owner(
folder.owner_id.as_deref(),
&user.id.to_string(),
&source_path,
)?;
let dest_folder_name = destination_path
.split('/')
.next_back()
.unwrap_or(&destination_path);
let dest_parent_path = if let Some(idx) = destination_path.rfind('/') {
&destination_path[..idx]
} else {
""
};
let dest_name = destination_path
.rsplit('/')
.next()
.unwrap_or(&destination_path);
let dest_parent_path = destination_path
.rfind('/')
.map(|i| &destination_path[..i])
.unwrap_or("");
let source_parent_path = source_path
.rfind('/')
.map(|i| &source_path[..i])
.unwrap_or("");
match resolved {
ResolvedResource::Folder(folder) => {
let move_dto = crate::application::dtos::folder_dto::MoveFolderDto {
parent_id: if dest_parent_path.is_empty() {
None
} else {
match folder_service.get_folder_by_path(dest_parent_path).await {
Ok(parent) => {
// SECURITY: verify destination parent belongs to caller (V-08)
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
Some(parent.id)
}
Err(_) => None,
}
},
};
folder_service
.move_folder_with_perms(&folder.id, move_dto, user.id)
.await
.map_err(|e| AppError::internal_error(format!("Failed to move folder: {}", e)))?;
if folder.name != dest_folder_name {
let rename_dto = crate::application::dtos::folder_dto::RenameFolderDto {
name: dest_folder_name.to_string(),
};
folder_service
.rename_folder_with_perms(&folder.id, rename_dto, user.id)
.await
.map_err(AppError::from)?;
}
} else {
let file = file_retrieval_service
.get_file_by_path(&source_path)
.await
.map_err(|_e| {
AppError::not_found(format!("Resource not found: {}", source_path))
})?;
assert_owner(file.owner_id.as_deref(), &user.id.to_string(), &source_path)?;
let dest_filename = destination_path
.split('/')
.next_back()
.unwrap_or(&destination_path);
let dest_parent_path = if let Some(idx) = destination_path.rfind('/') {
&destination_path[..idx]
} else {
""
};
let source_parent_path = if let Some(idx) = source_path.rfind('/') {
&source_path[..idx]
} else {
""
};
if source_parent_path != dest_parent_path {
// SECURITY: verify destination parent belongs to caller (V-08)
if !dest_parent_path.is_empty()
&& let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await
} else if let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await
{
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
}
file_management_service
.move_file_with_perms(&file.id, user.id, Some(dest_parent_path.to_string()))
Some(parent.id)
} else {
None
},
};
folder_service
.move_folder_with_perms(&folder.id, move_dto, user.id)
.await
.map_err(AppError::from)?;
if folder.name != dest_name {
let rename_dto = crate::application::dtos::folder_dto::RenameFolderDto {
name: dest_name.to_string(),
};
folder_service
.rename_folder_with_perms(&folder.id, rename_dto, user.id)
.await
.map_err(AppError::from)?;
}
if file.name != dest_filename {
}
ResolvedResource::File(file) => {
if source_parent_path != dest_parent_path {
// Resolve the destination's parent PATH into a folder ID
// before handing it to move_file_with_perms (which takes
// an Option<folder_id String>, not a path). Previously
// the path was passed straight through and the move
// would silently fail because no row matches a folder
// whose id literally equals the path text.
let target_parent_id = if dest_parent_path.is_empty() {
None
} else {
let parent = folder_service
.get_folder_by_path(dest_parent_path)
.await
.map_err(|_| {
AppError::not_found(format!(
"Destination parent not found: {}",
dest_parent_path
))
})?;
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
Some(parent.id)
};
file_management_service
.rename_file_with_perms(&file.id, user.id, dest_filename)
.move_file_with_perms(&file.id, user.id, target_parent_id)
.await
.map_err(AppError::from)?;
}
if file.name != dest_name {
file_management_service
.rename_file_with_perms(&file.id, user.id, dest_name)
.await
.map_err(AppError::from)?;
}
@@ -1476,6 +1537,15 @@ async fn handle_copy(
let user = extract_user(&req)?;
let source_path = path;
// Captured up front (cheap; used below for the destination lock guard).
// COPY doesn't mutate the source, so no source lock check — only the
// destination needs to clear (RFC 4918 §9.10.4).
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
// Get destination from Destination header
let destination = req
.headers()
@@ -1504,6 +1574,22 @@ async fn handle_copy(
// SECURITY: reject path-traversal in destination
reject_path_traversal(&destination_path)?;
// Normalize through the same path-prefixing the dispatcher applied
// to source_path. See the long comment in handle_move for why this
// matters — same root-cause class of asymmetric-path bugs.
let destination_path = resolve_webdav_path(&state, user.id, &destination_path)
.await
.unwrap_or(destination_path);
// Active-lock guard on the destination (RFC 4918 §9.10.4).
if let Some(resp) = enforce_native_lock(
&state.webdav_lock_store,
if_header_owned.as_deref(),
&destination_path,
) {
return Ok(resp);
}
// Get depth from Depth header
let depth = req
.headers()
@@ -1539,144 +1625,39 @@ async fn handle_copy(
}
}
// Resolve source: single-query when PathResolver is available (user-scoped)
if let Some(resolver) = &state.path_resolver {
match resolver.resolve_path_for_user(&source_path, user.id).await {
Ok(ResolvedResource::Folder(folder)) => {
let recursive = depth != "0";
// Resolve source via optimized resolver with legacy fallback; collapses
// the two near-identical branches the resolver-only + legacy-only
// versions used to keep.
let _ = file_retrieval_service; // referenced via resolve_or_legacy
let resolved = resolve_or_legacy(&state, &source_path, user.id)
.await
.ok_or_else(|| AppError::not_found(format!("Resource not found: {}", source_path)))?;
let dest_folder_name = destination_path
.split('/')
.next_back()
.unwrap_or(&destination_path);
let dest_parent_path = if let Some(idx) = destination_path.rfind('/') {
&destination_path[..idx]
} else {
""
};
let dest_name = destination_path
.rsplit('/')
.next()
.unwrap_or(&destination_path);
let dest_parent_path = destination_path
.rfind('/')
.map(|i| &destination_path[..i])
.unwrap_or("");
let target_parent_id = if dest_parent_path.is_empty() {
None
} else {
match folder_service.get_folder_by_path(dest_parent_path).await {
Ok(parent) => {
// SECURITY: verify destination parent belongs to caller (V-08)
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
Some(parent.id)
}
Err(_) => None,
}
};
if recursive {
let file_management_service = &state.applications.file_management_service;
file_management_service
.copy_folder_tree_with_perms(
&folder.id,
user.id,
target_parent_id,
Some(dest_folder_name.to_string()),
)
.await
.map_err(|e| {
AppError::internal_error(format!("Failed to copy folder tree: {}", e))
})?;
} else {
let create_dto = crate::application::dtos::folder_dto::CreateFolderDto {
name: dest_folder_name.to_string(),
parent_id: target_parent_id,
};
folder_service
.create_folder_with_perms(create_dto, user.id)
.await
.map_err(|e| {
AppError::internal_error(format!(
"Failed to create destination folder: {}",
e
))
})?;
}
}
Ok(ResolvedResource::File(file)) => {
let dest_parent_path = if let Some(idx) = destination_path.rfind('/') {
&destination_path[..idx]
} else {
""
};
let target_folder_id = if dest_parent_path.is_empty() {
None
} else {
match folder_service.get_folder_by_path(dest_parent_path).await {
Ok(parent) => {
// SECURITY: verify destination parent belongs to caller (V-08)
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
Some(parent.id)
}
Err(_) => None,
}
};
let file_management_service = &state.applications.file_management_service;
file_management_service
.copy_file_with_perms(&file.id, user.id, target_folder_id)
.await
.map_err(|e| AppError::internal_error(format!("Failed to copy file: {}", e)))?;
}
Err(_) => {
return Err(AppError::not_found(format!(
"Resource not found: {}",
source_path
)));
}
}
let target_parent_id = if dest_parent_path.is_empty() {
None
} else if let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await {
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
Some(parent.id)
} else {
// Fallback: legacy double-query path (with ownership check)
let folder_result = folder_service.get_folder_by_path(&source_path).await;
None
};
if let Ok(folder) = folder_result {
assert_owner(
folder.owner_id.as_deref(),
&user.id.to_string(),
&source_path,
)?;
match resolved {
ResolvedResource::Folder(folder) => {
let recursive = depth != "0";
let dest_folder_name = destination_path
.split('/')
.next_back()
.unwrap_or(&destination_path);
let dest_parent_path = if let Some(idx) = destination_path.rfind('/') {
&destination_path[..idx]
} else {
""
};
let target_parent_id = if dest_parent_path.is_empty() {
None
} else {
match folder_service.get_folder_by_path(dest_parent_path).await {
Ok(parent) => {
// SECURITY: verify destination parent belongs to caller (V-08)
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
Some(parent.id)
}
Err(_) => None,
}
};
if recursive {
let file_management_service = &state.applications.file_management_service;
file_management_service
@@ -1684,7 +1665,7 @@ async fn handle_copy(
&folder.id,
user.id,
target_parent_id,
Some(dest_folder_name.to_string()),
Some(dest_name.to_string()),
)
.await
.map_err(|e| {
@@ -1692,7 +1673,7 @@ async fn handle_copy(
})?;
} else {
let create_dto = crate::application::dtos::folder_dto::CreateFolderDto {
name: dest_folder_name.to_string(),
name: dest_name.to_string(),
parent_id: target_parent_id,
};
folder_service
@@ -1705,41 +1686,20 @@ async fn handle_copy(
))
})?;
}
} else {
let file = file_retrieval_service
.get_file_by_path(&source_path)
.await
.map_err(|_e| {
AppError::not_found(format!("Resource not found: {}", source_path))
})?;
assert_owner(file.owner_id.as_deref(), &user.id.to_string(), &source_path)?;
let dest_parent_path = if let Some(idx) = destination_path.rfind('/') {
&destination_path[..idx]
} else {
""
};
let target_folder_id = if dest_parent_path.is_empty() {
None
} else {
match folder_service.get_folder_by_path(dest_parent_path).await {
Ok(parent) => {
// SECURITY: verify destination parent belongs to caller (V-08)
assert_owner(
parent.owner_id.as_deref(),
&user.id.to_string(),
dest_parent_path,
)?;
Some(parent.id)
}
Err(_) => None,
}
};
}
ResolvedResource::File(file) => {
// M8b fix: copy_file_with_perms now accepts an optional new
// filename — without it, a copy to the same folder with a
// different name collided with the source on the
// (folder, name, user) unique index. Pass dest_name when it
// differs from the source so the INSERT lands with the
// intended name in a single round-trip; pass None for the
// "same name in a different folder" case to keep the existing
// semantics.
let file_management_service = &state.applications.file_management_service;
let copy_name = (file.name != dest_name).then(|| dest_name.to_string());
file_management_service
.copy_file_with_perms(&file.id, user.id, target_folder_id)
.copy_file_with_perms(&file.id, user.id, target_parent_id, copy_name)
.await
.map_err(|e| AppError::internal_error(format!("Failed to copy file: {}", e)))?;
}
src/interfaces/nextcloud/trashbin_handler.rs
+71 -10
View File
@@ -9,12 +9,15 @@ use quick_xml::{
};
use std::sync::Arc;
use crate::application::ports::file_ports::FileRetrievalUseCase;
use crate::application::ports::folder_ports::FolderUseCase;
use crate::application::ports::trash_ports::TrashUseCase;
use crate::common::di::AppState;
use crate::interfaces::errors::AppError;
use crate::interfaces::middleware::auth::{AuthUser, CurrentUser};
use crate::interfaces::nextcloud::webdav_handler::{
batch_resolve_ids, format_oc_id, write_text_element,
batch_resolve_ids, extract_nc_subpath_from_dest, format_oc_id, nc_to_internal_path,
write_text_element,
};
const HEADER_DAV: HeaderName = HeaderName::from_static("dav");
@@ -37,7 +40,12 @@ pub async fn handle_nc_trashbin(
handle_propfind(state, &user).await
}
"MOVE" if subpath_trimmed.starts_with("trash/") => {
handle_restore(state, &user, subpath_trimmed).await
let dest_header = req
.headers()
.get("destination")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
handle_restore(state, dest_header, &user, subpath_trimmed).await
}
"DELETE" if subpath_trimmed == "trash" || subpath_trimmed.is_empty() => {
handle_empty_trash(state, &user).await
@@ -98,6 +106,7 @@ async fn handle_propfind(
async fn handle_restore(
state: Arc<AppState>,
dest_header: Option<String>,
user: &CurrentUser,
subpath: &str,
) -> Result<Response<Body>, AppError> {
@@ -108,15 +117,67 @@ async fn handle_restore(
.as_ref()
.ok_or_else(|| AppError::internal_error("Trash service not available"))?;
trash_svc
.restore_item(&id, user.id)
.await
.map_err(|e| AppError::internal_error(format!("Failed to restore item: {}", e)))?;
// RFC 4918 §9.9.4 + Sabre convention: clients send `Destination` to
// tell the server where the restored item should land. We don't yet
// honor it for relocation (restore always lands at the original
// path), but we DO honor it for the collision check: if the requested
// destination is taken by a live resource the move must be refused
// with 412 — there is no `Overwrite: T` workflow for trash restore in
// either Sabre/DAV or the NC desktop client (a live file being
// silently replaced by an undeleted one would be a footgun).
if let Some(dest_header) = dest_header
&& let Some(dest_subpath) = extract_nc_subpath_from_dest(&dest_header, &user.username)
{
let dest_internal = nc_to_internal_path(&user.username, &dest_subpath)?;
let folder_service = &state.applications.folder_service;
let file_service = &state.applications.file_retrieval_service;
let dest_taken = file_service.get_file_by_path(&dest_internal).await.is_ok()
|| folder_service
.get_folder_by_path(&dest_internal)
.await
.is_ok();
if dest_taken {
return Ok(Response::builder()
.status(StatusCode::PRECONDITION_FAILED)
.body(Body::empty())
.unwrap());
}
}
Ok(Response::builder()
.status(StatusCode::CREATED)
.body(Body::empty())
.unwrap())
match trash_svc.restore_item(&id, user.id).await {
Ok(()) => Ok(Response::builder()
.status(StatusCode::CREATED)
.body(Body::empty())
.unwrap()),
Err(e) => {
// Collision at the original path — a live file/folder is sitting
// where the trashed one wants to come back to. Mirrors the G4/G5
// semantics in webdav_handler::handle_move ("Overwrite: F to an
// existing path → 412"); restore has no Overwrite header so the
// refusal is unconditional. The caller can resolve by renaming
// or trashing the conflicting live resource first.
//
// We string-match for the unique-index / duplicate-key signature
// because restore_item currently re-wraps every storage error as
// InternalError, so the original DomainError::AlreadyExists kind
// is not propagated. A follow-up should thread the kind through
// and let this be a kind-based check.
let msg = format!("{}", e);
if msg.contains("duplicate key")
|| msg.contains("unique constraint")
|| msg.to_ascii_lowercase().contains("already exists")
{
return Ok(Response::builder()
.status(StatusCode::PRECONDITION_FAILED)
.body(Body::empty())
.unwrap());
}
Err(AppError::internal_error(format!(
"Failed to restore item: {}",
e
)))
}
}
}
// ──────────────────── DELETE (empty trash) ────────────────────
src/interfaces/nextcloud/webdav_handler.rs
+201 -52
View File
@@ -381,10 +381,18 @@ async fn handle_head(
// ETag comes from `FileDto::etag` — see the same comment block on
// the GET handler. HEAD and GET must agree byte-for-byte; pulling
// both from the same DTO field guarantees that.
//
// We deliberately do NOT set `Content-Length: file.size` here even
// though RFC 7231 §4.3.2 says HEAD SHOULD return the same headers
// GET would. Our body is `Body::empty()`, so declaring a non-zero
// Content-Length tells the client "20 bytes are coming" — and on a
// keep-alive connection the client waits forever for them. Hyper
// derives `Content-Length: 0` from the empty body, which is honest
// about what's actually on the wire. Clients that need the file
// size use PROPFIND (which is what NC and Sabre clients do).
Ok(Response::builder()
.status(StatusCode::OK)
.header(header::CONTENT_TYPE, file.mime_type.as_ref())
.header(header::CONTENT_LENGTH, file.size)
.header(header::ETAG, format!("\"{}\"", file.etag))
.header(header::LAST_MODIFIED, modified_at.to_rfc2822())
.body(Body::empty())
@@ -543,6 +551,62 @@ fn parse_proppatch_favorite(body: &str) -> Option<u8> {
// ──────────────────── PUT ────────────────────
/// Strip the optional `W/` weak prefix and surrounding double-quotes
/// from one ETag value in an `If-Match` / `If-None-Match` list. Returns
/// `(is_weak, inner)`.
fn parse_etag_value(raw: &str) -> (bool, &str) {
let trimmed = raw.trim();
if let Some(rest) = trimmed.strip_prefix("W/") {
(true, rest.trim().trim_matches('"'))
} else {
(false, trimmed.trim_matches('"'))
}
}
/// RFC 7232 §3.2 — `If-None-Match` fails for PUT when:
/// - the header value is `*` and a current representation exists, OR
/// - any listed ETag matches the current representation (weak comparison
/// — weak validators in the request are equivalent to strong for the
/// match itself, only If-Match is required to be strong).
fn if_none_match_precondition_fails(header: &str, current_etag: Option<&str>) -> bool {
let v = header.trim();
if v == "*" {
return current_etag.is_some();
}
let Some(current) = current_etag else {
return false;
};
v.split(',').any(|tag| {
let (_, parsed) = parse_etag_value(tag);
!parsed.is_empty() && parsed == current
})
}
/// RFC 7232 §3.1 — `If-Match` fails for PUT when:
/// - the resource doesn't currently exist (no strong validator to match), OR
/// - the header isn't `*` and no listed ETag strong-matches the current one
/// (weak validators in the request never satisfy a strong-match).
fn if_match_precondition_fails(header: &str, current_etag: Option<&str>) -> bool {
let v = header.trim();
let Some(current) = current_etag else {
return true;
};
if v == "*" {
return false;
}
!v.split(',').any(|tag| {
let (is_weak, parsed) = parse_etag_value(tag);
!is_weak && !parsed.is_empty() && parsed == current
})
}
fn precondition_failed_response() -> Response<Body> {
Response::builder()
.status(StatusCode::PRECONDITION_FAILED)
.body(Body::empty())
.unwrap()
}
async fn handle_put(
state: Arc<AppState>,
req: Request<Body>,
@@ -566,6 +630,31 @@ async fn handle_put(
.and_then(|v| v.to_str().ok())
.and_then(|v| v.parse::<i64>().ok());
// ── Conditional preconditions (RFC 7232 §3.1 / §3.2) ─────────────
// Evaluated BEFORE body ingestion so a rejected PUT doesn't waste
// bandwidth or disk I/O on a body the server is going to throw away.
// The lookup is reused for the create-vs-update distinction below,
// so this is also free of an extra DB hit.
let existing = file_service.get_file_by_path(&internal_path).await.ok();
let current_etag = existing.as_ref().map(|f| f.etag.as_str());
if let Some(value) = req
.headers()
.get(header::IF_NONE_MATCH)
.and_then(|v| v.to_str().ok())
&& if_none_match_precondition_fails(value, current_etag)
{
return Ok(precondition_failed_response());
}
if let Some(value) = req
.headers()
.get(header::IF_MATCH)
.and_then(|v| v.to_str().ok())
&& if_match_precondition_fails(value, current_etag)
{
return Ok(precondition_failed_response());
}
// ── Direct PUT cap ───────────────────────────────────────────────
// We use `direct_put_max_bytes` (default 1 GiB), not `max_upload_size`
// (default 10 GB). Larger files must come through the chunked upload
@@ -594,8 +683,9 @@ async fn handle_put(
.await?;
let content_type = ingested.content_type.clone();
// Distinguish create (201) vs update (204) for the response status.
let existed = file_service.get_file_by_path(&internal_path).await.is_ok();
// Distinguish create (201) vs update (204) for the response status,
// using the lookup already done above for the precondition check.
let existed = existing.is_some();
// Single streaming path — handles both update and create internally,
// swapping the file row onto the already-ingested blob.
@@ -630,7 +720,18 @@ async fn handle_mkcol(
let folder_service = &state.applications.folder_service;
let internal_path = nc_to_internal_path(&user.username, subpath)?;
// If the folder already exists, return 405 per RFC 4918 §9.3.1
// RFC 4918 §9.3.1:
// - target already exists → 405 Method Not Allowed
// - parent collection of the target does NOT exist → 409 Conflict
// - parent exists and target does not → 201 Created
//
// Previous behaviour effectively performed `mkdir -p` and returned
// 201 even when intermediate ancestors were missing. Sabre/DAV and
// the actual NC server both return 409 here, so the legacy
// auto-create deviated from the reference implementation. NC desktop
// walks ancestors one MKCOL at a time anyway, so dropping the
// auto-create doesn't break real clients.
if folder_service
.get_folder_by_path(&internal_path)
.await
@@ -642,56 +743,39 @@ async fn handle_mkcol(
.unwrap());
}
// Collect path segments that need to be created (walk from root to leaf)
let segments: Vec<&str> = subpath.split('/').filter(|s| !s.is_empty()).collect();
if segments.is_empty() {
return Err(AppError::bad_request(
"MKCOL on the user root is not allowed",
));
}
let (target_name, parent_segments) = segments.split_last().expect("checked non-empty above");
let user_root = nc_to_internal_path(&user.username, "")?;
let mut current_path = user_root.clone();
let mut parent_id = folder_service
.get_folder_by_path(&user_root)
.await
.map_err(|_| AppError::not_found("User root folder not found"))?
.id
.clone();
let parent_path = if parent_segments.is_empty() {
user_root.clone()
} else {
format!("{}/{}", user_root, parent_segments.join("/"))
};
for segment in &segments {
current_path = format!("{}/{}", current_path, segment);
match folder_service.get_folder_by_path(&current_path).await {
Ok(existing) => {
parent_id = existing.id.clone();
}
Err(_) => {
let dto = CreateFolderDto {
name: segment.to_string(),
parent_id: Some(parent_id.clone()),
};
match folder_service.create_folder_with_perms(dto, user.id).await {
Ok(created) => {
parent_id = created.id.clone();
}
Err(e)
if e.message.contains("already exists")
|| e.message.contains("Already Exists") =>
{
// Race condition — folder created concurrently
let folder = folder_service
.get_folder_by_path(&current_path)
.await
.map_err(|_| {
AppError::internal_error("Folder exists but cannot be found")
})?;
parent_id = folder.id.clone();
}
Err(e) => {
return Err(AppError::internal_error(format!(
"Failed to create folder: {}",
e
)));
}
}
}
let parent_folder = match folder_service.get_folder_by_path(&parent_path).await {
Ok(folder) => folder,
Err(_) => {
return Ok(Response::builder()
.status(StatusCode::CONFLICT)
.body(Body::empty())
.unwrap());
}
}
};
let dto = CreateFolderDto {
name: target_name.to_string(),
parent_id: Some(parent_folder.id.clone()),
};
folder_service
.create_folder_with_perms(dto, user.id)
.await
.map_err(|e| AppError::internal_error(format!("Failed to create folder: {}", e)))?;
Ok(Response::builder()
.status(StatusCode::CREATED)
@@ -781,6 +865,19 @@ async fn handle_move(
.ok_or_else(|| AppError::bad_request("Missing Destination header"))?
.to_string();
// RFC 4918 §9.9.3: the `Overwrite` header has the default value `T`.
// `F` MUST cause the request to fail with 412 when the destination
// already exists; `T` (or absent) MUST replace the destination as if
// it didn't exist (the response then drops from 201 Created to 204
// No Content per §9.9.4 because the URI's resource was replaced
// rather than newly created).
let overwrite_forbidden = req
.headers()
.get("overwrite")
.and_then(|v| v.to_str().ok())
.map(|v| v.trim().eq_ignore_ascii_case("F"))
.unwrap_or(false);
// Parse destination path: extract subpath after /remote.php/dav/files/{user}/
let dest_subpath = extract_nc_subpath_from_dest(&destination, &user.username)
.ok_or_else(|| AppError::bad_request("Invalid Destination URL"))?;
@@ -790,6 +887,58 @@ async fn handle_move(
let file_service = &state.applications.file_retrieval_service;
let file_mgmt = &state.applications.file_management_service;
// ── Destination-collision precondition (RFC 4918 §9.9.4) ──────────
// Resolved once up-front so the file/folder branches below don't
// each have to repeat the check. `dest_existed_before` becomes the
// 204-vs-201 selector at response time.
let dest_internal_precheck = nc_to_internal_path(&user.username, &dest_subpath)?;
let dest_existing_file = file_service
.get_file_by_path(&dest_internal_precheck)
.await
.ok();
let dest_existing_folder = folder_service
.get_folder_by_path(&dest_internal_precheck)
.await
.ok();
let dest_existed_before = dest_existing_file.is_some() || dest_existing_folder.is_some();
if dest_existed_before {
if overwrite_forbidden {
return Ok(Response::builder()
.status(StatusCode::PRECONDITION_FAILED)
.body(Body::empty())
.unwrap());
}
// Overwrite: T (or absent) → delete the existing destination first,
// then proceed with the move. Trashing is fine: per RFC the source
// resource appears at the destination URI; what happens to the
// overwritten one is up to the server.
if let Some(existing_file) = &dest_existing_file {
file_mgmt
.delete_and_cleanup_with_perms(&existing_file.id, user.id)
.await
.map_err(|e| {
AppError::internal_error(format!("Failed to overwrite destination file: {}", e))
})?;
} else if let Some(existing_folder) = &dest_existing_folder {
folder_service
.delete_folder_with_perms(&existing_folder.id, user.id)
.await
.map_err(|e| {
AppError::internal_error(format!(
"Failed to overwrite destination folder: {}",
e
))
})?;
}
}
let final_status = if dest_existed_before {
StatusCode::NO_CONTENT
} else {
StatusCode::CREATED
};
// Try as file first.
if let Ok(file) = file_service.get_file_by_path(&src_internal).await {
let (dest_parent_sub, dest_name) = match dest_subpath.rsplit_once('/') {
@@ -833,7 +982,7 @@ async fn handle_move(
// Return ETag and OC-ETag so Nextcloud clients can track the moved file.
let dest_internal = nc_to_internal_path(&user.username, &dest_subpath)?;
let mut builder = Response::builder().status(StatusCode::CREATED);
let mut builder = Response::builder().status(final_status);
if let Ok(moved) = file_service.get_file_by_path(&dest_internal).await {
// Route through `FileDto::etag` so the MOVE response
// matches what a subsequent PROPFIND on the destination
@@ -909,7 +1058,7 @@ async fn handle_move(
}
return Ok(Response::builder()
.status(StatusCode::CREATED)
.status(final_status)
.body(Body::empty())
.unwrap());
}
@@ -922,7 +1071,7 @@ async fn handle_move(
/// Only accepts relative paths or absolute URLs whose path starts with the
/// expected DAV prefix. For full URLs the host is ignored — the path alone is
/// used — so an attacker cannot redirect the server to a different host.
fn extract_nc_subpath_from_dest(dest: &str, username: &str) -> Option<String> {
pub fn extract_nc_subpath_from_dest(dest: &str, username: &str) -> Option<String> {
let prefix = format!("/remote.php/dav/files/{}/", username);
// For full URLs, extract the path portion (everything after the authority).
let path = if dest.starts_with("http://") || dest.starts_with("https://") {