Remove html escape #1318
Merged
Remove html escape #1318
+985
−688
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Fix XSS vulnerabilities and enhance security testing across frontend - Add comprehensive XSS protection test covering runs, watchlist, and compare pages - Fix XSS vulnerability in compare.js by escaping comparison_identifiers - Improve replaceRepoIcon function with proper error handling and escaping 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix some js linting errors * Fix display of repo name + remove unnecessary escaping * Add stats page to xss test + fix compare test * Add usage scenario to XSS test * Add logs to XSS test * Add flow name to XSS test * Add xss test case for notes * Add escaping of notes - not needed, but to be consistent * Add xss test and fixes for eco-ci pages * Refactor setup of db data for frontend tests * Use consistent xss detection approach * Cleanup xss tests * Add usage scenario variables xss test and fixes * Fix URL encoding in frontend JavaScript Replace escapeString with encodeURIComponent for URL query parameters to ensure proper URL encoding instead of HTML entity encoding. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix also commented out lines * Fix XSS vulnerabilities in URL contexts and improve URL encoding - Fix XSS vulnerabilities in URI href attributes by validating http/https protocols only - Add createExternalIconLink helper for consistent and safe external link creation - Fix malformed URL in timeline badge src attribute - Add encodeURIComponent to buildQueryParams function for proper URL parameter encoding - Remove unnecessary encoding from commit hashes (inherently URL-safe) - Improve URI display logic with better variable naming and security comments 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Remove tests for HTML escaping in backend - is now done in frontend [skip ci] * Add comment about headless config * Add timeline page to xss test + improve URI encoding * Cleanup reset_db() * Update comment about removal of page error handler --------- Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the html escaping in the API and adds escaping to the frontend.
The initial design decision at the time was that we wanted a "clean DB", meaning that we clean everything before it gets in, as we control insert locations easier than read locations.
However over time and many developrs this became quite garbled and inconsistent having multiple escapings "just to be sure".
This PR removes all escaping on the server side and only adds escaping when we output to HTML (aka the Dashboard).
This means:
This solution is much cleaner and caters more for the diverse use cases of the GMT Suite as it is now:
TODO
@davidkopp This PR is so far largely untested.