sparkle modernization#1379
Open
robertobissanti wants to merge 27 commits into
Open
Conversation
robertobissanti
commented
Jul 7, 2026
robertobissanti
commented
- Migrate Sparkle 1.x to 2.x, bump deployment target to macOS 12
- Fix build tooling: path quoting, node-sass, stale lockfile
- Migrate Markdown preview from WebView to WKWebView
- Fix Auto Layout bug in Rendering preferences pane
- Fix image drag & drop and preview image width
- Add CHANGELOG and update README for this modernization pass
- Podfile: use_frameworks!, Sparkle ~> 2.9, MASPreferences ~> 1.4, PAPreferences ~> 0.5 - MPMainController.m: SUUpdater -> Sparkle.h, SPUUpdaterDelegate - MainMenu.xib: SUUpdater -> SPUStandardUpdaterController - project.pbxproj: MACOSX_DEPLOYMENT_TARGET 10.8 -> 12.0 - MacDown-Info.plist: note for EdDSA key migration Part of Gatekeeper/notarization compliance (Sparkle 1.x is incompatible with hardened runtime + notarization).
- Tools/update_build_number.sh: quote $(pwd -P) so it survives paths containing spaces (pre-existing bug, unrelated to the rest of this pass). - Tools/GitHub-style-generator: migrate node-sass (deprecated, native bindings) to sass (Dart Sass); update Makefile flags accordingly. - Gemfile: drop the dead travis gem, bump the cocoapods constraint. - Delete the stale Gemfile.lock (pinned Bundler 1.17.3 / CocoaPods 1.10.1, both incompatible with current Ruby); regenerate with bundle install. - Podfile.lock: sync with MASPreferences 1.4.1 / Sparkle 2.9.4.
Full replacement of the deprecated, legacy WebKit WebView with WKWebView for the preview pane: navigation/policy delegates, JS<->native messaging via WKScriptMessageHandler (replacing the WebScripting bridge), scroll sync, zoom, MathJax completion signaling, printing, and word counting. Also fixes two regressions this migration itself introduced along the way: - MathJax not rendering: the script tag pointed at the bundled MathJax.js, which is only the bootstrap loader (the config/jax/extensions/fonts it fetches relative to its own script URL at runtime were never vendored locally). Reverted to loading it from the CDN. - Preview HTML is now loaded via -loadFileURL:allowingReadAccessToURL: from a hidden sidecar file next to the document, instead of -loadHTMLString:baseURL:, since WKWebView restricts file:// sub-resource loads to the loaded page's own directory (this broke custom user styles and images referenced from outside the document's folder). Local stylesheets/scripts are now embedded inline in the HTML for the same reason. Print crash fixed by sizing the print operation's view, which WKWebView's printOperationWithPrintInfo: otherwise leaves at zero. See CHANGELOG.md for the full list.
Graphviz/Mermaid checkboxes used a fixedFrame (absolute position, never touched by Auto Layout) while the 'Show line numbers' row they're meant to align with is constraint-driven; any layout drift desynced them. Converted to proper constraints pinned to that row. Remaining pane spacing fixed by hand in Interface Builder.
- Dropping an image into the editor used to read the whole file into memory and inline it as a base64 data: URI (mislabeling every file as image/jpeg regardless of its real type). Now inserts a plain  Markdown reference instead, for any image type. - Preview images now always render at 100% of the pane's width, via a global rule in the base HTML template rather than per-image.
Author
|
Travis CI check appears stale — this repo already migrated to Dependabot; the xcode10.1 image is unlikely to run |
-openOrCreateFileForUrl: handed any resolved, reachable local link target straight to -[NSWorkspace openURL:], including .app bundles and other executables reached via an absolute file:// path or "../" traversal in the previewed (possibly untrusted) Markdown source. A malicious document could silently launch an arbitrary local application when its (disguised) link was clicked. - Check the resolved file's UTI and refuse to open anything conforming to public.application/public.executable, rather than pattern-matching the URL string (which encoding/traversal tricks could evade). - Confine auto-created link targets (for links pointing at nonexistent files) to the current document's own folder, closing the other half of CVE-2019-12138 (creating files outside the document's directory via "../").
The old .travis.yml pinned osx_image: xcode10.1, several major Xcode/macOS releases behind this project's current MACOSX_DEPLOYMENT_TARGET, and Travis CI's free macOS build queue for open source projects has been effectively dead for years -- the continuous-integration/travis-ci required status check on PRs never completes as a result. Replaced with .github/workflows/tests.yml. Note: the required-checks list in this repo's branch protection settings still needs a maintainer/admin to point it at the new workflow; that's not something a PR can change.
- NSToolbarItem[Group].minSize/maxSize are deprecated (macOS 12): let the system measure the item via Auto Layout constraints on the segmented control's width/height instead of setting an explicit size. - NSDragPboard is a deprecated pasteboard *name* constant that was being misused as a drag type/UTI in -registerForDraggedTypes:. Register the actual type the view's drag handlers care about (NSPasteboardTypeFileURL) instead.
Addresses the remaining warnings surfaced by a full build log (buildIssue_70666.txt): - MPDocument.m: modernize the CVE-2019-12138/12173 executable-type check from CoreServices' kUTTypeApplication/kUTTypeExecutable/UTTypeConformsTo (deprecated macOS 12) to the UniformTypeIdentifiers UTType class; migrate NSSavePanel.allowedFileTypes (deprecated macOS 12) to allowedContentTypes in prepareSavePanel:/exportHtml:/exportPdf:; migrate NSFileHandlingPanelOKButton (deprecated macOS 10.13) to NSModalResponseOK; silence -Wdeprecated-declarations specifically around NSAutoPagination, since Apple's own deprecation note names no replacement API/constant. - MPMainController.m, MPRenderer.m, MPUtilities.m: fix K&R-style C function declarations without (void) prototypes (deprecated in all C standards). - MPUtilities.m: MPGetDataMap now uses +[NSKeyedUnarchiver unarchivedObjectOfClasses:fromData:error:] with an explicit allowed-class set instead of the insecure, deprecated +unarchiveObjectWithFile:. - MPDocumentSplitView.m: colorUsingColorSpaceName: (deprecated macOS 10.14) -> colorUsingColorSpace: with an explicit NSColorSpace. - MPGeneralPreferencesViewController.m: NSOnState (deprecated macOS 10.14) -> NSControlStateValueOn. - macdown-cmd/main.m: fix two more (void) prototypes; stringByAddingPercentEscapesUsingEncoding: (deprecated macOS 10.11) -> stringByAddingPercentEncodingWithAllowedCharacters:; the deprecated launchAppWithBundleIdentifier:options:additionalEventParamDescriptor: launchIdentifier: + NSWorkspaceLaunchDefault pair (deprecated macOS 11) -> resolve the bundle identifier to a URL and use -openApplicationAtURL:configuration:completionHandler:. Deliberately not touched: the insertText: deprecation (NSTextView+Autocomplete.m, MPDocument.m) is flagged by Apple as intended only for the input system, not application code -- replacing it needs a review of the call sites' actual intent rather than a mechanical rename, so it's left as a known issue for now.
GBCli, handlebars-objc, hoedown, JJPluralForm, LibYAML, M13OrderedDictionary, MASPreferences and PAPreferences all still ship MACOSX_DEPLOYMENT_TARGET values as low as 10.6, below Xcode's currently supported minimum (10.13) and this project's own target (12.0). Xcode warns on every build instead of building those pod targets against a consistent deployment target. Force every pod sub-target's deployment target up to 12.0 in a post_install hook, matching the approach macdown3000 uses for the same dependency set.
- MacDown.xcodeproj: mark 'Update Build Number', 'Fetch Prism Resources', and 'Transpile Styles' as alwaysOutOfDate, since none of them declare outputs Xcode can use to skip them (and Update Build Number genuinely can't -- it depends on git state, not file contents). - Podfile post_install: patch hoedown's vendored headers to use angle-bracketed sibling includes instead of quoted ones, silencing CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER now that hoedown builds as a framework. Verified end-to-end: ran a real 'pod install' and confirmed the hook re-patches Pods/hoedown/src/*.h and hoedown-umbrella.h, and that the earlier deployment-target hook applied 12.0 across all pod targets. - Podfile.lock: checksum bump from the Podfile edits above (no dependency version changes).
Markdown pane: 8 'trailing constraint missing' warnings traced and found to be false positives (widths fully determined via equal-width chains). Editor pane: 4 'may cause clipping' warnings include at least one real, pre-existing issue (font-preview field's leading edge pinned two inconsistent ways depending on label text width) -- needs an Interface Builder fix with visual verification, not a blind XML edit. Html pane warnings are the already-documented ambiguous/misplaced residue from earlier this session.
…rs-objc as unfixable here - Generalized the post_install quoted-include patch into a reusable patch_quoted_framework_includes helper and applied it to PAPreferences-framework's umbrella header (same CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER warning as hoedown). Also live-patched the currently-installed copy so it's fixed without waiting on the next 'pod install'. - Documented (not fixed) two static-analyzer warnings in handlebars-objc's generated parser files -- they live in DerivedData, regenerated from the pod's own grammar on every clean build, so there's no durable location in this repo to patch them from.
pmh_parser.c is generated at build time (gitignored) from pmh_parser_head.c + pmh_parser_core.c + pmh_parser_foot.c via the Makefile, so the fixes belong in pmh_parser_head.c (the actual tracked source), not in the generated file -- editing the generated copy directly would just get overwritten on the next build. - get_element_type_names() -> (void) prototype. - Two 'assign, then use as the ternary's value via a comma operator' idioms rewritten as plain if/else, avoiding the comma-operator misuse warning without changing behavior. - pmh_styleparser.c: five more (void) prototype fixes (new_attr_value, new_font_styles, new_font_size, new_block, new_style_collection).
All 6 remaining call sites (5 in NSTextView+Autocomplete.m, 1 in MPDocument.m) were plain 'insert at wherever the cursor/selection currently is' cases, immediately adjacent in every file to other calls that already use -insertText:replacementRange:. Converted each to the same 2-arg NSTextInputClient method, passing NSMakeRange(NSNotFound, 0) -- Apple's documented sentinel meaning 'use the current selection, or the marked (IME composition) range if there is one', which is exactly what the deprecated 1-arg -insertText: did. This preserves IME/marked-text behavior correctly, which plain self.selectedRange would not have during active composition (this file already special-cases marked text elsewhere, e.g. hasMarkedText in completeMatchingCharacterForText:atLocation:).
inhibit_all_warnings! suppresses warnings per-file (COMPILER_FLAGS on each PBXBuildFile entry CocoaPods knows about at install time), but handlebars-objc.yy.m and y.tab.c are produced at build time by an Xcode Build Rule from .lm/.ym grammar sources, so CocoaPods never gets a chance to attach a per-file override to them. Confirmed via the generated xcconfig that the handlebars-objc target's OTHER_CFLAGS carried no warning suppression at all -- added -Wno-everything there in post_install, and live-patched the two already-generated xcconfig files so it takes effect without waiting on the next 'pod install'.
…ained Reported as a crash on the Underline toolbar action, but the bug wasn't in the underline code path itself (toggleForMarkupPrefix:suffix: in NSTextView+Autocomplete.m was untouched and correct) -- it was in how MPDocument holds its reference to the editor view. 'editor' was the only IBOutlet on the class declared unsafe_unretained; every sibling outlet (toolbar, splitView, editorContainer, preview, ...) is weak. Traced via git blame to the original 2014 project setup, never updated since. unsafe_unretained is a raw, non-zeroing pointer: if the MPEditorView is ever deallocated and recreated (window/layout churn), 'editor' keeps pointing at freed memory, and the next message sent to it -- from any action, not just Underline -- is a dangling-pointer access, i.e. EXC_BAD_ACCESS. weak makes the pointer nil itself out instead, turning a crash into a safe no-op.
…:from: The actual root cause of the reported crash (backtrace showed objc_retain/ objc_storeStrong right at the callee's entry, not inside its body): MPToolbarController's -selectedToolbarItemGroupItem: resolved the clicked segment's action via -methodForSelector: and invoked the IMP through a hand-cast 'void (*)(id)' function pointer -- one argument, when every Objective-C method actually receives three (self, _cmd, sender). Calling with the wrong signature left _cmd/sender as whatever garbage was in those registers/stack slots; the callee's own sender parameter (__strong under ARC) being retained against that garbage pointer is what crashed. Replaced with [NSApp sendAction:to:from:], the same standard AppKit dispatch mechanism already used a few lines away for the toolbar's non-grouped buttons (itemButton.target/.action), which is proven to work correctly against MPDocument as a plain explicit target.
|
@robertobissanti if you'd be interested in contributing any of this code to https://github.com/schuyler/macdown3000 I would be glad to review and incorporate it!
|
Author
|
Hi @schuyler, thanks for reaching out! SecurityFixed CVE-2019-12138 and CVE-2019-12173 (arbitrary program execution / directory traversal via a crafted preview link), same approach you'd already shipped in macdown3000. Moving off deprecated/dying platform tech
UI fixes
Bugs
One heads-up: the PR you're commenting on doesn't include all of the commits listed above — it's a snapshot from earlier. For the full, up-to-date set of changes, you'll want to check out and build my branch directly:
|
…r.xib 'location' and 'supportText' outlets are leftovers from a rename to locationTextField/supportTextField that was done in code (MPTerminal PreferencesViewController.m) but never cleaned up in the xib. Both stale entries pointed at the same destination views as the correctly-named outlets that are still wired, so this was harmless -- just a 'Failed to connect outlet ... missing setter or instance variable' warning logged on every nib load.
Xcode's Thread Performance Checker flagged a priority-inversion Hang Risk: main thread (user-interactive QoS, switching Preferences panes) waiting on a lock inside MASPreferences' -setSelectedViewController: while a lower-QoS thread held it. The strongest candidate in our own code: MPHomebrewSubprocessController (backs the Terminal pane's 'brew --prefix' check, kicked off from -viewWillAppear) used the pre-GCD -readToEndOfFileInBackgroundAndNotify + NSNotificationCenter pattern, whose background read/notification delivery run at a QoS the app doesn't control -- exactly the kind of thing that can cause this if a pane switch happens while it's mid-flight. Rewrote around NSTask.terminationHandler instead, explicitly dispatching the completion callback back to the main queue (callers update KVO-observed/bound UI properties from it). Capture self strongly in the terminationHandler deliberately: the controller is created as a bare local in MPDetectHomebrewPrefixWithCompletionhandler() with no other owner, so this is what keeps it alive for the task's duration; NSTask releases its terminationHandler after one invocation, so it's not a lasting cycle. This is a well-reasoned diagnosis from reading the code, not a profiled confirmation -- but a legitimate modernization on its own terms either way.
self.task.terminationHandler = ^{ ...self... } is a genuine structural
cycle (self -> _task -> terminationHandler -> block -> self), correctly
flagged by the compiler -- my earlier comment claiming NSTask releases
terminationHandler after firing was an unverified assumption, not a
documented guarantee.
The strong self-capture is still deliberate and necessary: nothing else
keeps this controller alive between -runWithCompletionHandler: returning
and the task actually exiting. Break the cycle explicitly instead, as the
first thing the block does (task.terminationHandler = nil;), wrapped in a
narrowly-scoped -Warc-retain-cycles suppression with a comment explaining
why it's safe.
…ibution - Podfile: patch MASPreferencesWindowController.m via post_install to force animate:NO on the Preferences window resize triggered by switching toolbar tabs. The animated resize hands off to Core Animation/WindowServer on a background-QoS thread, which the main thread then blocks on while committing setContentView:/recalculateKeyViewLoop -- the exact priority inversion Xcode's Thread Performance Checker flagged at MASPreferencesWindowController.m:222/311. The earlier MPHomebrewSubprocessController rewrite was a legitimate modernization but, per a recurrence of the same warning on a later run, was not the actual cause; CHANGELOG corrected accordingly. - Added 'Portions Copyright (c) 2026 Roberto Bissanti.' to the header of every tracked source file modified in this fork so far.
- Xcode 'Update to Recommended Settings': bumped LastUpgradeVersion in the MacDown scheme, switched workspace BuildSystemType from Original to Latest, and related objectVersion/settings bumps in the peg-markdown-highlight and version sub-project pbxprojs. - MPGeneralPreferencesViewController.xib: Auto Layout adjustments made directly in Interface Builder. - Added Gemfile.lock (regenerated bundler lockfile) and an empty GitHub-2020.css placeholder under MacDown/Resources/Styles. These were pending local/Xcode-driven changes not yet committed; grouped here since they're editor/tooling state rather than a logical code change.
|
@robertobissanti This is extremely cool. We've already addressed a bunch of these issues in MacDown 3000, but some of them are still hanging fire. In particular Sparkle 2 and WKWebView have been on the roadmap for a while. I had some concerns about whether WKWebView might break backwards compatibility in some way, but if the change is straightforward enough, maybe we should just go for it. If you'd be willing to file individual PRs for the changes that could be ported, that would be awesome. Otherwise I might have Claude Code take a crack at it over the weekend. Please let me know? Thanks! |
Author
|
@schuyler I was hoping for this suggestion. As for how to proceed, I’m already working on a three-way comparison between the original MacDown, MacDown3000 and my fork, to work out which files have been modified by both repositories and focus on those when making changes. I’m already identifying the most significant improvements in MacDown3000 that I haven’t implemented. In particular after digging into macdown3000's CHANGELOG in detail I identified this points:
One caveat: some of this (editor/preview fixes especially) is coupled to the current WebView pipeline. A few items might be redundant or need rework once WKWebView is in — I'll check as I go rather than assume everything ports cleanly. I’ll be in touch as soon as possible. Roberto. P.S. I think we should find a better place to speak about our collab. |
|
@robertobissanti Agreed. I'm kind of astonished that Github doesn't offer some kind of messaging feature. My DMs are open on BlueSky: https://bsky.app/profile/schuyler.info ... I'm sure there are other options... I'm open to suggestions.
|
This was referenced Jul 11, 2026
macnotes
added a commit
to macnotes/macdown
that referenced
this pull request
Jul 11, 2026
…fixes, Apple Silicon build
schuyler
added a commit
to schuyler/macdown3000
that referenced
this pull request
Jul 13, 2026
Ports four low-risk fixes triaged from upstream [MacDownApp/macdown#1379](MacDownApp/macdown#1379) — Roberto Bissanti's Sparkle-modernization branch (GitHub [@wltb](https://github.com/wltb)). Each fix was reviewed cold independently before landing; the full suite is green (**1074 tests, 0 unexpected**). ## Fixes | Commit | Upstream | What | |--------|----------|------| | `6494c90` | `05d5153` | **Fix EXC_BAD_ACCESS crash** — `MPDocument.editor` IBOutlet was `unsafe_unretained`; now `weak`. | | `806f6ef` | `2b09e4e` | **Deprecated `-insertText:` → `-insertText:replacementRange:`** at 6 call sites, with 6 new tests. | | `20ba916` | `9e62b44` + `c807ff7` | **Modernize `MPHomebrewSubprocessController`** (`NSTask.terminationHandler`, main-queue delivery, retain-cycle break) **and fix a real bug** — see note below. | | `4300766` | `604664b` | **Remove stale xib outlets** (`location`, `supportText`) that logged a "Failed to connect outlet" warning on every nib load. | ## Fix 3 is scope-upgraded (with approval) Reviewing the Homebrew modernization surfaced a latent bug: `launchPath = @"brew"` is a bare relative path, and `NSTask` never searches `PATH`, so `-launch` threw `NSInvalidArgumentException` every time — the Terminal preferences pane's Homebrew detection has **never actually worked** (it always reported "not installed"). Beyond the faithful port, this now resolves Homebrew's absolute path (`/opt/homebrew/bin/brew`, then `/usr/local/bin/brew`) and adopts the non-deprecated `executableURL` + `-launchAndReturnError:`. All three completion paths (not-found, launch-failure, success) deliver the handler on the main queue, since the caller mutates AppKit UI directly. This is a **user-visible behavior change**: the Terminal preferences pane will now correctly detect an installed Homebrew. ## Testing - Full suite: **1074 tests, 0 unexpected failures** (the 76 failures are the codebase's pre-existing `XCTExpectFailure` cases). - New tests: 6 for the `-insertText:replacementRange:` migration; 3 for the Homebrew controller (success delivers output on the main thread; not-found delivers nil on the main thread even when invoked off-main; the terminationHandler retain-cycle break holds) — the latter use a test-only seam stubbing brew resolution with `/bin/echo` for determinism without a real Homebrew install. ## Suggested follow-up (not in this PR) Two test-only categories still redeclare `editor` as `unsafe_unretained` (`MacDownTests/MPDocumentLifecycleTests.m:27`, `MacDownTests/MPScrollSyncTests.m:29`), stale since the weak-outlet fix. Confirmed harmless (ARC uses the primary class's `weak` synthesis and the tests hold strong locals) but misleading — worth a one-line cleanup. Ports by [@wltb](https://github.com/wltb) (Roberto Bissanti). Related to #1379 Related to #526
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.