Soundcheck: security review findings

[github-actions]

Security Review

Findings

Severity	File:Line	Skill	Finding	Fix
High	Sources/Reader/EditorTextView.swift:290	unsafe-api-consumption	RTF clipboard bytes passed to NSAttributedString(rtf:documentAttributes:) with no size cap — a crafted or oversized RTF payload triggers unbounded memory allocation and main-thread stall inside the AppKit RTF parser.	Gate on data.count: reject RTF payloads above ~2 MB before calling the parser; fall back to plain-text path.
High	Sources/Reader/EditorTextView.swift:286	insecure-design	Plain-text clipboard paste has no size cap; the pasted string is immediately inserted and triggers a full-document restyle() on the main thread, enabling the ReDoS chain below.	Check clipboard string length before inserting; truncate or reject pastes above a threshold (~2 MB) with a user-visible warning.
High	Sources/Reader/MarkdownStyler.swift:379	redos	Bold rule compiled with .dotMatchesLineSeparators — the .+? group spans the entire document, causing catastrophic backtracking on unclosed ** markers. restyle() runs on every single keystroke with the full document range.	Remove .dotMatchesLineSeparators from inline-emphasis rules (bold/italic are single-line constructs); use possessive/atomic groups or switch to a linear-time parser.
High	Sources/Reader/MarkdownStyler.swift:23	insecure-design	No document-size guard and no debounce on restyle() — all regex rules run synchronously on the main thread on every character edit regardless of document size.	Add a size gate (e.g., skip or async-dispatch restyle for documents over 500 KB); debounce calls by ~150 ms.
High	Sources/Reader/MainWindowController.swift:193	path-traversal	String(contentsOf: url, encoding: .utf8) reads an entire file with no size limit; no sandbox. A symlink to /dev/zero, a huge sparse file, or /dev/stdin causes unbounded memory allocation or an indefinite main-thread block.	Check file size with FileManager.default.attributesOfItem before reading; reject above ~50 MB; resolve symlinks and validate the resolved path is not a device node.
High	appcast.xml:7	supply-chain	SUFeedURL in Info.plist points directly to https://raw.githubusercontent.com/thejefflarson/reader/main/appcast.xml. Any attacker gaining write access to the repository can push a malicious update entry; Sparkle's EdDSA check is the only barrier.	Host appcast.xml on separate, write-access-controlled infrastructure (dedicated GitHub Pages branch with branch protection, S3, or CDN); require PR review for any change to the feed.
High	scripts/release.sh:76	injection	The NOTES shell variable is interpolated directly into an XML CDATA section without escaping the ]]> terminator sequence. A notes string containing ]]> closes the CDATA block early and allows injection of arbitrary XML elements (e.g., a forged <enclosure>) into appcast.xml, which is then served to all users.	Escape ]]> in $NOTES before interpolation (replace with ]]]]><![CDATA[>), or validate and abort on detection; switch appcast assembly to a proper XML library.
Medium	Sources/Reader/MarkdownSerializer.swift:25	injection	wrap() embeds url.absoluteString unconditionally into [label](url) markdown — including file://, app://, x-apple-*, and other non-http schemes from RTF clipboard .link attributes. These are written to disk on save and appear in any future rendering context.	Apply the same scheme allowlist used in MarkdownStyler.safeURL() inside MarkdownSerializer.wrap() before emitting the URL; strip the link wrapper for disallowed schemes.
Medium	Sources/Reader/MarkdownStyler.swift:253	redos	setextHeading2 pattern (-{2,})\s*$ causes O(n²) backtracking on lines ending with many hyphens not followed by a newline (e.g., EOF). Runs against the full document on every keystroke.	Replace \s*$ with [ \t]*$ to eliminate newline ambiguity that drives backtracking; also apply a length cap to skip the rule on large documents.
Medium	Sources/Reader/MarkdownStyler.swift:351	redos	tableRow outer regex (?m)^(\s*|.+|)\s*$ uses greedy .+ between | anchors; produces quadratic backtracking on long lines with many pipe characters but no trailing |.	Replace .+ with [^\n]+ (character class cannot backtrack across | boundaries, eliminating the quadratic path).
Medium	Sources/Reader/AppDelegate.swift:32	insecure-design	application(_:openFile:) accepts a raw filename string from Launch Services / CLI with no extension allowlist and no path validation, bypassing the NSOpenPanel content-type filter before calling open(url:).	Validate path extension against ["md","markdown","mdown","mkd","mkdn"]; resolve symlinks and verify the resolved URL stays within expected directories before passing to open(url:).
Medium	scripts/release.sh:46	insecure-design	$VERSION is passed unquoted to PlistBuddy and interpolated into XML attribute values without validation. Shell metacharacters or XML special characters in the version string can inject arbitrary PlistBuddy commands or corrupt appcast.xml.	Validate $VERSION against ^[0-9]+\.[0-9]+\.[0-9]+$ at script entry and abort if it does not match; quote all variable expansions.
Medium	scripts/release.sh:53	security-misconfiguration	codesign --force --deep --sign - produces an ad-hoc signature with no Developer ID; binary is indistinguishable from a tampered copy and Gatekeeper quarantines it on first launch.	Obtain an Apple Developer ID Application certificate; follow with xcrun notarytool submit and xcrun stapler staple so Gatekeeper can verify offline.
Medium	scripts/reader:46	injection	CLI script resolves arguments to absolute paths and checks existence but never validates file extension or content type; any file is forwarded to /usr/bin/open -b com.jefflarson.Reader.	Filter by extension after path resolution: accept only *.md, *.markdown, *.mdown, *.mkd, *.mkdn; abort with an error message for all other types.
Low	scripts/release.sh:104	injection	Python post-processing splices the new <item> block into appcast.xml via re.sub string concatenation with no XML parsing; any deviation in file structure silently no-ops the insertion, and injected content from $NOTES survives intact.	Replace the regex splice with Python's xml.etree.ElementTree: parse, build the <item> subtree programmatically, insert, and serialise.
Low	Package.swift:8	supply-chain	Sparkle declared as from: "2.6.4" (open upper bound); a future compromised Sparkle release within the 2.x range is automatically consumed on swift package update.	Pin to an exact version: .package(url: "...", exact: "2.9.1") so upgrades require a deliberate, reviewable Package.swift change.
Low	Sources/Reader/MainWindowController.swift:196	insecure-local-storage	noteNewRecentDocumentURL is called unconditionally for all opens including CLI-supplied paths; arbitrary sensitive paths (e.g., /etc/passwd) are permanently recorded in the macOS Recent Documents list, visible to other apps.	Only invoke noteNewRecentDocumentURL for user-initiated open/save panel interactions; set a flag in the CLI-argument path to suppress the call.
Low	Resources/Info.plist:70	cryptographic-failures	SUPublicEDKey is baked into the bundle; if the private signing key is compromised the public key cannot be rotated without shipping a new release — itself requiring the compromised key.	Protect the private key with a hardware security token and strict access controls; document a key-compromise response plan; consider a secondary out-of-band notification channel.
Low	Resources/Info.plist:68	insecure-design	No NSAppTransportSecurity dictionary is present; any future URLSession call added to the codebase will silently use the system-default policy rather than an enforced HTTPS-only policy.	Add NSAppTransportSecurity → NSAllowsArbitraryLoads = false with no exceptions to Info.plist.

---

Attack Chains

Chain A — Critical

An attacker who gains write access to the GitHub repository — a single compromised credential, leaked GitHub Actions token, or accepted malicious pull request — can push a replacement appcast.xml directly to the raw.githubusercontent.com URL that every deployed Reader client polls for updates, because that URL is hardcoded as SUFeedURL. Because release.sh signs every build with an ad-hoc identity instead of a Developer ID, there is no secondary code-signing check the user's Mac can enforce; Sparkle's EdDSA signature is the only barrier, and any update package signed with the repository's private signing key is silently accepted and installed by every auto-updating client. The open version bound on the Sparkle dependency adds a second vector: a future compromised Sparkle release above 2.6.4 is automatically pulled in on the next swift package update with no repository change required. The attacker walks away with arbitrary code execution on every machine that auto-updates Reader.

Chain B — High

A malicious insider or compromised CI pipeline invokes release.sh with a VERSION string containing XML special characters or shell metacharacters, which propagates unescaped into Info.plist via PlistBuddy and into the raw $ITEM shell variable; the NOTES argument then breaks out of the CDATA section because the release script embeds it with no ]]> escaping, injecting arbitrary XML elements such as a forged <enclosure> element pointing to an attacker-controlled download URL. The Python post-processing step splices this corrupted ITEM block into the live appcast.xml via a regex substitution rather than an XML parser, so malformed or injected markup survives intact and is committed to the repository. Because the resulting appcast.xml is served verbatim to all clients via the raw GitHub URL, the attacker's forged update entry is delivered to every Reader installation at the next update check.

Chain C — High

An attacker crafts a file with no .md extension (or a symlink to /dev/zero) and persuades a user to open it via the reader CLI script, which passes any file path through to /usr/bin/open without extension validation; AppDelegate.application(_:openFile:) then accepts the raw filename with no allowlist check and calls open(url:), which reads the entire file into memory via String(contentsOf:) with no size cap. Once the adversarial content lands in the text storage, restyle() fires on the main thread with no document-size guard, causing the bold regex with .dotMatchesLineSeparators, the setext-heading hyphen pattern, and the table-row greedy pattern to all run catastrophic backtracking passes over the entire document simultaneously. The user's application hangs indefinitely, producing a reliable local denial-of-service requiring no privileges beyond placing a file on disk.

Chain D — High

An attacker places adversarial content on the clipboard — either a multi-megabyte plain-text string or a crafted RTF payload that inflates to a large attributed string — and waits for or socially engineers the user into pasting it into Reader. The insertText call from readSelection(from:) triggers textStorageDidProcessEditing, which unconditionally calls restyle() with the full enlarged document and no size gate, simultaneously engaging the bold regex, the setext-heading pattern, and the table-row outer greedy quantifier in catastrophic backtracking on the main thread. The result is a complete, persistent main-thread hang that freezes the application without any file system access or elevated privileges required from the attacker.

---

Summary: 19 findings across 4 chains. Two chains reach Critical/High severity and threaten all users (Chain A: arbitrary code execution via update-feed takeover; Chain B: release-script injection into the live appcast). Two further chains (C and D) provide reliable denial-of-service via crafted files or clipboard content. Priority remediations: lock down appcast.xml hosting to write-access-controlled infrastructure, sanitise release script inputs, add a file-size cap and debounce to restyle(), and remove .dotMatchesLineSeparators from inline-emphasis regexes.

To apply fixes interactively, run /security-cleanup.

---

---

Generated by Soundcheck