Model static UI5 bindElement contexts by mbaluda · Pull Request #448 · advanced-security/codeql-sap-js

mbaluda · 2026-10-02T12:19:56Z

What This PR Contributes

Adds a runnable UI5 application that demonstrates client-side XSS through an inherited bindElement context:
- An Input writes to the absolute default-model path /customer/name.
- HTML controls read relative {name} beneath string and object-form bindElement contexts.
- A Playwright regression verifies every supported positive sink and every negative sink independently.
Models statically known default-model contexts assigned with:
- control.bindElement("/absolute/path").
- The legacy control.bindElement("/absolute/path", parameters) overload.
- control.bindElement({ path: "/absolute/path", ... }) object binding information.
Scopes context propagation precisely:
- Applies the context only to the target control and its descendants in the owning controller/view.
- Supports both XML and JSON view control hierarchies.
- Selects the nearest nested bindElement context.
- Preserves a closer items aggregation context instead of incorrectly using an outer element context.
- Excludes named-model binding information and model>path strings.
Adds positive and negative CodeQL coverage for exact path resolution and XSS:
- Vulnerable absolute-source-to-relative-sink flows for all supported overloads.
- XML and JSON view contexts.
- Sibling controls that do not inherit the context.
- Named-model bindings.
- Nested bindElement overrides.
- Nested items aggregation precedence.
Depends on Model manifest-created UI5 client data models #447, which provides path-specific manifest-created JSON model nodes and linear canonical binding flow.
Validation:
- npm ci && npm run test:runtime in javascript/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context — passed (1/1 Playwright test).
- codeql test run javascript/frameworks/ui5/test — passed (81/81 CodeQL tests).
- git diff --check — passed.

Future Works

Support bindObject.
Support dynamically computed binding contexts.
Model setBindingContext, createBindingContext, and context-producing APIs.
Generalize context propagation to additional aggregation APIs and factory callbacks.

Copilot

Copilot review overview

🟡 Changes recommended

JSON-specific logic lacks coverage, and aggregate runtime assertions can conceal incorrect sink behavior.

Review effort: Balanced
Findings: 2

Open (2)

Tests omit JSON control-containment coverage · New
Aggregate marker counts allow regression cancellation · New

Resolved since last review (1)

Support binding-info and legacy bindElement overloads

Provide a runnable UI5 app and Playwright test proving unsanitized flow through an inherited bindElement context before static modeling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Resolve relative default-model bindings through the nearest statically known bindElement context while preserving closer aggregation contexts and view isolation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Accept the Code Scanning output produced by workflow run 37006051980 for the bindElement context regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Extract static default-model paths from legacy two-argument calls and object binding infos while excluding named-model contexts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Exercise JSON control containment and context precedence, and assert every positive and negative runtime sink independently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

JavaScript-view bindings remain unsupported, and the lockfile contains organization-specific package URLs.

Review effort: Balanced
Findings: 1 · 2

Open (3)

Lockfile pins dependencies to inaccessible Azure Artifacts feed · New
Containment check omits JavaScript-view controls · New
Tests omit JSON control-containment coverage

Resolved since last review (1)

Aggregate marker counts allow regression cancellation

Copilot · 2026-10-03T14:33:44Z

+    },
+    "node_modules/@adobe/css-tools": {
+      "version": "4.5.0",
+      "resolved": "https://ms-feed-25.pkgs.visualstudio.com/1es-public/_packaging/npm-public/npm/registry/@adobe/css-tools/-/css-tools-4.5.0.tgz",


Updated all 498 lockfile tarball URLs to registry.npmjs.org while preserving the locked dependency versions. Commit: 8743ee1.

Accept the Code Scanning output produced by workflow run 37032867682 after the latest UI5 context regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

Control-reference ambiguity can produce false contexts, and relative aggregation composition lacks regression coverage.

Review effort: Balanced
Findings: 1 · 3

Open (4)

Lockfile pins dependencies to inaccessible Azure Artifacts feed
Restrict view lookup references to the owning controller · New
Add regression test for relative aggregation binding composition · New
Containment check omits JavaScript-view controls

Resolved since last review (2)

Aggregate marker counts allow regression cancellation
Tests omit JSON control-containment coverage

Copilot · 2026-10-03T14:33:44Z

+    not exists(UI5BindingPath itemsBinding |
+      itemsBinding = bindingPath.getNearestEnclosingItemsBinding() and
+      controlIsDescendantOrSelf(itemsBinding.getControlDeclaration(), context.getControl())
+    )


Added a relative aggregation regression asserting /customer + {orders} + {name} resolves to /customer/orders/name. Commit: 8743ee1.

mbaluda · 2026-10-03T14:05:47Z

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply the suggestion exactly.

Do not make changes beyond what is described in the linked review thread.

Co-authored-by: mbaluda <5237080+mbaluda@users.noreply.github.com>

Copilot

Copilot review overview

🔵 Needs a closer look

Path-resolution errors and ineffective regression fixtures still need correction.

Review effort: Balanced
Findings: 1 · 3

Open (4)

Lockfile pins dependencies to inaccessible Azure Artifacts feed
Add regression test for relative aggregation binding composition
Restrict view lookup references to the owning controller
Containment check omits JavaScript-view controls

Previously missed (4)

In code that hasn't changed since last review

Avoid duplicate separators in resolved binding paths

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll:212

With bindElement("/customer/"), UI5 resolves {name} to /customer/name, but this helper produces /customer//name. Manifest-backed flow compares absolute paths exactly, so an input bound to /customer/name no longer reaches this sink. Reuse an existing trailing separator, and add a path-resolution regression for this case.

Use VBox items aggregation for child controls

javascript/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/json_component/view/app.view.json:22

Both jsonCustomerDetails and jsonOtherDetails are sap.m.VBox controls, which use the inherited items aggregation, not content. UI5 therefore cannot attach these children as written, even though the expected results treat them as an inherited-context XSS flow and a nested context override. Change both child aggregations to items so the fixture represents a valid UI5 hierarchy.

Use a valid items aggregation for context binding

javascript/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/json_component/view/app.view.json:32

sap.ui.commons.Carousel has a content aggregation, not items, so items: "{/otherList}" does not create a list-entry context in UI5. The expected /otherList/name result thus validates a context that this fixture cannot produce. Use a control with an items aggregation and a valid JSONView binding/template, then update both expected-result files to verify aggregation precedence on that hierarchy.

Match cloned aggregation sink IDs

javascript/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/runtime/xss.spec.js:5

The List renders clones of its template, with IDs such as ...--aggregationSink-__clone0. The current suffix selector misses those clones, so the aggregationSink negative assertion passes even if a rendered clone contains the injected marker. Match cloned sink IDs as well as the original IDs.

Co-authored-by: mbaluda <5237080+mbaluda@users.noreply.github.com>

Copilot · 2026-10-03T14:33:44Z

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested ch...

Addressed the linked review findings in 8743ee1. The targeted CodeQL tests and Playwright regression pass.

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved context-resolution and fixture correctness issues can produce inaccurate XSS results.

Review effort: Balanced
Findings: 1 · 5

Open (6)

Lockfile pins dependencies to inaccessible Azure Artifacts feed
Prevent context inheritance across model overrides · New
Treat relative element bindings as context barriers · New
Avoid duplicate separators in resolved binding paths · New
Use view-prefixed IDs for controller lookups · New
Add regression test for relative aggregation binding composition

Resolved since last review (2)

Restrict view lookup references to the owning controller
Containment check omits JavaScript-view controls

+    not exists(bindingPath.getModelName()) and
+    controller = bindingPath.getView().getController() and
+    controlContainsBinding(this.getControl(), bindingPath)


+  not exists(StaticBindElementContext closer |
+    closer.appliesTo(bindingPath) and
+    controlIsStrictDescendant(closer.getControl(), result.getControl())


+  if contextPath = "/"
+  then result = "/" + relativePath


+      new sap.m.Input({
+        value: "{/customer/name}"
+      }),
+      new sap.ui.core.HTML("jsBoundSink", {


mbaluda added this pull request to stack #449 October 2, 2026 12:19

mbaluda changed the title ~~mbaluda/ui5 binding contexts~~ Model static UI5 bindElement contexts Oct 2, 2026

github-advanced-security Bot found potential problems Oct 2, 2026

View reviewed changes

Comment thread javascript/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/webapp/view/app.view.xml Dismissed

mbaluda requested a balanced review from Copilot October 2, 2026 12:56

Copilot started reviewing on behalf of mbaluda October 2, 2026 12:56

View session

This comment was marked as resolved.

Sign in to view

mbaluda force-pushed the mbaluda/ui5-binding-contexts branch from 6eb5273 to b591929 Compare October 2, 2026 13:41

github-advanced-security Bot found potential problems Oct 2, 2026

View reviewed changes

Comment thread javascript/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/webapp/view/app.view.xml Dismissed

Comment thread javascript/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/webapp/view/app.view.xml Dismissed

mbaluda requested a balanced review from Copilot October 2, 2026 14:34

Copilot started reviewing on behalf of mbaluda October 2, 2026 14:35

View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Comment thread javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll

Comment thread javascript/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/runtime/xss.spec.js Outdated

mbaluda and others added 5 commits October 2, 2026 18:12

Add runtime bindElement XSS regression

b1161f7

Provide a runnable UI5 app and Playwright test proving unsanitized flow through an inherited bindElement context before static modeling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Model static UI5 bindElement contexts

9103982

Resolve relative default-model bindings through the nearest statically known bindElement context while preserving closer aggregation contexts and view isolation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Update JavaScript SARIF expectation

673a67d

Accept the Code Scanning output produced by workflow run 37006051980 for the bindElement context regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Support bindElement context overloads

a3ae91c

Extract static default-model paths from legacy two-argument calls and object binding infos while excluding named-model contexts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Cover JSON bindElement context behavior

074f099

Exercise JSON control containment and context precedence, and assert every positive and negative runtime sink independently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI balanced review requested due to automatic review settings October 2, 2026 16:16

mbaluda force-pushed the mbaluda/ui5-binding-contexts branch from 0ff1a36 to 074f099 Compare October 2, 2026 16:16

Copilot started reviewing on behalf of mbaluda October 2, 2026 16:18

View session

github-advanced-security Bot found potential problems Oct 2, 2026

View reviewed changes

Comment thread ...rameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/json_component/view/app.view.json Dismissed

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Update JavaScript SARIF expectation

6a95d7e

Accept the Code Scanning output produced by workflow run 37032867682 after the latest UI5 context regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI balanced review requested due to automatic review settings October 2, 2026 16:33

Copilot started reviewing on behalf of mbaluda October 2, 2026 16:34

View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Copilot started work on behalf of mbaluda October 3, 2026 14:06

View session

Address bindElement review feedback

e9555da

Co-authored-by: mbaluda <5237080+mbaluda@users.noreply.github.com>

Copilot AI balanced review requested due to automatic review settings October 3, 2026 14:09

Copilot started reviewing on behalf of mbaluda October 3, 2026 14:11

View session

Copilot AI reviewed Oct 3, 2026

View reviewed changes

Fix UI5 bindElement context review findings

8743ee1

Co-authored-by: mbaluda <5237080+mbaluda@users.noreply.github.com>

Copilot AI balanced review requested due to automatic review settings October 3, 2026 14:32

Copilot started reviewing on behalf of mbaluda October 3, 2026 14:32

View session

Copilot finished work on behalf of mbaluda October 3, 2026 14:34

Copilot AI reviewed Oct 3, 2026

View reviewed changes

github-advanced-security Bot found potential problems Oct 3, 2026

View reviewed changes

Comment thread ...pt/frameworks/ui5/test/queries/UI5Xss/xss-bind-element-context/js_component/view/app.view.js Dismissed

mbaluda closed this Oct 3, 2026

Copilot stopped work on behalf of mbaluda due to an error October 3, 2026 21:37
Pull request is not open

Copilot stopped work on behalf of mbaluda due to an error October 3, 2026 21:39
Pull request is not open

mbaluda deleted the mbaluda/ui5-binding-contexts branch October 4, 2026 10:10

Conversation

mbaluda commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What This PR Contributes

Future Works

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Copilot AI Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

mbaluda commented Oct 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI commented Oct 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mbaluda commented Oct 2, 2026 •

edited

Loading