fix(snapshot): reject manifest path traversal - #107
Merged
Conversation
Import joined manifest Files and legacy File entries onto RootDir and opened the result. A git-shared or hand-edited manifest.json with ../ or an absolute path could read a gzip JSONL outside the snapshot and insert those rows. Resolve each path, require it stay under RootDir, and reject absolute and parent-directory entries. Export already rejects unsafe table names via tableShardDir. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 5, 2026, 2:23 PM ET / 18:23 UTC. ClawSweeper reviewWhat this changesValidate snapshot shard paths before full and incremental imports read them, preserving literal filenames and current-directory roots while documenting the existing symlink boundary. Merge readiness⛔ Blocked before merge - 1 item remains This remains a useful fix: current main and v0.14.8 still permit parent traversal during snapshot imports. The revised implementation resolves both previous findings, and no new blocking defect was found. Priority: P2 Review scores
Verification
How this fits togetherThe shared snapshot package imports compressed archive shards into downstream applications’ SQLite databases. Manifest paths select the files, and full and incremental imports share the same shard reader. flowchart TD
A[Snapshot manifest] --> B[Full or incremental import]
C[Caller-selected root] --> D[Lexical path validation]
B --> D
D -->|Unsafe path| E[Error and transaction rollback]
D -->|Local path| F[Read compressed shard]
F --> G[Import rows into SQLite]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep one shared lexical guard before shard reads, preserve valid legacy paths, and retain the explicit trusted-filesystem requirement. Do we have a high-confidence way to reproduce the issue? Yes: current main directly opens a joined ../ shard path, establishing a source-level reproduction with an outside gzip JSONL file. This read-only review did not execute it; the captured body reports a successful baseline reproduction. Is this the best way to solve the issue? Yes: validation in the shared reader protects both import APIs without duplicating their logic or changing the manifest format. The revised lexical check preserves legitimate paths and accurately documents its symlink limitation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 63101e688304. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Use Go's platform-aware lexical path guard without trimming the caller's root or shard names. Preserve current-directory imports and cover both public import APIs with intended-root and decoy-shard fixtures. Document the existing symlink trust boundary and prepare the complete v0.14.9 unreleased notes.
steipete
changed the title
Contributor
|
Maintainer proof for The original parent-directory traversal is reproducible through both public import APIs on main ( Validation on macOS arm64, Go 1.27.1:
The mandatory review scan initially stalled traversing the host's system temp directory; a fresh private task temp directory allowed the same scanner and isolated review to complete. Govulncheck reports no affected symbols or imported packages; its module-only advisory concerns the unused, unmaintained The standalone integration harness used Symlink following is unchanged from main and was exercised explicitly. This is a lexical path guard, not symlink-safe confinement; the README requires a trusted filesystem tree. Standalone synthetic integration programSave as package main
import (
"compress/gzip"
"context"
"encoding/json"
"fmt"
"os"
"path/filepath"
"github.com/openclaw/crawlkit/snapshot"
"github.com/openclaw/crawlkit/store"
)
func must(err error) { if err != nil { panic(err) } }
func shard(path, id string) {
must(os.MkdirAll(filepath.Dir(path), 0700))
f, err := os.Create(path); must(err)
gz := gzip.NewWriter(f)
must(json.NewEncoder(gz).Encode(map[string]any{"id":id}))
must(gz.Close()); must(f.Close())
}
func main() {
ctx := context.Background()
base, err := os.MkdirTemp("", "crawlkit-synthetic-proof-"); must(err); defer os.RemoveAll(base)
cwd, err := os.Getwd(); must(err); defer os.Chdir(cwd)
for _, mode := range []string{"full", "incremental"} {
for _, name := range []string{"nested", "dot", "dot-slash", "empty-root", "root-space", "shard-space", "parent", "absolute", "legacy-parent", "symlink"} {
root := filepath.Join(base, mode, name, "archive")
rel := "tables/things/000001.jsonl.gz"
if name == "root-space" { root += " " }
if name == "shard-space" { rel = " tables/things/000001.jsonl.gz " }
shard(filepath.Join(root, rel), "intended")
if name == "root-space" { shard(filepath.Join(filepath.Dir(root), "archive", rel), "foreign") }
if name == "shard-space" { shard(filepath.Join(root, "tables/things/000001.jsonl.gz"), "foreign") }
outside := filepath.Join(filepath.Dir(root), "outside.jsonl.gz")
shard(outside, "foreign")
if name == "parent" || name == "legacy-parent" { rel = "../outside.jsonl.gz" }
if name == "absolute" { rel = outside }
if name == "symlink" { rel = "linked.jsonl.gz"; must(os.Symlink(outside, filepath.Join(root, rel))) }
table := snapshot.TableManifest{Name:"things", Files:[]string{rel}, Columns:[]string{"id"}, Rows:1}
if name == "legacy-parent" { table.Files=nil; table.File=rel }
m := snapshot.Manifest{Version:1, Tables:[]snapshot.TableManifest{table}}
must(snapshot.WriteManifest(root,m))
must(os.Chdir(root))
if name == "dot" { root="." }; if name=="dot-slash" { root="./" }; if name=="empty-root" { root="" }
db, err := store.Open(ctx,store.Options{Path:":memory:", Schema:"create table things(id text primary key)"}); must(err)
_, err = db.DB().ExecContext(ctx,"insert into things values ('existing')"); must(err)
if mode == "full" { _, err=snapshot.Import(ctx,snapshot.ImportOptions{DB:db.DB(),RootDir:root}) } else { _,_,err=snapshot.ImportIncremental(ctx,snapshot.IncrementalImportOptions{DB:db.DB(),RootDir:root,Previous:snapshot.Manifest{Version:1},Current:m}) }
outcome := "accepted"; if err != nil { outcome="rejected" }
var rows string
must(db.DB().QueryRowContext(ctx,"select coalesce(group_concat(id, ','), '') from things").Scan(&rows))
fmt.Printf("%s %-14s %s rows=%s\n", mode,name,outcome,rows)
must(db.Close())
must(os.Chdir(cwd))
}
}
}The contributor's original commit is retained, and the changelog credits @SebTardif. Recommended squash title: |
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
2 participants
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.
What Problem This Solves
A snapshot manifest can list
../outside.jsonl.gzinFilesor legacyFile, causing full and incremental imports to read a gzip JSONL shard outside the caller-selected root and insert its rows into SQLite.What Changed
Validate manifest shard paths with Go's platform-aware
filepath.IsLocalbefore opening them. Preserve literal root and shard names, including whitespace and current-directory roots (.,./, and the existing empty-root behavior). This repairs the original patch's root/shard trimming and current-directory regressions while retaining its shared import guard and regression coverage.The check is lexical. Existing symlink following remains unchanged and is documented in the README; callers must supply a trusted filesystem tree. This PR does not claim symlink-safe confinement.
The unreleased notes credit @SebTardif and include all user-visible changes since v0.14.8, including the already-merged GitHub Script refresh.
Validation
snapshot.Importandsnapshot.ImportIncrementalcalls against temporary gzip JSONL fixtures and SQLite databases, on macOS arm64 with Go 1.27.1.foreignrow. The repaired code rejects parent, absolute, and legacy-parent paths, leaving the preexisting destination row intact.intendedrow for nested shards,.,./, empty roots, whitespace-bearing roots, and whitespace-bearing shard names; sibling/trimmed-name decoys never supply rows.GOMAXPROCS=2 make check,actionlint, independent Codex review, and exact-head CI are recorded in the proof comment.Contributor: Sebastien Tardif SebTardif@ncf.ca.