refactor(calm-hub): use centralized strict sanitization policy to prevent XSS#1859
refactor(calm-hub): use centralized strict sanitization policy to prevent XSS#1859rocketstack-matt wants to merge 2 commits intofinos:mainfrom
Conversation
Refactors CoreSchemaResource to use ResourceValidationConstants.STRICT_SANITIZATION_POLICY instead of creating a new policy instance. This ensures consistent application of security policies. Adds a regression test TestSanitizationSecurityShould to verify immunity against OWASP HtmlPolicyBuilder XSS vulnerability involving noscript and style tags. Amp-Thread-ID: https://ampcode.com/threads/T-493cb4c5-944e-42fc-8463-e96025b5776f Co-authored-by: Amp <amp@ampcode.com>
rocketstack-matt
requested review from
Thels,
grahampacker-ms and
jpgough-ms
as code owners
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the CoreSchemaResource class to use a centralized strict sanitization policy (ResourceValidationConstants.STRICT_SANITIZATION_POLICY) instead of creating a new policy instance locally. This ensures consistent application of XSS prevention policies across the codebase. The PR also adds a regression test (TestSanitizationSecurityShould) to verify immunity against the OWASP HtmlPolicyBuilder XSS vulnerability involving noscript and style tags referenced in security advisory #112.
- Centralizes sanitization policy to ensure consistency and easier maintenance
- Adds comprehensive security regression tests for XSS vulnerability scenarios
- Aligns
CoreSchemaResourcewith the pattern already used byPatternResource
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| calm-hub/src/main/java/org/finos/calm/resources/CoreSchemaResource.java | Refactored to reference the centralized STRICT_SANITIZATION_POLICY constant instead of creating a new HtmlPolicyBuilder instance |
| calm-hub/src/test/java/org/finos/calm/resources/TestSanitizationSecurityShould.java | New test class verifying the strict sanitization policy properly prevents XSS attacks, including specific noscript/style tag vulnerabilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| public class CoreSchemaResource { | ||
|
|
||
| private static final PolicyFactory STRICT_SANITIZATION_POLICY = new HtmlPolicyBuilder().toFactory(); | ||
| private static final PolicyFactory STRICT_SANITIZATION_POLICY = ResourceValidationConstants.STRICT_SANITIZATION_POLICY; |
There was a problem hiding this comment.
[nitpick] Consider using a static import for STRICT_SANITIZATION_POLICY instead of creating a local constant that references it. This would be more consistent with how PatternResource uses the same constant (see line 26 of PatternResource.java).
Current approach:
private static final PolicyFactory STRICT_SANITIZATION_POLICY = ResourceValidationConstants.STRICT_SANITIZATION_POLICY;Suggested approach:
import static org.finos.calm.resources.ResourceValidationConstants.STRICT_SANITIZATION_POLICY;This would eliminate the redundant local constant and align with the existing codebase pattern.
| public class TestSanitizationSecurityShould { | ||
|
|
||
| @Test | ||
| public void withstandNoscriptStyleInjectionAttack() { |
There was a problem hiding this comment.
[nitpick] Test method names in this codebase use snake_case rather than camelCase. Consider renaming to withstand_noscript_style_injection_attack for consistency with other test files like TestCoreSchemaResourceShould, TestArchitectureResourceShould, and even domain tests like TestDecisionShould.
| public void withstandNoscriptStyleInjectionAttack() { | |
| public void withstand_noscript_style_injection_attack() { |
| } | ||
|
|
||
| @Test | ||
| public void actAsStrictPolicy() { |
There was a problem hiding this comment.
[nitpick] Test method names in this codebase use snake_case rather than camelCase. Consider renaming to act_as_strict_policy for consistency with other test files.
| public void actAsStrictPolicy() { | |
| public void act_as_strict_policy() { |
Description
Security advisory https://github.com/finos/architecture-as-code/security/dependabot/112 highlights a potential XSS issue, analysis shows this is not an issue with the current implementation, but also that there are a couple of small improvements we could make, which are in this PR.
Refactors CoreSchemaResource to use ResourceValidationConstants.STRICT_SANITIZATION_POLICY
instead of creating a new policy instance. This ensures consistent application of security policies.
Adds a regression test TestSanitizationSecurityShould to verify immunity against
OWASP HtmlPolicyBuilder XSS vulnerability involving noscript and style tags.
Type of Change
Affected Components
cli/)shared/)calm-widgets/)calm-hub/)calm-hub-ui/)docs/)calm-plugins/vscode/)Commit Message Format ✅
Testing
Checklist