XSS vulnerability via unsanitized href in Action.OpenUrl

a.url is written directly into an href attribute with no validation. Lit's html tagged template does not strip javascript: URLs from attribute bindings, so a card payload containing "url": "javascript:alert(document.cookie)" becomes a working XSS vector when the user clicks the link.

The existing codebase already exposes openExternalUrlSafe (imported in grouped-render.ts) which validates URLs before opening them. The Action.OpenUrl handler should use a <button> with a click handler (or at minimum validate the scheme before placing it in href) instead of a raw anchor with an unchecked URL.

(You will also need to import openExternalUrlSafe from "../open-external-url.ts" at the top of this file.)

This is a comment left during a code review.
Path: ui/src/ui/chat/adaptive-card-render.ts
Line: 219-226

Comment:
**XSS vulnerability via unsanitized `href` in `Action.OpenUrl`**

`a.url` is written directly into an `href` attribute with no validation. Lit's `html` tagged template does **not** strip `javascript:` URLs from attribute bindings, so a card payload containing `"url": "javascript:alert(document.cookie)"` becomes a working XSS vector when the user clicks the link.

The existing codebase already exposes `openExternalUrlSafe` (imported in `grouped-render.ts`) which validates URLs before opening them. The `Action.OpenUrl` handler should use a `<button>` with a click handler (or at minimum validate the scheme before placing it in `href`) instead of a raw anchor with an unchecked URL.

```suggestion
  if (type === "Action.OpenUrl") {
    const url = typeof a.url === "string" ? a.url : "";
    if (!url) return nothing;
    return html`<button
      class="ac-action ac-action--link"
      type="button"
      @click=${() => openExternalUrlSafe(url)}
    >${title}</button>`;
  }
```

(You will also need to import `openExternalUrlSafe` from `"../open-external-url.ts"` at the top of this file.)

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 6f2eaab. Replaced <a href> with <button @click=${() => openExternalUrlSafe(url)}>. Imported openExternalUrlSafe which validates URL schemes (blocks javascript:, etc.).

templateData is parsed but never consumed

renderAdaptiveCard destructures only { card } from the ParsedAdaptiveCard, so the templateData value extracted by the parser (from <!--adaptive-card-data--> blocks) is completely ignored at render time. Any template variable substitution — the primary reason to send a separate data block — is silently skipped.

If template binding isn't implemented yet, either remove the DATA_RE/STRIP_DATA_RE parsing logic from adaptive-card-parse.ts (and the templateData field from ParsedAdaptiveCard) to avoid dead code, or add a TODO comment so it isn't forgotten.

This is a comment left during a code review.
Path: ui/src/ui/chat/adaptive-card-render.ts
Line: 19-22

Comment:
**`templateData` is parsed but never consumed**

`renderAdaptiveCard` destructures only `{ card }` from the `ParsedAdaptiveCard`, so the `templateData` value extracted by the parser (from `<!--adaptive-card-data-->` blocks) is completely ignored at render time. Any template variable substitution — the primary reason to send a separate data block — is silently skipped.

If template binding isn't implemented yet, either remove the `DATA_RE`/`STRIP_DATA_RE` parsing logic from `adaptive-card-parse.ts` (and the `templateData` field from `ParsedAdaptiveCard`) to avoid dead code, or add a `TODO` comment so it isn't forgotten.

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 6f2eaab. Added TODO comment at the destructure site. Template data parsing kept for forward-compatibility; binding implementation is a follow-up.

Return type lie — nothing cast to TemplateResult

When url is empty the function returns nothing as unknown as TemplateResult, which defeats TypeScript's type checking and could cause subtle rendering bugs if callers branch on the return value. The declared return type should be widened to TemplateResult | typeof nothing to match reality.

This is a comment left during a code review.
Path: ui/src/ui/chat/adaptive-card-render.ts
Line: 140-148

Comment:
**Return type lie — `nothing` cast to `TemplateResult`**

When `url` is empty the function returns `nothing as unknown as TemplateResult`, which defeats TypeScript's type checking and could cause subtle rendering bugs if callers branch on the return value. The declared return type should be widened to `TemplateResult | typeof nothing` to match reality.

```suggestion
function renderImage(el: Element): TemplateResult | typeof nothing {
  const url = typeof el.url === "string" ? el.url : "";
  const alt = typeof el.altText === "string" ? el.altText : "";
  const sizeClass = imageSizeClass(el.size);
  if (!url) {
    return nothing;
  }
  return html`<img class="ac-image ${sizeClass}" src="${url}" alt="${alt}" />`;
}
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 6f2eaab. Return type changed to TemplateResult | typeof nothing and the unsafe cast removed.

Reject unsafe protocols in Action.OpenUrl URLs

Action.OpenUrl values are written directly into an anchor href without any protocol allowlist, so a card payload like {"type":"Action.OpenUrl","url":"javascript:..."} will execute script when clicked. This is a security regression in the chat surface because other URL entry points already route through resolveSafeExternalUrl to block non-http(s)/blob schemes; this path should apply the same validation before rendering clickable links.

Useful? React with 👍 / 👎.

Fixed in 6f2eaab. Action.OpenUrl now uses openExternalUrlSafe() which only allows http:, https:, and blob: protocols. No raw href injection.

Validate actions is an array before calling renderActions

The parser only validates card.body, but renderAdaptiveCard assumes any truthy card.actions?.length is mappable. If a malformed card contains "actions":"oops" (or any non-array with a length), renderActions will call .map on a non-array and throw, breaking message rendering. Guard this with Array.isArray(card.actions) before passing it into renderActions.

Useful? React with 👍 / 👎.

Fixed in 6f2eaab. Added Array.isArray(card.actions) guard before calling renderActions.

Parse adaptive-card markers only for assistant role

The new marker parsing runs on every message because parsedCard is computed from markdownBase without checking role. This means non-assistant content (for example user messages or tool logs that include literal <!--adaptive-card-->... text) is silently transformed into card UI instead of being shown as plain markdown text, which changes message semantics and can surface card UI where it was not intended. Restrict parsing to the trusted role(s) that actually emit these markers.

Useful? React with 👍 / 👎.

Validate action entries before accessing action fields

renderActions passes each array element directly to renderAction, and renderAction immediately reads a.type/a.title. Because parsing only checks that actions is an array (not that each entry is a non-null object), a payload like "actions":[null] throws at runtime (Cannot read properties of null) and breaks rendering for the whole message bubble. Add a non-null object guard before dereferencing action fields.

Useful? React with 👍 / 👎.

Remove raw href from RichTextBlock OpenUrl runs

Even after Action.OpenUrl button clicks were routed through openExternalUrlSafe, this RichTextBlock branch still writes the unvalidated URL into an anchor href. That means non-primary navigations (for example middle-click or context-menu "open link") can bypass the safe opener and use the raw scheme directly, so disallowed protocols are still reachable from card content.

Useful? React with 👍 / 👎.

Wire ToggleVisibility to actual rendered element IDs

Action.ToggleVisibility looks up targets via [data-ac-id="..."], but this renderer never emits data-ac-id on card elements, so valid toggle actions cannot find any targets and always no-op. The toggle handler needs to query an attribute that is actually rendered (or elements need to expose matching data-ac-id values).

Useful? React with 👍 / 👎.

Preserve non-string types in template substitutions

bindTemplateData always coerces ${...} results to strings (or JSON strings for objects), so cards that bind numeric/boolean/object fields lose their original types. For example, ProgressBar.value: "${score}" becomes a string and then renders as 0% because renderProgressBar only accepts numbers; similar failures occur for other typed fields. This makes templated cards with non-text bindings render incorrect state even when templateData is valid.

Useful? React with 👍 / 👎.

Support numeric Column.width values

renderColumn treats width as a string-only field and falls back to "auto" for non-strings, but Adaptive Cards allows numeric widths for weighted columns. A valid card with widths like 1 and 2 will lose the intended proportional layout because both columns are rendered as auto-width.

Useful? React with 👍 / 👎.