Major update for stability

2026-09-08 15:22:19 +01:00
parent 3b95f53a25
commit b2f3c5aef6
24 changed files with 1059 additions and 212 deletions
@@ -12,31 +12,16 @@ struct RsyncCommandBuilder {

        if job.compress { args.append("-z") }

-        // Apple's bundled /usr/bin/rsync uses the Apple-specific -E flag for
-        // extended attributes. That flag is not compatible with a standard
-        // rsync server over SSH (for example Linux rsync 3.x), because Apple
-        // rsync expands it to --extended-attributes on the remote side.
-        //
-        // Therefore:
-        //   • Apple rsync + local job  -> -E
-        //   • Apple rsync + SSH job    -> omit xattr flag
-        //   • Modern/Homebrew rsync    -> -X (standard rsync xattrs)
        if job.preserveExtendedAttributes {
            if isAppleRsync(executable) {
-                if !job.isRemote {
-                    args.append("-E")
-                }
+                if !job.isRemote { args.append("-E") }
            } else {
                args.append("-X")
            }
        }

        if job.deleteExtraneous { args.append("--delete") }
-
-        // --info=progress2 is not supported by Apple's bundled rsync 2.6.9.
-        // --progress works on both Apple's rsync and modern Homebrew rsync.
        if job.showProgress { args.append("--progress") }
-
        if dryRun { args.append("--dry-run") }

        args += ["--human-readable", "--stats"]
@@ -50,14 +35,24 @@ struct RsyncCommandBuilder {
        }

        if job.isRemote {
-            var sshParts = ["ssh", "-p", String(job.remotePort)]
+            var sshParts = [
+                "ssh",
+                "-o", "BatchMode=yes",
+                "-o", "ConnectTimeout=15",
+                "-o", "ServerAliveInterval=30",
+                "-o", "ServerAliveCountMax=3",
+                "-p", String(job.remotePort)
+            ]
            let keyPath = normalizedPrivateKeyPath(job.sshKeyPath)
            if !keyPath.isEmpty {
-                sshParts += ["-i", keyPath]
+                sshParts += ["-o", "IdentitiesOnly=yes", "-i", keyPath]
            }
            args += ["-e", sshParts.map(shellEscape).joined(separator: " ")]
        }

+        // End option parsing before user-controlled filesystem operands. This prevents
+        // a local path beginning with '-' from being interpreted as another rsync option.
+        args.append("--")
        args.append(normalizeSource(job.sourcePath))
        args.append(job.destinationDisplay)
        return args
@@ -71,22 +66,21 @@ struct RsyncCommandBuilder {

    static func normalizedPrivateKeyPath(_ path: String) -> String {
        let trimmed = path.trimmingCharacters(in: .whitespacesAndNewlines)
-        guard trimmed.hasSuffix(".pub") else { return trimmed }
+        guard trimmed.hasSuffix(".pub") else { return (trimmed as NSString).expandingTildeInPath }

        let privatePath = String(trimmed.dropLast(4))
-        if FileManager.default.fileExists(atPath: privatePath) {
-            return privatePath
+        let expandedPrivatePath = (privatePath as NSString).expandingTildeInPath
+        if FileManager.default.fileExists(atPath: expandedPrivatePath) {
+            return expandedPrivatePath
        }
-        return trimmed
+        return (trimmed as NSString).expandingTildeInPath
    }

    private static func isAppleRsync(_ executable: String) -> Bool {
        executable == "/usr/bin/rsync"
    }

-    private static func normalizeSource(_ path: String) -> String {
-        path
-    }
+    private static func normalizeSource(_ path: String) -> String { path }

    static func shellEscape(_ value: String) -> String {
        if value.isEmpty { return "''" }
@@ -130,3 +124,169 @@ struct RsyncCommandBuilder {
        return result
    }
 }
+
+enum RsyncSecurityValidator {
+    static func validationMessage(for job: SyncJob, trigger: SyncRunTrigger = .manual) -> String? {
+        let source = job.sourcePath.trimmingCharacters(in: .whitespacesAndNewlines)
+        let destination = job.destinationPath.trimmingCharacters(in: .whitespacesAndNewlines)
+        guard !source.isEmpty, !destination.isEmpty else {
+            return "Source and destination are required."
+        }
+
+        if containsControlCharacters(source) || containsControlCharacters(destination) {
+            return "Paths may not contain control characters."
+        }
+
+        if job.watchEnabled && containsControlCharacters(job.effectiveWatchPath) {
+            return "The file-watch path may not contain control characters."
+        }
+
+        if job.exclusions.contains(where: containsControlCharacters) {
+            return "Exclusion patterns may not contain control characters."
+        }
+
+        if let advancedError = validateAdditionalArguments(job.additionalArguments) {
+            return advancedError
+        }
+
+        if job.isRemote {
+            let host = job.remoteHost.trimmingCharacters(in: .whitespacesAndNewlines)
+            let user = job.remoteUser.trimmingCharacters(in: .whitespacesAndNewlines)
+            guard isValidHost(host) else {
+                return "The SSH host is invalid. Use a hostname or IP address without spaces or command-line option prefixes."
+            }
+            if !user.isEmpty, !isValidUser(user) {
+                return "The SSH username is invalid. Use letters, numbers, '.', '_' or '-'."
+            }
+            guard (1...65535).contains(job.remotePort) else {
+                return "The SSH port must be between 1 and 65535."
+            }
+            if destination.hasPrefix("-") {
+                return "A remote destination may not begin with '-'."
+            }
+            if !isSafeRemotePath(destination) {
+                return "The remote destination contains characters that are unsafe for an SSH/rsync remote shell. Use a normal path containing letters, numbers, spaces, '/', '.', '_', '-', '~', '+', '@', '%' or '=' only."
+            }
+            if destination == host || destination == "\(user)@\(host)" {
+                return "Remote destination must be a path on the remote machine, not the host address."
+            }
+
+            let keyPath = RsyncCommandBuilder.normalizedPrivateKeyPath(job.sshKeyPath)
+            if !job.sshKeyPath.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+                if keyPath.hasSuffix(".pub") {
+                    return "Select the private SSH key, not the .pub public key."
+                }
+                var isDirectory: ObjCBool = false
+                guard FileManager.default.fileExists(atPath: keyPath, isDirectory: &isDirectory), !isDirectory.boolValue else {
+                    return "The selected private SSH key does not exist or is not a regular file."
+                }
+            }
+        } else if pathsOverlap(source: source, destination: destination) {
+            return "Source and destination overlap. Choose separate directory trees to prevent recursive copies or file-watch loops."
+        }
+
+        if job.deleteExtraneous,
+           (trigger == .scheduled || trigger == .watched),
+           !job.allowUnattendedDelete {
+            return "This job uses --delete but unattended destructive sync is not authorised. Edit the job and explicitly enable unattended delete."
+        }
+
+        return nil
+    }
+
+    private static func validateAdditionalArguments(_ raw: String) -> String? {
+        if !hasBalancedShellSyntax(raw) {
+            return "Advanced rsync arguments contain an unmatched quote or trailing escape character."
+        }
+
+        let arguments = RsyncCommandBuilder.shellSplit(raw)
+        let blockedExact: Set<String> = [
+            "-e", "-M", "--rsh", "--rsync-path", "--remote-option",
+            "--password-file", "--daemon", "--config", "--port", "--address",
+            "--sockopts", "--remove-source-files", "--copy-as", "--files-from",
+            "--exclude-from", "--include-from", "--log-file", "--log-file-format",
+            "--backup-dir", "--temp-dir", "--partial-dir", "--chown", "--usermap",
+            "--groupmap", "--rsync-path", "--write-batch", "--only-write-batch",
+            "--read-batch"
+        ]
+        let blockedPrefixes = [
+            "--rsh=", "--rsync-path=", "--remote-option=", "--password-file=",
+            "--config=", "--port=", "--address=", "--sockopts=", "--copy-as=",
+            "--delete", "--remove-source-files", "--files-from=", "--exclude-from=",
+            "--include-from=", "--log-file=", "--log-file-format=", "--backup-dir=",
+            "--temp-dir=", "--partial-dir=", "--chown=", "--usermap=", "--groupmap=",
+            "--write-batch=", "--only-write-batch=", "--read-batch="
+        ]
+
+        for argument in arguments {
+            if blockedExact.contains(argument) || blockedPrefixes.contains(where: { argument.hasPrefix($0) }) {
+                return "Advanced argument '\(argument)' is security-sensitive or destructive and is managed by Rsync Manager. Remove it and use the dedicated job controls instead."
+            }
+            if containsControlCharacters(argument) {
+                return "Advanced rsync arguments may not contain control characters."
+            }
+        }
+        return nil
+    }
+
+
+    private static func isSafeRemotePath(_ value: String) -> Bool {
+        guard !value.isEmpty, !containsControlCharacters(value) else { return false }
+        // Rsync invokes the remote side through SSH. Keep remote paths out of shell
+        // metacharacter territory so a configured path can never become a remote command.
+        let allowed = CharacterSet.alphanumerics.union(CharacterSet(charactersIn: " /._-~+@%="))
+        return value.unicodeScalars.allSatisfy { allowed.contains($0) }
+    }
+
+    private static func hasBalancedShellSyntax(_ value: String) -> Bool {
+        var quote: Character?
+        var escaping = false
+        for ch in value {
+            if escaping {
+                escaping = false
+                continue
+            }
+            if ch == "\\" {
+                escaping = true
+                continue
+            }
+            if let activeQuote = quote {
+                if ch == activeQuote { quote = nil }
+                continue
+            }
+            if ch == "\"" || ch == "'" { quote = ch }
+        }
+        return quote == nil && !escaping
+    }
+
+    private static func isValidHost(_ value: String) -> Bool {
+        guard !value.isEmpty, !value.hasPrefix("-"), !containsControlCharacters(value) else { return false }
+        let allowed = CharacterSet.alphanumerics.union(CharacterSet(charactersIn: ".-_:[]"))
+        return value.unicodeScalars.allSatisfy { allowed.contains($0) }
+    }
+
+    private static func isValidUser(_ value: String) -> Bool {
+        guard !value.hasPrefix("-"), !containsControlCharacters(value) else { return false }
+        let allowed = CharacterSet.alphanumerics.union(CharacterSet(charactersIn: "._-"))
+        return value.unicodeScalars.allSatisfy { allowed.contains($0) }
+    }
+
+    private static func containsControlCharacters(_ value: String) -> Bool {
+        value.unicodeScalars.contains { CharacterSet.controlCharacters.contains($0) }
+    }
+
+    private static func pathsOverlap(source: String, destination: String) -> Bool {
+        let src = canonicalLocalPath(source)
+        let dst = canonicalLocalPath(destination)
+        guard !src.isEmpty, !dst.isEmpty else { return false }
+        if src == dst { return true }
+        return dst.hasPrefix(src + "/") || src.hasPrefix(dst + "/")
+    }
+
+    private static func canonicalLocalPath(_ value: String) -> String {
+        let expanded = (value as NSString).expandingTildeInPath
+        let standardized = (expanded as NSString).standardizingPath
+        if standardized == "/" { return standardized }
+        return standardized.hasSuffix("/") ? String(standardized.dropLast()) : standardized
+    }
+}