-
Notifications
You must be signed in to change notification settings - Fork 3
ui5/webcomponents-react FP improvements for OOTB queries #244
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
base: main
Are you sure you want to change the base?
Conversation
…ecurity/codeql-sap-js into knewbury01/webcomponent-react
|
We keep the test pack at |
…ling to filter some results for out of the box xss query and improve test for it
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.
Pull request overview
This PR reduces false positives in the XssThroughDom query for UI5 webcomponents-react by adding sanitizers for components that don't allow arbitrary user input. The changes introduce a comprehensive test suite demonstrating XSS patterns with various UI5 input components and excludes 14 component types that only accept predefined selections or numeric values.
Key changes:
- Added sanitizer logic to filter out false positives from UI5 webcomponents-react components that constrain user input (e.g., checkboxes, sliders, color pickers)
- Implemented API modeling for UI5 webcomponents-react using CodeQL's type tracking
- Created a comprehensive test case with 25 UI5 input components to validate the sanitizer behavior
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| javascript/frameworks/ui5/lib/advanced_security/javascript_sap_ui5_all/Customizations.qll | Imports the new Sanitizers module into the customizations |
| javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5WebcomponentsReact.qll | Implements API modeling for UI5 webcomponents-react with type tracking and ref attribute handling |
| javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Sanitizers.qll | Defines the ExcludedSource sanitizer class that filters out 14 component types that don't allow arbitrary user input |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/src/index.tsx | React app entry point for the test application |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/src/App.tsx | Test application demonstrating XSS patterns with 25 UI5 webcomponents, including both vulnerable and false positive cases |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/qlpack.yml | CodeQL pack configuration for the test |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/public/index.html | HTML template for the test application |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/package.json | NPM dependencies for the test application including UI5 webcomponents v2.15 |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/codeql-pack.lock.yml | CodeQL pack dependency lock file |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/XssThroughDom.qlref | Query reference file |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/XssThroughDom.ql | Copy of the XssThroughDom query for testing the sanitizer customizations |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/XssThroughDom.expected | Expected test results showing 11 components flagged as vulnerable while 14 are correctly filtered out |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/README.md | Documentation explaining how to trigger XSS in the test application |
| javascript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/.eslintrc.json | ESLint configuration for the test application |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/XssThroughDom.ql
Outdated
Show resolved
Hide resolved
This reverts commit 881e5f9.
…-dangerouslySetInnerHTML/XssThroughDom.ql Co-authored-by: Copilot <[email protected]>
…vanced-security/codeql-sap-js into knewbury01/webcomponent-react
|
First round!
|
|
One more: Consider using |
...ript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/src/App.tsx
Outdated
Show resolved
Hide resolved
...ript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/src/App.tsx
Outdated
Show resolved
Hide resolved
...ript/frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/src/App.tsx
Outdated
Show resolved
Hide resolved
...frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/XssThroughDom.ql
Outdated
Show resolved
Hide resolved
...frameworks/ui5-webcomponents/test/queries/xss-input-dangerouslySetInnerHTML/XssThroughDom.ql
Outdated
Show resolved
Hide resolved
...ipt/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5WebcomponentsReact.qll
Outdated
Show resolved
Hide resolved
...ipt/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5WebcomponentsReact.qll
Show resolved
Hide resolved
jeongsoolee09
left a comment
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.
Second round!
| /** | ||
| * Refers to the ref attribute | ||
| * <SomeElement ref={x}> | ||
| */ | ||
| class RefAttribute extends JsxAttribute { | ||
| RefAttribute() { this.getName() = "ref" } | ||
| } |
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.
Unless you think it is meaningful as a standalone class, we can remove it and instead place the ref name restriction on UseRefDomValueSource as jsx.getAttributeByName("ref").getValue().flow().getALocalSource().getAPropertyRead("current") (note the getAttributeByName("ref").
Feel free to dismiss this if you think it is reusable in future definitions.
| /** | ||
| * Holds if the ref variable is assigned to a UI5 component via JSX | ||
| */ | ||
| predicate isRefAssignedToUI5Component(UseRefDomValueSource source) { | ||
| exists( | ||
| Variable refVar, JsxElement jsx, RefAttribute attr, VarRef componentVar, WebComponentImport decl | ||
| | | ||
| source.getElement() = jsx and | ||
| // The JSX element uses a UI5 webcomponent for react | ||
| jsx.getNameExpr() = componentVar and | ||
| decl.asExpr() = componentVar.getVariable().getADefinition() and | ||
| // The JSX element has a ref attribute pointing to our ref variable | ||
| jsx.getAnAttribute() = attr and | ||
| attr.getValue().(VarRef).getVariable() = refVar | ||
| ) | ||
| } |
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.
I guess you wanted to express this:
The UI5 component has a variable assigned to it and its
currentproperty is read somewhere.
... but there is no easy way to associate Input in the import statement and the Input in the component.
Well, React::ReactComponent.ref/0 actually does that cleanly; but we can't use it until we make UI5WebComponent extend ReactComponent. Let's mention this in the Future Works and raise an issue so that we can come back to it. For now we can let SSA handle it.
| }; | ||
| }, [handleComboBoxChange]); | ||
|
|
||
| /* `MultiComboBox`: component usage */ |
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.
I found that MultiComboBox actually can hold unrestricted string type: the value property holds whatever the user types into the search box for candidates.
| }; | ||
| }, [handleMultiComboBoxChange]); | ||
|
|
||
| /* `Select` component usage */ |
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.
Please record the kind of inputs (int / boolean / enum-like string, ...) the components can hold. For this one:
| /* `Select` component usage */ | |
| /* `Select`: Accepts enum-like string */ |
| exists(RefAttribute attrib | | ||
| attrib.getValue().flow().getALocalSource().getAPropertyRead("current") = this and | ||
| jsx.getAnAttribute() = attrib | ||
| ) |
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.
Readability improvement:
| exists(RefAttribute attrib | | |
| attrib.getValue().flow().getALocalSource().getAPropertyRead("current") = this and | |
| jsx.getAnAttribute() = attrib | |
| ) | |
| this = jsx.getAnAttribute().getValue().flow().getALocalSource().getAPropertyRead("current") |
| /** | ||
| * ``` javascript | ||
| * import { Input, Button } from '@ui5/webcomponents-react'; | ||
| * ``` | ||
| */ |
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.
Better description:
| /** | |
| * ``` javascript | |
| * import { Input, Button } from '@ui5/webcomponents-react'; | |
| * ``` | |
| */ | |
| /** | |
| * Classes imported from `@ui5/webcomponents-react`. e.g. | |
| * | |
| * ``` javascript | |
| * import { Input, Button } from '@ui5/webcomponents-react'; | |
| * ``` | |
| */ |
| /** | ||
| * Refers to the ref attribute | ||
| * <SomeElement ref={x}> | ||
| */ |
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.
| /** | |
| * Refers to the ref attribute | |
| * <SomeElement ref={x}> | |
| */ | |
| /** | |
| * The `ref` attribute of a JSX attribute. e.g. | |
| * | |
| * ``` javascript | |
| * <SomeElement ref={x}> | |
| * ``` | |
| */ |
jeongsoolee09
left a comment
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.
Probably the final round.
What This PR Contributes
ui5/webcomponents-reactinput types can be used for input in react apps (includes only types that have a value property)Future Works