Found via a different technique than the rest of this review round:
tracing the pattern of a very recently fixed advisory
(GHSA-7hm9-v7vf-7g4w, "Validate attribute view IDs before file access",
merged commit 0f5a0e7, 2026-07-20) to check whether every caller of the
function it protects received the same fix. One did not.
Summary
RemoveUnusedAttributeView(id string) (kernel/model/attribute_view.go:50)
builds a filesystem path directly from its id parameter via
filepath.Join(base, id+".json"), with no validation that id matches
SiYuan's node-ID format. If id contains path traversal sequences, the
resulting path can point outside the intended data/storage/av
directory to any file the kernel process can read. The function then
copies that file into SiYuan's own history directory before deleting the
original, giving arbitrary file read and arbitrary file deletion from a
single unvalidated string. The HTTP API handler for this exact
operation, kernel/api/av.go's removeUnusedAttributeView, was
correctly hardened for this in the recent fix for GHSA-7hm9-v7vf-7g4w,
which added ast.IsNodeIDPattern()/util.InvalidIDPattern() checks
throughout the kernel/av package and its HTTP callers. However, the
MCP tool that calls the same underlying function,
kernel/mcp/tools/database.go's database_clean, was not updated and
performs only an empty-string check before calling
RemoveUnusedAttributeView(id) directly. This is CWE-22 (Path
Traversal), specifically a fix that was applied to one caller of a
shared function but not to a sibling caller of the same function.
Details
Vulnerable function, unchanged by the recent fix,
kernel/model/attribute_view.go:50-77:
func RemoveUnusedAttributeView(id string) {
base := filepath.Join(util.DataDir, "storage", "av")
absPath := filepath.Join(base, id+".json")
if !filelock.IsExist(absPath) {
return
}
historyDir, err := getHistoryDir(HistoryOpClean)
...
newP := strings.TrimPrefix(absPath, util.DataDir)
historyPath := filepath.Join(historyDir, newP)
if filelock.IsExist(absPath) {
if err = filelock.Copy(absPath, historyPath); err != nil {
return
}
}
if err = filelock.RemoveWithoutFatal(absPath); err != nil {
...
}
...
}
No check that id matches ast.IsNodeIDPattern() exists anywhere in
this function.
Correctly-fixed HTTP caller, kernel/api/av.go:34-47:
func removeUnusedAttributeView(c *gin.Context) {
...
avID := arg["id"].(string)
if util.InvalidIDPattern(avID, ret) {
return
}
model.RemoveUnusedAttributeView(avID)
...
}
Unfixed MCP tool caller, kernel/mcp/tools/database.go:316-323:
func databaseClean(args map[string]any) (CallToolResult, error) {
id, _ := args["id"].(string)
if id != "" {
model.RemoveUnusedAttributeView(id)
return CallToolResult{Content: []ContentItem{{Type: "text", Text: "unused database cleaned: " + id}}}, nil
}
removed := model.RemoveUnusedAttributeViews()
return CallToolResult{Content: []ContentItem{{Type: "text", Text: fmt.Sprintf("%d unused database(s) cleaned", len(removed))}}}, nil
}
The only check is id != "". No ID-format validation, no path
containment check.
The other call sites of filepath.Join(..., id+".json")-style patterns
in the same file (RemoveUnusedAttributeViews(), the bulk-clean
variant, and UnusedAttributeViews()) are not affected: their id
values are always drawn from getAllAvIDs()/unusedAv.Item, i.e.
internally computed by scanning the real contents of the
storage/av directory, never from an external caller's raw string.
RemoveUnusedAttributeView (singular) is the only instance that accepts
an externally-supplied id directly, which is exactly why the recent
security fix added validation to the two callers it covered, and exactly
why this third caller, reachable via MCP, is a real gap rather than a
theoretical one.
The /mcp endpoint itself requires authentication
(kernel/mcp/server.go:31, model.CheckAuth, model.CheckAdminRole, model.CheckReadonly), so this is not remotely exploitable by an
unauthenticated party. The risk is specifically the MCP threat model:
SiYuan's MCP feature is designed to let an AI agent act on the user's
workspace with tool calls like this one. An agent that is manipulated,
via prompt injection from untrusted content it processes, into calling
database_clean with a crafted id would carry out this file
read/delete with the full privileges of the authenticated session,
despite the tool's apparent scope being "clean up unused databases
within my notes," not "read or delete arbitrary files on the host."
This is the same class of confused-deputy risk that MCP tool
authorization boundaries exist to prevent, and it is a materially
different threat model than a human deliberately typing raw admin
commands, which is why the maintainers' own recent fix treated this
exact function as needing hardening in the first place.
PoC
// MCP tool call, using an already-authenticated MCP session:
{
"name": "database_clean",
"arguments": {
"id": "../../../../../../home/user/.ssh/id_rsa"
}
}
Trace through RemoveUnusedAttributeView:
absPath := filepath.Join(base, id+".json") resolves outside
data/storage/av to the target file (subject to how many ../
segments are needed to reach the target from that base directory).
filelock.IsExist(absPath) succeeds if the target file exists.
filelock.Copy(absPath, historyPath) copies the target file's full
contents into SiYuan's history directory, arbitrary file read.
filelock.RemoveWithoutFatal(absPath) deletes the original file,
arbitrary file deletion.
(Not run against a live compiled kernel, same sandbox limitation noted
throughout this review; the vulnerable code path, the already-fixed
sibling, and the unfixed MCP caller are all read directly from source at
commit eef1056/1673b75... i.e. current HEAD as of this review,
verified against the actual fix commit 0f5a0e7 for GHSA-7hm9-v7vf-7g4w
to confirm this specific function and caller were not part of that fix.)
Impact
Any MCP client connected to a SiYuan instance (most realistically, an AI
agent granted MCP tool access by the workspace owner for note-management
tasks) can be induced, directly or via a prompt-injection chain, to read
the contents of any file the kernel process can access into SiYuan's own
history (from which it becomes retrievable through the ordinary,
already-authenticated history API) and to delete that file from its
original location. This bypasses the workspace boundary the MCP tool
surface is meant to operate within, and does so via the exact function
the maintainers already identified as needing ID validation for this
reason, just through a caller their fix did not reach.
## Affected products
| Field | Value |
|---|---|
| Ecosystem | **Go** |
| Package name | `github.com/siyuan-note/siyuan/kernel` |
| Affected versions | Present at current HEAD (commit `eef1056`/`1673b75`, reviewed 2026-08-03), i.e. after the fix for GHSA-7hm9-v7vf-7g4w (commit `0f5a0e7`, 2026-07-20), confirming this specific gap postdates and was not covered by that fix. |
| Patched versions | *(none yet, leave blank until a fix is released)* |
## Severity
| Field | Value |
|---|---|
| Vector string | `CVSS:3.1/AV:N/AC:L/PR:H/UI:N/S:C/C:H/I:H/A:N` |
| Score | **7.2 (High)**. Network vector via the `/mcp` endpoint, low complexity, high privileges required (admin-level auth to reach `/mcp` at all, `PR:H`), no user interaction needed once an MCP session exists, scope change since the impact escapes the intended workspace-data boundary into the host filesystem, high confidentiality and integrity impact (arbitrary file read and delete), no availability impact beyond the deleted file itself. Maintainers may wish to weigh the prompt-injection/confused-deputy angle when assessing real-world severity, since it meaningfully lowers the practical bar below "the account owner deliberately attacks themselves." |
## Weaknesses (CWE)
- **CWE-22**: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (primary)
- **CWE-73**: External Control of File Name or Path
## Notes for filing
- This is a direct, narrowly-scoped gap in the fix for
GHSA-7hm9-v7vf-7g4w rather than an unrelated new bug class; worth
linking to that advisory when filing, and worth the maintainers
double-checking whether any other callers of functions touched by that
fix (or by the other recent "restrict X by publish access" fixes
visible in the git history) have similar sibling-caller gaps, since
this is now the second time in this codebase's history that a fix
applied to one caller of a shared function missed another.
- Suggested fix: add the same `ast.IsNodeIDPattern(id)` /
`util.InvalidIDPattern` check at the top of
`RemoveUnusedAttributeView` itself (defense-in-depth, protects every
current and future caller at once) rather than only at individual call
sites, which is what allowed this specific caller to be missed.
Found via a different technique than the rest of this review round:
tracing the pattern of a very recently fixed advisory
(GHSA-7hm9-v7vf-7g4w, "Validate attribute view IDs before file access",
merged commit
0f5a0e7, 2026-07-20) to check whether every caller of thefunction it protects received the same fix. One did not.
Summary
RemoveUnusedAttributeView(id string)(kernel/model/attribute_view.go:50)builds a filesystem path directly from its
idparameter viafilepath.Join(base, id+".json"), with no validation thatidmatchesSiYuan's node-ID format. If
idcontains path traversal sequences, theresulting path can point outside the intended
data/storage/avdirectory to any file the kernel process can read. The function then
copies that file into SiYuan's own history directory before deleting the
original, giving arbitrary file read and arbitrary file deletion from a
single unvalidated string. The HTTP API handler for this exact
operation,
kernel/api/av.go'sremoveUnusedAttributeView, wascorrectly hardened for this in the recent fix for GHSA-7hm9-v7vf-7g4w,
which added
ast.IsNodeIDPattern()/util.InvalidIDPattern()checksthroughout the
kernel/avpackage and its HTTP callers. However, theMCP tool that calls the same underlying function,
kernel/mcp/tools/database.go'sdatabase_clean, was not updated andperforms only an empty-string check before calling
RemoveUnusedAttributeView(id)directly. This is CWE-22 (PathTraversal), specifically a fix that was applied to one caller of a
shared function but not to a sibling caller of the same function.
Details
Vulnerable function, unchanged by the recent fix,
kernel/model/attribute_view.go:50-77:No check that
idmatchesast.IsNodeIDPattern()exists anywhere inthis function.
Correctly-fixed HTTP caller,
kernel/api/av.go:34-47:Unfixed MCP tool caller,
kernel/mcp/tools/database.go:316-323:The only check is
id != "". No ID-format validation, no pathcontainment check.
The other call sites of
filepath.Join(..., id+".json")-style patternsin the same file (
RemoveUnusedAttributeViews(), the bulk-cleanvariant, and
UnusedAttributeViews()) are not affected: theiridvalues are always drawn from
getAllAvIDs()/unusedAv.Item, i.e.internally computed by scanning the real contents of the
storage/avdirectory, never from an external caller's raw string.RemoveUnusedAttributeView(singular) is the only instance that acceptsan externally-supplied
iddirectly, which is exactly why the recentsecurity fix added validation to the two callers it covered, and exactly
why this third caller, reachable via MCP, is a real gap rather than a
theoretical one.
The
/mcpendpoint itself requires authentication(
kernel/mcp/server.go:31,model.CheckAuth, model.CheckAdminRole, model.CheckReadonly), so this is not remotely exploitable by anunauthenticated party. The risk is specifically the MCP threat model:
SiYuan's MCP feature is designed to let an AI agent act on the user's
workspace with tool calls like this one. An agent that is manipulated,
via prompt injection from untrusted content it processes, into calling
database_cleanwith a craftedidwould carry out this fileread/delete with the full privileges of the authenticated session,
despite the tool's apparent scope being "clean up unused databases
within my notes," not "read or delete arbitrary files on the host."
This is the same class of confused-deputy risk that MCP tool
authorization boundaries exist to prevent, and it is a materially
different threat model than a human deliberately typing raw admin
commands, which is why the maintainers' own recent fix treated this
exact function as needing hardening in the first place.
PoC
Trace through
RemoveUnusedAttributeView:absPath := filepath.Join(base, id+".json")resolves outsidedata/storage/avto the target file (subject to how many../segments are needed to reach the target from that base directory).
filelock.IsExist(absPath)succeeds if the target file exists.filelock.Copy(absPath, historyPath)copies the target file's fullcontents into SiYuan's history directory, arbitrary file read.
filelock.RemoveWithoutFatal(absPath)deletes the original file,arbitrary file deletion.
(Not run against a live compiled kernel, same sandbox limitation noted
throughout this review; the vulnerable code path, the already-fixed
sibling, and the unfixed MCP caller are all read directly from source at
commit
eef1056/1673b75... i.e. current HEAD as of this review,verified against the actual fix commit
0f5a0e7for GHSA-7hm9-v7vf-7g4wto confirm this specific function and caller were not part of that fix.)
Impact
Any MCP client connected to a SiYuan instance (most realistically, an AI
agent granted MCP tool access by the workspace owner for note-management
tasks) can be induced, directly or via a prompt-injection chain, to read
the contents of any file the kernel process can access into SiYuan's own
history (from which it becomes retrievable through the ordinary,
already-authenticated history API) and to delete that file from its
original location. This bypasses the workspace boundary the MCP tool
surface is meant to operate within, and does so via the exact function
the maintainers already identified as needing ID validation for this
reason, just through a caller their fix did not reach.