Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(Query): Parse html string error responses to avoid displaying raw HTML as error message #29321

Merged
merged 12 commits into from
Jun 26, 2024
Merged

fix(Query): Parse html string error responses to avoid displaying raw HTML as error message #29321

merged 12 commits into from
Jun 26, 2024

Conversation

rtexelm
Copy link
Contributor

@rtexelm rtexelm commented Jun 21, 2024

SUMMARY

Adds a function to check if a string is HTML and not capable of JSON parsing to ensure the client does not show raw HTML where there should be an error message.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Change the return of the ChartDataRestApi response for a POST method to be an error with the message kwarg containing raw html in string form. e.g. self.response_500(message='<html><div>Error</div></html>')
  2. Load a dashboard, and click See more on the unexpected error alert produced by charts.
  3. The message displayed should be Server error or, in the case of a detected 404, Not found.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added the change:frontend Requires changing the frontend label Jun 21, 2024
@mistercrunch mistercrunch mentioned this pull request Jun 21, 2024
Merged
mistercrunch
mistercrunch approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given everything, this overall LGTM. If the html-handling module tends to grow in the future I'd advocate to refactor using cheerio, but for now I'd rather maintain the logic here than adding a dependency.

@rtexelm
Copy link
Contributor Author

rtexelm commented Jun 25, 2024

In the latest commit I've tried preserving the status and other properties from the response object so that it can be parsed inside the parseErrorJson by the retrieveErrorMessage function. In the past the properties of the complex response object were being ignored in the passage to parseErrorJason by the spread operator. By explicitly destructuring them it should preserve the values for parsing and also allow for passage to a logging function in the future to deduce the root of the unreadable errors.

I've also added a final check to the error message if the response is incapable of being read as a Response object.

mistercrunch
mistercrunch requested changes Jun 25, 2024
View reviewed changes
Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's address the t() comment, but otherwise LGTM, feel free to address the naming-related comments or save it for another PR

@rtexelm
Copy link
Contributor Author

rtexelm commented Jun 25, 2024

I'll save the name change for a new PR just so it becomes the focus in case I miss any usage cases across the codebase when replacing the calls to it.

@rtexelm rtexelm force-pushed the fix/dashboard-chart-errors-showing-html/sc-80623 branch from d54c418 to e325a7d Compare June 25, 2024 23:49
@rtexelm rtexelm merged commit de6a518 into apache:master Jun 26, 2024
33 checks passed
@mistercrunch
Copy link
Member

standing

mistercrunch pushed a commit to preset-io/superset that referenced this pull request Jun 28, 2024
@rtexelm @sadpandajoe
fix(Query): Parse html string error responses to avoid displaying raw…
038ca28 
… HTML as error message (apache#29321)

(cherry picked from commit de6a518)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistercrunch mistercrunch mistercrunch approved these changes

@geido geido Awaiting requested review from geido

@eschutho eschutho Awaiting requested review from eschutho

@michael-s-molina michael-s-molina Awaiting requested review from michael-s-molina

Assignees
No one assigned
Labels
change:frontend Requires changing the frontend packages size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rtexelm @mistercrunch @michael-s-molina