Skip to content

Python: Expand StringConstCompareBarrier sanitizer gaurds to cover additional constants #17540

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.

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

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

joefarebrother
Copy link
Contributor

Renames StringConstCompareBarrier to ConstCompareBarrier and expands it to cover comparisons to additional constats such as None.
This reduces FP flow in cases such as if x is None: return x.

@@ -142,7 +142,7 @@ module UrlRedirect {
/**
* A comparison with a constant string, considered as a sanitizer-guard.
*/
class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier {
class StringConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to follow the same pattern of deprecation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; I had missed it with a find-and-replace.
Updated.


def test_in_list_with_constants():
ts = TAINTED_STRING
if ts in ["safe", None, 3, False]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be considered tainted if there is a non-constant in this list? Should we have a test with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additional test added.

@yoff
Copy link
Contributor

yoff commented Sep 23, 2024

This looks like a very nice and useful generalisation :-)

Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM

@yoff yoff merged commit e7bc71f into github:main Sep 23, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants