fix: OTA-Log im Recovery-Skip-Pfad nicht mehr als Fehlschlag ausgeben

Auf dem Überspring-Pfad (Recovery lehnt ein Image ab, weil es bereits die
laufende Version trägt) bleibt s_error absichtlich NULL, landete aber
trotzdem in printf("ota: failed (rc=%d, %s)\r\n", rc, s_error) - ein
Nullzeiger an %s ist undefiniertes Verhalten und wurde bislang nur zufällig
von newlib als "(null)" abgefangen. Zusätzlich widersprach die Meldung
"failed" der direkt folgenden Zeile "recovery: image already up to date,
continuing normal boot".

ota_task() unterscheidet jetzt explizit zwischen dem Skip-Fall (eigene,
ehrliche "ota: skipped"-Zeile) und dem echten Fehlschlag (bestehende
"ota: failed"-Zeile, weiterhin mit dem "download"-Fallback für ein leeres
s_error). rc bleibt reiner Diagnosewert im Log, da das SDK den Transfer in
beiden Fällen abbricht, sobald der Trailer-Hook das Image ablehnt.

Restliche Leser von s_error/nl_ota_last_error() (nl_recovery.c, nl_api.c)
waren bereits NULL-sicher und mussten nicht angepasst werden.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

src/sys/nl_ota.c

@@ -94,10 +94,19 @@ static void ota_task(void *arg)
     (void)arg;
     log_url(s_url);
     rc = t_http_fwup(s_url);
-    /* Only reached on failure - the SDK resets the chip itself after a good image. */
-    if (s_error == NULL && !s_skipped)
+    /* Only reached on failure or a deliberate skip - the SDK resets the chip itself after a good
+     * image. rc is not a reliable success/failure signal here: the SDK aborts the transfer as
+     * soon as the trailer hook returns false, which happens both for a real rejection and for the
+     * deliberate "recovery image already up to date" skip. s_error/s_skipped are what actually
+     * distinguish the two, so branch on those instead of folding everything into one printf that
+     * would risk passing a NULL s_error to %s. */
+    if (s_skipped) {
+        printf("ota: skipped (rc=%d, image already up to date)\r\n", rc);
+    } else {
+        if (s_error == NULL)
             s_error = "download";
         printf("ota: failed (rc=%d, %s)\r\n", rc, s_error);
+    }
     {
         nl_settings_t *s;
         nl_app_lock();

tools/station/ninlight-station

@@ -113,7 +113,19 @@ PY
     # "status" soll mit 0 enden, sobald es seine Auskunft geben konnte, auch wenn noch nie
     # ausgeliefert wurde - ohne das "|| true" waere der Exit-Code des Skripts hier der des
     # fehlgeschlagenen Tests (1), weil das die letzte Anweisung im Zweig ist.
-    [ -r /run/ninlight-station/status.json ] && { echo "Zuletzt ausgeliefert:"; sed 's/^/  /' /run/ninlight-station/status.json; }
+    #
+    # "existiert nicht" (Station lief noch nie) und "existiert, ist aber nicht lesbar"
+    # sind zwei verschiedene Zustaende und duerfen nicht beide stillschweigend im
+    # "-r"-Test verschwinden: sonst faellt ein Rechte-Fehler auf status.json (siehe
+    # server.py, schreibt als root) nie auf, weil die Ausgabe genauso aussieht wie
+    # "noch nie ausgeliefert".
+    STATUS_FILE=/run/ninlight-station/status.json
+    if [ -r "$STATUS_FILE" ]; then
+        echo "Zuletzt ausgeliefert:"
+        sed 's/^/  /' "$STATUS_FILE"
+    elif [ -e "$STATUS_FILE" ]; then
+        echo "Fehler: $STATUS_FILE existiert, ist aber nicht lesbar (Rechte pruefen)" >&2
+    fi
     exit 0
     ;;
 *)

tools/station/server.py

@@ -10,6 +10,7 @@ Standard library only - the Pi gets no extra packages.
 from __future__ import annotations
 
 import argparse
+import contextlib
 import json
 import os
 import re
@@ -85,9 +86,24 @@ class Station:
             try:
                 self.state_path.parent.mkdir(parents=True, exist_ok=True)
                 fd, tmp = tempfile.mkstemp(dir=self.state_path.parent, prefix=".status-")
+                try:
                     with os.fdopen(fd, "w") as f:
                         json.dump(payload, f, indent=2)
+                    # The service runs as root and mkstemp() creates the file 0600, so
+                    # without this an operator reading status.json as a normal user (see
+                    # tools/station/ninlight-station) gets "permission denied" - and since
+                    # os.replace() preserves the mode it arrives with, that wrong mode
+                    # would otherwise survive onto the live file on every single write,
+                    # not just the first. os.chmod() ignores the process umask (unlike the
+                    # mode argument to mkstemp/open), so a restrictive umask on the Pi
+                    # can't quietly bring the 0600 back. Set it before the rename so
+                    # there's never a window where the path holds a 0600 file.
+                    os.chmod(tmp, 0o644)
                     os.replace(tmp, self.state_path)
+                except BaseException:
+                    with contextlib.suppress(OSError):
+                        os.unlink(tmp)
+                    raise
             except OSError as e:
                 print(f"warning: cannot write {self.state_path}: {e}", flush=True)
 

tools/station/tests/test_server.py

@@ -1,7 +1,9 @@
 import hashlib
 import http.client
 import json
+import os
 import socket
+import stat
 import subprocess
 import sys
 import threading
@@ -173,6 +175,43 @@ def test_state_file_records_a_missing_image(station):
     assert state["recent"][-1]["target"] == "g95_cw"
 
 
+def test_state_file_is_world_readable_even_under_a_restrictive_umask(tmp_path):
+    """status.json is written by a root service but read by a normal-user CLI.
+
+    tempfile.mkstemp() creates its file 0600 and os.replace() preserves whatever mode
+    the temp file had, so the server must explicitly widen the mode before the rename -
+    and must do so with chmod() rather than relying on the mode passed to mkstemp/open,
+    because that one is still narrowed by the process umask. A restrictive umask (as
+    root services on the Pi may well run under) must not bring the 0600 back.
+    """
+    old_umask = os.umask(0o077)
+    try:
+        s = Station(tmp_path)
+    finally:
+        os.umask(old_umask)
+    try:
+        make_image(s.image_dir / "ninlight_evk_ota.img", "evk", "1.0.0", b"w" * 100)
+
+        def world_readable() -> bool:
+            mode = stat.S_IMODE(s.state.stat().st_mode)
+            return bool(mode & stat.S_IROTH)
+
+        # Written once already at startup (Station.__init__ -> write_state()).
+        s.read_state(lambda _: world_readable())
+        assert world_readable(), "status.json is not world-readable after the initial write"
+
+        # A second write (a real delivery) must not lose the mode either - e.g. if the
+        # widening were only applied to the file's very first creation instead of to
+        # every temp file, mode 0600 would silently come back on every later write.
+        c = s.conn()
+        c.request("GET", "/ninlight_evk_ota.img")
+        c.getresponse().read()
+        s.read_state(lambda st: st["served"] >= 1)
+        assert world_readable(), "status.json lost its mode on a later overwrite"
+    finally:
+        s.stop()
+
+
 def test_recent_is_capped_at_eight(station):
     make_image(station.image_dir / "ninlight_evk_ota.img", "evk", "1.0.0", b"q" * 10)
     for _ in range(12):