Soundcheck: security review findings

Package.swift

@@ -5,7 +5,9 @@ let package = Package(
     name: "Reader",
     platforms: [.macOS(.v13)],
     dependencies: [
-        .package(url: "https://github.com/sparkle-project/Sparkle", from: "2.6.4"),
+        // Pinned to an exact version to prevent a future compromised Sparkle
+        // release from being silently consumed on the next `swift package update`.
+        .package(url: "https://github.com/sparkle-project/Sparkle", exact: "2.6.4"),
     ],
     targets: [
         .executableTarget(

Resources/Info.plist

@@ -64,10 +64,19 @@
 	<true/>
 	<key>SUEnableInstallerLauncherService</key>
 	<true/>
+	<!-- Feed must be hosted outside the source repo; see supply-chain finding. -->
 	<key>SUFeedURL</key>
-	<string>https://raw.githubusercontent.com/thejefflarson/reader/main/appcast.xml</string>
+	<string>https://thejefflarson.github.io/reader/appcast.xml</string>
 	<key>SUPublicEDKey</key>
 	<string>zBk/+O7F6ZvuCdEM7p7FQ3VHdkOFkRbJhMbZGRjqP3U=</string>
+	<!-- Explicitly enforce HTTPS-only for all URLSession connections so future
+	     code cannot accidentally use cleartext HTTP. This matches the system
+	     default on modern macOS but makes the policy explicit and auditable. -->
+	<key>NSAppTransportSecurity</key>
+	<dict>
+		<key>NSAllowsArbitraryLoads</key>
+		<false/>
+	</dict>
 	<key>UTImportedTypeDeclarations</key>
 	<array>
 		<dict>

Sources/Reader/AppDelegate.swift

@@ -29,7 +29,13 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
     // MARK: - File handling
 
     func application(_ sender: NSApplication, openFile filename: String) -> Bool {
-        openURL(URL(fileURLWithPath: filename))
+        // Validate the extension before accepting the path. CLI callers bypass
+        // NSOpenPanel's content-type filter, so we must enforce it here to
+        // prevent arbitrary files from being fed to the editor / Recent Documents.
+        let url = URL(fileURLWithPath: filename)
+        let allowedExtensions: Set<String> = ["md", "markdown", "mdown", "mkd", "mkdn"]
+        guard allowedExtensions.contains(url.pathExtension.lowercased()) else { return false }
+        openURL(url)
         return true
     }
 

Sources/Reader/EditorTextView.swift

@@ -283,11 +283,17 @@ final class EditorTextView: NSTextView, NSTextStorageDelegate {
         // For HTML payloads the pasteboard almost always also contains a
         // plain-text representation; we take that path and refuse to
         // parse HTML.
-        if let plain = pboard.string(forType: .string) {
+        // Cap plain-text paste to 1 MB to prevent main-thread stall on restyle().
+        let maxPasteChars = 1 * 1024 * 1024
+        if let plain = pboard.string(forType: .string), plain.count <= maxPasteChars {
             insertText(plain, replacementRange: selectedRange())
             return true
         }
+        // Cap RTF data to 4 MB before handing to NSAttributedString's parser,
+        // which allocates memory proportional to the input inside AppKit.
+        let maxRTFBytes = 4 * 1024 * 1024
         if let data = pboard.data(forType: .rtf),
+           data.count <= maxRTFBytes,
            let attr = NSAttributedString(rtf: data, documentAttributes: nil) {
             insertText(MarkdownSerializer.markdown(from: attr), replacementRange: selectedRange())
             return true

Sources/Reader/MainWindowController.swift

@@ -190,10 +190,23 @@ final class MainWindowController: NSWindowController, NSWindowDelegate {
 
     func open(url: URL) {
         do {
+            // Reject files larger than 10 MB to prevent an unbounded main-thread
+            // allocation. This also guards against symlinks to /dev/zero or huge
+            // sparse files, which String(contentsOf:) would read indefinitely.
+            let maxFileBytes = 10 * 1024 * 1024
+            let attrs = try FileManager.default.attributesOfItem(atPath: url.path)
+            if let size = attrs[.size] as? Int, size > maxFileBytes {
+                throw CocoaError(.fileReadTooLarge)
+            }
             let content = try String(contentsOf: url, encoding: .utf8)
             currentFileURL = url
             markdown = content
-            NSDocumentController.shared.noteNewRecentDocumentURL(url)
+            // Only record markdown files in Recent Documents; arbitrary paths
+            // (e.g. CLI-supplied /etc/passwd) must not appear in the system list.
+            let allowedExtensions: Set<String> = ["md", "markdown", "mdown", "mkd", "mkdn"]
+            if allowedExtensions.contains(url.pathExtension.lowercased()) {
+                NSDocumentController.shared.noteNewRecentDocumentURL(url)
+            }
             // Opened documents land in preview mode — a file you received
             // is something you read first, not something you're drafting.
             // New blank windows (⌘N) stay in edit mode.

Sources/Reader/MarkdownSerializer.swift

@@ -22,10 +22,20 @@ enum MarkdownSerializer {
 
         var content = text
 
+        // Only embed URLs whose schemes are safe for markdown links written to
+        // disk. RTF clipboard content can carry file://, app://, x-apple-*,
+        // and other schemes that should never be serialised into a .md file.
+        let allowedSchemes: Set<String> = ["http", "https", "mailto"]
         if let url = attributes[.link] as? URL {
+            guard let scheme = url.scheme?.lowercased(), allowedSchemes.contains(scheme) else {
+                return content
+            }
             return "[\(content)](\(url.absoluteString))"
         }
         if let urlString = attributes[.link] as? String, let url = URL(string: urlString) {
+            guard let scheme = url.scheme?.lowercased(), allowedSchemes.contains(scheme) else {
+                return content
+            }
             return "[\(content)](\(url.absoluteString))"
         }
 

Sources/Reader/MarkdownStyler.swift

@@ -23,6 +23,11 @@ final class MarkdownStyler {
     func restyle(_ storage: NSTextStorage) {
         let full = NSRange(location: 0, length: storage.length)
         guard full.length > 0 else { return }
+        // Refuse to run all regex rules synchronously on very large documents —
+        // that can produce a multi-second main-thread stall (and is the first
+        // link in the ReDoS chain). 1 MB is well above any realistic note size.
+        let maxStyleBytes = 1 * 1024 * 1024
+        guard storage.length <= maxStyleBytes else { return }
 
         storage.setAttributes(baseAttributes(), range: full)
         let occupied = NSMutableIndexSet()
@@ -253,7 +258,9 @@ final class MarkdownStyler {
     private static func setextHeading2() -> Rule {
         // `Title\n---` — but only if the title line isn't itself a list
         // bullet or blockquote (which would fight this interpretation).
-        let pattern = regex("(?m)^([^\\n]+)\\n(-{2,})\\s*$")
+        // Use [ \t]* instead of \s* to avoid O(n²) backtracking on lines
+        // ending with many hyphens that are not followed by a newline.
+        let pattern = regex("(?m)^([^\\n]+)\\n(-{2,})[ \\t]*$")
         return Rule(pattern: pattern, occupies: false) { match, ctx in
             let titleLine = match.range(at: 1)
             let titleText = (ctx.storage.string as NSString).substring(with: titleLine)
@@ -348,7 +355,11 @@ final class MarkdownStyler {
         // Match any line that looks like a table row: starts and ends with
         // `|`, has at least one interior `|`. Cells are monospace so columns
         // align visually in the source-is-rendered model.
-        let pattern = regex("(?m)^(\\s*\\|.+\\|)\\s*$")
+        //
+        // Pattern uses (?:\|[^|\n]+)+ instead of .+ to eliminate O(n²)
+        // backtracking: each cell is unambiguously "|<non-pipe chars>",
+        // so the engine can never re-split a match when the trailing | is absent.
+        let pattern = regex("(?m)^([ \\t]*(?:\\|[^|\\n]+)+\\|)[ \\t]*$")
         return Rule(pattern: pattern, occupies: false) { match, ctx in
             let whole = match.range
             ctx.storage.addAttribute(.font, value: Theme.codeFont, range: whole)
@@ -376,9 +387,13 @@ final class MarkdownStyler {
     // MARK: - Inline emphasis
 
     private static func bold() -> Rule {
+        // Do NOT use .dotMatchesLineSeparators here: with that flag and an
+        // unclosed ** marker, the lazy .+? expands to span the entire document,
+        // causing catastrophic backtracking on every keystroke. Constraining
+        // to [^\n] limits each match to a single line, making the rule O(n).
         let pattern = try! NSRegularExpression(
-            pattern: "(\\*\\*|__)(?=\\S)(.+?)(?<=\\S)\\1",
-            options: [.dotMatchesLineSeparators]
+            pattern: "(\\*\\*|__)(?=\\S)([^\\n]+?)(?<=\\S)\\1",
+            options: []
         )
         return Rule(pattern: pattern, occupies: false) { match, ctx in
             let whole = match.range

appcast.xml

@@ -4,7 +4,11 @@
      xmlns:dc="http://purl.org/dc/elements/1.1/">
     <channel>
         <title>Reader</title>
-        <link>https://raw.githubusercontent.com/thejefflarson/reader/main/appcast.xml</link>
+        <!-- Feed URL must be hosted outside the source repo so that a push to
+             main cannot immediately serve a malicious update to all users.
+             Deploy appcast.xml to a controlled CDN or GitHub Pages (gh-pages
+             branch with a separate deploy key) and update this URL. -->
+        <link>https://thejefflarson.github.io/reader/appcast.xml</link>
         <description>Reader updates.</description>
         <language>en</language>
                         <item>

scripts/reader

@@ -35,6 +35,17 @@ for arg in "$@"; do
         echo "reader: $arg: no such file" >&2
         exit 1
     fi
+    # Validate the file extension before forwarding to Launch Services.
+    # Any existing file would otherwise be opened by Reader, bypassing the
+    # content-type filter that NSOpenPanel enforces for interactive opens.
+    ext="${arg##*.}"
+    case "$ext" in
+        md|markdown|mdown|mkd|mkdn) ;;
+        *)
+            echo "reader: $arg: not a supported markdown file (.$ext)" >&2
+            exit 1
+            ;;
+    esac
     abs="$(/usr/bin/dirname "$arg")"
     abs="$(cd "$abs" && pwd)/$(/usr/bin/basename "$arg")"
     absolute_paths="$absolute_paths

scripts/release.sh

@@ -19,6 +19,12 @@ if [ -z "$VERSION" ]; then
     echo "usage: $0 <version> [\"release notes\"]"
     exit 1
 fi
+# Validate version format to prevent shell metacharacters and XML special
+# characters from being injected into Info.plist or appcast.xml.
+if ! printf '%s' "$VERSION" | grep -qE '^[0-9]+\.[0-9]+(\.[0-9]+)?(-[A-Za-z0-9]+)?$'; then
+    echo "error: VERSION must match X.Y[.Z][-qualifier] (e.g. 1.2.0), got: $VERSION"
+    exit 1
+fi
 : "${NOTES:=Reader $VERSION}"
 
 REPO="thejefflarson/reader"
@@ -43,14 +49,19 @@ cp Resources/AppIcon.icns "$APP_DIR/Contents/Resources/AppIcon.icns"
 
 # Patch Info.plist with the version at release time, then copy in.
 /usr/libexec/PlistBuddy \
-    -c "Set :CFBundleShortVersionString $VERSION" \
-    -c "Set :CFBundleVersion $VERSION" \
-    Resources/Info.plist >/dev/null 2>&1 || true
+    -c "Set :CFBundleShortVersionString ${VERSION}" \
+    -c "Set :CFBundleVersion ${VERSION}" \
+    Resources/Info.plist
 cp Resources/Info.plist "$APP_DIR/Contents/Info.plist"
 printf 'APPL????' > "$APP_DIR/Contents/PkgInfo"
 
-# Ad-hoc sign so Gatekeeper will at least see a signature.
-codesign --force --deep --sign - "$APP_DIR" 2>/dev/null || true
+# Sign with a real Developer ID so Gatekeeper accepts the app without
+# quarantine prompts. Ad-hoc signing (--sign -) produces a binary that
+# is indistinguishable from a tampered copy and is rejected by Gatekeeper
+# on first launch on any machine other than the build machine.
+# Set SIGN_IDENTITY in the environment to override (e.g. for a specific team).
+SIGN_IDENTITY="${SIGN_IDENTITY:-Developer ID Application}"
+codesign --force --deep --options runtime --sign "$SIGN_IDENTITY" "$APP_DIR"
 
 echo "==> zipping $APP_DIR"
 /usr/bin/ditto -c -k --sequesterRsrc --keepParent "$APP_DIR" "$ZIP_PATH"
@@ -67,13 +78,17 @@ fi
 DATE="$(/bin/date -u +"%a, %d %b %Y %H:%M:%S +0000")"
 DOWNLOAD_URL="https://github.com/$REPO/releases/download/v$VERSION/$ZIP_NAME"
 
+# Escape any ]]> sequences in NOTES so they cannot close the CDATA section
+# early and inject arbitrary XML into the appcast served to all users.
+NOTES_SAFE="${NOTES//]]>/]]]]><![CDATA[>}"
+
 ITEM="        <item>
             <title>Version $VERSION</title>
             <pubDate>$DATE</pubDate>
             <sparkle:version>$VERSION</sparkle:version>
             <sparkle:shortVersionString>$VERSION</sparkle:shortVersionString>
             <sparkle:minimumSystemVersion>13.0</sparkle:minimumSystemVersion>
-            <description><![CDATA[$NOTES]]></description>
+            <description><![CDATA[$NOTES_SAFE]]></description>
             <enclosure
                 url=\"$DOWNLOAD_URL\"
                 length=\"$LENGTH\"
@@ -89,7 +104,7 @@ if [ ! -f "$APPCAST" ]; then
      xmlns:dc="http://purl.org/dc/elements/1.1/">
     <channel>
         <title>Reader</title>
-        <link>https://raw.githubusercontent.com/$REPO/main/appcast.xml</link>
+        <link>https://thejefflarson.github.io/reader/appcast.xml</link>
         <description>Reader updates.</description>
         <language>en</language>
 $ITEM
@@ -101,14 +116,18 @@ else
 import sys, re
 path, item = sys.argv[1], sys.argv[2]
 text = open(path).read()
-text = re.sub(
+# Use re.subn so a structural mismatch (no insertion point found) is a hard
+# error rather than a silent no-op that leaves the appcast unmodified.
+new_text, n = re.subn(
     r"(<channel>.*?</description>\s*<language>[^<]*</language>\s*)",
     lambda m: m.group(1) + item + "\n",
     text,
     count=1,
     flags=re.DOTALL,
 )
-open(path, "w").write(text)
+if n == 0:
+    sys.exit("error: could not locate insertion point in " + path)
+open(path, "w").write(new_text)
 PY
 fi