Potential fix for code scanning alert no. 14: Incomplete multi-character sanitization #61
Conversation
…ter sanitization Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
Thanks for contributing to EngineScript! 🎉 If your PR fixes an issue or relates to a specific environment, please consider including the sanitized output We'll review your PR soon! |
|
| input = input | ||
| .replace(/\0/g, '') // Remove null bytes first | ||
| .replace(/[<>&"'`]/g, '') // Remove HTML/XML special characters that could break out of attributes | ||
| .replace(/javascript:/gi, '') // Remove javascript: protocol | ||
| .replace(/data:/gi, '') // Remove data: protocol | ||
| .replace(/vbscript:/gi, '') // Remove vbscript: protocol | ||
| .replace(/on\w+=/gi, '') // Remove event handlers |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To address the issue, the sanitization logic in the sanitizeLogContent method should be updated to ensure that all unsafe patterns are removed effectively. The best approach is to rewrite the regular expression /on\w+=/gi to match individual unsafe characters or patterns, or to use a well-tested sanitization library like sanitize-html for comprehensive sanitization.
In this case, we will rewrite the regular expression to match individual unsafe characters and apply the sanitization repeatedly until no changes occur. This ensures that all instances of unsafe patterns are removed, even in edge cases with overlapping or nested patterns.
| @@ -904,3 +904,3 @@ | ||
| .replace(/vbscript:/gi, '') // Remove vbscript: protocol | ||
| .replace(/on\w+=/gi, '') // Remove event handlers | ||
| .replace(/on(?:\w+|[^\s]*)=/gi, '') // Remove event handlers and unsafe attribute patterns | ||
| .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, ''); // Remove control chars but keep \t, \n, \r |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the sanitizeLogContent method to iteratively apply regex replacements until no further matches remain, then enforces a maximum length.
- Wraps the replace chain in a
do/whileloop to handle consecutive patterns - Moves the 50,000-character substring limit to after all sanitization passes
Comments suppressed due to low confidence (1)
config/var/www/admin/control-panel/dashboard.js:897
- There are no tests covering cases with multiple consecutive occurrences of malicious patterns. Please add unit tests for
sanitizeLogContentto validate that repeated matches are fully removed.
do {
| // Keep line breaks and basic formatting for readability but remove XSS vectors | ||
| return input | ||
| .replace(/\0/g, '') // Remove null bytes first | ||
| .replace(/[<>&"'`]/g, '') // Remove HTML/XML special characters that could break out of attributes | ||
| .replace(/javascript:/gi, '') // Remove javascript: protocol | ||
| .replace(/data:/gi, '') // Remove data: protocol | ||
| .replace(/vbscript:/gi, '') // Remove vbscript: protocol | ||
| .replace(/on\w+=/gi, '') // Remove event handlers | ||
| .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, '') // Remove control chars but keep \t, \n, \r | ||
| .substring(0, 50000); // Reasonable log size limit | ||
| let previous; | ||
| do { | ||
| previous = input; | ||
| input = input |
There was a problem hiding this comment.
The iterative sanitization loop scans the entire input string unbounded until no further changes, which could be expensive for very large inputs. Consider applying the substring limit inside the loop or capping the input length before each iteration to bound work per pass.
|
🎉 EngineScript Nginx Build Test PASSED 🎉 Nginx core component built successfully:
✅ Ready for deployment testing! |
Potential fix for https://github.com/EngineScript/EngineScript/security/code-scanning/14
To fix the issue, the sanitization logic should be updated to repeatedly apply the regular expression replacements until no further matches are found. This ensures that all instances of the targeted patterns, including multiple consecutive occurrences, are removed. The
sanitizeLogContentmethod should be modified to use a loop that applies the replacements iteratively until the input string remains unchanged. This approach is consistent with best practices for handling incomplete multi-character sanitization.Suggested fixes powered by Copilot Autofix. Review carefully before merging.