-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fix: dataset safe URL for explore_url #24686
fix: dataset safe URL for explore_url #24686
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24686 +/- ##
==========================================
- Coverage 68.99% 68.98% -0.02%
==========================================
Files 1903 1903
Lines 74077 74054 -23
Branches 8194 8196 +2
==========================================
- Hits 51112 51085 -27
- Misses 20844 20847 +3
- Partials 2121 2122 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@dpgaspar I was wondering whether there would be merit in adding some frontend tests given that it seems the explore URL logic is handled completely by the frontend. |
@@ -41,6 +41,7 @@ assists people when migrating to a new version. | |||
|
|||
### Breaking Changes | |||
|
|||
- [24686]https://github.com/apache/superset/pull/24686): All dataset's custom explore_url are handled as relative URLs on the frontend, behaviour controlled by PREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dpgaspar since we were already validating that all explore_urls were of the same domain, what would possibly break with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All explore_url
is handled as relative, so existing URLs like: http://www.google.com
will result on the following link: http://localhost:8088/tablemodelview/list/http://www.google.com
Previously we were supporting both types, absolute URL and relative, internally when no value was provided we handled the default as relative (dataset link to explore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way that we can make this somewhat backwards compatible? Maybe a migration that trims off the scheme/domain/host and attempts to make the url into a relative one if it can? Like if ./explore. is in the string at least?
Otherwise, we may want to consider this PR for a major version bump. cc @michael-s-molina for thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I do think this breaks backward compatibility, but users can still opt in for the old behaviour, that I think it's safer and more explicit/clearer then changing metadata with a db migration.
Ultimately I do question the utility of allowing users to set their own custom default URL for datasets.
@michael-s-molina is it possible to include this one on 3.0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, perfect, then, I tagged it for 3.0.
yes, added frontend tests |
(cherry picked from commit a9efd4b)
SUMMARY
Use a better approach to handle custom default URLs on Datasets. Previously we were validating the URL itself and checked if it belonged to the same request host but this approach is not fail proof.
This PR makes all
explore_url
on datasets be handled as relative URLs, relative URLs were already supported and used on this field, now we enforce it whenPREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION