chore: Reduce and restrict innerHTML use#26214
Conversation
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| export function sanitizeFileName(fileName: string) { |
There was a problem hiding this comment.
this can be used for images, or anywhere else we need to process a file path.
| return fileName.replace(/[^a-zA-Z0-9-_]/g, ''); | ||
| } | ||
|
|
||
| export function sanitizeUrl(url: string) { |
There was a problem hiding this comment.
This can be used to sanitize urls to remove evil content
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26214 +/- ##
==========================================
- Coverage 69.19% 69.18% -0.01%
==========================================
Files 1945 1944 -1
Lines 75928 75882 -46
Branches 8453 8441 -12
==========================================
- Hits 52537 52502 -35
+ Misses 21207 21201 -6
+ Partials 2184 2179 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| // eslint-disable-next-line no-param-reassign | ||
| element.innerHTML = ''; | ||
| element.replaceChildren(); |
There was a problem hiding this comment.
The old way is fine, but this is less icky. I'm NOT sure why this wasn't called out by eslint in any of these plugins. But, this should work fine (let's test to make sure)
|
This'll need tests for superset-ui-core utils added if we want to bother with that approach. |
|
👍 this approach looks good and should help with any future issues in this area. Not sure why the frontend build is failing. Would be good to bring up an ephemeral env to test this more thoroughly |
|
Can't bring up the ephemeral env without the build passing. Perhaps I overshot the mark here... I'll close this and open more incremental PRs to see what passes/fails. |
2 participants
SUMMARY
innerHTML is gross... this PR adds linting rules to restrict use of this method, and replaces or annotates existing usage.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
There are a few plugins that were touched that need testing:
Dynamic plugins have their web URL bundle sanitized... this should work, but if not, very few people will be affected since this is an Alpha feature.
Icon file paths are also being sanitized - we should check for these in general, though it seems things should work.
ADDITIONAL INFORMATION