[codex] fix(onboard): normalize channel setup metadata#66706
[codex] fix(onboard): normalize channel setup metadata#66706vincentkoc merged 2 commits intomainfrom
Conversation
openclaw-barnacle
bot
added
commands
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2943ca6651
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const selectionLabel = | ||
| normalizeOptionalString(next?.selectionLabel) ?? | ||
| normalizeOptionalString(existing?.selectionLabel) ?? |
There was a problem hiding this comment.
When a bundled channel registration provides meta.label but omits selectionLabel, this fallback order picks existing.selectionLabel before the newly resolved label, so setup pickers can keep showing the old canonical name even though other surfaces now use the new label. This inconsistency is introduced by the new normalization path and is visible because selection UIs prefer selectionLabel when present; falling back to label before existing.selectionLabel avoids mismatched channel names.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes the Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/plugins/channel-validation.ts
Line: 41-43
Comment:
**Inconsistent missing-field check for `blurb`**
The `blurb` check uses `typeof meta?.blurb !== "string"`, while every other field uses `!normalizeOptionalString(...)`. `normalizeOptionalString("")` returns `undefined`, so `normalizeChannelMeta` will silently fall back to the existing bundled blurb when a plugin supplies `blurb: ""` — but this check won't add `"blurb"` to the missing list, so no warning is emitted. The diagnostic under-reports repair for that case. Using the same predicate as the other fields would keep the two in sync:
```suggestion
if (!normalizeOptionalString(meta?.blurb)) {
missing.push("blurb");
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/commands/channel-setup/discovery.ts
Line: 100-106
Comment:
**Redundant double-normalization for catalog entries**
`installedCatalogEntries` and `installableCatalogEntries` are already normalized in the `.map()` step above, so the `normalizeChannelMeta` calls inside the later `for` loops (lines ~144–152 and ~155–163) re-normalize an already-normalized value with `existing: undefined` (the `!metaById.has` guard ensures the slot is empty). The second pass is idempotent but wasteful. Since the catalog entries are only written into `metaById` when they have no prior slot, the `.map()` normalization is redundant instead — dropping it there and keeping the loop normalization (where `existing` could eventually be populated by an earlier source) would be cleaner.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(onboard): normalize channel setup me..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (typeof meta?.blurb !== "string") { | ||
| missing.push("blurb"); | ||
| } |
There was a problem hiding this comment.
blurb
The blurb check uses typeof meta?.blurb !== "string", while every other field uses !normalizeOptionalString(...). normalizeOptionalString("") returns undefined, so normalizeChannelMeta will silently fall back to the existing bundled blurb when a plugin supplies blurb: "" — but this check won't add "blurb" to the missing list, so no warning is emitted. The diagnostic under-reports repair for that case. Using the same predicate as the other fields would keep the two in sync:
| if (typeof meta?.blurb !== "string") { | |
| missing.push("blurb"); | |
| } | |
| if (!normalizeOptionalString(meta?.blurb)) { | |
| missing.push("blurb"); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/channel-validation.ts
Line: 41-43
Comment:
**Inconsistent missing-field check for `blurb`**
The `blurb` check uses `typeof meta?.blurb !== "string"`, while every other field uses `!normalizeOptionalString(...)`. `normalizeOptionalString("")` returns `undefined`, so `normalizeChannelMeta` will silently fall back to the existing bundled blurb when a plugin supplies `blurb: ""` — but this check won't add `"blurb"` to the missing list, so no warning is emitted. The diagnostic under-reports repair for that case. Using the same predicate as the other fields would keep the two in sync:
```suggestion
if (!normalizeOptionalString(meta?.blurb)) {
missing.push("blurb");
}
```
How can I resolve this? If you propose a fix, please make it concise.| .map((entry) => ({ | ||
| ...entry, | ||
| meta: normalizeChannelMeta({ | ||
| id: entry.id as ChannelChoice, | ||
| meta: entry.meta, | ||
| }), | ||
| })); |
There was a problem hiding this comment.
installedCatalogEntries and installableCatalogEntries are already normalized in the .map() step above, so the normalizeChannelMeta calls inside the later for loops (lines ~144–152 and ~155–163) re-normalize an already-normalized value with existing: undefined (the !metaById.has guard ensures the slot is empty). The second pass is idempotent but wasteful. Since the catalog entries are only written into metaById when they have no prior slot, the .map() normalization is redundant instead — dropping it there and keeping the loop normalization (where existing could eventually be populated by an earlier source) would be cleaner.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/channel-setup/discovery.ts
Line: 100-106
Comment:
**Redundant double-normalization for catalog entries**
`installedCatalogEntries` and `installableCatalogEntries` are already normalized in the `.map()` step above, so the `normalizeChannelMeta` calls inside the later `for` loops (lines ~144–152 and ~155–163) re-normalize an already-normalized value with `existing: undefined` (the `!metaById.has` guard ensures the slot is empty). The second pass is idempotent but wasteful. Since the catalog entries are only written into `metaById` when they have no prior slot, the `.map()` normalization is redundant instead — dropping it there and keeping the loop normalization (where `existing` could eventually be populated by an earlier source) would be cleaner.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
vincentkoc
force-pushed
the
fix/channel-setup-metadata-normalization
branch
from
2943ca6 to
4d83843
Compare
🔒 Aisle Security AnalysisWe found 4 potential security issue(s) in this PR:
1. 🟠 External plugin can spoof bundled channel identity by reusing core channel id (e.g., "telegram") and inheriting canonical metadata
Description
Because the manifest registry allows non-bundled plugins to override bundled plugin ids (see comment: “Bundled plugin ids are reserved unless the operator explicitly overrides them.”), an attacker who can get a plugin loaded from
This creates a phishing/trust-boundary bypass risk: users may enter channel credentials/tokens believing they are configuring the bundled channel, enabling credential exfiltration. Vulnerable code: meta: normalizeChannelMeta({
id,
meta: rawMeta,
existing: resolveBundledChannelMeta(id),
})RecommendationDo not allow non-bundled plugins to inherit bundled channel metadata (or reuse bundled ids) without an explicit, high-friction opt-in. Options:
Example (conceptual): const bundled = resolveBundledChannelMeta(id);
const canInheritBundledMeta = params.source.startsWith(BUNDLED_PLUGINS_ROOT); // or use manifest origin
return {
...params.plugin,
id,
meta: normalizeChannelMeta({
id,
meta: rawMeta,
existing: canInheritBundledMeta ? bundled : undefined,
}),
};Also consider surfacing a clear UI warning when a plugin overrides a bundled channel id (and show the plugin publisher/source) so users cannot be tricked by canonical labels/docs. 2. 🟡 Terminal/log escape injection via unsanitized channel/meta IDs in plugin diagnostics
DescriptionPlugin/channel IDs taken from plugin registrations are interpolated directly into diagnostic messages, and later printed to the terminal without sanitization.
Vulnerable code (message creation): message: `channel "${id}" meta.id mismatch ("${rawMetaId}"); using registered channel id`,Vulnerable sink (terminal output): return `- ${prefix}${plugin}: ${diag.message}${source}`;RecommendationSanitize untrusted identifiers before placing them into human-facing diagnostic strings (or at the final output sink). Use the existing Example fix at diagnostic creation: import { sanitizeForLog } from "../terminal/ansi.js";
const safeId = sanitizeForLog(id);
const safeRawMetaId = rawMetaId ? sanitizeForLog(rawMetaId) : rawMetaId;
message: `channel "${safeId}" meta.id mismatch ("${safeRawMetaId}"); using registered channel id`,Alternatively, enforce a strict allowlist for plugin/channel IDs (e.g., 3. 🟡 Terminal escape injection via unsanitized channel plugin metadata in setup output
DescriptionChannel plugin metadata fields are interpolated directly into terminal-facing strings without sanitization.
Vulnerable code: export function formatChannelPrimerLine(meta: ChannelMeta): string {
return `${meta.label}: ${meta.blurb}`;
}
export function formatChannelSelectionLine(...): string {
...
return `${meta.label} — ${meta.blurb} ... ${docs}${extras ? ` ${extras}` : ""}`;
}Although RecommendationSanitize/escape any plugin-controlled strings before rendering them to a terminal UI. A practical approach is to strip ANSI escape sequences and other control characters (including newlines) from Example (render-time sanitization): import stripAnsi from "strip-ansi";
function sanitizeForTerminal(s: string): string {
// Remove ANSI sequences, then remove remaining C0 controls except common whitespace.
const noAnsi = stripAnsi(s);
return noAnsi
.replace(/[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F]/g, "")
.replace(/[\r\n]/g, " ");
}
export function formatChannelPrimerLine(meta: ChannelMeta): string {
return `${sanitizeForTerminal(meta.label)}: ${sanitizeForTerminal(meta.blurb)}`;
}Additionally, consider validating 4. 🟡 Terminal hyperlink injection via untrusted channel/plugin docsPath (OSC 8 control chars not sanitized)
Description
Data flow:
Vulnerable code: return `\u001b]8;;${safeUrl}\u0007${safeLabel}\u001b]8;;\u0007`;This becomes exploitable because RecommendationSanitize all C0 control characters (and ideally DEL) from both Example fix: function stripControlChars(value: string): string {
// Remove C0 controls (0x00-0x1F) and DEL (0x7F)
return value.replace(/[\x00-\x1F\x7F]/g, "");
}
export function formatTerminalLink(label: string, url: string, opts?: { fallback?: string; force?: boolean }): string {
const safeLabel = stripControlChars(label);
const safeUrl = stripControlChars(url);
const allow = opts?.force === true ? true : opts?.force === false ? false : process.stdout.isTTY;
if (!allow) {
return opts?.fallback ?? `${safeLabel} (${safeUrl})`;
}
return `\u001b]8;;${safeUrl}\u0007${safeLabel}\u001b]8;;\u0007`;
}Optionally also constrain Analyzed PR: #66706 at commit Last updated on: 2026-04-14T18:14:50Z |
2 participants
Summary
Describe the problem and fix in 2–5 bullets:
undefined: undefinedand degrade picker labels when a setup/runtime plugin registered incompletemeta.config channelsand make Telegram/external channel setup look broken.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
src/commands/channel-setup/discovery.tslet incomplete pluginmetaoverwrite the canonical bundled channel metadata for the same channel id, andsrc/flows/channel-setup.tsbuilt the primer from a separate raw metadata path instead of the resolved setup entries.Regression Test Plan (if applicable)
src/plugins/channel-validation.test.ts,src/commands/channel-setup/discovery.test.ts,src/plugins/loader.test.ts,src/commands/onboard-channels.e2e.test.tsmetamust be repaired to sane labels/docs/blurb, bundled channels must preserve canonical bundled metadata, and setup primer/picker must never renderundefinedlabels for malformed external plugins.dmScopeprimer test still covers the shared primer rendering path.User-visible / Behavior Changes
Users no longer see
undefined: undefinedin the “How channels work” note or broken picker labels when a plugin registers incomplete channel metadata. Bundled channels keep their canonical labels/docs in setup even if a setup plugin omits those fields.Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
meta: { id: "external-chat" }/meta: { id: "telegram" })Steps
metaomitslabel,selectionLabel,docsPath, orblurb.config channelsshows the “How channels work” note or channel picker.Expected
Actual
undefined: undefinedin the primer and degrade picker labels/hints for the malformed channel entry.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
undefinedin primer; malformed external setup plugin remains selectable with sane picker text; bundled Telegram metadata is preserved when an installed plugin omits display fields; registry-time normalization repairs incomplete channel metadata and emits a warning diagnostic.meta.id, import-cycle regressions after adding the new normalization seam.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.ChannelMeta.