Skip to content

Fix query-filter escaping and HTML injection in UI filter editors - #204

Open
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-high-remaining
Open

vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-high-remaining

Conversation

@vharseko

@vharseko vharseko commented Sep 18, 2026

Copy link
Copy Markdown
Member

Summary

Closes the remaining high-severity CodeQL alerts: js/incomplete-sanitization #739–#743, js/xss-through-dom #745–#748 and java/implicit-cast-in-compound-assignment #910. (#744 was dismissed as a false positive: elementId is an editor-generated index path, never user text.)

#743 — policy.js unique policy (the one that matters)

The value under validation is user-supplied (it arrives with the create/update request, including self-registration) and was embedded in a _queryFilter string literal with only " escaped. A trailing \ made the filter unparseable (policy evaluation fails instead of returning a result), and a \" sequence could alter the filter structure. Same class as CVE-2026-62961; the fix reuses the auth/queryFilter.escapeStringValue helper introduced there.

New policyUniqueTest.js drives the real policy.js (it is a script, not a module: it is read from the classpath and evaluated with stubbed host globals) and asserts the exact _queryFilter handed to openidm.query. Failed on master for the backslash cases.

#739 — BackgridUtils.queryFilter

Admin-UI grid filter text: backslash is now escaped before the quote. No privilege impact (the admin builds their own query), but a \ in the filter box used to break the grid. BackgridUtilsTest.js (previously an empty stub) covers it.

#747 / #748 — LinkQualifierFilterEditor / UserQueryFilterEditor

Property names taken from the stored mapping/role condition were concatenated into <option value="…">…</option> markup and into a jQuery attribute selector. Options are now built through the DOM API ($("<option>").attr("value", …).text(…)) and matched by comparing option.value.

The new UserQueryFilterEditorTest.js also caught a pre-existing functional bug: the lookup searched for "/" + tempValue although the stored name is already a JSON pointer (/mail), so it never matched and appended a duplicate //mail option for every known property. The pointer is now normalised (/mail kept, bare mail/mail).

#745 / #746 — PoliciesDialogView

.html(option.text() + "&#9734;").text(option.text() + "☆"); the star characters are literals now (the templates already use a literal ).

#740–#742 — GenericEditResourceView

selector.replace("\\", "") removed only the first escaping backslash of a \.a\.b selector; for nested relationship properties with more than one dot the JSONEditor path was wrong. Now replace(/\\/g, ""). Functional, not security.

#910 — ReconTypeBase

short ignoreFieldsint.

Second commit — the remaining warning-level alerts in policy.js (#768–#770, #782, #783, #895, #896)

  • validNameFormat: \u00D3, \u00F3, \u0178 were listed twice in the character class; second occurrences dropped (same semantics).
  • cannotContainDuplicates: the loop index i was undeclared and leaked into the global scope.
  • policyProcessor: getAppliedConditionalPolicies ended with } instead of },, so ASI cut the module's var list short and validate, mergePolicies, getAdditionalPolicies, updateResourceConfig and processRequest became globals.
  • The policyRequirements = validate(…) assignments were dead — validate() reports through retArray and returns nothing.

Not changed, worth a separate look: the same regex starts with [A-Za'-\u0105…], which is not a-z' but a range from ' (U+0027) to ą (U+0105) — it lets digits and most punctuation through validNameFormat. It reads like a typo for A-Za-z', but fixing it would tighten name validation, so it needs a product decision.

Test plan

  • policyUniqueTest.js — fails on master for a\ and x\" or …, passes here; full ScriptRunnerTest (8 JS test modules, including policyFilterTest which also exercises policy.js) green via mvn -pl openidm-zip -am package after both commits
  • Admin UI grunt build: QUnit 101/101 (8 new tests; 5 failed before the change), eslint clean
  • openidm-ui-common build green
  • CodeQL on this PR closes #739–#743, #745–#748, #910 and #768–#770, #782, #783, #895, #896

- policy.js "unique": the user-supplied value is now escaped with
  auth/queryFilter.escapeStringValue, so a backslash or quote in it can
  neither break the _queryFilter literal nor inject predicates
- BackgridUtils.queryFilter: escape backslashes before quotes
- UserQueryFilterEditor / LinkQualifierFilterEditor: build <option>
  elements through the DOM API instead of string concatenation; the
  UserQueryFilterEditor lookup also no longer prefixes an extra "/",
  which used to add a duplicate option for every known property
- PoliciesDialogView: set the default-action stars as text
- GenericEditResourceView: strip every escaping backslash from the
  selector, not only the first one
- ReconTypeBase: widen ignoreFields to int

Resolves CodeQL alerts #739-#743 (js/incomplete-sanitization),
#745-#748 (js/xss-through-dom) and #910
(java/implicit-cast-in-compound-assignment).
@vharseko vharseko added security Security fix / CVE remediation javascript Pull requests that update Javascript code java Pull requests that update Java code test Tests and test infrastructure (unit, e2e, smoke) bug Something isn't working labels Sep 18, 2026
- validNameFormat: drop the code points listed twice in the character class
- cannotContainDuplicates: declare the loop index
- policyProcessor: a missing comma ended the module's var list early, so
  validate, mergePolicies, getAdditionalPolicies, updateResourceConfig
  and processRequest leaked into the global scope
- drop the dead assignments of validate()'s (undefined) result

Resolves CodeQL alerts #768-#770, #782, #783, #895, #896.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working java Pull requests that update Java code javascript Pull requests that update Javascript code security Security fix / CVE remediation test Tests and test infrastructure (unit, e2e, smoke)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant