add UpdateMechanism for .dmg files
#92
Conversation
WalkthroughMaven project version Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xml(2 hunks)src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java(1 hunks)src/main/resources/MacIntegrationsBundle.properties(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyse
src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java
Outdated
Show resolved
Hide resolved
WalkthroughThis change introduces a new Mac-specific update mechanism for Cryptomator. The pull request updates the Maven runtime API version to 1.8.0-SNAPSHOT, adds a snapshot repository, registers a new UpdateMechanism service provider in the module system, implements the DmgUpdateMechanism class to handle DMG-based updates on macOS (including DMG mounting, application copying, and process restart logic), and adds localized strings for the update workflow. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xml(2 hunks)src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java(1 hunks)src/main/resources/MacIntegrationsBundle.properties(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyse
| hdiutil attach "${DMG_PATH}" -mountpoint "/Volumes/Cryptomator_${MOUNT_ID}" -nobrowse -quiet && | ||
| cp -R "/Volumes/Cryptomator_${MOUNT_ID}/Cryptomator.app" 'Cryptomator.app' && | ||
| hdiutil detach "/Volumes/Cryptomator_${MOUNT_ID}" -quiet | ||
| """; | ||
| var command = List.of("/bin/zsh", "-c", script); | ||
| var processBuilder = new ProcessBuilder(command); | ||
| processBuilder.directory(workDir.toFile()); | ||
| processBuilder.environment().put("DMG_PATH", assetPath.toString()); | ||
| processBuilder.environment().put("MOUNT_ID", UUID.randomUUID().toString()); | ||
| Process p = processBuilder.start(); | ||
| try { | ||
| if (p.waitFor() != 0) { | ||
| LOG.error("Failed to extract DMG, exit code: {}, output: {}", p.exitValue(), new String(p.getErrorStream().readAllBytes())); | ||
| throw new IOException("Failed to extract DMG, exit code: " + p.exitValue()); |
There was a problem hiding this comment.
Mountpoint needs to be created and cleaned up in the DMG extraction script
hdiutil attach … -mountpoint expects the target directory to exist; with the current script, the very first attach fails with No such file or directory, so the update path never succeeds. In addition, the detach command is only run when the copy succeeds, leaving the DMG mounted whenever attach or copy fails. Please create/cleanup the mountpoint and ensure we always detach.
String script = """
- hdiutil attach "${DMG_PATH}" -mountpoint "/Volumes/Cryptomator_${MOUNT_ID}" -nobrowse -quiet &&
- cp -R "/Volumes/Cryptomator_${MOUNT_ID}/Cryptomator.app" 'Cryptomator.app' &&
- hdiutil detach "/Volumes/Cryptomator_${MOUNT_ID}" -quiet
+ set -e
+ MOUNT_POINT="/Volumes/Cryptomator_${MOUNT_ID}"
+ cleanup() {
+ hdiutil detach "${MOUNT_POINT}" -quiet >/dev/null 2>&1 || true
+ rmdir "${MOUNT_POINT}" >/dev/null 2>&1 || true
+ }
+ trap cleanup EXIT
+ mkdir -p "${MOUNT_POINT}"
+ hdiutil attach "${DMG_PATH}" -mountpoint "${MOUNT_POINT}" -nobrowse -quiet
+ cp -R "${MOUNT_POINT}/Cryptomator.app" 'Cryptomator.app'
+ trap - EXIT
+ cleanup
""";
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java around
lines 52 to 65, the DMG extraction script fails because the mountpoint directory
is not created and the detach is only executed on success; update the script to
(1) enable strict error handling (set -e), (2) create the mountpoint directory
(mkdir -p), (3) install a cleanup function that always tries to detach the DMG
and remove the mountpoint, registered via trap EXIT so detach runs on failure or
success, (4) perform the attach, copy, then remove the trap and invoke cleanup
on success; replace the current one-liner script with this safe flow so attach
won't fail for missing directory and the DMG is always detached.
There was a problem hiding this comment.
It doesn't fail. macOS treats /Volumes/* different from other paths.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
hdiutil attach mountpoint /Volumes auto create directory macOS
💡 Result:
Short answer: hdiutil will normally create a volume directory under /Volumes automatically when you attach an image. If you supply -mountpoint or -mountroot, the target directory must already exist (so create it with mkdir -p first).
Examples:
-
Let macOS pick the name under /Volumes (auto-created):
hdiutil attach /path/to/image.dmg. [1][2] -
Mount to a specific path (ensure the path exists first):
mkdir -p /Volumes/MyMount
hdiutil attach -mountpoint /Volumes/MyMount /path/to/image.dmg. [1][3]
Notes:
- -mountroot requires that the given directory exists; -mountrandom will create randomized subdirs under an existing path. [1][2]
Sources: hdiutil man pages and Apple discussions. [1] [2] [3]
🌐 Web query:
macOS /Volumes directory autofs automount special behavior
💡 Result:
Short summary of macOS /Volumes + autofs/automount behavior and gotchas
-
/Volumes is the system directory where macOS creates mount‑point folders for attached volumes (external disks, network mounts). If a real folder with the same name already exists (often left behind after an unclean unmount), macOS will mount the volume under a different name (e.g. "Name 1"). Delete the stale folder in /Volumes to restore the expected mountpoint. [1][2]
-
Autofs/automount creates "trigger" mount points and mounts on demand. Because autofs owns the trigger nodes, automounted shares can end up mounted at autofs-controlled locations rather than the Finder's usual mount representation — this can make them appear differently in Terminal vs Finder and can break apps that expect a specific /Volumes path. [3]
-
Since Catalina (and Big Sur+) Apple split the boot disk into a read‑only system volume and a writable Data volume. The Data volume is mounted at /System/Volumes/Data and macOS uses firmlinks to present a unified filesystem. This changed where some volume data and special firmlinks live and can affect mount resolution and tooling that assumes everything lives directly under /. (Backup/clone software and automount setups sometimes need adjustments for these firmlink/mountpoint differences.) [4][5]
-
Practical consequences and troubleshooting:
- If automounted shares don't show under /Volumes or appear "duplicated", check /Volumes for stale directories and remove them (after ensuring no data loss). [1][2]
- Automounter mounts may require explicit maps (auto_master/auto_*), and Kerberos/credential issues can cause automounted SMB shares to appear mounted but fail to list. Check automount maps, credentials, and kerberos tickets. [3]
- For Catalina/Big Sur+ issues (firmlink/mountpoint oddities, .VolumeIcon/.Volume‑Data behavior, backup tools), review how the Data volume is mounted at /System/Volumes/Data and adapt mount targets accordingly. [4][5]
Sources
- Ghost/stale mount behavior and /Volumes guidance. [1][2]
- How autofs/automount works and automounter behavior. [3]
- Catalina/Big Sur APFS volume group, firmlinks, and /System/Volumes/Data effects. [4][5]
If you want, I can:
- show exact files to inspect (/etc/auto_master, /etc/auto_smb, /etc/auto_*),
- give sample automount lines that mount directly under /Volumes,
- or help diagnose a specific automount problem if you paste your auto_master / automount map and a sample mount command.
@overheadhunter, thank you for the clarification! I found documentation indicating that -mountpoint typically requires the directory to exist first, but if you've tested this and /Volumes has special handling that makes it work without pre-creation, I'll defer to your experience.
However, there's still a separate concern about cleanup: the current script uses && chaining, which means if either hdiutil attach or cp fails, the hdiutil detach won't run, potentially leaving the DMG mounted. Would it make sense to add a trap or similar mechanism to ensure detachment even on failure? For example:
trap 'hdiutil detach "/Volumes/Cryptomator_${MOUNT_ID}" -quiet 2>/dev/null || true' EXIT
hdiutil attach "${DMG_PATH}" -mountpoint "/Volumes/Cryptomator_${MOUNT_ID}" -nobrowse -quiet
cp -R "/Volumes/Cryptomator_${MOUNT_ID}/Cryptomator.app" 'Cryptomator.app'This would ensure the volume is always detached regardless of whether the copy succeeds or fails. What do you think?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java (1)
93-93: Restart fix looks good (path-based open).The previous “open -a …” issue is resolved; using
open "${…/Cryptomator.app}"is correct and relaunches reliably.
🧹 Nitpick comments (4)
src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java (4)
57-66: Capture child process output to a log to avoid buffer stalls and aid debugging.Reading only stderr after
waitFor()risks buffer blockage on large output and loses stdout. Redirect both to a file.var processBuilder = new ProcessBuilder(command); processBuilder.directory(workDir.toFile()); + processBuilder.redirectErrorStream(true); + processBuilder.redirectOutput(ProcessBuilder.Redirect.appendTo(workDir.resolve("unpack.log").toFile())); processBuilder.environment().put("DMG_PATH", assetPath.toString()); processBuilder.environment().put("MOUNT_ID", UUID.randomUUID().toString()); Process p = processBuilder.start(); try { if (p.waitFor() != 0) { - LOG.error("Failed to extract DMG, exit code: {}, output: {}", p.exitValue(), new String(p.getErrorStream().readAllBytes())); - throw new IOException("Failed to extract DMG, exit code: " + p.exitValue()); + LOG.error("Failed to extract DMG, exit code: {}. See {}", + p.exitValue(), workDir.resolve("unpack.log")); + throw new IOException("Failed to extract DMG; see unpack.log for details. Exit code: " + p.exitValue()); }
76-84: Be defensive in install path detection.Path inference may fail for non-standard launches (e.g., helpers, custom launchers). Consider a fallback to /Applications/Cryptomator.app (or ~/Applications) instead of throwing, with a warning.
Would you confirm typical values of
ProcessHandle.current().info().command()for your distributed app? If it can be the JVM path, deriving the bundle root via-Xdock:name/-Xdock:iconcontexts or sysprops may be more reliable.
87-94: Handle permissions fallback and quarantine; keep using path-based open.If /Applications is not writable, fall back to ~/Applications. Also strip quarantine xattrs to avoid first‑launch blocks from Gatekeeper after programmatic copy.
- String script = """ - while kill -0 ${CRYPTOMATOR_PID} 2> /dev/null; do sleep 0.2; done; - if [ -d "${CRYPTOMATOR_INSTALL_PATH}" ]; then - echo "Removing old installation at ${CRYPTOMATOR_INSTALL_PATH}"; - rm -rf "${CRYPTOMATOR_INSTALL_PATH}" - fi - mv 'Cryptomator.app' "${CRYPTOMATOR_INSTALL_PATH}"; - open "${CRYPTOMATOR_INSTALL_PATH}"; - """; + String script = """ + set -euo pipefail + while kill -0 "${CRYPTOMATOR_PID}" 2>/dev/null; do sleep 0.2; done + TARGET="${CRYPTOMATOR_INSTALL_PATH}" + if [ -d "${TARGET}" ]; then + echo "Removing old installation at ${TARGET}" + rm -rf "${TARGET}" + fi + if mv 'Cryptomator.app' "${TARGET}" 2>/dev/null; then + : + else + echo "No write access to ${TARGET}, falling back to ~/Applications" + ALT="${HOME}/Applications/Cryptomator.app" + mkdir -p "${HOME}/Applications" + rm -rf "${ALT}" + mv 'Cryptomator.app' "${ALT}" + TARGET="${ALT}" + fi + # Clear quarantine if present to avoid Gatekeeper prompts on self-updates + xattr -dr com.apple.quarantine "${TARGET}" >/dev/null 2>&1 || true + open "${TARGET}" + """;
95-95: Make install script creation idempotent.On retried updates, CREATE_NEW can fail if the file exists. Prefer CREATE + TRUNCATE_EXISTING.
- Files.writeString(workDir.resolve("install.sh"), script, StandardCharsets.US_ASCII, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW); + Files.writeString(workDir.resolve("install.sh"), script, StandardCharsets.US_ASCII, + StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING);Optional: set restrictive perms if desired (not required since we invoke via
zsh install.sh).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Deploy to GitHub Packages
- GitHub Check: Deploy to Maven Central
- GitHub Check: Analyse
🔇 Additional comments (2)
src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java (2)
83-84: Exception type check.
restartthrowsUpdateFailedException; ensure it’s unchecked. If it’s a checked exception, add it to the method signature or wrap it.
31-37: Asset selection handles actual .dmg names; no x86_64.dmg or universal.dmg fallback needed.The code’s use of “arm64.dmg” and default “x64.dmg” correctly matches the Cryptomator GitHub assets (e.g. Cryptomator-1.17.1-arm64.dmg and Cryptomator-1.17.1-x64.dmg), and the default branch covers both “x86_64” and “amd64” JVM values.
Likely an incorrect or invalid review comment.
| hdiutil attach "${DMG_PATH}" -mountpoint "/Volumes/Cryptomator_${MOUNT_ID}" -nobrowse -quiet && | ||
| cp -R "/Volumes/Cryptomator_${MOUNT_ID}/Cryptomator.app" 'Cryptomator.app' && | ||
| hdiutil detach "/Volumes/Cryptomator_${MOUNT_ID}" -quiet | ||
| """; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Always detach DMG and verify signature; avoid leaks on failure.
&& skips detach on copy failure, leaving the image mounted. Add trap + strict mode. Also verify codesign/notarization before proceeding.
- String script = """
- hdiutil attach "${DMG_PATH}" -mountpoint "/Volumes/Cryptomator_${MOUNT_ID}" -nobrowse -quiet &&
- cp -R "/Volumes/Cryptomator_${MOUNT_ID}/Cryptomator.app" 'Cryptomator.app' &&
- hdiutil detach "/Volumes/Cryptomator_${MOUNT_ID}" -quiet
- """;
+ String script = """
+ set -euo pipefail
+ MOUNT_POINT="/Volumes/Cryptomator_${MOUNT_ID}"
+ cleanup() {
+ hdiutil detach "${MOUNT_POINT}" -quiet >/dev/null 2>&1 || true
+ }
+ trap cleanup EXIT
+ hdiutil attach "${DMG_PATH}" -mountpoint "${MOUNT_POINT}" -nobrowse -quiet
+ # Prefer ditto for app bundles (preserves attrs/ACLs properly):
+ ditto "${MOUNT_POINT}/Cryptomator.app" 'Cryptomator.app'
+ # Basic signature/gatekeeper check before install:
+ codesign --verify --deep --strict 'Cryptomator.app'
+ spctl --assess --type execute -vv 'Cryptomator.app'
+ trap - EXIT
+ cleanup
+ """;If you have an expected Team ID, also assert it (e.g., via codesign -dv --verbose=4 and grep TeamIdentifier).
🏁 Script executed:
#!/bin/bash
# Locate and display content of DmgUpdateMechanism.java to inspect the update script
fd src -t f -g 'DmgUpdateMechanism.java' --exec sed -n '1,200p' {} \;Length of output: 110
🏁 Script executed:
#!/bin/bash
# Find DmgUpdateMechanism.java and display lines 40–80
fd 'DmgUpdateMechanism\.java' --exec cat {} \; | head -n 80 | tail -n 41Length of output: 1965
🏁 Script executed:
#!/bin/bash
# Check for any codesign, spctl, or signature verification in the entire codebase
rg -n 'codesign|spctl|notariz' --type javaLength of output: 0
Always ensure the DMG is detached on failure and the app is properly signed before use
- In the
unpackmethod (src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java), prepend your shell script withset -euo pipefailand useso the volume is always unmounted, even if the copy or any command fails.trap 'hdiutil detach "$MOUNT_POINT" -quiet || true' EXIT
- Prefer macOS’s
dittoovercp -Rto copy the app bundle (it preserves extended attributes and ACLs). - After copying, run
(and optionally assert the expected TeamIdentifier via
codesign --verify --deep --strict 'Cryptomator.app' spctl --assess --type execute -vv 'Cryptomator.app'
codesign -dv --verbose=4), to guard against tampered or unsigned binaries.
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/macos/update/DmgUpdateMechanism.java around
lines 52 to 55, the shell script used to mount, copy and detach the DMG must be
hardened and the app copy verified: prepend the script with strict shell options
(set -euo pipefail) and add a trap that always attempts to detach the mount
point on EXIT (using a MOUNT_POINT variable), replace cp -R with ditto to
preserve extended attributes and ACLs when copying the .app bundle, and after
copying run codesign --verify --deep --strict and spctl --assess --type execute
-vv on the copied 'Cryptomator.app' (optionally parse codesign -dv --verbose=4
to assert TeamIdentifier) so the volume is unmounted on failure and the app is
properly signed and verifiable.
No description provided.