Skip to content
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

New check: B113: TrojanSource - Bidirectional control characters #757

Merged
merged 14 commits into from
Jun 24, 2024

Conversation

Lucas-C
Copy link
Contributor

@Lucas-C Lucas-C commented Nov 16, 2021

Close #749

@sigmavirus24
Copy link
Member

Could you rebase please?

@Lucas-C
Copy link
Contributor Author

Lucas-C commented Nov 17, 2021

@sigmavirus24: Sure, done

@Lucas-C
Copy link
Contributor Author

Lucas-C commented Nov 17, 2021

I added a commit to please the pep8 check

@Lucas-C
Copy link
Contributor Author

Lucas-C commented Nov 18, 2021

I just fixed the typo I made in the last commit

@Lucas-C
Copy link
Contributor Author

Lucas-C commented Nov 18, 2021

I added the missing "SPDX-License-Identifier: Apache-2.0" comment to please flake8

Copy link
Member

@sigmavirus24 sigmavirus24 left a comment

Choose a reason for hiding this comment

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

But I'd like another person to take a gander

'\u2066', '\u2067', '\u2068', '\u2069')


@test.test_id('B113')
Copy link
Member

Choose a reason for hiding this comment

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

PR #743 is also using B113. First to merge would get it, other would need to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm fine with that 😄

@Lucas-C
Copy link
Contributor Author

Lucas-C commented Nov 28, 2021

Just checking: this is pending approval from @lukehinds and/or @ghugo ?

@sigmavirus24
Copy link
Member

Or @ericwb

@Lucas-C
Copy link
Contributor Author

Lucas-C commented Jan 17, 2022

Ping @lukehinds / @ghugo / @sigmavirus24 / @ericwb:
Hi and happy new year! 😊
I take the liberty to ping about this PR : what's missing to get it approved & merged please?

@Lucas-C
Copy link
Contributor Author

Lucas-C commented Feb 7, 2022

FYI it has been added to Pylint already: pylint-dev/pylint#5311

@Lucas-C
Copy link
Contributor Author

Lucas-C commented Mar 1, 2022

I have made some fixups to please the pre-commit hooks, rebased my branch against main,
and I think test_trojansource should be passing now...

At least it does on my environment, with Python 3.7:

$ pytest -vv -k trojansource
...
tests/functional/test_functional.py::FunctionalTests::test_trojansource PASSED                                                  [ 50%]
tests/functional/test_functional.py::FunctionalTests::test_trojansource_latin1 PASSED                                           [100%]

Could you please approve the pipeline execution @ericwb?

Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Could you rebase on main and resolve conflicts? Thanks

Also, since this seems more closely related to a type of injection, I
think bandit ID of 613 would be more appropriate.

bandit/plugins/trojansource.py Outdated Show resolved Hide resolved
bandit/plugins/trojansource.py Outdated Show resolved Hide resolved
bandit/plugins/trojansource.py Outdated Show resolved Hide resolved
bandit/plugins/trojansource.py Outdated Show resolved Hide resolved
doc/source/plugins/trojansource.rst Outdated Show resolved Hide resolved
@Lucas-C
Copy link
Contributor Author

Lucas-C commented Jul 17, 2022

I see you've added commits, is a rebase still needed @ericwb ?

@ericwb
Copy link
Member

ericwb commented Jun 24, 2024

The bandit-baseline fails in the pep8 build job. I suspect this is normal since a new plugin was added, so the baseline comparison would be different.

I'll merge and see if it resolves, otherwise I might have to revert.

@ericwb ericwb merged commit ec384b8 into PyCQA:main Jun 24, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add check for potential misuse of unicode
4 participants