fix(updater): read the designated requirement off the stream it is on (#708) (#717)

`codesign -d -r-` writes the requirement to stdout and puts only the `-d`
display header (`Executable=…`) on stderr. `signing_requirement` searched
stderr, so the `designated => ` prefix could never match and every in-app
update on macOS ended at "codesign did not report a designated
requirement" — every build, every channel, with nothing a user could do
but download the app again by hand.

Verified against codesign rather than reasoned about:

    $ codesign -d -r- /bin/ls
    stdout: designated => identifier "com.apple.ls" and anchor apple
    stderr: Executable=/bin/ls

Both streams are read now, stdout first. Which half goes where is
codesign's own business and has moved before; a requirement printed
anywhere in the output is the requirement, and the updater has no reason
to be the stricter party about where it appeared.

The parse is split out of the process call, which is the part that
matters for it staying fixed. Fused to `Command::output`, it could only
run against a real signed bundle, so nothing in a test suite ever
executed it — that is why a total failure of the macOS update path
shipped and stayed. `/bin/ls` is the bundle it was missing: Apple-signed,
on every macOS, and it answers `-d -r-` with a requirement of its own, so
the stream split is now asserted against the tool instead of against our
belief about it.

Both tests were run against the old stderr-only parse; both fail there.

CHANGELOG.md

@@ -153,6 +153,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **In-app updates work again on macOS** (#708). Every "Update and Relaunch"
+  failed with `codesign did not report a designated requirement`, on every
+  build and both channels, with nothing a user could do but download the app by
+  hand. The updater compares the signing requirement of the staged app against
+  the installed one, and read `codesign -d -r-`'s answer off stderr — where
+  codesign puts only the `Executable=` header. The requirement is on stdout, so
+  the check could never match. Both streams are read now, and the parse is
+  split from the process call so a test can hold it against codesign itself
+  rather than against our belief about it.
+
 - **A tree pull that has to be retried no longer ends with the window deleting
   the tabs it was pulling.** A window told to rebuild itself from the machine —
   a daemon back as a new process, a restart handoff that was refused, a remote

src/bin/tty7-updater.rs

@@ -278,6 +278,27 @@ mod macos {
         Ok(String::from_utf8_lossy(&output.stdout).trim().to_string())
     }
 
+    /// The designated requirement out of what `codesign -d -r-` printed.
+    ///
+    /// Split from the call below so the parse can be exercised without a
+    /// bundle to point at — which is why nothing caught it reading the wrong
+    /// stream. `codesign` writes the requirement to **stdout** and puts only
+    /// the `-d` display header (`Executable=…`) on stderr, so a parse that
+    /// read stderr could never match: every in-app update on macOS failed
+    /// with "codesign did not report a designated requirement", on every
+    /// build and every release, with nothing a user could do about it (#708).
+    ///
+    /// Both streams are read, stdout first. Which stream carries which half is
+    /// codesign's own business and has moved before; a requirement found
+    /// anywhere in the output is the requirement, and the updater has no
+    /// reason to be the stricter party about where it was printed.
+    fn designated_requirement(stdout: &str, stderr: &str) -> Option<String> {
+        [stdout, stderr]
+            .into_iter()
+            .flat_map(str::lines)
+            .find_map(|line| line.strip_prefix("designated => ").map(str::to_string))
+    }
+
     fn signing_requirement(app: &Path) -> Result<String, String> {
         let output = Command::new("/usr/bin/codesign")
             .args(["-d", "-r-"])
@@ -296,10 +317,11 @@ mod macos {
                 String::from_utf8_lossy(&output.stderr).trim()
             ));
         }
-        String::from_utf8_lossy(&output.stderr)
-            .lines()
-            .find_map(|line| line.strip_prefix("designated => ").map(str::to_string))
-            .ok_or_else(|| "codesign did not report a designated requirement".to_string())
+        designated_requirement(
+            &String::from_utf8_lossy(&output.stdout),
+            &String::from_utf8_lossy(&output.stderr),
+        )
+        .ok_or_else(|| "codesign did not report a designated requirement".to_string())
     }
 
     fn replace_and_relaunch(
@@ -457,6 +479,58 @@ mod macos {
     mod tests {
         use super::*;
 
+        /// The stream split, held against `codesign` itself rather than
+        /// against what the updater believes about it.
+        ///
+        /// This is the test #708 was missing. The parse read stderr, where
+        /// `codesign` puts only the display header, so every in-app update on
+        /// macOS failed — and no unit test could see it, because the parse was
+        /// fused to the process call and the process needs a signed bundle.
+        ///
+        /// `/bin/ls` is that bundle: Apple-signed, present on every macOS, and
+        /// it answers `-d -r-` with a designated requirement of its own. If
+        /// this ever fails because the requirement moved streams again, the
+        /// function under test already reads both — so it failing means
+        /// `codesign` stopped printing one at all, which the updater must not
+        /// discover from a user's failed update.
+        #[test]
+        fn the_designated_requirement_is_read_off_the_stream_codesign_uses() {
+            let out = Command::new("/usr/bin/codesign")
+                .args(["-d", "-r-", "/bin/ls"])
+                .output()
+                .expect("codesign is part of macOS");
+            assert!(out.status.success(), "codesign refused /bin/ls");
+
+            let stdout = String::from_utf8_lossy(&out.stdout);
+            let stderr = String::from_utf8_lossy(&out.stderr);
+            let requirement = designated_requirement(&stdout, &stderr)
+                .expect("codesign reports a designated requirement for /bin/ls");
+            assert!(
+                requirement.contains("identifier"),
+                "a designated requirement names an identifier: {requirement:?}"
+            );
+
+            // Named rather than merely relied on: the updater used to read
+            // only the stream that carries none of it.
+            assert!(
+                stdout.contains("designated => "),
+                "the requirement is on stdout; if this moved, so must the doc above"
+            );
+        }
+
+        /// Reading both streams is what keeps the choice above from being a
+        /// guess about a future macOS.
+        #[test]
+        fn a_requirement_on_either_stream_is_found_and_neither_is_an_error() {
+            let line = "designated => identifier \"com.example.app\" and anchor apple";
+            let want = Some("identifier \"com.example.app\" and anchor apple".to_string());
+
+            assert_eq!(designated_requirement(line, "Executable=/x"), want);
+            assert_eq!(designated_requirement("Executable=/x", line), want);
+            assert_eq!(designated_requirement("Executable=/x", ""), None);
+            assert_eq!(designated_requirement("", ""), None);
+        }
+
         fn bundle(path: &Path, marker: &str) {
             fs::create_dir_all(path.join("Contents/MacOS")).unwrap();
             fs::write(path.join("marker"), marker).unwrap();