input validation added for various modules

[chakilamsuryat]

SUMMARY

No input validations there on UI

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

We will restricting the user for input validation and will thrown error as shown above.

TESTING INSTRUCTIONS

Can test charts, dashboards, annotations, annotation_layers, row_level_security, user info editing views on the UI for input validation given for names and descriptions.

ADDITIONAL INFORMATION

There is no input validation present for Dashboard, Chart, Annotation Layers, row-level-security, and for user info details.
This is as part of security fixes for vulnerabilities raised as able to input scripts also currently in the fields of name and descriptions of various views.

[chakilamsuryat]

Can suggest if any changes required from my end as well.

[korbit-ai]

I've completed my review and didn't find any issues.

Files scanned

​

File Path	Reviewed
superset/annotation_layers/schemas.py	✅
superset/annotation_layers/annotations/schemas.py	✅
superset/row_level_security/schemas.py	✅
superset/dashboards/schemas.py	✅
superset/charts/schemas.py	✅
superset/security/manager.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

• You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
• You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
• Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
• Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
• Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

• Check out our docs on how you can make Korbit work best for you and your team.
• Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

​

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Issue Categories

​

Category	Enabled
Naming	✅
Database Operations	✅
Documentation	✅
Logging	✅
Error Handling	✅
Systems and Environment	✅
Objects and Data Structures	✅
Readability and Maintainability	✅
Asynchronous Processing	✅
Design Patterns	✅
Third-Party Libraries	✅
Performance	✅
Security	✅
Functionality	✅

Feedback and Support

• Tell us what you think of Korbit
• Schedule a call with our team
• Email us @ [email protected]

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

[chakilamsuryat]

Raised issue, No input validations there on UI #31723 -> which is closed by rusackas and has been moved to discussion No input validations there on UI #31772.

[sadpandajoe]

@chakilamsuryat so what happens to any dashboard/chart now that has characters that are no longer allowed? Will they break when a user tries to save?

[rusackas]

I added a comment on an issue that there are plenty of valid use cases for these characters, and we're unaware of any risk (XSS or otherwise). If you spot a security issue, please report it privately to [email protected], but otherwise, I'd like to know more about the rationale for the change.

[chakilamsuryat]

@sadpandajoe , if given input invalid characters as shared in screenshot, it will give popup of error messaging saying that its not allowed and that name will not save.

[chakilamsuryat]

@rusackas , Improper Input Validation- It is observed that application fails to validate and sanitize user supplied input containing malicious JavaScript which may be executed on browser leading to Stored Cross-Site Scripting.

[sadpandajoe]

@sadpandajoe , if given input invalid characters as shared in screenshot, it will give popup of error messaging saying that its not allowed and that name will not save.

@chakilamsuryat so we only show that this isn't possible after a user may have already made edits on an existing dashboard and trying to save then we let them know that they can't save? That seems pretty bad for the user. Also if they save as a new dashboard, then anything shared will have to be reshared with users.

[chakilamsuryat]

we only show that this isn't possible after a user may have already made edits on an existing dashboard and trying to save then we let them know that they can't save? That seems pretty bad for the user. Also if they save as a new dashboard, then anything shared will have to be reshared with users.

Few options:

1. Can show on UI over dashboard like for the new instructions initially for some time. So of customer is ok with name change to valid name then he can proceed. Else if ok with new dashboard, can copy previous dashboard, make changes and save with valid name.
2. We can try enabling this validation for suppose created greater than or equal to today dashboards only.

We can think of such options for user experience. But as issue is related security vulnerability, I think some validation at input should be there as part of it.

[sadpandajoe]

I believe we have already fixed this in the past and that's why we do allow these characters. I'm going to mention @dpgaspar as he might remember.