fix(agent-bff): sanitize action result html before relaying

[Tonours]

What

The BFF sanitizes the agent's HTML at its own boundary, on the three surfaces that relayed it verbatim:

• the execute success body html,
• the 400 action_error details.html,
• every htmlBlock content in the /form layout, including inside a page.

One helper, three call sites. The /form and /execute 200 descriptions say so, in both the generic and the unfolded OpenAPI document.

fixes PRD-1095

Why

Rendering the old verbatim html through innerHTML executed arbitrary script in any consumer. A contract note alone leaves that live for whoever does not read it, and PRD-1097 owns the response typing anyway. So we sanitize at the boundary and no consumer has to know.

The pin is exact on purpose. 2.17.6+ resolves htmlparser2 ^12, which is ESM-only with no CJS build, and Jest 29 cannot run it. Below 2.17.2 the zero-padded numeric reference fix (a javascript: URL detection bypass) is missing. 2.17.5 is the only version with both.

The policy

Default tag allowlist — rich text (p, b, strong, a, tables, lists) survives; script, svg, iframe, img, on* attributes and javascript: URLs do not.

On top of it, style is allowed on any tag but filtered: ~38 presentational properties (color, background, font, padding, margin, border, width, text-align…) with values matching /^[^;{}()]*$/. No parentheses means no url(...) and no expression(...); position, top, left and z-index are not on the list, so styled output cannot overlay the host page.

The default allowlist alone was not viable: it drops style entirely, and this repo's own demo (datasource-demo-fintech/src/customizations/kyc_cases.ts) builds its whole result html out of inline styles.

Size cap

Parsing is super-linear in nesting depth, and the agent's html was previously relayed without being parsed at all — so sanitizing introduces a CPU cost the agent controls. Measured on 2.17.5, <div> nested N deep:

input	wall time
110 KB (10k deep)	23 ms
275 KB (25k deep)	210 ms
1.1 MB (100k deep)	1 872 ms

An action interpolating a record field into its html — the exact pattern behind this ticket — would let a user write that field and block the event loop. So html over 256 KB is dropped rather than sanitized, and the drop is logged Warn with the length. At the cap the parse costs ~120 ms.

The layout traversal is bounded the same way: past 10 nested page levels the inner elements are dropped to [] and logged, so a deep layout cannot blow the stack. Dropping rather than relaying the subtree is deliberate — an unrecursed subtree would ship unsanitized htmlBlock content.

message is not sanitized, on either surface. It is plain text by contract; sanitizing it would mangle a legitimate < or &.

Contract changes a consumer can see

• An html that sanitizes to nothing collapses to "no html": the success body sends html: null (it sent "" before), and the error omits details rather than relaying { html: "" }.
• A non-string html is still null / no details, as before.
• <img> is dropped, element and all. Nothing in the repo needs it and allowing it buys a tracking-pixel surface for no demonstrated use.

Scope and safety

• No route added, no schema changed: ActionExecuteSuccessBody.html was already string | null and details was already optional.
• A malformed agent layout (a non-array layout, a null element, a page whose elements is not an array) passes through instead of throwing. The sanitizer walks agent-controlled JSON, so it does not trust its own types.
• Lockfile delta is +85 / -0: sanitize-html@2.17.5 and its transitive deps, nothing re-resolved.
• Every drop is logged: over the cap (Warn), and a sanitizer throw (Error, dead at the pinned version but the execute path runs after the action committed, so a 500 there means a retry and a double execution).

How to test

• cd packages/agent-bff && yarn jest test/action test/openapi
• By hand: execute an action returning <p>ok</p><img src=x onerror=alert(1)> — it comes back as <p>ok</p>. Run the fintech demo's KYC Approved action — the green banner and the table keep their styling.

Full package suite: 1367 green.

Known limitation

Agent html size stays unbounded, as before. The exact pin stops Dependabot until Jest has an ESM story.

Definition of Done

General

• Write an explicit title for the Pull Request, following Conventional Commits specification
• Test manually the implemented changes
• Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

• Consider the security impact of the changes made

[linear-code]

PRD-1095

[qltysh]

4 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with many parameters (count = 4): mapActionForm	2
qlty	Structure	Function with many returns (count = 4): mapActionExecuteResult	1
qlty	Structure	Function with high complexity (count = 16): sanitizeActionLayout	1

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (5)

Rating	File	% Diff	Uncovered Line #s
	packages/agent-bff/src/action/agent-action-client.ts	100.0%
	packages/agent-bff/src/action/action-execute-mapper.ts	100.0%
	packages/agent-bff/src/action/action-form-mapper.ts	100.0%
	packages/agent-bff/src/action/action-routes-middleware.ts	100.0%
	packages/agent-bff/src/action/sanitize-action-html.ts	100.0%
	Total	100.0%

🚦 See full report on Qlty Cloud »

🛟 Help

• Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

• Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

• File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

  • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

[nbouliol]

The /form route still relays layout verbatim, and htmlBlock.content (action-form-mapper.ts:56, via agent-action-client.ts:58) is the same untrusted agent HTML - same vector, still open. Sanitize it here too, or a follow-up ticket?

[Tonours]

Fixed, in this PR rather than a follow-up: same trust boundary, same untrusted agent string, and the fix is one call.

mapActionForm now runs the layout through sanitizeActionLayout (sanitize-action-html.ts), which walks htmlBlock.content including the ones nested in a page, and leaves every other component untouched. Single caller, so /form on both the folded and unfolded routes is covered. The /form 200 description says so now, mirroring /execute. Tests: three cases in action-form-mapper.test.ts (top-level block, nested-in-page block, block that is entirely active markup → empty content).

Two other things on this push:

• sanitizeActionHtml collapses a fully stripped html to null, so details is omitted instead of relaying { html: "" } (your inline comment).
• The lockfile was reverted to a minimal delta. The previous push had re-resolved the whole tree: 1703 insertions / 1124 deletions, 209 version bumps on unrelated dev deps (@actions/*, @octokit/*, semantic-release, sigstore). It is now +85 / -0 — sanitize-html@2.17.5 and its transitive deps, nothing else.

On the qlty comment (mapActionExecuteResult, 4 returns): pre-existing, not introduced here. git show origin/main:packages/agent-bff/src/action/action-execute-mapper.ts has the same 4 returns; this PR only changed the html: line inside one of them. They are guard clauses, which is what the repo asks for. Not touching it.

[Tonours]

Two more pushes, both from things this PR introduced rather than from the ticket.

Explicit sanitize options. The library defaults drop style on every tag. packages/datasource-demo-fintech/src/customizations/kyc_cases.ts:60 builds its entire result html out of inline styles, so the default policy would have shipped this repo's own demo as unstyled text. There is a constant now: default tags untouched, style allowed, allowedStyles restricted to ~38 presentational properties with values matching /^[^;{}()]*$/ — no parentheses means no url(...) and no expression(...), and position / top / left / z-index are off the list, so styled output cannot overlay the host page. img stays off. Details on the sanitize-action-html.ts thread.

Size cap. Before this PR the agent's html was relayed without being parsed at all; sanitizing hands the agent a CPU cost. Parsing is super-linear in nesting depth — measured on 2.17.5, <div> nested N deep: 10k → 23 ms, 25k → 210 ms, 100k → 1 872 ms. An action interpolating a record field into its html (the pattern behind this ticket) lets a user write that field and block the event loop. Html over 256 KB is now dropped instead of sanitized, logged Warn with the length; at the cap the parse costs ~120 ms.

The drop is logged rather than silent, which is why mapActionForm and mapActionExecuteResult now take the Logger the middleware already had in scope — same shape as the existing Warn on the 501 path.

Also: a malformed agent layout (non-array layout, a null element, a page whose elements is not an array) now passes through instead of throwing. sanitizeActionLayout walks agent-controlled JSON, so it does not trust its own types.

message is deliberately not sanitized on either surface — it is plain text by contract, and sanitizing it would mangle a legitimate < or &.

[nbouliol]

Round-2 pass. The three round-1 findings are addressed: htmlBlock content is sanitized (recursion included), a fully stripped html now relays as no html, and the options are explicit. The third is only half closed though - see the allowedTags comment. Five findings on the new code below.

[nbouliol]

allowedTags is still the library default, so img is dropped and class/id stripped - an existing htmlBlock logo or Forest class regresses here. Allow img and class explicitly?

[nbouliol]

DynamicLayoutElementPage.elements is DynamicFormElement, which excludes Page (datasource-customizer/src/decorators/actions/types/fields.ts:277), so a page cannot nest a page - is this cap guarding a reachable shape?

[nbouliol]

Still open - a page cannot nest a page, so this cap guards an unreachable shape. Drop it, or is there a path I am missing?

[Tonours]

The customizer type is right — a typed Node form cannot nest Page. This helper does not see that type. It walks ForestServerActionFormLayoutElement from the agent's hook JSON, and that union is recursive (page.elements includes page). Toolkit ActionLayoutElement is the same. ensureFormIsCorrect only forbids mixing pages at the root; parseLayout / buildLayoutSchema will serialize a nested page if one shows up. Rails / Python / a custom getForm never go through the customizer types. Dropping the cap makes a deep page chain a RangeError on /form, which is why it is here.

[nbouliol]

Sanitizes all three surfaces at the boundary, and the style policy now names the URL carriers instead of banning parentheses - verified on 2.17.5 that rgb()/calc()/var() survive while URL(, url (, \75 rl(, image-set( and -moz-binding( do not. Boundary tests are exact-valued. The only thing left is the layout depth cap guarding an unreachable shape, which is dead code rather than a defect.