From 7813d5d169a26f5d9b1338d16c9f10f0df3c0a9b Mon Sep 17 00:00:00 2001 From: Mark Pitman Date: Tue, 13 Oct 2026 21:16:24 -0700 Subject: [PATCH] Harden local image reading Only files with an allowlisted image extension are opened, so a reference such as ../../.ssh/id_rsa is no longer embedded. Symlinks are resolved before the path check, so a link cannot reach outside the allowed area. The parent-directory check now uses filepath.Rel instead of a string prefix, which also removes the sibling-directory prefix match. Only regular files are read, and the size cap applies while reading, not only to the stat result. The 3-level parent allowance stays, because markdown repos often keep images in a sibling directory. Content sniffing for unknown extensions is gone: extensionless images are no longer embedded. Closes #66 --- CLAUDE.md | 2 +- main.go | 126 +++++++++++++++++++++++++-------------------------- main_test.go | 100 ++++++++++++++++++++++++++++++++++------ 3 files changed, 150 insertions(+), 78 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index bc93a3f..ffe104c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,7 +32,7 @@ Requires Go (see `go.mod`/`mise.toml` for version) and, for packaging, `just`, ` Everything happens in `main()` in `main.go`, in a straight-line pipeline: 1. **Flag parsing** — `-o`, `-v/-version`, `-h/-help`, `-b/-bare`. First positional arg is the input markdown file; a single `-` reads the markdown from stdin instead, in which case `baseDir` for image resolution is the working directory rather than the input file's directory. -2. **Image inlining** (`newMarkdown` → `imageTransformer` + `htmlImageRenderer` → `embedImage`/`imageToDataURI`) — rewrites relative image references into base64 `data:` URIs so the output HTML is fully self-contained/offline-viewable. Markdown images (inline and reference-style) are rewritten by an AST transformer, so images inside code spans and code blocks are left alone. Raw `` tags (`src` and `srcset`) are rewritten by a custom renderer for `HTMLBlock`/`RawHTML` nodes. `embedImage` strips query/fragment and decodes percent-encoding. `getMimeType` uses the extension, then content sniffing, and skips files that are not images. This includes path-traversal guards (caps `..` traversal depth) and a 10MB per-image size cap. +2. **Image inlining** (`newMarkdown` → `imageTransformer` + `htmlImageRenderer` → `embedImage`/`imageToDataURI`) — rewrites relative image references into base64 `data:` URIs so the output HTML is fully self-contained/offline-viewable. Markdown images (inline and reference-style) are rewritten by an AST transformer, so images inside code spans and code blocks are left alone. Raw `` tags (`src` and `srcset`) are rewritten by a custom renderer for `HTMLBlock`/`RawHTML` nodes. `embedImage` strips query/fragment and decodes percent-encoding. `imageToDataURI` only reads files with an allowlisted image extension (`getMimeType`), resolves symlinks before checking the path (at most 3 `..` levels above the base directory), accepts only regular files, and caps images at 10MB. 3. **Markdown parsing** via Goldmark (built by `newMarkdown`), configured with: - `extension.GFM` (tables, task lists, strikethrough, etc.) - `extension.Typographer` (smart quotes/dashes) diff --git a/main.go b/main.go index 65014b1..f089a79 100644 --- a/main.go +++ b/main.go @@ -13,7 +13,6 @@ import ( "io" "io/fs" "log" - "net/http" "net/url" "os" "path/filepath" @@ -373,88 +372,92 @@ func isRelativePath(path string) bool { return !filepath.IsAbs(path) } -// imageToDataURI reads an image file and converts it to a base64 data URI +// maxImageSize is the largest image file that is embedded +const maxImageSize = 10 * 1024 * 1024 // 10MB + +// maxParentLevels is how many directory levels above the base directory an image may be +const maxParentLevels = 3 + +// imageToDataURI reads an image file and converts it to a base64 data URI. +// It returns "" and logs a warning when the file must not or cannot be embedded. func imageToDataURI(imagePath string, baseDir string) string { - // Resolve the full path relative to the markdown file - fullPath := filepath.Join(baseDir, imagePath) - - // Clean and validate the path to prevent path traversal attacks - cleanedPath, err := filepath.Abs(fullPath) + // Check the extension first so that non-image files are never opened + mimeType := getMimeType(imagePath) + if mimeType == "" { + log.Printf("Warning: %s does not have a supported image extension, skipping", imagePath) + return "" + } + + // Resolve symlinks in both paths so that a link cannot point outside the allowed area + resolvedBase, err := filepath.EvalSymlinks(baseDir) if err != nil { - log.Printf("Warning: Invalid image path %s: %v", fullPath, err) + log.Printf("Warning: Invalid base directory %s: %v", baseDir, err) return "" } - - // Ensure the resolved path is within or relative to the base directory - cleanedBase, err := filepath.Abs(baseDir) + resolvedBase, err = filepath.Abs(resolvedBase) if err != nil { log.Printf("Warning: Invalid base directory %s: %v", baseDir, err) return "" } - - // Check if the cleaned path starts with the base directory or is a reasonable relative reference - // We allow accessing parent directories for flexibility with markdown repos - if !strings.HasPrefix(cleanedPath, cleanedBase) { - relPath, err := filepath.Rel(cleanedBase, cleanedPath) - if err != nil { - log.Printf("Warning: Unable to determine relative path for %s: %v", imagePath, err) - return "" - } - - // If the path goes outside the base directory, check parent traversal limits - if strings.HasPrefix(relPath, "..") { - // Allow up to 3 levels of parent directory traversal for flexibility - // Count the number of ".." path components - components := strings.Split(filepath.ToSlash(relPath), "/") - parentLevels := 0 - for _, component := range components { - if component == ".." { - parentLevels++ - } - } - if parentLevels > 3 { - log.Printf("Warning: Image path %s goes too many levels above base directory", imagePath) - return "" - } + resolvedPath, err := filepath.EvalSymlinks(filepath.Join(resolvedBase, imagePath)) + if err != nil { + log.Printf("Warning: Unable to resolve image file %s: %v", imagePath, err) + return "" + } + + // Allow images in the base directory, below it, or a few levels above it + // (for markdown repos that keep images in a sibling directory) + relPath, err := filepath.Rel(resolvedBase, resolvedPath) + if err != nil { + log.Printf("Warning: Unable to determine relative path for %s: %v", imagePath, err) + return "" + } + parentLevels := 0 + for _, component := range strings.Split(filepath.ToSlash(relPath), "/") { + if component != ".." { + break } + parentLevels++ } - - // Check file size before reading (limit to 10MB to prevent memory issues) - fileInfo, err := os.Stat(cleanedPath) + if parentLevels > maxParentLevels { + log.Printf("Warning: Image path %s goes too many levels above base directory", imagePath) + return "" + } + + // Read at most one byte past the limit, so a file that grows after the check is still capped + f, err := os.Open(resolvedPath) if err != nil { - log.Printf("Warning: Unable to stat image file %s: %v", cleanedPath, err) + log.Printf("Warning: Unable to read image file %s: %v", resolvedPath, err) return "" } - - const maxSize = 10 * 1024 * 1024 // 10MB - if fileInfo.Size() > maxSize { - log.Printf("Warning: Image file %s is too large (%d bytes, max %d bytes)", cleanedPath, fileInfo.Size(), maxSize) + defer f.Close() + + fileInfo, err := f.Stat() + if err != nil { + log.Printf("Warning: Unable to stat image file %s: %v", resolvedPath, err) return "" } - - // Read the image file - data, err := os.ReadFile(cleanedPath) + if !fileInfo.Mode().IsRegular() { + log.Printf("Warning: Image file %s is not a regular file", resolvedPath) + return "" + } + + data, err := io.ReadAll(io.LimitReader(f, maxImageSize+1)) if err != nil { - log.Printf("Warning: Unable to read image file %s: %v", cleanedPath, err) + log.Printf("Warning: Unable to read image file %s: %v", resolvedPath, err) return "" } - - mimeType := getMimeType(cleanedPath, data) - if mimeType == "" { - log.Printf("Warning: File %s is not a recognized image type, skipping", cleanedPath) + if len(data) > maxImageSize { + log.Printf("Warning: Image file %s is too large (max %d bytes)", resolvedPath, maxImageSize) return "" } - // Encode to base64 - encoded := base64.StdEncoding.EncodeToString(data) - - // Return data URI - return fmt.Sprintf("data:%s;base64,%s", mimeType, encoded) + return fmt.Sprintf("data:%s;base64,%s", mimeType, base64.StdEncoding.EncodeToString(data)) } -// getMimeType returns the image MIME type for a file, or "" when it is not an image. -// It trusts the file extension first, then sniffs the content. -func getMimeType(path string, data []byte) string { +// getMimeType returns the MIME type for a supported image extension, or "" for any other file. +// The extension allowlist keeps non-image files (keys, configs) out of the output. +func getMimeType(path string) string { switch strings.ToLower(filepath.Ext(path)) { case ".jpg", ".jpeg": return "image/jpeg" @@ -475,9 +478,6 @@ func getMimeType(path string, data []byte) string { case ".ico": return "image/x-icon" } - if detected := http.DetectContentType(data); strings.HasPrefix(detected, "image/") { - return detected - } return "" } diff --git a/main_test.go b/main_test.go index 22c8eaf..4228e7c 100644 --- a/main_test.go +++ b/main_test.go @@ -115,28 +115,100 @@ func TestNonEmbeddableImagesAreLeftAlone(t *testing.T) { } func TestGetMimeType(t *testing.T) { - png := []byte("\x89PNG\r\n\x1a\n") + tests := map[string]string{ + "a.PNG": "image/png", + "a.avif": "image/avif", + "a.tif": "image/tiff", + "a.tiff": "image/tiff", + "a.svg": "image/svg+xml", + "noext": "", + "a.dat": "", + "a.txt": "", + "id_rsa": "", + } + for path, want := range tests { + if got := getMimeType(path); got != want { + t.Errorf("getMimeType(%q) = %q, want %q", path, got, want) + } + } +} + +func TestImageToDataURIPathRules(t *testing.T) { + root := t.TempDir() + base := filepath.Join(root, "a", "b", "c", "d", "docs") + write := func(path string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("image data"), 0644); err != nil { + t.Fatal(err) + } + } + if err := os.MkdirAll(base, 0755); err != nil { + t.Fatal(err) + } + write(filepath.Join(base, "in.png")) + write(filepath.Join(base, "sub", "in.png")) + write(filepath.Join(base, "..", "up1.png")) + write(filepath.Join(base, "..", "..", "..", "up3.png")) + write(filepath.Join(base, "..", "..", "..", "..", "up4.png")) + write(filepath.Join(base+"-secret", "x.png")) // sibling that shares the name prefix + write(filepath.Join(base, "key.pem")) + write(filepath.Join(root, "outside", "target.png")) + + if err := os.Symlink(filepath.Join(root, "outside", "target.png"), filepath.Join(base, "link.png")); err != nil { + t.Skip("symlinks not supported:", err) + } + if err := os.Symlink(filepath.Join(base, "in.png"), filepath.Join(base, "inlink.png")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(base, "key.pem"), filepath.Join(base, "disguised.png")); err != nil { + t.Fatal(err) + } + tests := []struct { - path string - data []byte - want string + path string + embed bool }{ - {"a.PNG", nil, "image/png"}, - {"a.avif", nil, "image/avif"}, - {"a.tif", nil, "image/tiff"}, - {"a.tiff", nil, "image/tiff"}, - {"a.svg", nil, "image/svg+xml"}, - {"noext", png, "image/png"}, - {"a.dat", png, "image/png"}, - {"a.txt", []byte("hello"), ""}, + {"in.png", true}, + {"sub/in.png", true}, + {"../up1.png", true}, + {"../../../up3.png", true}, + {"../../../../up4.png", false}, + {"../docs-secret/x.png", true}, // one level up is allowed + {"key.pem", false}, + {"missing.png", false}, + {"inlink.png", true}, + {"link.png", false}, // symlink to a file outside the allowed area + {"sub", false}, // directory } for _, tt := range tests { - if got := getMimeType(tt.path, tt.data); got != tt.want { - t.Errorf("getMimeType(%q) = %q, want %q", tt.path, got, tt.want) + got := imageToDataURI(tt.path, base) != "" + if got != tt.embed { + t.Errorf("imageToDataURI(%q) embedded = %v, want %v", tt.path, got, tt.embed) } } } +func TestImageToDataURISizeLimit(t *testing.T) { + base := t.TempDir() + atLimit := filepath.Join(base, "limit.png") + tooBig := filepath.Join(base, "big.png") + if err := os.WriteFile(atLimit, make([]byte, maxImageSize), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(tooBig, make([]byte, maxImageSize+1), 0644); err != nil { + t.Fatal(err) + } + if imageToDataURI("limit.png", base) == "" { + t.Error("file at the size limit was not embedded") + } + if imageToDataURI("big.png", base) != "" { + t.Error("file over the size limit was embedded") + } +} + func TestTitleIsEscaped(t *testing.T) { tests := []struct { name string