Skip to content

Python: Handle guards being compared to boolean literals #21296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yoff wants to merge 4 commits into
base: main
Choose a base branch
from
+85 −1
Open

Python: Handle guards being compared to boolean literals #21296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2bab38
python: add tests for guards compared to booleans
yoff Feb 8, 2026
4fd3fc6
python: handle guards compared to boolean literals
yoff Feb 8, 2026
9c2ce6a
python: add change note
yoff Feb 8, 2026
1132e78
Apply suggestions from code review
yoff Feb 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions python/ql/lib/change-notes/2026-02-08-guards-compared-to-boolean-literals.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* When a guard such as `isSafe(x)` is defined, we now also automatically handle comparisons to boolean literals such as `isSafe(x) is True`, `isSafe(x) == True`, `isSafe(x) is not False`, and `isSafe(x) != False`.
30 changes: 29 additions & 1 deletion python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,14 +553,42 @@ ControlFlowNode guardNode(ConditionBlock conditionBlock, boolean flipped) {
result = conditionBlock.getLastNode() and
flipped = false
or
// Recursive case: if a guard node is a `not`-expression,
// Recursive cases:
// if a guard node is a `not`-expression,
// the operand is also a guard node, but with inverted polarity.
exists(UnaryExprNode notNode |
result = notNode.getOperand() and
notNode.getNode().getOp() instanceof Not
|
notNode = guardNode(conditionBlock, flipped.booleanNot())
)
or
// if a guard node is compared to a boolean literal,
// the other operand is also a guard node,
// but with polarity depending on the literal (and on the comparison).
exists(CompareNode cmpNode, Cmpop op, ControlFlowNode b, boolean bool |
(
cmpNode.operands(result, op, b) or
cmpNode.operands(b, op, result)
) and
not result.getNode() instanceof BooleanLiteral and
(
// comparing to the boolean
(op instanceof Eq or op instanceof Is) and
// `bool` is the value being compared against, here the value of `b`
b.getNode().(BooleanLiteral).booleanValue() = bool
or
// comparing to the negation of the boolean
(op instanceof NotEq or op instanceof IsNot) and
// again, `bool` is the value being compared against, but here it is the value of `not b`
b.getNode().(BooleanLiteral).booleanValue() = bool.booleanNot()
)
|
// if `bool` is true, we should preserve `flipped`, otherwise we should flip it
// `flipped xor (not bool)` achieves that.
flipped in [true, false] and
cmpNode = guardNode(conditionBlock, flipped.booleanXor(bool.booleanNot()))
Comment on lines +575 to +590
Copy link
Contributor

@tausbn tausbn Feb 13, 2026

Choose a reason for hiding this comment

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

It bugs me a bit that the first disjunct is all "bool is the boolean value being compared against!", and the second one is "bool is the boolean value being compared against... except not!".

I wonder if it would be more clear if bool was renamed to should_flip, with the booleanNot application being swapped between the two branches i.e.

(op instanceof Eq or ...) and
should_flip = b.getNode().(BooleanLiteral).booleanValue().booleanNot()
or
(op instanceof NotEq or ...) and
should_flip = b.getNode().(BooleanLiteral).booleanValue()

And then we just straight up do flipped.booleanXor(should_flip) at the end. (This is also marginally more efficient, as it avoids a double negation, which I'm pretty sure the compiler doesn't simplify.)

Alternatively we could use three booleans: flipped (as we do now), bool for the boolean value being compared with (which can then be factored out), and a third one for whether to flip or not. The final calculation then becomes a three-way xor between these values. This would be less efficent, though, and I'm not sure it would improve readability much.

)
}

/**
Expand Down
8 changes: 8 additions & 0 deletions python/ql/test/library-tests/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,12 @@ isSanitizer
| test_logical.py:176:24:176:24 | ControlFlowNode for s |
| test_logical.py:185:24:185:24 | ControlFlowNode for s |
| test_logical.py:193:24:193:24 | ControlFlowNode for s |
| test_logical.py:199:28:199:28 | ControlFlowNode for s |
| test_logical.py:206:28:206:28 | ControlFlowNode for s |
| test_logical.py:211:28:211:28 | ControlFlowNode for s |
| test_logical.py:214:28:214:28 | ControlFlowNode for s |
| test_logical.py:219:28:219:28 | ControlFlowNode for s |
| test_logical.py:226:28:226:28 | ControlFlowNode for s |
| test_logical.py:231:28:231:28 | ControlFlowNode for s |
| test_logical.py:234:28:234:28 | ControlFlowNode for s |
| test_reference.py:31:28:31:28 | ControlFlowNode for s |
44 changes: 44 additions & 0 deletions python/ql/test/library-tests/dataflow/tainttracking/customSanitizer/test_logical.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,49 @@ def test_with_exception_neg():

ensure_not_tainted(s)

def test_comparison_with_bool():
s = TAINTED_STRING

if is_safe(s) == True:
ensure_not_tainted(s)
else:
ensure_tainted(s) # $ tainted

if is_safe(s) == False:
ensure_tainted(s) # $ tainted
else:
ensure_not_tainted(s)

if is_safe(s) != True:
ensure_tainted(s) # $ tainted
else:
ensure_not_tainted(s)

if is_safe(s) != False:
ensure_not_tainted(s)
else:
ensure_tainted(s) # $ tainted
Comment on lines +198 to +216
Copy link
Contributor

@tausbn tausbn Feb 13, 2026

Choose a reason for hiding this comment

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

For completeness, we should add versions of these tests with the arguments to ==/!= swapped. (I sincerely hope no one has ever written False is not is_safe(s), so let's not bother with those.)


if is_safe(s) is True:
ensure_not_tainted(s)
else:
ensure_tainted(s) # $ tainted

if is_safe(s) is False:
ensure_tainted(s) # $ tainted
else:
ensure_not_tainted(s)

if is_safe(s) is not True:
ensure_tainted(s) # $ tainted
else:
ensure_not_tainted(s)

if is_safe(s) is not False:
ensure_not_tainted(s)
else:
ensure_tainted(s) # $ tainted

# Make tests runable

test_basic()
Expand All @@ -211,3 +254,4 @@ def test_with_exception_neg():
test_with_exception_neg()
except:
pass
test_comparison_with_bool()