fix(security): contain path traversal, classify driver errors, stop leaking paths over HTTP

[juicycleff]

Three things, in the order they depend on each other. The filesystem drivers now contain path traversal. Every driver classifies its failures through one shared set of sentinels. The HTTP surface reports both correctly, without handing server internals to whoever asked.

Path traversal

localdriver had a safeJoin guard already. Only Put called it. Get, Head, Delete, Copy (both sides), List, CreateBucket and DeleteBucket each built their own path with a bare filepath.Join(rootDir, bucket, key), so a key of ../../etc/passwd reached the filesystem on every one of them. You could read it back with Get and remove it with Delete. sftpdriver was worse, because objectPath joined the base path, the bucket and the key with a plain path.Join and returned whatever came out, so every one of these traversals applied just as well to whatever remote server you pointed it at.

Two subtler problems sat in the same code.

Containment was checked once, against the root, after every segment had already been joined. That let a key spend the bucket segment on its way out and still land inside the root, so Put("tenant-a", "../tenant-b/secret.txt") wrote into a different bucket and passed the check. Buckets are the tenancy boundary here, so root-only containment was simply the wrong invariant. It is now enforced per segment, against the directory the preceding segments produced.

Then there's DeleteBucket("."). It handed the whole storage root to os.RemoveAll, because . resolves to rootDir itself and the facade rejects an empty bucket name but says nothing at all about a single dot. A segment that resolves to its own parent is refused now, which also stops Delete(bucket, ".") removing a bucket directory through the object path.

Rooted segments get refused now instead of quietly reinterpreted. filepath.Join treats /etc/passwd as relative and would store that object at <root>/<bucket>/etc/passwd, handing you back an object at a key you never asked for. Leading separators, backslashes and volume names are rejected on every platform, because filepath.IsAbs answers false for /etc/passwd when you build for Windows.

Error classification

The canonical sentinels now live in driver/, so an out-of-tree driver in its own module can wrap them without importing the root package: ErrNotFound, ErrObjectNotFound, ErrBucketNotFound, ErrBucketExists, ErrPermissionDenied, ErrQuotaExceeded. The two specific not-found sentinels unwrap to the general one, so you match ErrNotFound when any missing resource is handled the same way and the specific one when the distinction changes what you do.

trove.Permanent(err) gives every consumer the same retry taxonomy instead of each hand-rolling a switch. Unclassified errors count as transient on purpose. The cost is lopsided: retrying something permanent wastes a backoff budget, while dead-lettering something transient throws away work that would have succeeded.

One more. azuredriver used to classify deletes by matching "BlobNotFound" and "404" against the rendered message, which breaks the moment the SDK rewords anything. It reads the typed service error code now.

HTTP status codes, and what reaches the client

With the guards in place, every traversal rejection arrived at extension/handler unclassified and turned into a 500 carrying the raw driver message. Two separate defects fell out of that.

Malformed keys answered the wrong status. A percent-encoded traversal returned 500 on PUT, DELETE and list, and 404 on GET and HEAD. The encoding is the interesting part: net/http's mux cleans a literal ../ out of the path and answers 307, while %2e%2e%2f gets matched escaped and only then unescaped into the path value, so it reaches the driver intact. The tests use that form. A literal ../ would have tested nothing.

New driver.ErrInvalidPath, wrapped from both guards, maps to 400. It deliberately does not unwrap to ErrNotFound. Nothing was looked up, so the request is malformed rather than the resource missing, and answering 404 would both misdescribe the failure and let a client use rejected keys to probe which objects exist. GET and HEAD also used to report every failure as 404, which made a backend outage indistinguishable from a missing object.

The second defect: raw error text went straight into 5xx bodies. Driver errors wrap os-level ones, so a failed write answered with this.

{"error":"localdriver: create dir: mkdir /var/folders/.../001/data/nested: permission denied"}

That's real test output, not a hypothetical. It hands the absolute storage root and the directory layout underneath it to anyone who can provoke a failure, which on a public endpoint is anyone at all.

The rule now is that a classified error's message gets published and an unclassified one gets logged and replaced.

400 {"error":"localdriver: path segment \"../../etc/passwd\" escapes its parent directory"}
404 {"error":"localdriver: object \"a\" not found in bucket \"b\""}
500 {"error":"internal error"}      logged with op + request path

So classifying an error is now also a claim that its message is safe to publish. The driver guards already held up their end, naming the caller's own segment but never the directory it resolved against, and the driver contract documents the obligation. If a condition cannot be described without server-side detail, leave it unclassified and let it be an opaque 500.

This covers the object, bucket and CAS routes. DELETE /buckets/%2e returning 500 was the case that would have handed os.RemoveAll the storage root before the guards existed.

Testing

The traversal suites run every operation against traversal keys and bucket names, and assert the rejection alongside proof that a planted out-of-root file and a sibling bucket's object are byte-identical afterwards. An error on its own does not prove containment. Control cases run throughout, so a guard that rejected everything would fail rather than pass.

extension/handler/errors_test.go covers the status table across all routes, reproduces the absolute-path disclosure against a real read-only directory, and pins the message-preservation behaviour so the leak fix cannot be satisfied by blanking every message.

Two source-level assertions guard the wiring, since the SFTP operations need a live server and the handler failure paths mostly need a broken backend, so nothing else here would notice a new call site that skipped the guard. One checks that no remote path gets built with a bare path.Join. The other checks that no handler writes raw error text into a response. That second one is what caught cas.go.

The last commit adds a Test step to the Extension CI job. That job only ran go build, so the handler tests above would have been compiled in CI and then thrown away. They pass as-is. Nothing was hiding behind the gap.

Worth knowing before you review

ErrInvalidPath is deliberately not part of trovetest.RunDriverSuite. S3, GCS and Azure keys are opaque strings where ../ is ordinary text, so requiring it there would fail drivers that are behaving correctly. Conformance coverage for it needs a separate opt-in suite for drivers whose keys address a real hierarchical namespace.

DELETE of a missing object still answers 204, matching S3's idempotent semantics. It is pinned in the status table with a comment so it does not read as an oversight.

upload.go has no error paths yet. Its handlers return canned JSON without calling into Trove, so the source-level guard passes it trivially today and will start failing it once real driver calls land there.

The gosec G703 suppression on localdriver's os.Rename is documented inline. The path is containment-checked before it reaches the sink, but gosec cannot recognise a filepath.Rel check as a sanitizer. Silencing it structurally would mean laundering the path through something the analyzer cannot follow, which would hide real traversal bugs there later.