From 06e3a013145a315edc2cbd9e04f64c25b4e12f85 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 23 Apr 2025 14:16:12 -0400 Subject: [PATCH 1/3] Change definition of `getFactoryNodeInternal` Some variants of AMD module such as [sap.ui.define])(https://sdk.openui5.org/api/sap.ui#methods/sap.ui.define) can accept a boolean as its last parameter. Therefore, explicitly state the index of the factory method parameter as `1`. --- javascript/ql/lib/semmle/javascript/AMD.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/AMD.qll b/javascript/ql/lib/semmle/javascript/AMD.qll index 3239dba9026d..afb105b5d5d1 100644 --- a/javascript/ql/lib/semmle/javascript/AMD.qll +++ b/javascript/ql/lib/semmle/javascript/AMD.qll @@ -91,7 +91,7 @@ class AmdModuleDefinition extends CallExpr instanceof AmdModuleDefinition::Range Function getFactoryFunction() { TValueNode(result) = this.getFactoryNodeInternal() } private EarlyStageNode getFactoryNodeInternal() { - result = TValueNode(this.getLastArgument()) + result = TValueNode(this.getArgument(1)) or DataFlow::localFlowStep(result, this.getFactoryNodeInternal()) } From f0dfa51fb9e10fb61130fbc5e30718916c421826 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 23 Apr 2025 14:36:27 -0400 Subject: [PATCH 2/3] Dynamically determine the callback argument's position An additional parameter may be anywhere in the parameter list and shift around the exact index of the callback argument in the parameter list. So, "dynamically" determine the index by type-checking a parameter in the parameter list. Note 1: There may be multiple matches since we're using `_` (don't care) as the argument index. Note 2: We could have used DataFlow::InvokeNode.getCallback if the supertype were not CallExpr, but jumping to data flow node is an overkill here. --- javascript/ql/lib/semmle/javascript/AMD.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/AMD.qll b/javascript/ql/lib/semmle/javascript/AMD.qll index afb105b5d5d1..185fc62346c0 100644 --- a/javascript/ql/lib/semmle/javascript/AMD.qll +++ b/javascript/ql/lib/semmle/javascript/AMD.qll @@ -91,7 +91,9 @@ class AmdModuleDefinition extends CallExpr instanceof AmdModuleDefinition::Range Function getFactoryFunction() { TValueNode(result) = this.getFactoryNodeInternal() } private EarlyStageNode getFactoryNodeInternal() { - result = TValueNode(this.getArgument(1)) + exists(Function factoryFunction | factoryFunction = this.getArgument(_) | + result = TValueNode(factoryFunction) + ) or DataFlow::localFlowStep(result, this.getFactoryNodeInternal()) } From c327e6ff070e172b83c9692981962704cc470214 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 5 May 2025 11:19:11 -0400 Subject: [PATCH 3/3] Remove type enforcement on the TValueNode --- javascript/ql/lib/semmle/javascript/AMD.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/AMD.qll b/javascript/ql/lib/semmle/javascript/AMD.qll index 185fc62346c0..065b8125f636 100644 --- a/javascript/ql/lib/semmle/javascript/AMD.qll +++ b/javascript/ql/lib/semmle/javascript/AMD.qll @@ -91,9 +91,7 @@ class AmdModuleDefinition extends CallExpr instanceof AmdModuleDefinition::Range Function getFactoryFunction() { TValueNode(result) = this.getFactoryNodeInternal() } private EarlyStageNode getFactoryNodeInternal() { - exists(Function factoryFunction | factoryFunction = this.getArgument(_) | - result = TValueNode(factoryFunction) - ) + result = TValueNode(this.getArgument(_)) or DataFlow::localFlowStep(result, this.getFactoryNodeInternal()) }