From 1db8fe4d4f521ceded584d5bca2c8d17705986ba Mon Sep 17 00:00:00 2001 From: "Vyctor H. Brzezowski" Date: Sat, 15 Aug 2026 19:47:38 -0300 Subject: [PATCH] fix(ui): reject blank required strings in Control UI update readers (#124264) * fix(ui): reject blank required strings in Control UI update readers The Control UI update readers accepted `""` for fields the canonical UpdateAvailableSchema/UpdateScheduleStateSchema declare as NonEmptyString, so a malformed Gateway payload rendered an update banner with blank version text. Root cause is the reader treating "is a string" as the contract for fields whose canonical contract is "is a non-empty string". Fixed at that boundary with the protocol's own dependency-free `isNonEmptyProtocolString` primitive: required NonEmptyString fields now reject the payload when blank, and optional ones are dropped rather than discarding the whole payload. Deliberately does NOT re-derive the canonical schema in the browser: - `additionalProperties: false` is a producer-side contract the Gateway enforces on its own outbound results (src/gateway/server-methods/update.ts). Rejecting unknown keys client-side would turn every additive protocol field into a blank overlay, since a service-worker-cached document keeps an older bundle across a Gateway upgrade. - Canonical `maxLength` counts grapheme clusters (typebox 1.3.6 guard/string.mjs), so the previous `subject.length <= 120` copy dropped valid emoji subjects the schema accepts. The bound is producer-side and is gone rather than approximated. - typebox itself stays out of this module: it is in the Control UI startup graph, which has a hard gzip budget (scripts/check-control-ui-performance.mts). Drift is now pinned in tests instead of duplicated in production: the boundary suite runs fixtures through both the reader and Value.Check over the canonical schemas, asserting that anything the schema accepts still reaches the overlay. Production LOC: net -7 against main. * fix(ui): restore the five-entry cap on tolerant commit filtering readUpdateAvailableValue's per-entry filter/map dropped the existing five-commit bound while switching from all-or-nothing rejection to tolerant filtering. The canonical schema caps commits at 5 (packages/gateway-protocol/src/schema/config.ts), and the Updates page renders every entry this reader returns, so the cap is the protocol's own render-side contract, not extra strictness. Slice to MAX_COMMITS after filtering and cover an eight-entry valid payload. --- ui/src/app/update-overlay-helpers.test.ts | 204 +++++++++++++++++++++- ui/src/app/update-schedule-dto.ts | 198 +++++++++++---------- 2 files changed, 303 insertions(+), 99 deletions(-) diff --git a/ui/src/app/update-overlay-helpers.test.ts b/ui/src/app/update-overlay-helpers.test.ts index 5346515a1265..2d0a5889aede 100644 --- a/ui/src/app/update-overlay-helpers.test.ts +++ b/ui/src/app/update-overlay-helpers.test.ts @@ -1,6 +1,11 @@ +import { Value } from "typebox/value"; +import { afterEach, describe, expect, it, vi } from "vitest"; // @vitest-environment node // Control UI tests cover localized update and recovery status copy. -import { afterEach, describe, expect, it, vi } from "vitest"; +import { + UpdateAvailableSchema, + UpdateScheduleStateSchema, +} from "../../../packages/gateway-protocol/src/schema/config.js"; import type { GatewayBrowserClient, GatewayHelloOk } from "../api/gateway.ts"; import { i18n } from "../i18n/index.ts"; import type { @@ -12,7 +17,12 @@ import { formatUpdateCampaignLabel, resolveUpdateStatusBanner, } from "./update-overlay-helpers.ts"; -import { readUpdateAvailable, readUpdateSchedule } from "./update-schedule-dto.ts"; +import { + readUpdateAvailable, + readUpdateAvailableValue, + readUpdateSchedule, + readUpdateScheduleValue, +} from "./update-schedule-dto.ts"; const translations: Record = { "updates.status": "Update {status}: {reason}. {guidance}", @@ -209,6 +219,196 @@ describe("update schedule hydration", () => { ), ).toBe("Update held ยท resumes in 12:41"); }); + + it.each([ + [ + "availability channel", + { currentVersion: "2026.8.1", latestVersion: "2026.8.2", channel: "" }, + ], + [ + "availability currentVersion", + { currentVersion: "", latestVersion: "2026.8.2", channel: "s" }, + ], + ])("rejects a blank required %s, as the canonical schema does", (_label, payload) => { + expect(readUpdateAvailableValue(payload)).toBeNull(); + expect(Value.Check(UpdateAvailableSchema, payload)).toBe(false); + }); + + it("rejects a blank required schedule channel, as the canonical schema does", () => { + const blankSchedule = { channel: "", autoEnabled: true }; + expect(readUpdateScheduleValue(blankSchedule)).toBeNull(); + expect(Value.Check(UpdateScheduleStateSchema, blankSchedule)).toBe(false); + }); + + it("drops blank optional strings instead of discarding the whole payload", () => { + expect( + readUpdateAvailableValue({ + currentVersion: "2026.8.1", + latestVersion: "2026.8.2", + channel: "stable", + currentSha: "", + upstreamRef: "", + }), + ).toEqual({ currentVersion: "2026.8.1", latestVersion: "2026.8.2", channel: "stable" }); + }); + + // The canonical schemas are closed, but they are a producer-side contract the + // Gateway enforces on its own outbound results. A service-worker-cached + // document keeps an older bundle across a Gateway upgrade, so an additive + // field must never blank the overlay. + it("keeps rendering when a newer Gateway adds an unknown field", () => { + const withFutureField = { + currentVersion: "2026.8.1", + latestVersion: "2026.8.2", + channel: "stable", + releaseNotesUrl: "https://example.invalid/notes", + }; + expect(readUpdateAvailableValue(withFutureField)).toEqual({ + currentVersion: "2026.8.1", + latestVersion: "2026.8.2", + channel: "stable", + }); + + const scheduleWithFutureField = { + channel: "dev", + autoEnabled: true, + install: { kind: "git", git: { status: "current", futureNested: 1 } }, + rolloutCohort: "canary", + }; + expect(readUpdateScheduleValue(scheduleWithFutureField)).toEqual({ + channel: "dev", + autoEnabled: true, + install: { kind: "git", git: { status: "current" } }, + }); + }); + + it("ignores prototype-named wire keys without polluting the result", () => { + const hostile = JSON.parse( + '{"currentVersion":"2026.8.1","latestVersion":"2026.8.2","channel":"stable","__proto__":{"polluted":true},"constructor":"x","toString":"y"}', + ); + const parsed = readUpdateAvailableValue(hostile); + expect(parsed).toEqual({ + currentVersion: "2026.8.1", + latestVersion: "2026.8.2", + channel: "stable", + }); + expect(Object.hasOwn(parsed as object, "toString")).toBe(false); + expect(({} as { polluted?: boolean }).polluted).toBeUndefined(); + }); + + // Canonical maxLength counts grapheme clusters; String#length counts UTF-16 + // code units, so a length-based copy of that rule drops valid emoji subjects. + it("preserves a commit subject the canonical schema accepts but String#length overcounts", () => { + const subject = "\u{1F44D}".repeat(100); + const payload = { + currentVersion: "2026.8.1", + latestVersion: "2026.8.2", + channel: "stable", + commits: [{ sha: "abc1234", subject }], + }; + expect(subject.length).toBeGreaterThan(120); + expect(Value.Check(UpdateAvailableSchema, payload)).toBe(true); + expect(readUpdateAvailableValue(payload)?.commits).toEqual([{ sha: "abc1234", subject }]); + }); + + it("keeps valid commits when a sibling entry is malformed", () => { + expect( + readUpdateAvailableValue({ + currentVersion: "2026.8.1", + latestVersion: "2026.8.2", + channel: "stable", + commits: [{ sha: "", subject: "dropped" }, { sha: "abc", subject: "kept" }, { sha: 7 }], + })?.commits, + ).toEqual([{ sha: "abc", subject: "kept" }]); + }); + + // The canonical schema caps commits at 5 entries (maxItems: 5) and the + // Updates page renders every entry this reader returns. An out-of-contract + // producer payload past that cap must not grow the rendered list. + it("caps commits at five entries even when every entry is valid", () => { + const commits = Array.from({ length: 8 }, (_, index) => ({ + sha: `sha${index}`, + subject: `commit ${index}`, + })); + expect( + readUpdateAvailableValue({ + currentVersion: "2026.8.1", + latestVersion: "2026.8.2", + channel: "stable", + commits, + })?.commits, + ).toEqual(commits.slice(0, 5)); + }); + + // Drift guard: whatever the canonical schema accepts must still reach the + // overlay. This fails if a schema change outgrows the reader. + it.each([ + [ + "availability", + { currentVersion: "2026.8.1", latestVersion: "2026.8.2", channel: "stable" }, + UpdateAvailableSchema, + readUpdateAvailableValue, + ], + [ + "availability with git detail", + { + currentVersion: "2026.8.1", + latestVersion: "2026.8.2", + channel: "dev", + currentSha: "aaa", + upstreamRef: "origin/main", + upstreamSha: "bbb", + commitsBehind: 3, + commits: [{ sha: "abc", subject: "fix things" }], + }, + UpdateAvailableSchema, + readUpdateAvailableValue, + ], + [ + "schedule with package target", + { + channel: "beta", + autoEnabled: true, + install: { kind: "package" }, + target: { kind: "package", version: "2026.8.1-beta.1" }, + }, + UpdateScheduleStateSchema, + readUpdateScheduleValue, + ], + [ + "schedule with diverged git install and campaign", + { + channel: "dev", + autoEnabled: false, + install: { + kind: "git", + git: { + status: "diverged", + currentSha: "aaa", + commitAtMs: 1, + installedAtMs: 2, + commitsAhead: 1, + commitsBehind: 2, + }, + }, + target: { kind: "git", upstreamRef: "origin/main", upstreamSha: "bbb", commitsBehind: 2 }, + campaign: { + id: "c1", + state: "countdown", + announcedAtMs: 1, + applyAtMs: 2, + holdUntilMs: 3, + forceAtMs: 4, + updatedAtMs: 5, + }, + }, + UpdateScheduleStateSchema, + readUpdateScheduleValue, + ], + ])("round-trips canonical-valid %s unchanged", (_label, payload, schema, read) => { + expect(Value.Check(schema, payload)).toBe(true); + expect(read(payload)).toEqual(payload); + }); }); describe("update status localization", () => { diff --git a/ui/src/app/update-schedule-dto.ts b/ui/src/app/update-schedule-dto.ts index 48b5252bb019..885339126841 100644 --- a/ui/src/app/update-schedule-dto.ts +++ b/ui/src/app/update-schedule-dto.ts @@ -1,10 +1,39 @@ // Normalizes the Gateway's update-availability and update-schedule payloads into // the shapes the Control UI renders. These readers are the trust boundary for // wire data, so they stay separate from the lifecycle controllers that consume them. +// +// Deliberately NOT a copy of UpdateAvailableSchema/UpdateScheduleStateSchema +// (packages/gateway-protocol/src/schema/config.ts). Those are closed +// producer-side contracts the Gateway enforces on its own outbound results +// (src/gateway/server-methods/update.ts), so re-deriving them here would be a +// second contract that drifts. Rejecting unknown keys would also turn every +// additive protocol field into a blank update overlay: the Control UI is +// service-worker cached, so an already-open document keeps an older bundle +// across a Gateway upgrade (ui/src/app/sw-refresh.runtime.ts). This reader +// narrows only what the overlay renders, tolerates unknown and out-of-range +// producer data, and enforces the one rule whose violation renders blank UI: +// canonical NonEmptyString fields that are required must be non-empty. +// update-overlay-helpers.test.ts pins that against Value.Check over the +// canonical schemas; typebox stays out of this module because it sits in the +// Control UI startup graph, which has a hard gzip budget +// (scripts/check-control-ui-performance.mts). import { isRecord } from "@openclaw/normalization-core/record-coerce"; +import { isNonEmptyProtocolString } from "../../../packages/gateway-protocol/src/protocol-value-normalization.js"; import type { GatewayHelloOk } from "../api/gateway.ts"; import type { UpdateAvailable, UpdateScheduleState } from "../api/types.ts"; +/** Narrows wire counters and timestamps declared as Type.Integer({ minimum }). */ +function isBoundedInteger(value: unknown, minimum: number): value is number { + return Number.isInteger(value) && (value as number) >= minimum; +} + +// Mirrors commits: Type.Array(UpdateCommitSchema, { maxItems: 5 }) in +// packages/gateway-protocol/src/schema/config.ts. The Updates page renders +// every entry this reader returns, so the render-side cap is the protocol's +// own contract, not extra strictness: keep it even though the rest of this +// reader is tolerant of out-of-range producer data. +const MAX_COMMITS = 5; + export function readUpdateAvailable(hello: GatewayHelloOk | null): UpdateAvailable | null { const snapshot = hello?.snapshot; if (!isRecord(snapshot)) { @@ -15,84 +44,79 @@ export function readUpdateAvailable(hello: GatewayHelloOk | null): UpdateAvailab } export function readUpdateAvailableValue(update: unknown): UpdateAvailable | null { - if (!isRecord(update)) { + if ( + !isRecord(update) || + !isNonEmptyProtocolString(update.currentVersion) || + !isNonEmptyProtocolString(update.latestVersion) || + !isNonEmptyProtocolString(update.channel) + ) { return null; } + // Per-entry filtering rather than all-or-nothing: one malformed commit should + // not hide the rest of the list. Subject length stays unbounded here because + // the canonical maxLength counts grapheme clusters, which a String#length + // check silently misreads for emoji and combining marks. The MAX_COMMITS + // slice below still applies after filtering: the Updates page renders every + // returned entry, so an out-of-range producer payload must not grow the + // rendered list past the protocol's own cap. const rawCommits = update.commits; - const commits = - Array.isArray(rawCommits) && - rawCommits.length <= 5 && - rawCommits.every( - (commit): commit is { sha: string; subject: string } => - isRecord(commit) && - typeof commit.sha === "string" && - commit.sha.length > 0 && - typeof commit.subject === "string" && - commit.subject.length <= 120, - ) - ? rawCommits.map((commit) => ({ sha: commit.sha, subject: commit.subject })) - : undefined; - return typeof update.currentVersion === "string" && - typeof update.latestVersion === "string" && - typeof update.channel === "string" - ? { - currentVersion: update.currentVersion, - latestVersion: update.latestVersion, - channel: update.channel, - ...(typeof update.currentSha === "string" ? { currentSha: update.currentSha } : {}), - ...(typeof update.upstreamRef === "string" ? { upstreamRef: update.upstreamRef } : {}), - ...(typeof update.upstreamSha === "string" ? { upstreamSha: update.upstreamSha } : {}), - ...(Number.isInteger(update.commitsBehind) && Number(update.commitsBehind) >= 0 - ? { commitsBehind: Number(update.commitsBehind) } - : {}), - ...(commits ? { commits } : {}), - } - : null; + const commits = Array.isArray(rawCommits) + ? rawCommits + .filter( + (commit): commit is { sha: string; subject: string } => + isRecord(commit) && + isNonEmptyProtocolString(commit.sha) && + typeof commit.subject === "string", + ) + .map((commit) => ({ sha: commit.sha, subject: commit.subject })) + .slice(0, MAX_COMMITS) + : undefined; + return { + currentVersion: update.currentVersion, + latestVersion: update.latestVersion, + channel: update.channel, + ...(isNonEmptyProtocolString(update.currentSha) ? { currentSha: update.currentSha } : {}), + ...(isNonEmptyProtocolString(update.upstreamRef) ? { upstreamRef: update.upstreamRef } : {}), + ...(isNonEmptyProtocolString(update.upstreamSha) ? { upstreamSha: update.upstreamSha } : {}), + ...(isBoundedInteger(update.commitsBehind, 0) ? { commitsBehind: update.commitsBehind } : {}), + ...(commits?.length ? { commits } : {}), + }; } function readScheduleTarget(value: unknown): UpdateScheduleState["target"] | null { if (!isRecord(value)) { return null; } - if (value.kind === "package" && typeof value.version === "string") { - return { kind: "package", version: value.version }; + if (value.kind === "package") { + return isNonEmptyProtocolString(value.version) + ? { kind: "package", version: value.version } + : null; } - if ( - value.kind === "git" && - typeof value.upstreamRef === "string" && - typeof value.upstreamSha === "string" && - Number.isInteger(value.commitsBehind) && - Number(value.commitsBehind) >= 0 - ) { - return { - kind: "git", - upstreamRef: value.upstreamRef, - upstreamSha: value.upstreamSha, - commitsBehind: Number(value.commitsBehind), - }; + if (value.kind === "git") { + return isNonEmptyProtocolString(value.upstreamRef) && + isNonEmptyProtocolString(value.upstreamSha) && + isBoundedInteger(value.commitsBehind, 0) + ? { + kind: "git", + upstreamRef: value.upstreamRef, + upstreamSha: value.upstreamSha, + commitsBehind: value.commitsBehind, + } + : null; } return null; } +/** Optional install metadata: a malformed entry is dropped, never fatal to the status. */ function readGitInstallMetadata(value: Record): { currentSha?: string; commitAtMs?: number; installedAtMs?: number; -} | null { - if ( - (value.currentSha !== undefined && - (typeof value.currentSha !== "string" || value.currentSha.length === 0)) || - (value.commitAtMs !== undefined && - (!Number.isInteger(value.commitAtMs) || Number(value.commitAtMs) < 0)) || - (value.installedAtMs !== undefined && - (!Number.isInteger(value.installedAtMs) || Number(value.installedAtMs) < 0)) - ) { - return null; - } +} { return { - ...(typeof value.currentSha === "string" ? { currentSha: value.currentSha } : {}), - ...(value.commitAtMs === undefined ? {} : { commitAtMs: Number(value.commitAtMs) }), - ...(value.installedAtMs === undefined ? {} : { installedAtMs: Number(value.installedAtMs) }), + ...(isNonEmptyProtocolString(value.currentSha) ? { currentSha: value.currentSha } : {}), + ...(isBoundedInteger(value.commitAtMs, 0) ? { commitAtMs: value.commitAtMs } : {}), + ...(isBoundedInteger(value.installedAtMs, 0) ? { installedAtMs: value.installedAtMs } : {}), }; } @@ -103,38 +127,25 @@ function readGitUpdateStatus( return null; } const metadata = readGitInstallMetadata(value); - if (!metadata) { - return null; - } if (value.status === "current") { return { ...metadata, status: "current" }; } - if ( - value.status === "behind" && - Number.isInteger(value.commitsBehind) && - Number(value.commitsBehind) > 0 - ) { - return { ...metadata, status: "behind", commitsBehind: Number(value.commitsBehind) }; + if (value.status === "behind" && isBoundedInteger(value.commitsBehind, 1)) { + return { ...metadata, status: "behind", commitsBehind: value.commitsBehind }; } - if ( - value.status === "ahead" && - Number.isInteger(value.commitsAhead) && - Number(value.commitsAhead) > 0 - ) { - return { ...metadata, status: "ahead", commitsAhead: Number(value.commitsAhead) }; + if (value.status === "ahead" && isBoundedInteger(value.commitsAhead, 1)) { + return { ...metadata, status: "ahead", commitsAhead: value.commitsAhead }; } if ( value.status === "diverged" && - Number.isInteger(value.commitsAhead) && - Number(value.commitsAhead) > 0 && - Number.isInteger(value.commitsBehind) && - Number(value.commitsBehind) > 0 + isBoundedInteger(value.commitsAhead, 1) && + isBoundedInteger(value.commitsBehind, 1) ) { return { ...metadata, status: "diverged", - commitsAhead: Number(value.commitsAhead), - commitsBehind: Number(value.commitsBehind), + commitsAhead: value.commitsAhead, + commitsBehind: value.commitsBehind, }; } if ( @@ -153,38 +164,31 @@ function readGitUpdateStatus( function readScheduleCampaign(value: unknown): UpdateScheduleState["campaign"] | null { if ( !isRecord(value) || - typeof value.id !== "string" || + !isNonEmptyProtocolString(value.id) || (value.state !== "waiting-for-idle" && value.state !== "countdown" && value.state !== "applying") || - !Number.isInteger(value.announcedAtMs) || - Number(value.announcedAtMs) < 0 || - !Number.isInteger(value.forceAtMs) || - Number(value.forceAtMs) < 0 || - !Number.isInteger(value.updatedAtMs) || - Number(value.updatedAtMs) < 0 || - (value.applyAtMs !== undefined && - (!Number.isInteger(value.applyAtMs) || Number(value.applyAtMs) < 0)) || - (value.holdUntilMs !== undefined && - (!Number.isInteger(value.holdUntilMs) || Number(value.holdUntilMs) < 0)) + !isBoundedInteger(value.announcedAtMs, 0) || + !isBoundedInteger(value.forceAtMs, 0) || + !isBoundedInteger(value.updatedAtMs, 0) ) { return null; } return { id: value.id, state: value.state, - announcedAtMs: Number(value.announcedAtMs), - ...(value.applyAtMs === undefined ? {} : { applyAtMs: Number(value.applyAtMs) }), - ...(value.holdUntilMs === undefined ? {} : { holdUntilMs: Number(value.holdUntilMs) }), - forceAtMs: Number(value.forceAtMs), - updatedAtMs: Number(value.updatedAtMs), + announcedAtMs: value.announcedAtMs, + ...(isBoundedInteger(value.applyAtMs, 0) ? { applyAtMs: value.applyAtMs } : {}), + ...(isBoundedInteger(value.holdUntilMs, 0) ? { holdUntilMs: value.holdUntilMs } : {}), + forceAtMs: value.forceAtMs, + updatedAtMs: value.updatedAtMs, }; } export function readUpdateScheduleValue(value: unknown): UpdateScheduleState | null { if ( !isRecord(value) || - typeof value.channel !== "string" || + !isNonEmptyProtocolString(value.channel) || typeof value.autoEnabled !== "boolean" ) { return null;