Skip to content

Remove unused locals and fix ASI issues flagged by CodeQL in the JavaScript - #214

Open
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-notes-js
Open

vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-notes-js

Conversation

@vharseko

@vharseko vharseko commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

Summary

Fourth note-level CodeQL batch: the JavaScript findings — js/unused-local-variable (93), js/automatic-semicolon-insertion (7), js/unneeded-defensive-code (1). 87 fixed across 42 files, 14 dismissed as false positives.

This PR deliberately also touches files that #204 and #209 change (policy.js, router-authz.js, autoPurgeAuditRecon.js, postOperation-roles.js, temporalConstraints.js, defaultMapping.js, UserQueryFilterEditor.js); the overlaps are neighbouring line removals and will be resolved at merge time, whichever lands second. The overlap with #210 (FormGenerationUtils.js, RelationshipWidget.js) is already resolved: the branch is rebased onto master with #210 in it.

Rule Count Change
js/unused-local-variable 79 Declarations that are never read removed: bare var x, = [] / = {} / = this, side-effect-free DOM lookups, $.Deferred(), AbstractModel.extend(...), an unused require('roles/effectiveRoles'); two dead helpers (getUserById in getavailableuserstoassign.js, join in gettasksview.js); the unused allUsedClasses / usableForQueriesClasses block in AuditEventHandlersView. Where a var list was shortened, the following object/array literal was re-indented (eslint indent). The excludeMappings → excludeMapping typo in autoPurgeAuditRecon.js is the same fix as in #209.
js/automatic-semicolon-insertion 7 Missing ; added in reconResults.js, resetPassword.js, router-authz.js, policy.js ×2, autoPurgeAuditRecon.js, defaultMapping.js.
js/unneeded-defensive-code 1 policy.js: resource is never null at this point — processRequest replaces an unmatched getResource() result with an empty entry before the action branch — so the if (resource === null) … else … around the validation was dead; the body is unwrapped. Small correctness gain: an undefined resourcePath now fails with "No resource specified" like a null one instead of a TypeError.

Dismissed as false positives (14): the 13 router-authz.js "unused function" alerts (ownDataOnly, isOneOfMyWorkflows, reauthIfProtectedAttributeChange, …) — they are referenced by name from the customAuthz expressions in conf/script/access.js, which passesAccessConfig() evaluates with eval(); and policy.js addPolicy, the documented API that custom policy scripts call, again through eval() inside additionalPolicyLoader.load().

Not addressed here: CodeQL #930 (js/useless-assignment-to-local, policyRequirements in policy.js) is the existing #895 re-reported under a new number because unwrapping the if re-indented that line. It stays with #895/#896, which #209 defers until #204 lands.

Test plan

  • Every edited .js parse-checked with Node and linted with the module's eslint config
  • Grunt builds of openidm-ui-common, openidm-ui-admin, openidm-ui-enduser: eslint clean, QUnit green, BUILD SUCCESS
  • mvn -pl openidm-zip -am package — ScriptRunnerTest green over all JS test modules (incl. policyFilterTest, effectiveRolesTest, temporalConstraintsTest, conditionalRolesTest)
  • CodeQL on this PR closes #761–#767, #788–#820, #822–#846, #849–#854, #860, #864, #867, #868, #872–#879, #908, #922, #923, #929

@vharseko vharseko added javascript Pull requests that update Javascript code refactor Code refactoring without behavior change workflow Activiti workflow engine / scripting labels Sep 18, 2026
Comment on lines +894 to +895
policyRequirements = validate(policies, conditionalPolicies, fallbackPolicies, fullObject,
propName, getPropertyValue(fullObject, propName), failedPolicyRequirements);

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The clean-up stays behaviour-neutral, and its one behavioural edit closes a gap.

  • policy.js:882 now guards request.resourcePath with the same === null || === undefined test as the lookup at policy.js:854, so no action reaches resource.properties with resource unset.
  • The locals removed from router-authz.js (returnVal, requestedRoles, params) are all function-local, so the scope the eval'd customAuthz expressions see is unchanged.

issue (non-blocking): The new comment says getResource() never yields null, but it does.

openidm-zip/src/main/resources/bin/defaults/script/policy.js:881, :518, :857-861

getResource() falls through to return null; at :518 whenever no configured resource matches, e.g. a validateObject action on a path absent from policy.json. The empty entry comes from processRequest's own fallback at :857-861, and that fallback is what makes the removed resource === null branch dead. Behaviour at the head is correct, but the comment points the next maintainer at the wrong function: dropping :857-861 on its word turns that request's result: true into a TypeError on resource.properties. The js/unneeded-defensive-code row of the PR description repeats the framing.

            // resource is never null here: an unconfigured resource was replaced by an empty entry above
            if (request.resourcePath === null || request.resourcePath === undefined) {
                throw "No resource specified";
            }

…Script

- Drop locals that are declared but never read across the admin/common/
  end-user UI and the bundled scripts, including two dead helper functions
- Add the missing semicolons where automatic semicolon insertion was
  relied on
- policy.js: remove the unreachable "resource === null" branch and treat
  an undefined resourcePath like a null one

Resolves CodeQL alerts #761-#767, #788-#854, #860, #864, #867, #868,
#872-#879, #908, #922, #923.
getResource() does return null for an unconfigured path; it is the
empty-entry fallback in processRequest that makes the removed
resource === null branch dead.
@vharseko
vharseko force-pushed the fix-codeql-notes-js branch from ff5912b to cc87532 Compare October 3, 2026 06:16
@vharseko

vharseko commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Fixed. The comment now says the guarantee comes from the fallback above, not from getResource(): getResource() does return null at :518, and it is the empty entry built at :857-861 that makes the removed branch dead. The js/unneeded-defensive-code row of the description is reworded the same way.

@vharseko
vharseko requested a review from maximthomas October 3, 2026 06:16
@vharseko vharseko added ui Admin and end-user web UI (openidm-ui-*) packaging What the distribution ships: zip layout, default conf, samples bug Something isn't working labels Oct 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working javascript Pull requests that update Javascript code packaging What the distribution ships: zip layout, default conf, samples refactor Code refactoring without behavior change ui Admin and end-user web UI (openidm-ui-*) workflow Activiti workflow engine / scripting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants