fix(query-engine): stop stripping angle brackets from warehouse error SQL

[Makisuo]

Problem

cleanErrorMessage scrubbed HTML error pages with .replace(/<[^>]+>/g, " "). ClickHouse errors echo the failing query back, so a query with a <= and a later >= (for example $__timeFilter in several UNION ALL branches) came back with everything in between removed. The SQL that ran was correct, but the error text an agent or user saw showed a different, mangled query, which sent debugging the wrong way.

The cleaner runs on every warehouse failure (toWarehouseQueryError, mapWarehouseError), so this affected run_sql, inspect_chart_data, raw_sql dashboard widgets, raw_query alert rules and the HTTP raw-SQL route.

Fix

• Strip only real HTML tags: a known tag name, optionally with quoted attributes. a < 5 AND b > 3 and <= ... >= survive; <b>, <p class="x"> and doctype tags are still removed.
• The "cut at the start of an HTML page" check no longer allows a space after <, so a comparison against a column named title doesn't truncate the message.

Tests

• errors.test.ts: regression test with three UNION branches of >=/<= plus a < 5 AND b > 3. It fails with the old regex. A second test confirms inline HTML is still stripped.
• raw-sql.test.ts: $__timeFilter expanded across three UNION branches keeps all six comparisons.
• bunx vitest run src/execution in packages/query-engine: 117 passed.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Summary by CodeRabbit

• Bug Fixes
  • Improved SQL error-message cleanup so comparison operators such as <= and >= remain intact, while inline HTML tags are removed and their text is preserved.
  • Fixed time-filter expansion across multiple UNION ALL branches, preserving the final branch in the prepared query.

[maple-review-bot]

Maple review

🟢 Confidence 4/5 · likely safe to merge
The narrowed tag grammar loses tag forms the old regex stripped; I ran the new cleaner on an nginx doctype body and unquoted-attribute tags to confirm.
_{quality 90/100 · 1 warning · tests covered · risk low}

Narrows the HTML scrubber in cleanErrorMessage to a known-tag list so echoed SQL keeps its <=/>= comparisons, with tests for both the scrubber and $__timeFilter UNION expansion. The regression is fixed, but the new tag grammar drops forms the old regex removed.

• cleanErrorMessage replaces the generic <[^>]+> scrub with a named-tag HTML_TAG regex
• The HTML-page cut regex no longer allows whitespace after <
• New tests cover UNION <=/>= echo, inline HTML stripping and $__timeFilter expansion

Findings

🟠 Warning · F1 · HTML_TAG leaves <!DOCTYPE html> and unquoted-attribute tags in cleaned errors

correctness · packages/query-engine/src/execution/errors.ts:81-82

The !doctype alternative only matches a tag whose attributes are all name="value", so <!DOCTYPE html> (bare text before >) does not match and survives into the cleaned message: cleanErrorMessage("Request failed with status 503: <!DOCTYPE html>\n<html><head>…") now returns Request failed with status 503: <!DOCTYPE html>, where the old /<[^>]+>/g removed it. The same grammar change lets <br clear=all> and <meta charset=utf-8> through, so error text an agent or user sees can carry markup again — the PR description's "doctype tags are still removed" does not hold. Relaxing the attribute tail to [^>]* after the tag name strips these while still skipping <= and a < 5.

	/<\/?(?:!doctype|html|head|body|center|h[1-6]|hr|title|p|br|div|span|pre|meta|a|b|i|strong|em)\b[^>]*>/gi

🤖 Prompt to fix this finding with an AI agent

Findings from an automated review of commit fe092dd9448314ee618fc2245cc7bb2f0eb80654. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F1 · Warning · correctness · packages/query-engine/src/execution/errors.ts:81-82
`HTML_TAG` leaves `<!DOCTYPE html>` and unquoted-attribute tags in cleaned errors
The `!doctype` alternative only matches a tag whose attributes are all `name="value"`, so `<!DOCTYPE html>` (bare text before `>`) does not match and survives into the cleaned message: `cleanErrorMessage("Request failed with status 503: <!DOCTYPE html>\n<html><head>…")` now returns `Request failed with status 503: <!DOCTYPE html>`, where the old `/<[^>]+>/g` removed it. The same grammar change lets `<br clear=all>` and `<meta charset=utf-8>` through, so error text an agent or user sees can carry markup again — the PR description's "doctype tags are still removed" does not hold. Relaxing the attribute tail to `[^>]*` after the tag name strips these while still skipping `<=` and `a < 5`.
Replace those lines with:
	/<\/?(?:!doctype|html|head|body|center|h[1-6]|hr|title|p|br|div|span|pre|meta|a|b|i|strong|em)\b[^>]*>/gi

What was checked

• Ran the new HTML_TAG and page-cut on a < 5 AND b > 3, <=/>=, <b>, <p class="x">, <table><tr><td> and confirmed comparisons survive
• Ran adversarial inputs (<a x=y … runs, unterminated quotes) through HTML_TAG: no catastrophic backtracking
• HTML_TAG and the cut regex are only reached from cleanErrorMessage (errors.ts:326, errors.ts:347)

