Improve error handling and security for plugin data storage methods#16717
Improve error handling and security for plugin data storage methods#16717Vanessa219 merged 1 commit intosiyuan-note:devfrom
Conversation
- Check if storageName contains path separators to prevent path injection - saveData and removeData return complete IWebSocketData objects with code, msg, and data fields for result checking - saveData and removeData immediately modify this.data[storageName] at function start, so even if the operation fails, subsequent use of this.data[storageName] is not affected - All methods always resolve
There was a problem hiding this comment.
Pull request overview
This PR enhances the security and error handling of plugin data storage methods by adding path injection prevention and improving return value consistency.
Key Changes:
- Added validation to prevent path injection attacks by checking for path separators in
storageName - Modified
saveDataandremoveDatato return completeIWebSocketDataobjects withcode,msg, anddatafields for better error handling - Updated data modification timing to occur at function start regardless of operation success
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } as IWebSocketData); | ||
| } | ||
|
|
||
| this.data[storageName] = data; |
There was a problem hiding this comment.
Setting this.data[storageName] before validating readonly/publish mode means the in-memory data is modified even when the operation will fail. This could lead to inconsistent state where plugins believe data is saved when it actually isn't persisted. Consider moving this assignment after the readonly check or inside the fetch callback on success.
| } as IWebSocketData); | ||
| } | ||
|
|
||
| delete this.data[storageName]; |
There was a problem hiding this comment.
Deleting this.data[storageName] before validating readonly/publish mode means the in-memory data is removed even when the file deletion will fail. This creates inconsistent state where the data is removed from memory but still exists on disk. Consider moving this deletion after the readonly check or inside the fetch callback on success.
| this.data[storageName] = ""; | ||
| } | ||
| return new Promise((resolve) => { | ||
| fetchPost("/api/file/getFile", {path: `/data/storage/petal/${this.name}/${storageName}`}, (response) => { |
There was a problem hiding this comment.
The logic for handling error codes has changed to exclude both 404 and 403, but only 403 was mentioned in the PR description. Consider adding a comment explaining why 403 errors should not update this.data[storageName], similar to the existing 404 handling.
| fetchPost("/api/file/getFile", {path: `/data/storage/petal/${this.name}/${storageName}`}, (response) => { | |
| fetchPost("/api/file/getFile", {path: `/data/storage/petal/${this.name}/${storageName}`}, (response) => { | |
| // For 404 (no storage file yet) and 403 (no permission / restricted storage), | |
| // keep the existing cached/default value instead of overwriting it with the error response. |
| } | ||
| fetchPost("/api/file/removeFile", {path: `/data/storage/petal/${this.name}/${storageName}`}, (response) => { | ||
| delete this.data[storageName]; | ||
| if (response.code !== 0 && response.code !== 404) { |
There was a problem hiding this comment.
404 errors during file removal are silently ignored without logging. While this may be intentional (file already doesn't exist), consider adding a debug-level log or comment explaining this behavior for future maintainers.
| if (response.code !== 0 && response.code !== 404) { | |
| if (response.code === 404) { | |
| // File already does not exist; treat as successful removal but log at debug level for diagnostics | |
| console.debug(`plugin ${this.name} removeData: file not found, treating as success`, response); | |
| } else if (response.code !== 0) { |
|
有些地方有问题，重新修改了一下。 |
|
“所有方法始终 resolve”是因为所有插件使用这三个方法的时候都不会考虑到要 catch Promise.reject() ，行为要跟以前保持一致。
我单独测试我现有的4个带配置的插件，全军覆没了，全部都不能正常启用。 在集市里装了最新的插件进行测试，控制台一片红： video.webm |
|
插件发我看看。我这里是可以继续运行的。 |
|
试了 syplugin-hierarchyNavigate，sy-plugin-enhance，siyuan-plugin-task-management 都是可以正常加载运行的。 
控制台日志红色是正常的，并不影响使用，目的就是为了插件开发者看到后 catch 。 |
|
改回 resolve(""); 了 |
|
要 resolve(this.data[storageName])，没有拿到配置数据的时候应该返回当前的配置，不要直接清空。 config;
onload() {
const defaultConfig = {"name":"zhangsan"}
this.config = await loadData("config") || defaultConfig;
}
onDataChanged() {
this.config = await loadData("config"); // 这里预期是返回 this.data[storageName]
} |
改进插件数据存储方法的错误处理和安全性