fix(webdav): correct COPY/MOVE semantics, dead property persistence, and lock enforcement - #2903
fix(webdav): correct COPY/MOVE semantics, dead property persistence, and lock enforcement#2903devLythen wants to merge 6 commits into
Conversation
Signed-off-by: Lythen <intro-iu@outlook.com>
Signed-off-by: Lythen <intro-iu@outlook.com>
Signed-off-by: Lythen <intro-iu@outlook.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the server’s WebDAV behavior by aligning COPY/MOVE responses and lock handling with RFC 4918 expectations, and by introducing persistent “dead property” storage so PROPPATCH/PROPFIND can round-trip custom properties across requests and MOVE operations.
Changes:
- Add shared-lock support and adjust lock confirmation to validate against the user-resolved resource path.
- Persist dead WebDAV properties in the DB (new model + migration) and surface them via PROPFIND; migrate them on MOVE.
- Fix COPY/MOVE target naming semantics by introducing a named-copy path (
CopyTo) and extending transfer-task metadata to carry the destination name.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/webdav/xml.go | Updates lockinfo parsing/output; changes PROPFIND body parsing/validation. |
| server/webdav/webdav.go | Adjusts request dispatch, lock confirmation logic, and property lookup call signatures. |
| server/webdav/prop.go | Implements DB-backed dead property persistence and MOVE migration. |
| server/webdav/lock.go | Extends lock model and in-memory lock system to support shared locks. |
| server/webdav/file.go | Refines MOVE/COPY filesystem behavior (destination validation, overwrite semantics, named copy). |
| internal/model/webdav_property.go | Adds DB model for persisted dead properties. |
| internal/db/db.go | Auto-migrates the new WebDAVProperty table. |
| internal/fs/fs.go | Adds CopyTo API for named COPY operations. |
| internal/fs/other.go | Extends transfer task metadata with DstName. |
| internal/fs/copy_move.go | Threads dstName through transfer execution and stream naming to preserve destination name. |
Suppressed comments (1)
server/webdav/file.go:126
copyFilesverifies thatdstDirexists but does not verify it is a directory. IfdstDirexists as a file, COPY should fail with 409 (Conflict) rather than attempting a copy into a non-collection path.
if _, err = fs.Get(ctx, dstDir, &fs.GetArgs{}); err != nil {
if errs.IsObjectNotFound(err) {
return http.StatusConflict, err
}
return http.StatusMethodNotAllowed, err
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Lythen <intro-iu@outlook.com>
…D body Signed-off-by: Lythen <intro-iu@outlook.com>
PIKACHUIM
left a comment
There was a problem hiding this comment.
🙏 感谢贡献
感谢 @devLythen 提交此PR!我已完成代码评审,以下是评审结果。
📖 PR背景与需求
PR标题:fix(webdav): correct COPY/MOVE semantics, dead property persistence, and lock enforcement
需求说明:修复 WebDAV 服务器的多个关键问题:
- COPY 方法稳定返回 500 错误
- COPY/MOVE 状态码语义不符合 RFC 4918 规范
- PROPPATCH 设置的自定义属性(死属性)无法持久化
- 锁检查使用错误的路径(未解析用户路径)
- 不支持共享锁(shared locks)
预期目标:
- COPY/MOVE 操作正常工作,覆盖语义符合 RFC 4918
- 死属性可持久化到数据库,MOVE 时自动迁移
- 锁检查使用解析后的实际资源路径
- 支持共享锁机制
- 通过 Litmus WebDAV 测试套件验证
📋 问题摘要
- ✅ 功能性:修复了多个关键 bug,通过权威测试套件验证
⚠️ 数据库迁移:新增表但未提供迁移脚本⚠️ 代码复杂度:单个 PR 改动较大(11 个文件)- 💡 测试覆盖:通过 Litmus 全量测试(40/41 通过)
📂 逐文件分析
internal/model/webdav_property.go(新增文件)
改动意图:定义 WebDAV 死属性的数据模型。
代码修改逻辑:
- 新增
WebDAVProperty结构体,包含Path,Namespace,Name,Lang,InnerXML字段 - 使用复合索引
path,namespace,name确保属性唯一性
合理性评估:
-
✅ 优点:
- 字段设计合理,符合 WebDAV 属性规范
- 索引设计正确,避免重复属性
-
⚠️ 疑问:InnerXML存储为[]byte,是否需要考虑大小限制?某些客户端可能设置大型属性值。- 缺少
CreatedAt/UpdatedAt时间戳字段,不利于调试和审计。
internal/db/db.go
改动意图:在自动迁移中注册新的 WebDAVProperty 模型。
合理性评估:
- ✅ 优点:正确添加到 AutoMigrate 列表
⚠️ 问题:未提供显式的数据库迁移脚本,依赖 GORM AutoMigrate 可能在某些数据库引擎上产生问题
server/webdav/prop.go
改动意图:实现死属性的持久化、读取、移动逻辑。
代码修改逻辑:
- 重构
props(),propnames(),allprop()函数,增加name参数用于查询死属性 - 实现
patch()函数,使用数据库事务持久化 PROPPATCH 操作 - 新增
getDeadProps()从数据库读取属性 - 新增
moveDeadProps()在 MOVE 时迁移属性(包括子树) - 添加 40 次重试机制处理 SQLite "database is locked" 错误
合理性评估:
-
✅ 优点:
- 使用数据库事务保证原子性
- MOVE 时正确处理属性树迁移(使用 LIKE + ESCAPE)
- SQL 注入防护:使用参数化查询 + ESCAPE 转义通配符
-
⚠️ 疑问:- 40 次重试机制是否合理?
for attempt := 0; attempt < 40+time.Sleep(time.Duration(attempt+1) * 50 * time.Millisecond)最多等待 41 秒。这对 WebDAV 请求来说是否过长? - 错误处理不完整:
database := db.GetDb(); if database == nil { return nil, errors.New(...) }—— 数据库未初始化时返回错误,但调用方可能未正确处理此错误。 - moveDeadProps 中的 LIKE 转义:使用
\\%和\\_转义,但未转义反斜杠本身。如果路径中包含\,可能导致转义失效。
- 40 次重试机制是否合理?
详细建议:
-
优化重试策略:
for attempt := 0; attempt < 10; attempt++ { // 减少到 10 次 // ... if err == nil || !strings.Contains(err.Error(), "database is locked") { break } time.Sleep(time.Duration(attempt+1) * 20 * time.Millisecond) // 最多 2.2 秒 }
-
完善 LIKE 转义:
escapedSrc := strings.ReplaceAll( strings.ReplaceAll( strings.ReplaceAll(src, "\\", "\\\\"), "%", "\\%"), "_", "\\_")
server/webdav/lock.go
改动意图:支持共享锁,修复锁路径解析问题。
代码修改逻辑:
- 在
memLS.Create()中支持共享锁:如果已存在同名共享锁,生成新 token 并加入sharedTokensmap - 在
memLS.Unlock()中处理共享锁:删除单个 token,仅当所有 token 都释放时才删除锁节点 - 在
confirmLocks()中添加用户路径解析:将资源标签路径通过user.JoinPath()转换为实际路径
合理性评估:
-
✅ 优点:
- 共享锁实现符合 WebDAV 规范
- 路径解析修复了锁检查失效的问题
-
⚠️ 疑问:sharedTokens map[string]struct{}未初始化检查,如果在非共享锁上调用共享锁逻辑可能 panic- 用户路径解析仅在
confirmLocks()中添加,其他地方(如handleLock)是否也需要?
server/webdav/file.go
改动意图:修复 COPY/MOVE 语义,支持命名复制。
代码修改逻辑:
- 为
copyFiles()和moveFiles()添加dstName参数 - 修复 Destination 头部解析,支持绝对路径和相对路径
- 调用
fs.CopyTo()实现命名复制 - MOVE 完成后调用
moveDeadProps()迁移属性
合理性评估:
-
✅ 优点:
- 支持了 WebDAV 规范中的命名复制/移动
- Destination 解析更健壮
-
⚠️ 疑问:- 错误处理:如果
moveDeadProps()失败,MOVE 操作是否应该回滚?当前代码未处理此场景。
- 错误处理:如果
internal/fs/copy_move.go & internal/fs/fs.go
改动意图:支持命名复制,修复同目录复制回退。
代码修改逻辑:
transfer()函数增加dstName参数- 新增
fs.CopyTo()函数支持目标重命名 - 当同存储复制失败时,如果
dstName != ""则在完成后重命名 - 修复同目录复制时缺少
MakeDir调用的问题
合理性评估:
-
✅ 优点:
- 补全了命名复制功能
- 修复了同目录复制的 bug
-
⚠️ 疑问:op.Rename()的错误未处理,如果重命名失败会导致文件名不符合预期- 命名逻辑较复杂,建议添加单元测试
server/webdav/xml.go
改动意图:防御性解析 PROPFIND 请求体,拒绝恶意 XML。
代码修改逻辑:
- 限制请求体大小为 64KB,防止 DoS
- 检测空命名空间前缀(
xmlns:=""),拒绝非法声明 - 使用
hasEmptyNamespacePrefix()函数扫描 XML 字节流
合理性评估:
-
✅ 优点:
- 防止了 DoS 攻击
- 拒绝了某些客户端发送的非法 XML
-
⚠️ 疑问:- 手写 XML 解析器:
hasEmptyNamespacePrefix()使用字节扫描而非 XML 解析器,可能存在边界情况(如注释、CDATA 中的xmlns:)。建议使用标准 XML 解析器检测。
- 手写 XML 解析器:
🎯 总体评价
功能性:⭐⭐⭐⭐ - 修复了多个关键 bug,通过 Litmus 测试验证,但单个 PR 改动过大
安全性:⭐⭐⭐⭐ - 大部分安全问题处理得当,LIKE 转义有小瑕疵
代码质量:⭐⭐⭐ - 实现正确但部分逻辑较复杂,缺少单元测试
实现方案:⭐⭐⭐⭐ - 实现符合 WebDAV 规范,但数据库迁移和错误处理可优化
建议操作:🔄 Request Changes(建议修改后合并)
理由:
- ✅ 功能完整性高:通过 Litmus 40/41 测试,解决了实际问题
⚠️ 关键问题:- 缺少数据库迁移脚本(P1)
- LIKE 转义不完整,可能存在注入风险(P1)
- 错误处理不完整,可能导致数据不一致(P2)
- 💡 改进建议:
- 拆分 PR:将死属性持久化、锁修复、COPY/MOVE 修复分成 3 个独立 PR
- 添加集成测试覆盖关键路径
🔧 建议修改清单
P1(必须修改):
- 添加数据库迁移脚本到
internal/bootstrap/patch/目录 - 修复
moveDeadProps()中的 LIKE 转义,增加反斜杠转义 - 在
handleCopy/handleMove中处理moveDeadProps()失败的情况
P2(建议修改):
- 减少重试次数和等待时间(40 次 → 10 次)
- 优化
hasEmptyNamespacePrefix()使用标准 XML 解析器 - 为
WebDAVProperty表添加时间戳字段
📊 测试结果分析
Litmus 测试结果(40/41 通过):
- basic: 16/16 ✅
- copymove: 13/13 ✅(修复前 9/13)
- props: 30/30 ✅(修复前 9/14)
- locks: 40/41
⚠️ (修复前 24/34) - http: 3/3 ✅
唯一失败测试:locks.fail_complex_cond_put —— PR 描述中已说明这是上游 golang.org/x/net/webdav 的已知限制(不支持 ETag + 锁令牌组合条件),主流客户端不使用此特性。
💬 沟通建议
感谢你通过 Litmus 全量测试验证了修复效果!这种严谨的测试方法非常值得肯定。
建议:
- 补充数据库迁移脚本后可以合并
- 考虑将此大 PR 拆分为独立的功能 PR,便于审查和回滚
- 如果
locks.fail_complex_cond_put未来需要支持,可以向上游提交 patch
再次感谢你的细致工作!👏
Summary / 摘要
修复 WebDAV
COPY方法稳定返回500 Internal Server Error的问题,同时修复了 COPY/MOVE 状态码语义、死属性持久化、锁路径解析和共享锁支持。COPY 同目录/跨目录、绝对/相对 Destination 均正常工作,覆盖语义符合 RFC 4918
PROPPATCH设置的自定义属性可持久化,PROPFIND可检索,MOVE时自动迁移锁检查使用用户路径解析后的实际资源路径,支持共享锁
拒绝含非法 XML 声明的 PROPFIND 请求体
This PR has breaking changes.
/ 此 PR 包含破坏性变更。
This PR changes public API, config, storage format, or migration behavior.
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
This PR requires corresponding changes in related repositories.
/ 此 PR 需要关联仓库同步修改。
Related repository PRs / 关联仓库 PR:
Related Issues / 关联 Issue
Testing / 测试
go test ./server/webdav ./internal/fs其中
locks.fail_complex_cond_put未通过Checklist / 检查清单
/ 我已阅读 CONTRIBUTING。
/ 我确认此贡献符合仓库许可证、贡献规范和行为准则。
gofmt,go fmt, orprettierwhere applicable./ 我已按适用情况使用
gofmt、go fmt或prettier格式化变更代码。/ 我已在适用情况下请求相关维护者或代码所有者审查。