Only strip a tar's root directory when it has one (audit L-2) #144

Merged

qwc merged 1 commit from fix/tar-single-root into main

2026-08-30 20:42:38 +02:00

qwc commented

2026-08-30 20:17:24 +02:00

Owner

Audit item L-2, filed as a Low "mirror the zip logic" cleanup. It's a live data-loss bug, so it's worth reading as more than a tidy-up.

What was wrong

stripSingleRootTar removed the first path component from every tar entry, unconditionally:

// This is a simplified approach — strips first component if it looks like a single root
parts := strings.SplitN(name, "/", 2)

Zip and 7z don't do that — detectSingleRoot checks that every entry actually shares one root before stripping anything. Two consequences for .tar.gz / .tar.bz2 / .tar.xz uploads:

A tarball whose files sit at the top level lost a real directory. guide/intro.html was written as intro.html, assets/style.css as style.css, while index.html was untouched. Every link into those directories 404s.
Two top-level directories collapsed into one. api/index.html and guide/index.html both became index.html, so one silently overwrote the other. I wrote the test expecting the first symptom and it also caught this one — half the upload just disappears.

The fix

Tar is a stream, so it can't be inspected before extraction the way a zip's directory can, and reading it twice would mean buffering the whole archive. So the equivalent check moved to after extraction: write the real paths, then unwrap if exactly one top-level directory came out. Costs a handful of renames on a directory that's already on disk.

The root is staged into a temp directory before its children are moved up — without that, an archive shaped like docs/docs/index.html can't be unwrapped, since the child would be renamed onto its own parent. There's a test for that case.

Tests

internal/docs/tar_single_root_test.go builds real tarballs and checks the extracted tree: top-level layout preserved, a genuine single root still stripped (the behaviour worth keeping), two roots both surviving, and the root-sharing-its-child-name case.

The first and third fail on main:

--- FAIL: TestExtractTarKeepsTopLevelLayout
    expected guide/intro.html to be extracted with its directory intact, got ""
--- FAIL: TestExtractTarWithMultipleRoots
    expected both roots preserved, got map[index.html:<html>guide</html>]

Existing archive tests, including TestExtractTarGz (which covers the single-root case), pass unchanged. Full suite green.

Worth knowing

Any docs uploaded as a tarball that wasn't wrapped in a single directory are already extracted wrong on disk — this fixes new uploads, not old ones. Re-uploading those versions after deploying will lay them out correctly. Zip uploads were never affected.

Assisted by Claude Opus 5.

Audit item **L-2**, filed as a Low "mirror the zip logic" cleanup. It's a live data-loss bug, so it's worth reading as more than a tidy-up. ## What was wrong `stripSingleRootTar` removed the first path component from **every** tar entry, unconditionally: ```go // This is a simplified approach — strips first component if it looks like a single root parts := strings.SplitN(name, "/", 2) ``` Zip and 7z don't do that — `detectSingleRoot` checks that every entry actually shares one root before stripping anything. Two consequences for `.tar.gz` / `.tar.bz2` / `.tar.xz` uploads: - **A tarball whose files sit at the top level lost a real directory.** `guide/intro.html` was written as `intro.html`, `assets/style.css` as `style.css`, while `index.html` was untouched. Every link into those directories 404s. - **Two top-level directories collapsed into one.** `api/index.html` and `guide/index.html` both became `index.html`, so one silently overwrote the other. I wrote the test expecting the first symptom and it also caught this one — half the upload just disappears. ## The fix Tar is a stream, so it can't be inspected before extraction the way a zip's directory can, and reading it twice would mean buffering the whole archive. So the equivalent check moved to after extraction: write the real paths, then unwrap if exactly one top-level directory came out. Costs a handful of renames on a directory that's already on disk. The root is staged into a temp directory before its children are moved up — without that, an archive shaped like `docs/docs/index.html` can't be unwrapped, since the child would be renamed onto its own parent. There's a test for that case. ## Tests `internal/docs/tar_single_root_test.go` builds real tarballs and checks the extracted tree: top-level layout preserved, a genuine single root still stripped (the behaviour worth keeping), two roots both surviving, and the root-sharing-its-child-name case. The first and third fail on `main`: ``` --- FAIL: TestExtractTarKeepsTopLevelLayout expected guide/intro.html to be extracted with its directory intact, got "" --- FAIL: TestExtractTarWithMultipleRoots expected both roots preserved, got map[index.html:<html>guide</html>] ``` Existing archive tests, including `TestExtractTarGz` (which covers the single-root case), pass unchanged. Full suite green. ## Worth knowing Any docs uploaded as a tarball that wasn't wrapped in a single directory are **already extracted wrong on disk** — this fixes new uploads, not old ones. Re-uploading those versions after deploying will lay them out correctly. Zip uploads were never affected. --- Assisted by Claude Opus 5.

qwc added 1 commit

2026-08-30 20:17:24 +02:00

Only strip a tar's root directory when it has one (audit L-2)

CI / test (pull_request) Successful in 1m21s

Details

CI / build (pull_request) Successful in 46s

Details

CI / docker (pull_request) Has been skipped

Details

9567be2e7f

Tar extraction stripped the first path component off every entry,
unconditionally, while zip and 7z first check whether the archive really
has a single common root. Two ways that went wrong:

  - An archive whose files are already at the top level lost a real
    directory: guide/intro.html was written as intro.html, breaking
    every link to it.
  - An archive with two top-level directories had them collapse into
    one, so api/index.html and guide/index.html overwrote each other.
    Half the upload silently disappeared.

A tar is a stream and reading it twice would mean buffering the whole
archive, so the single-root check happens after extraction instead of
before it: extract the real paths, then unwrap if exactly one top-level
directory came out. The root is moved aside first so an archive like
docs/docs/ can still be unwrapped.

Assisted-by: Claude Opus 5

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marcel M. Otte <marcel.otte@mmo.to>

qwc referenced this pull request

2026-08-30 20:20:56 +02:00

Constrain access roles and sources at store entry (audit L-1, L-7) #145

qwc referenced this pull request

2026-08-30 20:24:02 +02:00

Stop putting raw errors on screen (audit L-4) #146

qwc referenced this pull request

2026-08-30 20:26:24 +02:00

Stream non-HTML responses past the overlay injector (audit L-5) #147

qwc merged commit 0d8ad2c154 into main

2026-08-30 20:42:38 +02:00

qwc deleted branch fix/tar-single-root