Add File Move Operation to PATCH Endpoint (only the relevant commits) #191
Conversation
- Add move operation to PATCH endpoint for files - Use Operation: move, Target-Type: file, Target: path - Automatically creates parent directories if needed - Preserves all internal links using FileManager.renameFile() - Validates paths and prevents overwrites - Add tests for edge cases
- Add new error codes for clearer validation messages - Use consistent returnCannedResponse pattern throughout - Reorder validation checks for better flow - Separate file operations from applyPatch operations early - Maintain upstream coding style and patterns
…ile move - Add path traversal protection to prevent access outside vault - Use returnCannedResponse consistently throughout handleMoveOperation - Add proper error codes to ErrorCode enum and ERROR_CODE_MESSAGES - Validate paths early to prevent directory path destinations - Check parent directory existence before creating - Add comprehensive security tests for path traversal attempts - Normalize paths to handle backslashes and multiple slashes
- Add move operation to PATCH endpoint enum - Update Target parameter description for move operations - Include example for file move functionality - Documents Operation: move, Target-Type: file usage
|
Forgive me, I might have misremembered my conversation, but I wonder if you could consider adding that support via an API extension instead? That funcionality is documented here: https://github.com/coddingtonbear/obsidian-local-rest-api/wiki/Adding-your-own-API-Routes-via-an-Extension ; maybe you ran into an obstacle when trying to go that direction? |
|
Note that I am a different person. :-D You discussed that with @duquesnay; I just took his work from PR #177 that I needed but was not being worked on (and that's fine, people have lives), so I figured I'd do the commit reshuffling you had requested. I didn't know about the API extensions; they might be a good way to dial in new operations, but maybe Move would be such an essential operation that it would belong in the API proper? |
|
I really appreciate you taking the time to do this! There’s an open discussion about adding a WebDAV-style MOVE method here, and that feels like a more natural direction to me right now. Part of the hesitation is that REST itself doesn’t really have a concept of MOVE — you can already achieve the same thing with a combination of GET, PUT, and DELETE. So while I do think it’s helpful to have a clean way to express “move,” I’m leaning toward the WebDAV-style approach over extending PATCH in this way. |
|
No worries! Using a custom verb would definitely work, and I agree that MOVE would be more logical endpoint than PATCH. Actually, PATCH probably isn't a great method for MOVE because the original resource gets lost in the operation. In the case of the Obsidian API, GET-PUT-DELETE is not a viable alternative to a Move operation because Move also handles internal links (and backlinks, I believe). Otherwise my LLM repo organizer would've been easier to implement on the file system level. |
|
You know, I'm not sure how it never occurred to me, but I honestly hadn't at all considered that one of the side-effects of having a dedicated way of moving a file would be re-writing links as appropriate. Still, definitely feeling like adding a In any case, would you forgive me if we moved this conversation over to #190 for the time being so both approaches can be considered side-by-side? |
I would. :-D |
|
I implemented a custom MOVE operation (with the kind assistance of Claude Code). I'll provide my opinion in Discussion #190. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a WebDAV-style MOVE operation for moving files within an Obsidian vault. The implementation adds a new HTTP MOVE method that accepts a destination path via a Destination header and handles various security and validation concerns.
Key Changes
- Added new error codes for MOVE operation validation (
MissingDestinationHeader,InvalidDestinationPath,PathTraversalNotAllowed,DestinationAlreadyExists,FileOperationFailed) - Implemented
_vaultMoveandvaultMovemethods to handle file moving with path traversal protection - Added route registration for the MOVE HTTP method using Express middleware
- Created comprehensive test coverage for the MOVE operation including edge cases
- Updated OpenAPI documentation to include the new MOVE endpoint
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Added 5 new ErrorCode enum values for MOVE operation validation |
| src/requestHandler.ts | Implemented MOVE operation handler with security checks and route registration; reorganized imports alphabetically |
| src/requestHandler.test.ts | Added comprehensive test suite for MOVE operation covering success and failure scenarios; reorganized imports |
| src/constants.ts | Added error messages for new MOVE-related error codes |
| docs/src/openapi.jsonnet | Updated import paths and added MOVE method to OpenAPI spec |
| docs/src/lib/move.jsonnet | Created new OpenAPI documentation for MOVE endpoint |
| docs/openapi.yaml | Updated generated OpenAPI YAML with MOVE documentation and comment improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MissingDestinationHeader = 40001, | ||
| InvalidDestinationPath = 40002, | ||
| PathTraversalNotAllowed = 40003, |
There was a problem hiding this comment.
Error codes are not in sequential order. The new error codes (40001-40003) are inserted between RequestMethodValidOnlyForFiles = 40510 and ErrorPreparingSimpleSearch = 50010, but they should be placed before the 40510 error code to maintain proper ascending order. This breaks the logical ordering of error codes in the enum.
| } | ||
|
|
||
| // Validate operations for applyPatch target types | ||
| if (!["append", "prepend", "replace"].includes(operation)) { |
There was a problem hiding this comment.
Missing null/undefined check for operation parameter before using it with includes(). If the Operation header is not provided, operation will be undefined, and calling includes() on the array will not properly validate this case. This can lead to operation being undefined when passed to line 536, which could cause runtime errors. Add a check like if (!operation || !['append', 'prepend', 'replace'].includes(operation)) to handle the missing header case properly.
| if (!["append", "prepend", "replace"].includes(operation)) { | |
| if (!operation || !["append", "prepend", "replace"].includes(operation)) { |
| try { | ||
| // Create parent directories if needed | ||
| const parentDir = newPath.substring(0, newPath.lastIndexOf('/')); | ||
| if (parentDir && !await this.app.vault.adapter.exists(parentDir)) { |
There was a problem hiding this comment.
The condition parentDir && !await is incorrect syntax and will cause parsing issues. The await keyword should come before the entire expression. This should be if (parentDir && !(await this.app.vault.adapter.exists(parentDir))) with proper parentheses around the awaited expression.
| if (parentDir && !await this.app.vault.adapter.exists(parentDir)) { | |
| if (parentDir && !(await this.app.vault.adapter.exists(parentDir))) { |
| } | ||
|
|
||
| // Normalize the new path | ||
| const newPath = rawNewPath.replace(/\\/g, '/').replace(/\/+/g, '/').replace(/^\/|\/$/g, ''); |
There was a problem hiding this comment.
Path normalization occurs after the path traversal check on line 701, but the normalization on line 717 removes leading slashes with /^\/|\/$/g. This means a path like //etc/passwd would pass the check on line 701 (doesn't start with single /) but then get normalized to etc/passwd on line 717. The path traversal validation should happen after normalization, not before, to prevent bypasses.
| ``` | ||
| ## Append Content Below a Heading | ||
| ## Append, Prepend, or Replace Content Below a Heading |
There was a problem hiding this comment.
[nitpick] While updating heading comments to include all operations (append, prepend, replace) provides better context, this change is inconsistent with the PR's stated purpose of adding MOVE functionality. These are documentation improvements to existing PATCH operation examples and should ideally be in a separate commit or PR focused on documentation updates.
|
Hey there @mairas -- it looks like you have some comments from Copilot to either fix or respond to before we can go forward on this one. Let me know if you have any questions or need a hand on those. Regarding the docs: I think using the |
coddingtonbear
left a comment
coddingtonbear
left a comment
There was a problem hiding this comment.
Just commented above re: the next steps on this and adding this "Request Changes" just so it shows up accurately on the PR list.
3 participants
First, a disclaimer: I needed the Move operation for my own vault scripting and began working on the topic a couple of weeks ago. I didn't notice that Adam closed PR #177 meanwhile based on discussion with Guillaume.
Basically, I've just done the commit-wrangling requested by Adam in PR #177; there are no functional changes to any of @duquesnay's code. Despite the resolution of the original PR, I decided to publish the PR nevertheless, mainly for discussion or as an alternative solution. No ill will or passive-aggressiveness intended. :-)