_{fe092dd · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 35 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: aa6ed285-bd3a-4f80-89b8-b9cf4b258bf5

📥 Commits

Reviewing files that changed from the base of the PR and between fe092dd and ada991c.

📒 Files selected for processing (2)

• packages/query-engine/src/execution/errors.test.ts
• packages/query-engine/src/execution/errors.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 87c87bd8-a90e-4c73-bc88-8947f2d2d32e

📥 Commits

Reviewing files that changed from the base of the PR and between b262760 and fe092dd.

📒 Files selected for processing (3)

• packages/query-engine/src/execution/errors.test.ts
• packages/query-engine/src/execution/errors.ts
• packages/query-engine/src/runtime/raw-sql.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

cleanErrorMessage now removes recognized HTML tags without stripping SQL comparison operators. Tests cover inline HTML removal and time-filter expansion across three UNION ALL branches.

Changes

SQL error cleanup

Layer / File(s)	Summary
HTML cleanup and SQL regression coverage packages/query-engine/src/execution/errors.ts, packages/query-engine/src/execution/errors.test.ts, packages/query-engine/src/runtime/raw-sql.test.ts	cleanErrorMessage now strips recognized HTML tags and uses the same pattern for HTML-page cutoff detection. Tests check that SQL comparisons remain intact, inline HTML tags are removed, and time comparisons are expanded across three UNION ALL branches.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: 🔵 Low · up to fe092

Some warehouse failures may display residual HTML markup in their error text. This is a bounded presentation regression, so merge with owner awareness and follow-up.

Security Architecture Review

Security architecture risk: 🔵 Low · up to fe092

The change preserves SQL diagnostics without changing query execution or permissions. Some additional markup can remain in error messages. The inspected consumers return text or typed errors, but downstream display handling is not fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Caller SQL echoed by a warehouse and upstream error bodies can contribute markup to diagnostic messages consumed by existing query and inspection surfaces. The change expands preserved message content, not demonstrated execution authority or cross-tenant reachability. Complete downstream rendering coverage remains unresolved.

Trust Boundaries and Controls

• observed — MCP failures are explicitly text content carried through protocol serialization, rather than an inspected HTML execution sink. The raw SQL HTTP handler obtains the current tenant and supplies that tenant's organization ID to execution. These inspected boundaries provide counterevidence to an introduced authority bypass, but do not establish escaping in every client renderer.

Hardening Proposals

• proposed — Keep warehouse diagnostics untrusted at display boundaries and apply output-context escaping there; do not rely on this SQL-preserving cleaner as an HTML sanitizer.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preserving angle-bracket comparison operators in warehouse error SQL.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[maple-review-bot]

1 inline note from Maple's review. The score and summary are in the review comment above.

[maple-review-bot]

Maple review

🟢 Confidence 4/5 · likely safe to merge
The new tag allowlist silently narrows what an error scrubber in shared warehouse code removes; regex behavior verified by running it.
_{quality 98/100 · 1 note · tests covered · risk medium}

Narrows the HTML scrubber in cleanErrorMessage to a tag allowlist so echoed SQL with <=/>= survives, and drops the space in the HTML-page truncation check. The doctype and unquoted-attribute gap is fixed; the allowlist itself now misses most HTML tags.

• HTML_TAG in cleanErrorMessage replaces the generic /<[^>]+>/g scrub
• HTML-page truncation check no longer allows whitespace after <
• Tests added for UNION-branch SQL and for inline/doctype HTML

Findings

🔵 Note · F2 · HTML_TAG allowlist omits script, style, table and most other tags

correctness · packages/query-engine/src/execution/errors.ts:82

The alternation lists 18 tag names, so inline HTML that the page-start cut does not catch keeps its markup and its text: cleanErrorMessage("Bad gateway <table><tr><td>x</td></tr></table>") returns the tags unchanged, and <script>alert(1)</script> or <style>body{…}</style> survive whole, where the old /<[^>]+>/g removed them. Error messages still read as HTML for pages that do not begin with <html>/<!doctype>/<head>.

Extend the tag alternation with the remaining common tags (`script`, `style`, `table`, `tbody`, `thead`, `tr`, `td`, `th`, `ul`, `ol`, `li`, `img`, `link`, `form`, `input`, `iframe`, `svg`, `nav`, `footer`, `section`, `article`), keeping the attribute group that preserves tag-like SQL.

🤖 Prompt to fix this finding with an AI agent

Findings from an automated review of commit ada991c0f7ee478f0001795f8e67a350da8722bf. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F2 · Note · correctness · packages/query-engine/src/execution/errors.ts:82
`HTML_TAG` allowlist omits `script`, `style`, `table` and most other tags
The alternation lists 18 tag names, so inline HTML that the page-start cut does not catch keeps its markup and its text: `cleanErrorMessage("Bad gateway <table><tr><td>x</td></tr></table>")` returns the tags unchanged, and `<script>alert(1)</script>` or `<style>body{…}</style>` survive whole, where the old `/<[^>]+>/g` removed them. Error messages still read as HTML for pages that do not begin with `<html>`/`<!doctype>`/`<head>`.
Suggested fix: Extend the tag alternation with the remaining common tags (`script`, `style`, `table`, `tbody`, `thead`, `tr`, `td`, `th`, `ul`, `ol`, `li`, `img`, `link`, `form`, `input`, `iframe`, `svg`, `nav`, `footer`, `section`, `article`), keeping the attribute group that preserves tag-like SQL.

Fixed since the last review

• ✅ F1 · HTML_TAG leaves <!DOCTYPE html> and unquoted-attribute tags in cleaned errors

What was checked

• Ran the new regex on <!DOCTYPE html>, <!DOCTYPE html PUBLIC "-//W3C//DTD…"> and <p class=x>: all stripped, so F1 is gone
• Ran a < 5 AND b > 3, x<b AND y>3 and three >=/<= branches: none mangled
• Leftover HTML reaches React as an escaped text node; no dangerouslySetInnerHTML path renders error messages

_{ada991c · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}