From 986e1cb597ccce863fa238d637623f3dd854b2c9 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 29 Nov 2024 14:33:40 +0100 Subject: [PATCH 01/10] Add ValidatePredicateGetReturns query and tests --- .../style/ValidatePredicateGetReturns.ql | 40 +++++++++++++++++++ .../ValidatePredicateGetReturns.expected | 2 + .../ValidatePredicateGetReturns.qlref | 1 + .../ValidatePredicateGetReturns/test.qll | 28 +++++++++++++ 4 files changed, 71 insertions(+) create mode 100644 ql/ql/src/queries/style/ValidatePredicateGetReturns.ql create mode 100644 ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected create mode 100644 ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref create mode 100644 ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql new file mode 100644 index 000000000000..2a3de1cfb3b5 --- /dev/null +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -0,0 +1,40 @@ +/** + * @name Predicates starting with "get" should return a value + * @description Checks if predicates that start with "get" actually return a value. + * @kind problem + * @problem.severity warning + * @id ql/predicates-get-should-return-value + * @tags correctness + * maintainability + * @precision high + */ + +import ql +import codeql_ql.ast.Ast + +/** + * Identifies predicates whose names start with "get" followed by an uppercase letter. + * This ensures that only predicates like "getValue" are matched, excluding names like "getter". + */ +predicate isGetPredicate(Predicate pred) { pred.getName().regexpMatch("get[A-Z].*") } + +/** + * Checks if a predicate has a return type. + */ +predicate hasReturnType(Predicate pred) { + exists(Type returnType | pred.getReturnType() = returnType) +} + +/** + * Checks if a predicate is an alias using getAlias(). + */ +predicate isAlias(Predicate pred) { + pred instanceof ClasslessPredicate and exists(pred.(ClasslessPredicate).getAlias()) +} + +from Predicate pred +where + isGetPredicate(pred) and + not hasReturnType(pred) and + not isAlias(pred) +select pred, "This predicate starts with 'get' but does not return a value." diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected new file mode 100644 index 000000000000..b78fe32cc75b --- /dev/null +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected @@ -0,0 +1,2 @@ +| test.qll:4:11:4:18 | ClasslessPredicate getValue | This predicate starts with 'get' but does not return a value. | +| test.qll:25:11:25:28 | ClasslessPredicate getImplementation2 | This predicate starts with 'get' but does not return a value. | diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref new file mode 100644 index 000000000000..e116f69d6b22 --- /dev/null +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref @@ -0,0 +1 @@ +queries/style/ValidatePredicateGetReturns.ql diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll new file mode 100644 index 000000000000..b62b0e2dbaf3 --- /dev/null +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll @@ -0,0 +1,28 @@ +import ql + +// NOT OK -- Predicate starts with "get" but does not return a value +predicate getValue() { none() } + +// OK -- starts with get and returns a value +string getData() { result = "data" } + +// OK -- starts with get but followed by a lowercase letter, probably should be ignored +predicate getterFunction() { none() } + +// OK -- starts with get and returns a value +string getImplementation() { result = "implementation" } + +// OK -- is an alias +predicate getAlias = getImplementation/0; + +// OK -- Starts with "get" but followed by a lowercase letter, probably be ignored +predicate getvalue() { none() } + +// OK -- Does not start with "get", should be ignored +predicate retrieveValue() { none() } + +// NOT OK -- starts with get and does not return value +predicate getImplementation2() { none() } + +// OK -- is an alias +predicate getAlias2 = getImplementation2/0; From a763dd7267ea1f88ddf904578b656a12e6d12ee1 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 29 Nov 2024 14:55:53 +0100 Subject: [PATCH 02/10] Fixed github-advanced-security bot warning --- ql/ql/src/queries/style/ValidatePredicateGetReturns.ql | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql index 2a3de1cfb3b5..d6982c757106 100644 --- a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -21,9 +21,7 @@ predicate isGetPredicate(Predicate pred) { pred.getName().regexpMatch("get[A-Z]. /** * Checks if a predicate has a return type. */ -predicate hasReturnType(Predicate pred) { - exists(Type returnType | pred.getReturnType() = returnType) -} +predicate hasReturnType(Predicate pred) { exists(pred.getReturnType()) } /** * Checks if a predicate is an alias using getAlias(). From a5521b90fceecad9e7b38e4af48955b3e45b89c9 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Fri, 29 Nov 2024 15:13:46 +0100 Subject: [PATCH 03/10] Update ql/ql/src/queries/style/ValidatePredicateGetReturns.ql Co-authored-by: Anders Schack-Mulligen --- ql/ql/src/queries/style/ValidatePredicateGetReturns.ql | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql index d6982c757106..297d141754e7 100644 --- a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -26,9 +26,7 @@ predicate hasReturnType(Predicate pred) { exists(pred.getReturnType()) } /** * Checks if a predicate is an alias using getAlias(). */ -predicate isAlias(Predicate pred) { - pred instanceof ClasslessPredicate and exists(pred.(ClasslessPredicate).getAlias()) -} +predicate isAlias(Predicate pred) { exists(pred.(ClasslessPredicate).getAlias()) } from Predicate pred where From 029b567bb722f2416676e8f551e6ef63418ed07d Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Fri, 29 Nov 2024 15:19:19 +0100 Subject: [PATCH 04/10] Update ql/ql/src/queries/style/ValidatePredicateGetReturns.ql Co-authored-by: Anders Schack-Mulligen --- ql/ql/src/queries/style/ValidatePredicateGetReturns.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql index 297d141754e7..eae483d287f4 100644 --- a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -16,7 +16,7 @@ import codeql_ql.ast.Ast * Identifies predicates whose names start with "get" followed by an uppercase letter. * This ensures that only predicates like "getValue" are matched, excluding names like "getter". */ -predicate isGetPredicate(Predicate pred) { pred.getName().regexpMatch("get[A-Z].*") } +predicate isGetPredicate(Predicate pred) { pred.getName().regexpMatch("(get|as)[A-Z].*") } /** * Checks if a predicate has a return type. From e33f7aa1c7f3180670cf580fc9473358e9ae3cf2 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 29 Nov 2024 15:23:06 +0100 Subject: [PATCH 05/10] Added test cases for 'as' prefix --- .../ValidatePredicateGetReturns.expected | 1 + .../queries/style/ValidatePredicateGetReturns/test.qll | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected index b78fe32cc75b..a79e572ed1cd 100644 --- a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected @@ -1,2 +1,3 @@ | test.qll:4:11:4:18 | ClasslessPredicate getValue | This predicate starts with 'get' but does not return a value. | | test.qll:25:11:25:28 | ClasslessPredicate getImplementation2 | This predicate starts with 'get' but does not return a value. | +| test.qll:31:11:31:17 | ClasslessPredicate asValue | This predicate starts with 'get' but does not return a value. | diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll index b62b0e2dbaf3..b18649ea8cc7 100644 --- a/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll @@ -26,3 +26,12 @@ predicate getImplementation2() { none() } // OK -- is an alias predicate getAlias2 = getImplementation2/0; + +// NOT OK -- starts with as and does not return value +predicate asValue() { none() } + +// OK -- starts with as but followed by a lowercase letter, probably should be ignored +predicate assessment() { none() } + +// OK -- starts with as and returns a value +string asString() { result = "string" } From 96c1086dfc674ae6e5ddece815d0991e8ae9f309 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 29 Nov 2024 15:35:18 +0100 Subject: [PATCH 06/10] Modified comments to reflect 'as' changes --- ql/ql/src/queries/style/ValidatePredicateGetReturns.ql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql index eae483d287f4..065e24f9b2e4 100644 --- a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -1,6 +1,6 @@ /** - * @name Predicates starting with "get" should return a value - * @description Checks if predicates that start with "get" actually return a value. + * @name Predicates starting with "get" or "as" should return a value + * @description Checks if predicates that start with "get" or "as" actually return a value. * @kind problem * @problem.severity warning * @id ql/predicates-get-should-return-value @@ -13,7 +13,7 @@ import ql import codeql_ql.ast.Ast /** - * Identifies predicates whose names start with "get" followed by an uppercase letter. + * Identifies predicates whose names start with "get", "as" followed by an uppercase letter. * This ensures that only predicates like "getValue" are matched, excluding names like "getter". */ predicate isGetPredicate(Predicate pred) { pred.getName().regexpMatch("(get|as)[A-Z].*") } From a462ec91f5d6e8e881f370a6823202f8c6f1c400 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 29 Nov 2024 15:54:58 +0100 Subject: [PATCH 07/10] Now the error message reflects properly the prefix --- .../queries/style/ValidatePredicateGetReturns.ql | 14 +++++++++++++- .../ValidatePredicateGetReturns.expected | 2 +- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql index 065e24f9b2e4..ad7db6e05907 100644 --- a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -28,9 +28,21 @@ predicate hasReturnType(Predicate pred) { exists(pred.getReturnType()) } */ predicate isAlias(Predicate pred) { exists(pred.(ClasslessPredicate).getAlias()) } +/** + * Returns "get" if the predicate name starts with "get", otherwise "as". + */ +string getPrefix(Predicate pred) { + if pred.getName().matches("get%") + then result = "get" + else + if pred.getName().matches("as%") + then result = "as" + else result = "" +} + from Predicate pred where isGetPredicate(pred) and not hasReturnType(pred) and not isAlias(pred) -select pred, "This predicate starts with 'get' but does not return a value." +select pred, "This predicate starts with '" + getPrefix(pred) + "' but does not return a value." diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected index a79e572ed1cd..57469246ae82 100644 --- a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected @@ -1,3 +1,3 @@ | test.qll:4:11:4:18 | ClasslessPredicate getValue | This predicate starts with 'get' but does not return a value. | | test.qll:25:11:25:28 | ClasslessPredicate getImplementation2 | This predicate starts with 'get' but does not return a value. | -| test.qll:31:11:31:17 | ClasslessPredicate asValue | This predicate starts with 'get' but does not return a value. | +| test.qll:31:11:31:17 | ClasslessPredicate asValue | This predicate starts with 'as' but does not return a value. | From 7c1aa844594211ae00c934feffb1f77883cd5d40 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 29 Nov 2024 17:24:58 +0100 Subject: [PATCH 08/10] Fixed bug where some predicates were flagged without return type even thought they had --- ql/ql/src/queries/style/ValidatePredicateGetReturns.ql | 2 +- .../test/queries/style/ValidatePredicateGetReturns/test.qll | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql index ad7db6e05907..f65b883a09b0 100644 --- a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -21,7 +21,7 @@ predicate isGetPredicate(Predicate pred) { pred.getName().regexpMatch("(get|as)[ /** * Checks if a predicate has a return type. */ -predicate hasReturnType(Predicate pred) { exists(pred.getReturnType()) } +predicate hasReturnType(Predicate pred) { exists(pred.getReturnTypeExpr()) } /** * Checks if a predicate is an alias using getAlias(). diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll index b18649ea8cc7..b68aab08fd78 100644 --- a/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll @@ -35,3 +35,8 @@ predicate assessment() { none() } // OK -- starts with as and returns a value string asString() { result = "string" } + +// OK -- starts with get and returns a value +HiddenType getInjectableCompositeActionNode() { + exists(HiddenType hidden | result = hidden.toString()) +} From 67745e6332564b343537ab63e08330a056eeed4f Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 2 Dec 2024 09:10:54 +0100 Subject: [PATCH 09/10] Reused isGetPredicate to retrieve the prefix of the predicate --- .../style/ValidatePredicateGetReturns.ql | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql index f65b883a09b0..4d0196509c33 100644 --- a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -16,7 +16,9 @@ import codeql_ql.ast.Ast * Identifies predicates whose names start with "get", "as" followed by an uppercase letter. * This ensures that only predicates like "getValue" are matched, excluding names like "getter". */ -predicate isGetPredicate(Predicate pred) { pred.getName().regexpMatch("(get|as)[A-Z].*") } +predicate isGetPredicate(Predicate pred, string prefix) { + prefix = pred.getName().regexpCapture("(get|as)[A-Z].*", 1) +} /** * Checks if a predicate has a return type. @@ -28,21 +30,9 @@ predicate hasReturnType(Predicate pred) { exists(pred.getReturnTypeExpr()) } */ predicate isAlias(Predicate pred) { exists(pred.(ClasslessPredicate).getAlias()) } -/** - * Returns "get" if the predicate name starts with "get", otherwise "as". - */ -string getPrefix(Predicate pred) { - if pred.getName().matches("get%") - then result = "get" - else - if pred.getName().matches("as%") - then result = "as" - else result = "" -} - -from Predicate pred +from Predicate pred, string prefix where - isGetPredicate(pred) and + isGetPredicate(pred, prefix) and not hasReturnType(pred) and not isAlias(pred) -select pred, "This predicate starts with '" + getPrefix(pred) + "' but does not return a value." +select pred, "This predicate starts with '" + prefix + "' but does not return a value." From 7db9b7d758859042797625363fc6c0a2fef9b2c1 Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 2 Dec 2024 12:43:52 +0100 Subject: [PATCH 10/10] Now flag aliases with the 'get' or 'as' prefix that resolve to predicates lacking a return type. Co-authored-by: asgerf --- .../style/ValidatePredicateGetReturns.ql | 17 ++++++------ .../ValidatePredicateGetReturns.expected | 3 +++ .../ValidatePredicateGetReturns/test.qll | 27 ++++++++++++++++++- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql index 4d0196509c33..a79e4f69569e 100644 --- a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql +++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql @@ -21,18 +21,17 @@ predicate isGetPredicate(Predicate pred, string prefix) { } /** - * Checks if a predicate has a return type. + * Checks if a predicate has a return type. This is phrased negatively to not flag unresolved aliases. */ -predicate hasReturnType(Predicate pred) { exists(pred.getReturnTypeExpr()) } - -/** - * Checks if a predicate is an alias using getAlias(). - */ -predicate isAlias(Predicate pred) { exists(pred.(ClasslessPredicate).getAlias()) } +predicate hasNoReturnType(Predicate pred) { + not exists(pred.getReturnTypeExpr()) and + not pred.(ClasslessPredicate).getAlias() instanceof PredicateExpr + or + hasNoReturnType(pred.(ClasslessPredicate).getAlias().(PredicateExpr).getResolvedPredicate()) +} from Predicate pred, string prefix where isGetPredicate(pred, prefix) and - not hasReturnType(pred) and - not isAlias(pred) + hasNoReturnType(pred) select pred, "This predicate starts with '" + prefix + "' but does not return a value." diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected index 57469246ae82..4b36d2c5c328 100644 --- a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected @@ -1,3 +1,6 @@ | test.qll:4:11:4:18 | ClasslessPredicate getValue | This predicate starts with 'get' but does not return a value. | | test.qll:25:11:25:28 | ClasslessPredicate getImplementation2 | This predicate starts with 'get' but does not return a value. | +| test.qll:28:11:28:19 | ClasslessPredicate getAlias2 | This predicate starts with 'get' but does not return a value. | | test.qll:31:11:31:17 | ClasslessPredicate asValue | This predicate starts with 'as' but does not return a value. | +| test.qll:48:11:48:19 | ClasslessPredicate getAlias4 | This predicate starts with 'get' but does not return a value. | +| test.qll:61:11:61:22 | ClasslessPredicate getDistance2 | This predicate starts with 'get' but does not return a value. | diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll index b68aab08fd78..2cc4dec64d20 100644 --- a/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll +++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll @@ -24,7 +24,7 @@ predicate retrieveValue() { none() } // NOT OK -- starts with get and does not return value predicate getImplementation2() { none() } -// OK -- is an alias +// NOT OK -- is an alias for a predicate which does not have a return value predicate getAlias2 = getImplementation2/0; // NOT OK -- starts with as and does not return value @@ -40,3 +40,28 @@ string asString() { result = "string" } HiddenType getInjectableCompositeActionNode() { exists(HiddenType hidden | result = hidden.toString()) } + +// OK +predicate implementation4() { none() } + +// NOT OK -- is an alias +predicate getAlias4 = implementation4/0; + +// OK -- is an alias +predicate alias5 = implementation4/0; + +int root() { none() } + +predicate edge(int x, int y) { none() } + +// OK -- Higher-order predicate +int getDistance(int x) = shortestDistances(root/0, edge/2)(_, x, result) + +// NOT OK -- Higher-order predicate that does not return a value even though has 'get' in the name +predicate getDistance2(int x, int y) = shortestDistances(root/0, edge/2)(_, x, y) + +// OK +predicate unresolvedAlias = unresolved/0; + +// OK -- unresolved alias +predicate getUnresolvedAlias = unresolved/0;