Conversation
…with reasons Semgrep's go.lang.security.filepath-clean-misuse rule fires on every filepath.Clean / path.Clean call on user-controlled input regardless of surrounding defence. Two sites in this repo trip the rule but are safe by construction; rather than rewrite working code to placate a single-statement linter, suppress the rule inline with the reasoning captured next to the call. internal/api/notes_handlers.go (tar import): Clean here normalises hdr.Name; the traversal defence is the explicit reject of "/" prefix + ".." substring on the next line, plus an absolute-path containment check after filepath.Join into notesDir. All three layers must fail for a traversal to land — Semgrep sees only the first. internal/api/router.go (SPA file server): Clean normalises a URL path for SPA-vs-asset classification, not security. The handler reads only from `assets fs.FS`, which is fs.Sub(embed.FS, "dist"). io/fs rejects any path containing ".." via fs.ValidPath, and embed.FS holds only compile-time files — path traversal cannot reach the host filesystem regardless of what cleanPath becomes. No behaviour change. Pre-existing one-line comments at the same sites are folded into the new explanatory blocks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Semgrep's
go.lang.security.filepath-clean-misuserule fires on everyfilepath.Clean/path.Cleancall on user-controlled input regardless of surrounding defence. Two sites in this repo trip the rule but are safe by construction; this PR adds inline// nosemgrep:annotations with the reasoning captured next to the call rather than rewriting working code to placate a single-statement linter.Findings handled
internal/api/notes_handlers.go:488(tar import)Cleanhere normaliseshdr.Name; it is not the security boundary. The actual defence is three layers:/prefix or..substring (immediately after the Clean)filepath.Join(notesDir, ...)thenfilepath.Abscontainment check (absDestmust start withabsBase + os.PathSeparator)MaxNoteBytesso a malicious entry can't cause a write blow-up eitherAll three must fail simultaneously for a traversal to land — Semgrep sees only the first statement.
internal/api/router.go:237(SPA file server)Cleanhere normalises a URL path for SPA-vs-asset classification. The handler reads only fromassets fs.FS, which isfs.Sub(embed.FS, "dist")(seeui/embed.go). Two architectural guarantees apply:io/fsrejects any path containing..viafs.ValidPathbefore it reachesembed.FSembed.FSonly contains files baked in at compile time — there is no host-filesystem reachability regardless of whatcleanPathends upPath traversal is not possible here.
Why suppress instead of "fix"
Semgrep's autofix in both cases is
filepath.FromSlash(path.Clean(...)), which doesn't change the threat model — it just shuts the linter up. The recommendedcyphar/filepath-securejoinwould add a dependency and replace working multi-layer defence with a single library call that is no stronger here. The right answer is to keep the existing defence and document why the rule doesn't apply, which is what this PR does.Test plan
go vet -tags sqlite_fts5 ./internal/api/...cleango build -tags sqlite_fts5 ./internal/api/...cleanCompatibility
Drop-in. Pure documentation/comment changes — no behaviour change. Two pre-existing one-line comments at the same sites are folded into the new explanatory blocks; nothing else moves.
🤖 Generated with Claude Code