Conversation
- router-authz.js: the list helpers iterated one past the end, so contains(list, undefined) and containsIgnoreCase([], undefined) were true - correlateTreeToQueryFilter.js: compare typeof against the string "undefined", not the value - effectiveAssignments.js: apply the effectiveRolesPropName default before it is used to read the object - Declare the locals that were leaking into the global scope (a typo in autoPurgeAuditRecon.js, a ';' that ended a var list early in temporalConstraints.js, missing var elsewhere) and stop re-declaring the mapping-config binding in defaultMapping.js - Remove the unreachable return in info/login.js Resolves CodeQL alerts #749, #759, #760, #771-#781, #784-#787, #907.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third warning-level CodeQL batch: the JavaScript findings in the bundled server-side scripts (
openidm-zip). 19 alerts fixed, 10 dismissed as false positives. The 7 alerts inpolicy.js(#768–#770, #782, #783, #895, #896) are deferred until #204, which rewrites part of that file, has landed.Three real bugs (bug below); the rest are scoping fixes.
index-out-of-bounds— bugrouter-authz.jscontains/containsIgnoreCaseloopedi <= a.length, reading the slot past the end.contains(list, undefined)andcontainsIgnoreCase([], undefined)therefore returnedtrue. Now<. NewrouterAuthzTest.jsevaluates the real script with stubbed host globals (the function declarations are hoisted, so they stay reachable after the trailing access check throws) — 8 cases, 2 failed before the fix.comparison-between-incompatible-types— bugui/correlateTreeToQueryFilter.jstypeof linkQualifier !== undefinedis always true; now compares against"undefined". No behavioural change in real invocations: the sync engine (Correlation.java) and the admin UI always bindlinkQualifier.use-before-declaration— bugroles/effectiveAssignments.jseffectiveRolesPropName = "effectiveRoles"default was applied afterobject[effectiveRolesPropName]had already been read, so without an explicitglobalsentry inmanaged.jsonthe script readobject[undefined]. The default now precedes the first use.missing-variable-declarationaudit/autoPurgeAuditRecon.jsexcludeMappings, usedexcludeMapping— typo.missing-variable-declarationroles/temporalConstraints.jsvar constraintExpired = false;— the;ended the declaration list, sodateUtilbecame a global.missing-variable-declarationpolicyFilter.js,defaultMapping.js,effectiveRoles.js,postOperation-roles.js×4,relationshipHelper.js,samples/multiplepasswords/script/pwpolicy.jsvar.use-before-declarationroles/defaultMapping.jsvar config = getConfig(…)were hoisted to script scope and overwrote theconfigbinding (the mapping configuration) after it had been read; renamed tounassignmentConfig/assignmentConfig.use-before-declarationsamples/usecase/script/roles/effectiveRoles.jsvar rolesPropName = rolesPropName === undefined ? "roles" : rolesPropName;.unreachable-statementinfo/login.jsreturn valafter theif/elsethat always returns or throws.Dismissed as false positives: #899–#906
useless-expression— the trailing bare expression is the Rhino idiom for a script's return value (a top-levelreturnis a syntax error in a script); #893/#894useless-assignment-to-local—sourceis read by the transform script evaluated througheval(p.transform.source)two lines later.Test plan
routerAuthzTest.js(new):contains(["a","b"], undefined)andcontainsIgnoreCase([], undefined)returnedtruebefore,falsenow; positive/negative cases passmvn -pl openidm-zip -am package—ScriptRunnerTestgreen over all 9 JS test modules, includingeffectiveRolesTest,temporalConstraintsTestandconditionalRolesTestwhich exercise the edited role scripts