Potential fix for code scanning alert no. 1: Uncontrolled data used in path expression#2
Conversation
…n path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses code scanning alert #1 (“Uncontrolled data used in path expression”) in the webapp /file endpoint by strengthening path traversal protections when reading files under a skill directory.
Changes:
- Canonicalizes the skill root directory using
filepath.Abs+filepath.EvalSymlinks. - Canonicalizes the requested file path and verifies it resolves within the canonical root using
filepath.Relbefore callingos.ReadFile.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| abs, err := filepath.EvalSymlinks(candidateAbs) | ||
| if err != nil { | ||
| resp.Error = "invalid path" | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| _ = json.NewEncoder(w).Encode(resp) | ||
| return |
There was a problem hiding this comment.
filepath.EvalSymlinks(candidateAbs) will fail with os.IsNotExist for legitimate requests to files that simply don’t exist, causing a 400 "invalid path" instead of the prior 404 from os.ReadFile. If the intent is to preserve existing behavior, consider treating os.IsNotExist(err) differently (e.g., perform the within-root check on candidateAbs and return 404 / let ReadFile handle it), while still using EvalSymlinks + re-check for existing paths to block symlink escapes.
| rootAbs, err := filepath.Abs(skillDir) | ||
| if err != nil { | ||
| resp.Error = "invalid path" | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| _ = json.NewEncoder(w).Encode(resp) | ||
| return | ||
| } | ||
| rootEval, err := filepath.EvalSymlinks(rootAbs) | ||
| if err != nil { |
There was a problem hiding this comment.
rootAbs/rootEval are derived solely from skillDir (constant for the lifetime of this server) but are recomputed on every /file request. Consider resolving/validating the root once before registering the handler (and failing Serve early if resolution fails), then closing over the resolved root to reduce per-request overhead and simplify the handler.
| if err != nil { | ||
| resp.Error = "invalid path" | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| _ = json.NewEncoder(w).Encode(resp) | ||
| return | ||
| } |
There was a problem hiding this comment.
The handler repeats the same 5-line "set error + status + encode JSON + return" block for each failure case. Consider extracting a small helper (e.g., writeJSONError(w, status, msg)) to keep the path-validation logic readable and reduce the chance of inconsistent headers/status codes when this code is modified later.
Potential fix for https://github.com/inhere/skillc/security/code-scanning/1
Use a canonical “resolve-and-verify-within-root” check before
os.ReadFile:skillDirto an absolute, symlink-evaluated root (filepath.Abs+filepath.EvalSymlinks).filepath.Rel(reject ifrel == ".."or starts with".."+separator).This preserves existing functionality (serving files under the skill directory) while making the validation robust against symlink and normalization bypasses.
Edits are only in
internal/app/webapp/server.go, inside the/filehandler around current lines 97–108. No new imports are needed.Suggested fixes powered by Copilot Autofix. Review carefully before merging.