fix: address review feedback for pure cilus/vpilotconfig

ECMA-335 terminal byte (hyphen/specials), 3DES/MD5 goldens, EncodeBodyPadded length-prefix contract, cmd import forbid stub, hygiene PE path handling.
2026-08-13 13:05:41 +08:00 · 2026-07-28 22:22:54 -04:00
parent f2549d9023
commit 5699ce3792
6 changed files with 186 additions and 36 deletions
--- a/internal/clientinject/cilus/cilus.go
+++ b/internal/clientinject/cilus/cilus.go
@@ -8,6 +8,21 @@
 // The compressed length encodes the size of the following blob bytes (UTF-16
 // payload + terminal). Payload budgets for in-place PE overwrite count body
 // bytes only (UTF-16 + terminal), not the length prefix.
+//
+// # In-place PE overwrite contract
+//
+// When replacing a stock string whose body slot is budgetBytes long:
+//
+//  1. newBody := EncodeBody(s)  (must fit: FitsBudget(s, budgetBytes))
+//  2. Rewrite the compressed length prefix to len(newBody) (not budgetBytes).
+//  3. Write EncodeBodyPadded(s, budgetBytes) at the body file offset so any
+//     residual stock bytes past the real terminal are zeroed and cannot leak.
+//
+// EncodeBodyPadded is NOT a valid #US body of length budgetBytes when the
+// string is shorter than the budget: the terminal sits at len(EncodeBody(s))-1,
+// with zeros after it. Callers must always rewrite the length prefix to the
+// unpadded body length. DecodeBody on the full padded buffer will fail or
+// produce garbage — only DecodeBody(padded[:len(EncodeBody(s))]) is valid.
 package cilus

 import (
@@ -67,8 +82,10 @@ func DecodeUserString(blob []byte) (string, error) {
 }

 // EncodeBody encodes only the body (UTF-16LE + terminal), with no length
-// prefix. Used for in-place overwrite at body_file_offset when the length
-// prefix stays the same. Always succeeds for any Go string.
+// prefix. Always succeeds for any Go string.
+//
+// For in-place PE overwrite of a fixed stock slot, use EncodeBodyPadded and
+// rewrite the compressed length prefix to len(EncodeBody(s)); see package doc.
 func EncodeBody(s string) []byte {
 	// utf16.Encode handles surrogates for runes > 0xFFFF.
 	u16 := utf16.Encode([]rune(s))
@@ -132,9 +149,20 @@ func FitsBudget(s string, budgetBytes int) bool {
 	return BodyBudgetBytes(s) <= budgetBytes
 }

-// EncodeBodyPadded encodes s as a #US body and zero-fills to budgetBytes so
-// leftover stock characters cannot leak on in-place overwrite. Returns
-// ErrBudgetExceeded if the encoded body is longer than budgetBytes, and
+// EncodeBodyPadded encodes s as a #US body and zero-fills the remainder of a
+// stock slot of size budgetBytes so leftover stock characters cannot leak.
+//
+// Contract (normative for PE patchers):
+//
+//   - The returned buffer is budgetBytes long: EncodeBody(s) followed by zeros.
+//   - When len(EncodeBody(s)) < budgetBytes, the buffer is NOT a valid #US body
+//     of length budgetBytes (terminal is early; zeros are residual orphan bytes).
+//   - The caller MUST rewrite the compressed length prefix to len(EncodeBody(s)),
+//     then write this padded buffer at the body file offset.
+//   - When the new body length equals the stock budget, the length prefix may be
+//     left unchanged (same body size).
+//
+// Returns ErrBudgetExceeded if EncodeBody(s) is longer than budgetBytes, and
 // ErrInvalidBudget if budgetBytes is negative.
 func EncodeBodyPadded(s string, budgetBytes int) ([]byte, error) {
 	if budgetBytes < 0 {
@@ -153,12 +181,23 @@ func EncodeBodyPadded(s string, budgetBytes int) ([]byte, error) {
 	return out, nil
 }

-// terminalByte returns 0x00 if every UTF-16 code unit is ≤ 0x007F, else 0x01.
-// Per ECMA-335 II.24.2.4: "a final byte holding a 0 or 1; 1 if any UTF16
-// character has a high byte, or is a surrogate".
+// terminalByte returns the ECMA-335 II.24.2.4 #US terminal byte for u16.
+//
+// The final byte is 1 if and only if any UTF-16 code unit has any bit set in
+// its top byte, or its low byte is any of: 0x01–0x08, 0x0E–0x1F, 0x27, 0x2D,
+// 0x7F. Otherwise it is 0.
+//
+// Note: ASCII hyphen ('-', 0x2D) forces terminal 0x01. Stock VATSIM JWT URLs
+// containing "fsd-jwt" therefore end with 0x01, not 0x00. Conversely, many
+// BMP characters with a non-zero low byte outside the special set (e.g. 'é'
+// U+00E9) yield terminal 0x00 when the high byte is clear.
 func terminalByte(u16 []uint16) byte {
 	for _, c := range u16 {
-		if c > 0x007F {
+		lo := byte(c)
+		if c > 0x00FF ||
+			(lo >= 0x01 && lo <= 0x08) ||
+			(lo >= 0x0E && lo <= 0x1F) ||
+			lo == 0x27 || lo == 0x2D || lo == 0x7F {
 			return 0x01
 		}
 	}
--- a/internal/clientinject/cilus/cilus_test.go
+++ b/internal/clientinject/cilus/cilus_test.go
@@ -64,22 +64,43 @@ func TestRoundTripNonASCII(t *testing.T) {
 }

 func TestTerminalByte(t *testing.T) {
-	// All ASCII → terminal 0x00.
-	body := EncodeBody("hello")
-	if body[len(body)-1] != 0x00 {
-		t.Fatalf("ASCII terminal: got 0x%02x want 0x00", body[len(body)-1])
+	// ECMA-335 II.24.2.4: terminal is 1 iff any UTF-16 unit has top-byte bits
+	// set, or low byte in {0x01–0x08, 0x0E–0x1F, 0x27, 0x2D, 0x7F}.
+	cases := []struct {
+		s    string
+		term byte
+	}{
+		{"hello", 0x00}, // pure ASCII, no specials
+		{"", 0x00},
+		{"https://auth.vatsim.net/api/fsd-jwt", 0x01}, // hyphen 0x2D in fsd-jwt
+		{"https://voice1.vatsim.net", 0x00},           // no special low bytes
+		{"http://fsd.vatsim.net/", 0x00},
+		{"a-b", 0x01},  // hyphen 0x2D
+		{"a'b", 0x01},  // apostrophe 0x27
+		{"\x7f", 0x01}, // DEL
+		{"\x01", 0x01},
+		{"\x08", 0x01},
+		{"\x0e", 0x01},
+		{"\x1f", 0x01},
+		{"héllo", 0x00}, // U+00E9: high clear, low 0xE9 not special
+		{"café", 0x00},
+		{"\u0080", 0x00},     // high clear, low 0x80 not special
+		{"\u0100", 0x01},     // high byte set
+		{"\U0001F600", 0x01}, // surrogate pair → high bytes set
 	}
-
-	// High byte present → terminal 0x01.
-	body = EncodeBody("héllo")
-	if body[len(body)-1] != 0x01 {
-		t.Fatalf("non-ASCII terminal: got 0x%02x want 0x01", body[len(body)-1])
-	}
-
-	// Surrogate pair → terminal 0x01.
-	body = EncodeBody("\U0001F600")
-	if body[len(body)-1] != 0x01 {
-		t.Fatalf("surrogate terminal: got 0x%02x want 0x01", body[len(body)-1])
+	for _, tc := range cases {
+		body := EncodeBody(tc.s)
+		if body[len(body)-1] != tc.term {
+			t.Errorf("%q: terminal 0x%02x want 0x%02x", tc.s, body[len(body)-1], tc.term)
+		}
+		got, err := DecodeBody(body)
+		if err != nil {
+			t.Errorf("%q: DecodeBody: %v", tc.s, err)
+			continue
+		}
+		if got != tc.s {
+			t.Errorf("%q: DecodeBody got %q", tc.s, got)
+		}
 	}
 }

@@ -176,6 +197,30 @@ func TestEncodeBodyPadded(t *testing.T) {
 	if !errors.Is(err, ErrInvalidBudget) {
 		t.Fatalf("want ErrInvalidBudget, got %v", err)
 	}
+
+	// Footgun: full padded buffer is NOT a valid #US body of budget length
+	// (terminal is early; zeros follow). DecodeBody on full pad must not
+	// round-trip to s. Callers must rewrite length prefix to unpadded body len.
+	if _, err := DecodeBody(padded); err == nil {
+		// Even length? budget 10 is even → DecodeBody rejects even length.
+		// Prefer asserting it does not equal s if it somehow decoded.
+		t.Fatal("DecodeBody(full padded) should fail (even length or bad terminal position)")
+	}
+	// Odd budget with residual zeros after terminal: body of "hi" is 5 bytes
+	// (h,0,i,0,term); pad to 7 → even? 7 is odd. term at index 4, then zeros.
+	// DecodeBody treats last byte as terminal (0) and chars include trailing
+	// zero units — not equal to "hi", or terminal mismatch if term was 0x01.
+	oddPad, err := EncodeBodyPadded("a-b", 9) // body has hyphen → term 0x01; body len 7
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(EncodeBody("a-b")) != 7 {
+		t.Fatalf("a-b body len %d", len(EncodeBody("a-b")))
+	}
+	// Full pad DecodeBody: last byte is 0 (padding), not the real terminal.
+	if got, err := DecodeBody(oddPad); err == nil && got == "a-b" {
+		t.Fatal("DecodeBody(full padded) must not equal original string")
+	}
 }

 func TestCompressedLengthForms(t *testing.T) {
@@ -333,12 +378,16 @@ func TestDecodeErrors(t *testing.T) {
 	if _, err := DecodeBody([]byte{0x61, 0x00, 0x02}); !errors.Is(err, ErrInvalidBody) {
 		t.Fatalf("bad terminal: %v", err)
 	}
-	// Terminal mismatch: non-ASCII char with terminal 0.
-	// 'é' = U+00E9 → needs terminal 0x01.
-	bad := []byte{0xE9, 0x00, 0x00}
+	// Terminal mismatch: high-byte char (U+0100) requires terminal 0x01.
+	bad := []byte{0x00, 0x01, 0x00} // U+0100 LE + wrong terminal 0x00
 	if _, err := DecodeBody(bad); !errors.Is(err, ErrInvalidBody) {
 		t.Fatalf("terminal mismatch: %v", err)
 	}
+	// Hyphen requires terminal 0x01; wrong terminal 0x00.
+	badHyphen := []byte{0x2D, 0x00, 0x00}
+	if _, err := DecodeBody(badHyphen); !errors.Is(err, ErrInvalidBody) {
+		t.Fatalf("hyphen terminal mismatch: %v", err)
+	}
 }

 func TestDecodeIgnoresTrailing(t *testing.T) {
--- a/internal/clientinject/vpilotconfig/vpilotconfig.go
+++ b/internal/clientinject/vpilotconfig/vpilotconfig.go
@@ -181,6 +181,7 @@ func Format(cfg *Config) ([]byte, error) {
 		NetworkLogin:     loginEnc,
 		NetworkPassword:  passEnc,
 	}
+	// xml.MarshalIndent only fails for unsupported types; xmlRoot is fixed.
 	body, err := xml.MarshalIndent(root, "", "  ")
 	if err != nil {
 		return nil, fmt.Errorf("vpilotconfig: marshal: %w", err)
--- a/internal/clientinject/vpilotconfig/vpilotconfig_test.go
+++ b/internal/clientinject/vpilotconfig/vpilotconfig_test.go
@@ -6,6 +6,7 @@ import (
 	"crypto/des"
 	"encoding/base64"
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 )
@@ -23,12 +24,49 @@ func TestDeriveKey(t *testing.T) {
 	if !bytes.Equal(k1[16:24], k1[0:8]) {
 		t.Fatalf("key extension mismatch: %x vs %x", k1[16:24], k1[0:8])
 	}
+	// Fixed production golden (MD5 of ConfigGUID || first 8 of that MD5).
+	// MD5(5575ac09-f2de-4a1e-808b-e3398e17f8bf) = 9bf2bf12df6e3fb45c0db019e1982a10
+	wantHex := "9bf2bf12df6e3fb45c0db019e1982a109bf2bf12df6e3fb4"
+	gotHex := fmt.Sprintf("%x", k1)
+	if gotHex != wantHex {
+		t.Fatalf("DeriveKey hex:\n got %s\nwant %s", gotHex, wantHex)
+	}
 	// TripleDES accepts the key.
 	if _, err := des.NewTripleDESCipher(k1); err != nil {
 		t.Fatal(err)
 	}
 }

+// Production ciphertext goldens (3DES-ECB-PKCS7 + std Base64), cross-checked
+// against OpenSSL. Lock vPilot crypto so MD5/key/ECB/PKCS7 cannot soft-pass.
+func TestEncryptGoldens(t *testing.T) {
+	cases := []struct {
+		plain string
+		b64   string
+	}{
+		{"http://status.vatsim.net/", "pZ9u441bE4a2NCGgqMxKNvwAIy0qEA+AwXB8c3sV90c="},
+		{"", "QV3c4DoqB1Y="},
+		{"AUTOMATIC|fsd.connect.vatsim.net", "vN0yvTPHb8iCqOBSDgJOad3JH+XEldSS/AAjLSoQD4BBXdzgOioHVg=="},
+	}
+	for _, tc := range cases {
+		got, err := Encrypt(tc.plain)
+		if err != nil {
+			t.Fatalf("Encrypt(%q): %v", tc.plain, err)
+		}
+		if got != tc.b64 {
+			t.Fatalf("Encrypt(%q):\n got %s\nwant %s", tc.plain, got, tc.b64)
+		}
+		// Decrypt golden back to plaintext.
+		plain, err := Decrypt(tc.b64)
+		if err != nil {
+			t.Fatalf("Decrypt golden for %q: %v", tc.plain, err)
+		}
+		if plain != tc.plain {
+			t.Fatalf("Decrypt golden: got %q want %q", plain, tc.plain)
+		}
+	}
+}
+
 func TestEncryptDecryptRoundTrip(t *testing.T) {
 	cases := []string{
 		"",
--- a/scripts/check-hygiene.sh
+++ b/scripts/check-hygiene.sh
@@ -98,15 +98,15 @@ fi
 echo "==> Hygiene: no PE binaries / *.exe / *.dll in git"

 # Fail if .research/ is somehow tracked (gitignored local RE extracts).
-research_tracked="$(git ls-files '.research' '.research/*' 2>/dev/null || true)"
-if [[ -n "$research_tracked" ]]; then
-  echo "    FAIL: .research/ must not be tracked (gitignored RE extracts only):"
-  printf '    %s\n' $research_tracked
+research_hit=0
+while IFS= read -r -d '' f; do
+  echo "    FAIL: .research/ must not be tracked (gitignored RE extracts only): $f"
+  research_hit=1
  failed=1
-fi
+done < <(git ls-files -z -- '.research' '.research/*' 2>/dev/null || true)

 pe_hit=0
-while IFS= read -r f; do
+while IFS= read -r -d '' f; do
  [[ -z "$f" || ! -f "$f" ]] && continue
  base="$(basename "$f")"
  # Extension check (case-insensitive).
@@ -118,17 +118,23 @@ while IFS= read -r f; do
      continue
      ;;
  esac
+  # Skip known text-only extensions before PE magic read (speed; legal posture
+  # still catches *.exe/*.dll above and MZ on remaining paths).
+  case "${base}" in
+    *.go|*.md|*.txt|*.yml|*.yaml|*.json|*.xml|*.html|*.css|*.js|*.ts|*.sh|*.mod|*.sum|*.toml|*.csv|*.svg|*.gitignore|*.editorconfig|Makefile|Dockerfile*|LICENSE*|NOTICE*|*.proto|*.rhai)
+      continue
+      ;;
+  esac
  # PE magic "MZ" at start of file (Windows PE / DOS stub).
-  # Skip empty files and non-regular paths already filtered.
  magic="$(dd if="$f" bs=2 count=1 2>/dev/null | LC_ALL=C od -An -tx1 | tr -d ' \n')"
  if [[ "$magic" == "4d5a" ]]; then
    echo "    FAIL: PE magic MZ at start of tracked file: $f"
    pe_hit=1
    failed=1
  fi
-done < <(git ls-files -z 2>/dev/null | tr '\0' '\n')
+done < <(git ls-files -z 2>/dev/null || true)

-if [[ "$pe_hit" -eq 0 && -z "$research_tracked" ]]; then
+if [[ "$pe_hit" -eq 0 && "$research_hit" -eq 0 ]]; then
  echo "    OK"
 fi

--- a/scripts/check-import-graph.sh
+++ b/scripts/check-import-graph.sh
@@ -248,6 +248,23 @@ check_no_imports "internal/clientinject" "${MODULE}/internal/clientinject/..." \
  "${MODULE}/internal/auth" \
  "${MODULE}/internal/serviceapi"

+# cmd/openfsd-client — auxiliary Client Setup tool; never FSD/server internals.
+# Skips cleanly until the package lands (same pattern as other missing pkgs).
+# GUI deps (e.g. Fyne) are third-party and allowed; only hard-forbid server-side
+# packages listed below. Tighten to an allowlist when the cmd exists if needed.
+check_no_imports "cmd/openfsd-client" "${MODULE}/cmd/openfsd-client/..." \
+  "${MODULE}/internal/server" \
+  "${MODULE}/internal/web" \
+  "${MODULE}/internal/afv" \
+  "${MODULE}/internal/db" \
+  "${MODULE}/internal/postoffice" \
+  "${MODULE}/internal/session" \
+  "${MODULE}/internal/cluster" \
+  "${MODULE}/internal/sweatbox" \
+  "${MODULE}/internal/metar" \
+  "${MODULE}/internal/auth" \
+  "${MODULE}/internal/serviceapi"
+
 if [[ "$failed" -ne 0 ]]; then
  echo
  echo "Import graph check FAILED. See AGENTS.md §2."