Ensure sanitizeContent attribute is respected

[mbaluda]

Key changes:

UI5 Control and Sanitization Logic

• Adds the predicate UI5Control::isHTMLSanitized to identify controls where sanitization is enabled or disabled via the sanitizeContent/sanitizeValue property.
• Updated the detection logic for HTML injection sinks to exclude controls where sanitization is enabled

UI5 Control and Sanitization Logic

• Augmented existing tests to only flag HTML injection sinks when the control is not sanitized (models/sink xss-html-control xss-js-view xss-json-view README)

CodeQL Workflow Updates

• Upgraded the CodeQL GitHub Actions

[Copilot]

Pull request overview

This PR enhances the CodeQL analysis for UI5 applications by ensuring the sanitizeContent attribute is properly respected when detecting HTML injection vulnerabilities. The changes extract control-related logic into a dedicated module and improve the detection of sanitized vs. unsanitized HTML controls across all view types (XML, JSON, and JavaScript).

Key changes:

• Extracted UI5Control and UI5ControlProperty classes from UI5View.qll into a new UI5Control.qll file
• Introduced isSanitizedControl() predicate to identify controls with sanitization enabled/disabled both declaratively and programmatically
• Updated HTML injection sink detection to exclude sanitized controls from vulnerability reports
• Enhanced test coverage with scenarios for declarative and programmatic sanitization control
• Upgraded CodeQL GitHub Actions from v3 to v4

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5Control.qll	New file containing extracted UI5Control and UI5ControlProperty classes with isSanitizedControl() predicate for sanitization detection
javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll	Removed UI5Control and UI5ControlProperty class definitions (moved to UI5Control.qll) and removed XML-specific sanitization checks
javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll	Added import for new UI5Control module
javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll	Added sanitization check to filter sanitized controls from HTML injection sinks
javascript/frameworks/ui5/test/models/sink/UI5ViewSinkTest.ql	Updated query to exclude sanitized controls from sink detection
javascript/frameworks/ui5/test/models/sink/sink1.xml	Added test case for sanitized HTML control
javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-control/webapp/view/app.view.xml	Added comprehensive test cases for declarative and programmatic sanitization scenarios
javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-control/webapp/controller/app.controller.js	Added controller logic to test programmatic sanitization control via setProperty()
javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-control/UI5Xss.expected	Updated expected results to reflect proper sanitization detection
javascript/frameworks/ui5/test/queries/UI5Xss/xss-json-view/webapp/view/app.view.json	Added test case for sanitized HTML control in JSON view
javascript/frameworks/ui5/test/queries/UI5Xss/xss-json-view/UI5Xss.expected	Updated expected results for JSON view sanitization
javascript/frameworks/ui5/test/queries/UI5Xss/xss-js-view/webapp/view/app.view.js	Added test case for sanitized HTML control in JS view
javascript/frameworks/ui5/test/queries/UI5Xss/xss-js-view/UI5Xss.expected	Updated expected results for JS view sanitization
javascript/frameworks/ui5/test/README.md	Updated documentation to describe sanitization testing scenarios
.github/workflows/code_scanning.yml	Upgraded CodeQL Actions from v3 to v4

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jeongsoolee09]

Second round!

[jeongsoolee09]

The property names for sap.ui.richttexteditor.RichTextEditor and sap.ui.core.HTML are different:

• sanitizeValue for RichTextEditor, and
• sanitizeContent for HTML.

So, I suggest these:

• Add an abstract class in UI5XssQuery with abstract predicate contentSanitized.
• Add class for each in UI5XssQuery that extends UI5Control and implements contentSanitized with their respective properties.
• Plug them in into isBarrier.

[mbaluda]

The property names for sap.ui.richttexteditor.RichTextEditor and sap.ui.core.HTML are different:

Good catch! I have fixed the implementation and added sink tests for both the controls (with their specific property names)

• Add an abstract class in UI5XssQuery with abstract predicate contentSanitized.
• Add class for each in UI5XssQuery that extends UI5Control and implements contentSanitized with their respective properties.

I think that the presence of the sanitizing property should be modelled in the Control not in the Query.
I changed the naming to isHTMLSanitized for clarity.

• Plug them in into isBarrier.

The link between the sink and the primarysink is only established at the very end in UI5XSS, I don't think the barrier can be an element of the .view file.

[jeongsoolee09]

We can straighten out the double negation.

Suggested change

	not node.getArgument(1).mayHaveBooleanValue(val.booleanNot())
	node.getArgument(1).mayHaveBooleanValue(val)

[mbaluda]

That is not equivalent!
There are cases where we don't know what the value may be, so not mayHaveVal(true) would be equivalent to mustHaveVal(false) but that doesn't exist

[jeongsoolee09]

I still believe there's a bug in this predicate. If you quick-evaluate the body of this test predicate:

private predicate test(int startline) {
  this.isSanitizePropertySetTo("sanitizeContent", true) and
  this.hasLocationInfo(_, _, startline, _, _)  // for easy inspection
}

It finds the HTML control at line 50 (id="htmUnsanitized") with this API call attached:

// disable sanitization programmatically
this.getView().byId("htmUnsanitized").setProperty("sanitizeContent", false);

...when this.isSanitizePropertySetTo("sanitizeContent", true) shouldn't hold here.

[jeongsoolee09]

Very minor, but I guess getValue conveys the meaning better (and we should avoid using .toString() on non-primitive QL types anyways).

Suggested change

	this.getProperty(propName).toString() = val.toString()
	this.getProperty(propName).getValue() = val.toString()

[mbaluda]

I agree in principle, but getValue() does not return what you'd expect for an XmlElement

[jeongsoolee09]

Two things:

1. Since we have "false sanitization" here (.sanitizeContent = true), can we say in the comment if it's really sanitized using SAFE and UNSAFE like we're doing for model tests, like /* SAFE: {reason} */ and /* UNSAFE: {reason} */?
2. HTML.setSanitizeContent and RichTextEditor.setSanitizeValue are missing. Please add those in the test codes (query + model) and support them in the QL code as well.
3. Minor, I noticed the missing l in htmUnsanitized. Can you fix the typo?

[jeongsoolee09]

Left another round.

[jeongsoolee09]

I found it helps to draw a table like this (this is also what I'm using when reviewing this PR):

	sanitizeContent=true	sanitizeContent=false	sanitizeContent= N/A
setSanitizeContent(true)	SAFE	SAFE	SAFE
setSanitizeContent(false)	UNSAFE	UNSAFE	UNSAFE
setSanitizeContent N/A	SAFE	UNSAFE	UNSAFE

For now, I find it hard to understand what table the isSanitizePropertySetTo + isHTMLSanitized combination infers, mainly because some of the rows are filtered on isHTMLSanitized and in isHTMLSanitized: isSanitizePropertySetTo negates rows, and this result is negated once again in isHTMLSanitized.

I strongly recommend encoding the above table only in one predicate and make the other predicate simply use its result directly.

Then, what we need is to just encode this table (again, all in a single predicate!), but in a principled way: we can exploit the fact that setProperty/setSanitizeContent take precedence over setting the attribute of UI5Control. So, you can encode the rows in the above table in a broad stroke by filtering first with what value is set through setProperty/setSanitizeContent, and take out the remaining edge cases.