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.

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

Escape HTML tags in error messages on Source Data Information screen #889

Merged

Conversation

dhmjhu
Copy link
Contributor

@dhmjhu dhmjhu commented Jul 2, 2024

Summary

On the Source Data Information screen, not all parts of error messages returned by the server are properly escaped.

Example

Trying to select a Kilosort directory with an invalid params.py resulted in the following message:

Request Failed
invalid syntax (, line 3)

Screenshot of error message

The actual response from the server was:

{"message": "invalid syntax (<string>, line 3)", "type": "SyntaxError"}

Cause

While angle brackets in text are converted to HTML entities, type is left alone.

const [type, ...splitText] = result.message.split(":");
const text = splitText.length
? splitText.join(":").replaceAll("<", "&lt").replaceAll(">", "&gt")
: result.traceback
? `<small><pre>${result.traceback.trim().split("\n").slice(-2)[0].trim()}</pre></small>`
: "";
const message = `<h4 style="margin: 0;">Request Failed</h4><small>${type}</small><p>${text}</p>`;

Fix

Performing the same replacements on type should resolve this.

I have not tested this change, as I don't have an appropriate environment set up right now.

@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Jul 2, 2024

Thanks for catching this and pushing a fix!

I have not tested this change, as I don't have an appropriate environment set up right now.

Please give it a try and let us know (preferably with screenshot of new error to match expectation), since I don't have a version of the data with invalid params myself

The developer installation instructions can be found here: https://nwb-guide.readthedocs.io/en/stable/developer_guide.html#installation

@dhmjhu
Copy link
Contributor Author

dhmjhu commented Jul 2, 2024

I'm not sure when I can get around to setting it up, but here's a minimal example: guide-error-example.zip

It's not valid data, but it gets far enough to trigger this bug.

@rly
Copy link
Collaborator

rly commented Jul 6, 2024

Thanks @dhmjhu for the PR and minimal example! I was able to demonstrate the fix:

image

@CodyCBakerPhD
Copy link
Collaborator

@rly Looks like you should generate and set a new token for chromatic

@rly
Copy link
Collaborator

rly commented Jul 7, 2024

Ah, I reset the Chromatic project token and it still failed. Then I realized this is a security feature of GitHub Actions -- PRs from forks do not have access to this repo's secrets. So Chromatic cannot run (unless we add the token to the fork). There are workarounds that have some risk. How do you think we should proceed @CodyCBakerPhD ?

@CodyCBakerPhD CodyCBakerPhD changed the base branch from main to staging July 7, 2024 16:08
@CodyCBakerPhD
Copy link
Collaborator

I would propose merging this to a copy of current main (staging) then merging that into main after chromatic checks pass

I've adjusted the target of this PR

@rly
Copy link
Collaborator

rly commented Jul 10, 2024

Good idea. Let's do that.

@rly rly merged commit 5aa4a4c into NeurodataWithoutBorders:staging Jul 10, 2024
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants