Enable S rule #1857

artem-shelkovnikov · 2023-10-30T12:34:33Z

Slightly controversial potentially rule:

Enabling "S" rule for linter: https://pypi.org/project/flake8-bandit/

There are a lot of subrules, but generally this group is about security-related issues.

Reported problems:

connectors/sources/directory.py:60:16: S324 Probable use of insecure hash functions in `hashlib`: `md5`
connectors/sources/dropbox.py:67:20: S105 Possible hardcoded password assigned to: "ACCESS_TOKEN"
connectors/sources/microsoft_teams.py:63:31: S105 Possible hardcoded password assigned to: "GRAPH_ACQUIRE_TOKEN_URL"
connectors/sources/mssql.py:56:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mssql.py:60:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mssql.py:64:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mssql.py:68:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mssql.py:72:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mysql.py:57:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mysql.py:60:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mysql.py:63:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mysql.py:66:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mysql.py:69:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/mysql.py:474:38: S608 Possible SQL injection vector through string-based query construction
connectors/sources/oracle.py:42:13: S608 Possible SQL injection vector through string-based query construction
connectors/sources/oracle.py:47:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/oracle.py:51:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/oracle.py:55:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/oracle.py:59:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/outlook.py:620:17: S112 `try`-`except`-`continue` detected, consider logging the exception
connectors/sources/postgresql.py:52:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/postgresql.py:57:13: S608 Possible SQL injection vector through string-based query construction
connectors/sources/postgresql.py:64:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/postgresql.py:68:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/postgresql.py:72:16: S608 Possible SQL injection vector through string-based query construction
connectors/sources/s3.py:193:18: S324 Probable use of insecure hash functions in `hashlib`: `md5`
connectors/sources/salesforce.py:32:18: S105 Possible hardcoded password assigned to: "TOKEN_ENDPOINT"
connectors/utils.py:220:36: S602 `subprocess` call with `shell=True` identified, security issue
connectors/utils.py:418:9: S110 `try`-`except`-`pass` detected, consider logging the exception
connectors/utils.py:641:12: S324 Probable use of insecure hash functions in `hashlib`: `md5`

All SQL-reported problems will be investigated and fixed later, thus I've added an ignore rule for now, all others I've manually checked and marked as non-problem.

For now I've also ignored it in test files, manually checking them, but would like to discuss it further - to me it makes sense to enable it for tests too, so that if some credentials are leaked by accident, then the linter will complain.

navarone-feekery · 2023-10-30T13:12:40Z

connectors/sources/salesforce.py

@@ -29,7 +29,7 @@

 BASE_URL = "https://<domain>.my.salesforce.com"
 API_VERSION = "v58.0"
-TOKEN_ENDPOINT = "/services/oauth2/token"
+TOKEN_ENDPOINT = "/services/oauth2/token"  # noqa S105


Wild that it thinks this is a password 🤷🏻

Yeah I think the word "TOKEN" triggered it

navarone-feekery · 2023-10-30T13:13:31Z

connectors/cli.py

@@ -128,6 +127,28 @@ async def _start_service(actions, config, loop):
        return await multiservice.run()


+def get_event_loop(uvloop=False):


Out of curiosity, why did get_event_loop have to be moved?

Cause it had a "try/except" with except that does nothing. I've added a logger statement there and moved to the only place where the function is used

navarone-feekery · 2023-10-30T13:14:54Z

For now I've also ignored it in test files, manually checking them, but would like to discuss it further - to me it makes sense to enable it for tests too, so that if some credentials are leaked by accident, then the linter will complain.

I agree having this on for tests makes sense. If it ends up raising too many false positives then we can always disable it.

github-actions · 2023-11-06T12:12:06Z

💔 Failed to create backport PR(s)

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 1857 --autoMerge --autoMergeMethod squash

Enable S rule

b059f99

artem-shelkovnikov requested a review from a team October 30, 2023 12:34

github-actions bot added auto-backport v8.12.0.0 labels Oct 30, 2023

navarone-feekery previously approved these changes Oct 30, 2023

View reviewed changes

Merge branch 'main' into artem/enable-s-linter-rule

124c9f3

artem-shelkovnikov dismissed navarone-feekery’s stale review via 124c9f3 November 1, 2023 16:18

artem-shelkovnikov added 4 commits November 1, 2023 17:26

Fix merge

5d36d68

Fix merge + sort ignores

cd83489

Revert change to convert_to_b64

18ff5d5

Merge branch 'main' into artem/enable-s-linter-rule

ae2dfb4

artem-shelkovnikov mentioned this pull request Nov 2, 2023

Increase test coverage for connectors.utils module #1864

Merged

artem-shelkovnikov enabled auto-merge (squash) November 3, 2023 12:15

Merge branch 'main' into artem/enable-s-linter-rule

254209c

artem-shelkovnikov requested a review from navarone-feekery November 3, 2023 14:04

vidok approved these changes Nov 6, 2023

View reviewed changes

artem-shelkovnikov merged commit b53220a into main Nov 6, 2023

artem-shelkovnikov deleted the artem/enable-s-linter-rule branch November 6, 2023 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable S rule #1857

Enable S rule #1857

artem-shelkovnikov commented Oct 30, 2023

navarone-feekery Oct 30, 2023

artem-shelkovnikov Oct 30, 2023

navarone-feekery Oct 30, 2023

artem-shelkovnikov Oct 30, 2023

navarone-feekery commented Oct 30, 2023

github-actions bot commented Nov 6, 2023

		@@ -128,6 +127,28 @@ async def _start_service(actions, config, loop):
		return await multiservice.run()


		def get_event_loop(uvloop=False):

Enable S rule #1857

Enable S rule #1857

Conversation

artem-shelkovnikov commented Oct 30, 2023

navarone-feekery Oct 30, 2023

Choose a reason for hiding this comment

artem-shelkovnikov Oct 30, 2023

Choose a reason for hiding this comment

navarone-feekery Oct 30, 2023

Choose a reason for hiding this comment

artem-shelkovnikov Oct 30, 2023

Choose a reason for hiding this comment

navarone-feekery commented Oct 30, 2023

github-actions bot commented Nov 6, 2023

💔 Failed to create backport PR(s)