fix(loader): normalize archive separators before preserving filenames

CodeRabbit review on #217: a ZIP entry named with Windows separators
(e.g. "..\outside.png") survives filepath.Base on Unix and would be
written verbatim as a ZIP entry name, which Windows consumers may
interpret as path traversal. Add archiveBaseName helper that replaces
"\" with "/" before deriving the bare base name for OriginalName.
This commit is contained in:
Antoine Aflalo
2026-07-21 19:51:47 -04:00
parent 21f92bc6f5
commit 4cb5d10d3f
2 changed files with 82 additions and 1 deletions
+24 -1
View File
@@ -298,7 +298,7 @@ func ExtractChapter(ctx context.Context, filePath string, keepFilenames bool) (*
FilePath: outputPath,
}
if keepFilenames {
page.OriginalName = allocateUniqueBaseName(filepath.Base(path), pageIndex, usedOriginalNames)
page.OriginalName = allocateUniqueBaseName(archiveBaseName(path), pageIndex, usedOriginalNames)
}
chapter.Pages = append(chapter.Pages, page)
@@ -340,6 +340,29 @@ func isJunkFile(path string) bool {
return false
}
// archiveBaseName returns the bare base name of an archive entry, with
// Windows-style backslash separators normalized to forward slashes before
// the last-segment split. The archives library surfaces zip entry names
// verbatim, so a name like "..\evil.png" (common from Windows-created
// archives) would otherwise pass through filepath.Base unchanged on non-
// Windows hosts and be written into the output CBZ as a path-traversal
// shape. Normalizing the separators first and then taking the segment
// after the final one guarantees the returned name is a bare base name
// with no directory components, no backslashes, and no forward slashes —
// which is what the downstream writer expects as a safe ZIP entry name.
//
// This intentionally does NOT touch filepath.Base calls in isJunkFile or
// the ComicInfo.xml / converted.txt detection: those comparisons just
// fail to match, and the entry then falls through to the non-image
// extension filter, so no traversal-shaped data escapes the loader.
func archiveBaseName(path string) string {
normalized := strings.ReplaceAll(path, "\\", "/")
if idx := strings.LastIndex(normalized, "/"); idx >= 0 {
return normalized[idx+1:]
}
return normalized
}
// allocateUniqueBaseName returns baseName when it is not already taken by an
// earlier page in this chapter, or a collision-resolved variant otherwise.
//
+58
View File
@@ -281,6 +281,64 @@ func TestExtractChapter_KeepFilenames_DeduplicatesCollidingNames(t *testing.T) {
})
}
// writeBackslashCBZ builds a CBZ whose entry names mix Windows-style
// backslash separators with a normal forward-slash page. The fixture
// reproduces the CodeRabbit finding for PR #217: archive entries like
// "subdir\page.png" and "..\evil.png" must not surface verbatim as the
// OriginalName written into the output CBZ, since Windows ZIP consumers
// can interpret backslashes as path traversal.
func writeBackslashCBZ(t *testing.T, dir string) string {
t.Helper()
cbzPath := filepath.Join(dir, "backslash.cbz")
f, err := os.Create(cbzPath)
require.NoError(t, err)
w := zip.NewWriter(f)
// archive/zip stores entry names verbatim, so the bytes on the wire
// carry the backslashes through to fs.WalkDir in ExtractChapter.
for _, entry := range []string{`subdir\page.png`, `..\evil.png`, "cover.png"} {
fw, err := w.Create(entry)
require.NoError(t, err)
_, err = fw.Write([]byte("dummy image bytes for " + entry))
require.NoError(t, err)
}
require.NoError(t, w.Close())
require.NoError(t, f.Close())
return cbzPath
}
func TestExtractChapter_KeepFilenames_NormalizesWindowsSeparators(t *testing.T) {
tmpDir := t.TempDir()
cbzPath := writeBackslashCBZ(t, tmpDir)
chapter, err := ExtractChapter(context.Background(), cbzPath, true)
require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }()
require.Len(t, chapter.Pages, 3, "expected 3 pages: two backslash entries + one normal")
// Every OriginalName must be a bare base name: no backslashes, no
// forward slashes, and unique across the chapter.
seen := make(map[string]struct{}, len(chapter.Pages))
for _, page := range chapter.Pages {
assert.NotEmpty(t, page.OriginalName, "page %d must have OriginalName set", page.Index)
assert.NotContains(t, page.OriginalName, `\`, "page %d OriginalName must not contain backslash, got %q", page.Index, page.OriginalName)
assert.NotContains(t, page.OriginalName, "/", "page %d OriginalName must not contain forward slash, got %q", page.Index, page.OriginalName)
assert.Equal(t, filepath.Base(page.OriginalName), page.OriginalName,
"page %d OriginalName should be a bare base filename", page.Index)
if _, dup := seen[page.OriginalName]; dup {
t.Errorf("duplicate OriginalName across pages: %q", page.OriginalName)
}
seen[page.OriginalName] = struct{}{}
}
// The two backslash entries must collapse to their bare bases; the
// "..\evil.png" entry's leading "..\evil" must not be carried through.
assert.Contains(t, seen, "page.png", "subdir\\page.png should normalize to bare page.png")
assert.Contains(t, seen, "evil.png", "..\\evil.png should normalize to bare evil.png")
assert.Contains(t, seen, "cover.png", "cover.png should pass through unchanged")
}
func TestExtractChapter_Cleanup(t *testing.T) {
chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz", false)
require.NoError(t, err)