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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Lucas-C
Copy link

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

Close #749

@sigmavirus24
Copy link
Member

Could you rebase please?

@Lucas-C
Copy link
Author

Lucas-C commented Nov 17, 2021

@sigmavirus24: Sure, done

@Lucas-C
Copy link
Author

Lucas-C commented Nov 17, 2021

I added a commit to please the pep8 check

@Lucas-C
Copy link
Author

Lucas-C commented Nov 18, 2021

I just fixed the typo I made in the last commit

@Lucas-C
Copy link
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
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
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
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
Author

Lucas-C commented Feb 7, 2022

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

@Lucas-C
Copy link
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
Author

Lucas-C commented Jul 17, 2022

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

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