fix(core): stop discarding API errors that quote an HTML tag - #42489
fix(core): stop discarding API errors that quote an HTML tag#42489gaurav0107 wants to merge 1 commit into
Conversation
b0fa8d7 to
8275e97
Compare
8275e97 to
33a7bd5
Compare
checkForHtml() asked "does this string contain a known HTML tag?", which is the right question for rendering decisions but the wrong one on the error path. A database driver that echoes the offending fragment back in its syntax error -- for instance ClickHouse reporting a bad metric expression containing <a> -- tripped the tag scan, so parseErrorJson() handed the message to retrieveErrorMessage(), which prefers the HTTP status and returned "Bad request". The real diagnostic was thrown away even though the API had returned it in full. Require the string to begin with markup before treating it as an HTML error page. A page served by a proxy or gateway starts with <!doctype html> or a tag; a server-authored message starts with prose. Both callers of checkForHtml() -- parseStringResponse() and parseErrorJson() -- are fixed by the single guard, and the shared isProbablyHTML() helper is untouched so cell-value rendering keeps its existing semantics. Fixes apache#33500
33a7bd5 to
39a84f5
Compare
|
Heads up on CI, since this branch has stale queued runs on three earlier SHAs. The workflow For reference, Frontend Build CI did complete green on |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #fa78ebActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
| Language | Invalidated translations |
|---|---|
fr |
493 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (fr):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42489 +/- ##
=======================================
Coverage 65.26% 65.26%
=======================================
Files 2794 2794
Lines 157615 157616 +1
Branches 36045 36045
=======================================
+ Hits 102868 102869 +1
Misses 52769 52769
Partials 1978 1978
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
SUMMARY
Fixes #33500.
When an API error message happens to contain an HTML tag, the UI throws the
whole message away and shows a generic
Unexpected error Bad request. Thereproduction in the issue is a custom metric containing
<a>: the databaserejects it,
/api/v1/chart/datareturns the full syntax error inmessage,and the user sees nothing useful.
Root cause is in
checkForHtml():isProbablyHTML()answers "does this string contain a known HTML taganywhere?". That is the right question for the rendering helpers it was
written for (
safeHtmlSpan,sanitizeHtmlIfNeeded, used on table cellvalues), but the wrong one on the error path, which needs "is this an HTML
error page rather than prose?". A driver message that quotes
<a>mid-sentence trips the tag scan, so
parseErrorJson()hands it toretrieveErrorMessage(), which prefers the HTTP status and returnsBad request.The fix requires the string to begin with markup before it can be treated as
an error page. A page served by a proxy or gateway starts with
<!doctype html>or a tag; a server-authored message starts with prose. Thispreserves the behaviour #29321 added — raw gateway HTML is still collapsed
into a status-code message — while letting real diagnostics through.
The guard goes in
checkForHtml()rather than at either call site, so bothconsumers (
parseStringResponse()andparseErrorJson(), and therefore boththe string, JSON and text-fallback branches of
getClientErrorObject()) arecovered by one change. The shared
isProbablyHTML()inutils/html.tsxisdeliberately left alone.
No new sanitisation is needed: the resulting string is rendered as a React
text child (
BasicErrorAlertrenders<p>{body}</p>), and there is nodangerouslySetInnerHTMLanywhere on the error path.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual change beyond the error text itself. For the message in the issue:
Before —
Unexpected error/Bad requestAfter —
Unexpected error/Error: HTTPDriver ... DB::Exception: Syntax error: failed at position 37 ('<') (line 1, col 37): <a> AS ...TESTING INSTRUCTIONS
Automated:
cd superset-frontend npx cross-env NODE_ENV=test npx jest \ packages/superset-ui-core/test/query \ packages/superset-ui-core/src/utils/html.test.tsx \ src/middleware/asyncEvent.test.tsThe new test
parseErrorJson keeps a server message that only quotes an HTML tagfails onmasterwitherror: "Bad request"and passes with thischange. The four pre-existing HTML tests in that file are the regression
guard for #29321 and stay green — their fixtures (
<div>...,<!doctype html>...) all start with markup.asyncEvent.test.tscovers theone caller of
parseErrorJson()outside this module.Manual:
expression back (ClickHouse in the issue report).
<a>.Bad request.ADDITIONAL INFORMATION