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.
This commit is contained in:
Vyctor H. Brzezowski
2026-08-15 19:47:38 -03:00
committed by GitHub
parent 5374c110fb
commit 1db8fe4d4f
2 changed files with 303 additions and 99 deletions
+202 -2
View File
@@ -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<string, string> = {
"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", () => {
+101 -97
View File
@@ -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<string, unknown>): {
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;