From 48aefff9643f28b2e6c78097dd1b2c9fb8e28a62 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Jun 2026 00:44:59 +0100 Subject: [PATCH] Add SPURIOUS and MISSING to some comments --- .../tst-IncompleteHostnameRegExp.rb | 10 +++++----- .../tst-IncompleteUrlSubstringSanitization.rb | 16 ++++++++-------- .../cwe-116/IncompleteSanitization/tst.rb | 2 +- .../security/cwe-1333-exponential-redos/tst.rb | 14 +++++++------- .../UnsafeDeserialization.rb | 4 ++-- .../ConditionalBypass.rb | 2 +- 6 files changed, 24 insertions(+), 24 deletions(-) diff --git a/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.rb b/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.rb index 047e2cc6d69..50e2e257dce 100644 --- a/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.rb +++ b/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.rb @@ -27,7 +27,7 @@ def foo convert1({ hostname: 'test.example.com$' }); # $ Alert // NOT OK - domains = [ { hostname: 'test.example.com$' } ]; # NOT OK - but not flagged due to limitations of TypeTracking. + domains = [ { hostname: 'test.example.com$' } ]; # $ MISSING: Alert # NOT OK - but not flagged due to limitations of TypeTracking. @@ -39,7 +39,7 @@ def foo /^(http:\/\/sub.example.com\/)/i; # $ Alert // NOT OK /^https?:\/\/api.example.com/; # $ Alert // NOT OK Regexp.new('^http://localhost:8000|' + "^https?://.+\\.example\\.com/"); # $ Alert // NOT OK - Regexp.new("^http[s]?:\/\/?sub1\\.sub2\\.example\\.com\/f\/(.+)"); # NOT OK + Regexp.new("^http[s]?:\/\/?sub1\\.sub2\\.example\\.com\/f\/(.+)"); # $ MISSING: Alert # NOT OK /^https:\/\/[a-z]*.example.com$/; # $ Alert // NOT OK Regexp.compile('^protos?://(localhost|.+.example.net|.+.example-a.com|.+.example-b.com|.+.example.internal)'); # $ Alert // NOT OK @@ -48,11 +48,11 @@ def foo Regexp.new('^http://localhost:8000|' + "^https?://.+.example\\.com/"); # $ Alert // NOT OK primary = 'example.com$'; - Regexp.new('test.' + primary); # NOT OK, but not detected + Regexp.new('test.' + primary); # $ MISSING: Alert # NOT OK, but not detected - Regexp.new('test.' + 'example.com$'); # NOT OK + Regexp.new('test.' + 'example.com$'); # $ MISSING: Alert # NOT OK - Regexp.new('^http://test\.example.com'); # NOT OK + Regexp.new('^http://test\.example.com'); # $ MISSING: Alert # NOT OK /^http:\/\/(..|...)\.example\.com\/index\.html/; # OK, wildcards are intentional /^http:\/\/.\.example\.com\/index\.html/; # OK, the wildcard is intentional diff --git a/ruby/ql/test/query-tests/security/cwe-020/IncompleteUrlSubstringSanitization/tst-IncompleteUrlSubstringSanitization.rb b/ruby/ql/test/query-tests/security/cwe-020/IncompleteUrlSubstringSanitization/tst-IncompleteUrlSubstringSanitization.rb index c7b4a55642b..76a0a9ccdca 100644 --- a/ruby/ql/test/query-tests/security/cwe-020/IncompleteUrlSubstringSanitization/tst-IncompleteUrlSubstringSanitization.rb +++ b/ruby/ql/test/query-tests/security/cwe-020/IncompleteUrlSubstringSanitization/tst-IncompleteUrlSubstringSanitization.rb @@ -1,11 +1,11 @@ def test (x) - x.index("internal") != nil; # NOT OK, but not flagged - x.index("localhost") != nil; # NOT OK, but not flagged + x.index("internal") != nil; # $ MISSING: Alert # NOT OK, but not flagged + x.index("localhost") != nil; # $ MISSING: Alert # NOT OK, but not flagged x.index("secure.com") != nil; # $ Alert // NOT OK x.index("secure.net") != nil; # $ Alert // NOT OK x.index(".secure.com") != nil; # $ Alert // NOT OK - x.index("sub.secure.") != nil; # NOT OK, but not flagged - x.index(".sub.secure.") != nil; # NOT OK, but not flagged + x.index("sub.secure.") != nil; # $ MISSING: Alert # NOT OK, but not flagged + x.index(".sub.secure.") != nil; # $ MISSING: Alert # NOT OK, but not flagged x.index("secure.com") === nil; # $ Alert // NOT OK x.index("secure.com") === 0; # $ Alert // NOT OK @@ -33,7 +33,7 @@ def test (x) x.index("https://secure.com:443") != nil; # $ Alert // NOT OK x.index("https://secure.com/") != nil; # $ Alert // NOT OK - x.index(".cn") != nil; # NOT OK, but not flagged + x.index(".cn") != nil; # $ MISSING: Alert # NOT OK, but not flagged x.index(".jpg") != nil; # OK x.index("index.html") != nil; # OK x.index("index.js") != nil; # OK @@ -43,8 +43,8 @@ def test (x) x.index("secure=true") != nil; # OK (query param) x.index("&auth=") != nil; # OK (query param) - x.index(getCurrentDomain()) != nil; # NOT OK, but not flagged - x.index(location.origin) != nil; # NOT OK, but not flagged + x.index(getCurrentDomain()) != nil; # $ MISSING: Alert # NOT OK, but not flagged + x.index(location.origin) != nil; # $ MISSING: Alert # NOT OK, but not flagged x.index("tar.gz") + offset; # OK x.index("tar.gz") - offset; # OK @@ -68,7 +68,7 @@ def test (x) else doSomeThingWithTrustedURL(x); end - + x.start_with?("https://secure.com/foo/bar"); # OK - a forward slash after the domain makes prefix checks safe. x.index("https://secure.com/foo/bar") >= 0 # $ Alert // NOT OK - the url can be anywhere in the string. x.index("https://secure.com") >= 0 # $ Alert // NOT OK diff --git a/ruby/ql/test/query-tests/security/cwe-116/IncompleteSanitization/tst.rb b/ruby/ql/test/query-tests/security/cwe-116/IncompleteSanitization/tst.rb index 09af97e96b8..0fddda9a6d5 100644 --- a/ruby/ql/test/query-tests/security/cwe-116/IncompleteSanitization/tst.rb +++ b/ruby/ql/test/query-tests/security/cwe-116/IncompleteSanitization/tst.rb @@ -220,7 +220,7 @@ def good13a(s) s = s.sub('[', '') # OK s = s.sub(']', '') # OK s.sub(/{/, '').sub(/}/, '') # OK - s.sub(']', '').sub('[', '') # $ Alert // probably OK, but still flagged + s.sub(']', '').sub('[', '') # $ SPURIOUS: Alert // probably OK, but still flagged end def good13b(s1) diff --git a/ruby/ql/test/query-tests/security/cwe-1333-exponential-redos/tst.rb b/ruby/ql/test/query-tests/security/cwe-1333-exponential-redos/tst.rb index 8f45aff3c45..0cac356ea20 100644 --- a/ruby/ql/test/query-tests/security/cwe-1333-exponential-redos/tst.rb +++ b/ruby/ql/test/query-tests/security/cwe-1333-exponential-redos/tst.rb @@ -304,10 +304,10 @@ bad66 = /^ab(c+)+$/ # $ Alert # NOT GOOD bad67 = /(\d(\s+)*){20}/ # $ Alert -# GOOD - but we spuriously conclude that a rejecting suffix exists. +# GOOD - but we spuriously conclude that a rejecting suffix exists. good36 = /(([^\/]|X)+)(\/[\S\s]*)*$/ # $ Alert -# GOOD - but we spuriously conclude that a rejecting suffix exists. +# GOOD - but we spuriously conclude that a rejecting suffix exists. good37 = /^((x([^Y]+)?)*(Y|$))/ # $ Alert # NOT GOOD @@ -326,18 +326,18 @@ bad71 = /(a?a?)*b/ # $ Alert good38 = /(a?)*b/ # NOT GOOD - but not detected -bad72 = /(c?a?)*b/ +bad72 = /(c?a?)*b/ # $ MISSING: Alert # NOT GOOD bad73 = /(?:a|a?)+b/ # $ Alert -# NOT GOOD - but not detected. -bad74 = /(a?b?)*$/ +# NOT GOOD - but not detected. +bad74 = /(a?b?)*$/ # $ MISSING: Alert # NOT GOOD bad76 = /PRE(([a-c]|[c-d])T(e?e?e?e?|X))+(cTcT|cTXcTX$)/ # $ Alert -# NOT GOOD - but not detected +# NOT GOOD bad77 = /^((a)+\w)+$/ # $ Alert # NOT GOOD @@ -362,7 +362,7 @@ bad84 = /^((?:a{0|-)|\w\{\d)+X$/ # $ Alert bad85 = /^((?:a{0,|-)|\w\{\d,)+X$/ # $ Alert bad86 = /^((?:a{0,2|-)|\w\{\d,\d)+X$/ # $ Alert -# NOT GOOD +# NOT GOOD bad87 = /^((?:a{0,2}|-)|\w\{\d,\d\})+X$/ # NOT GOOD diff --git a/ruby/ql/test/query-tests/security/cwe-502/unsafe-deserialization/UnsafeDeserialization.rb b/ruby/ql/test/query-tests/security/cwe-502/unsafe-deserialization/UnsafeDeserialization.rb index 8afc514ce6c..379d6a5819b 100644 --- a/ruby/ql/test/query-tests/security/cwe-502/unsafe-deserialization/UnsafeDeserialization.rb +++ b/ruby/ql/test/query-tests/security/cwe-502/unsafe-deserialization/UnsafeDeserialization.rb @@ -67,7 +67,7 @@ class UsersController < ActionController::Base # TODO: false positive; we aren't detecting flow from `:json` to the call argument. more_options = { allow_blank: true } more_options[:mode] = :json - object4 = Oj.load json_data, more_options # $ Alert + object4 = Oj.load json_data, more_options # $ SPURIOUS: Alert end # GOOD @@ -127,7 +127,7 @@ class UsersController < ActionController::Base # GOOD def route17 yaml_data = params[:key] - object = Psych.parse_stream(yaml_data) + object = Psych.parse_stream(yaml_data) object = Psych.parse(yaml_data) object = Psych.parse_file(yaml_data) end diff --git a/ruby/ql/test/query-tests/security/cwe-807-user-controlled-bypass/ConditionalBypass.rb b/ruby/ql/test/query-tests/security/cwe-807-user-controlled-bypass/ConditionalBypass.rb index 1a6dd87ab79..b6e2b6a50ab 100644 --- a/ruby/ql/test/query-tests/security/cwe-807-user-controlled-bypass/ConditionalBypass.rb +++ b/ruby/ql/test/query-tests/security/cwe-807-user-controlled-bypass/ConditionalBypass.rb @@ -18,7 +18,7 @@ class FooController < ActionController::Base def bad_handler3 # BAD. Not detected: its the last statement in the method, so it doesn't # match the heuristic for an action. - login if params[:login] + login if params[:login] # $ MISSING: Alert end def bad_handler4