Skip to content

Commit 8c2bbdd

Browse files
jdeanwallacejdeanwallace
authored
Add Bandit to CircleCI (#1907)
Related tiny-pilot/tinypilot-pro#1594 This PR updates `ruff`, [migrates our ruff config to the newer format](https://docs.astral.sh/ruff/editors/migration/#migrating-from-ruff-lsp), enables [ruff's `flake8-bandit` rules](https://docs.astral.sh/ruff/rules/#flake8-bandit-s), and addresses the [Bandit security warnings raised](https://app.circleci.com/pipelines/github/tiny-pilot/tinypilot/4829/workflows/5ef2a71e-986a-4dd2-b2e3-8b440caa47f0/jobs/36695?invite=true#step-103-14222_64), namely: - [suspicious-url-open-usage (S310)](https://docs.astral.sh/ruff/rules/suspicious-url-open-usage/#suspicious-url-open-usage-s310) - [subprocess-without-shell-equals-true (S603)](https://docs.astral.sh/ruff/rules/subprocess-without-shell-equals-true/#subprocess-without-shell-equals-true-s603) - [start-process-with-partial-path (S607)](https://docs.astral.sh/ruff/rules/start-process-with-partial-path/#start-process-with-partial-path-s607) A note on [how Bandit works](PyCQA/bandit#333 (comment)): > Unfortunately bandit isn't a code-flow analysis tool so it can't reason about what `args` is. It just flags a warning. You manually check the warning and decide to either add `# nosec` at the end of the line or to turn off this B603 test if you find it unhelpful. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1907"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
1 parent 39f927a commit 8c2bbdd

File tree

12 files changed

+79
-50
lines changed

12 files changed

+79
-50
lines changed

‎.ruff.toml‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,15 @@
11
src = ["app"]
22

3+
# Assume Python 3.9.
4+
target-version = "py39"
5+
6+
[lint]
37
select = [
48
"D", # Enable pydocstyle rules.
59
"F", # Enable pyflakes rules.
610
"I", # Enable isort rules.
711
"Q", # Enable flake8-quotes rules.
12+
"S", # Enable flake8-bandit rules.
813
]
914
ignore = [
1015
# Disable rules that require everything to have a docstring.
@@ -26,10 +31,7 @@ exclude = [
2631
"venv",
2732
]
2833

29-
# Assume Python 3.9
30-
target-version = "py39"
31-
32-
[flake8-quotes]
34+
[lint.flake8-quotes]
3335
# Use consistent quotes regardless of whether it allows us to minimize escape
3436
# sequences.
3537
avoid-escape = false
@@ -38,9 +40,9 @@ docstring-quotes = "double"
3840
inline-quotes = "single"
3941
multiline-quotes = "double"
4042

41-
[isort]
43+
[lint.isort]
4244
force-single-line = true
4345
force-to-top = ["log"]
4446

45-
[pydocstyle]
47+
[lint.pydocstyle]
4648
convention = "google"

‎app/debug_logs.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ def collect():
1818
"""
1919
try:
2020
return subprocess.check_output([
21-
'sudo', '/opt/tinypilot-privileged/scripts/collect-debug-logs', '-q'
21+
'/usr/bin/sudo',
22+
'/opt/tinypilot-privileged/scripts/collect-debug-logs', '-q'
2223
])
2324
except subprocess.CalledProcessError as e:
2425
raise LogCollectionScriptFailedError(str(e)) from e

‎app/hostname.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,15 @@ def change(new_hostname):
4242
new_hostname: The new hostname as string.
4343
"""
4444
try:
45-
return subprocess.check_output([
46-
'sudo', '/opt/tinypilot-privileged/scripts/change-hostname',
47-
new_hostname
48-
],
49-
stderr=subprocess.STDOUT,
50-
universal_newlines=True)
45+
# The command arguments are trusted because the new hostname is
46+
# validated by the caller.
47+
return subprocess.check_output( # noqa: S603
48+
[
49+
'/usr/bin/sudo',
50+
'/opt/tinypilot-privileged/scripts/change-hostname',
51+
new_hostname
52+
],
53+
stderr=subprocess.STDOUT,
54+
universal_newlines=True)
5155
except subprocess.CalledProcessError as e:
5256
raise HostnameChangeError(str(e.output).strip()) from e

‎app/local_system.py‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,13 @@ def _exec_shutdown(restart_after):
2929
param = '--poweroff'
3030

3131
try:
32-
result = subprocess.run(['sudo', '/sbin/shutdown', param, 'now'],
33-
capture_output=True,
34-
text=True,
35-
check=True)
32+
# The command arguments are trusted because they aren't based on user
33+
# input.
34+
result = subprocess.run( # noqa: S603
35+
['/usr/bin/sudo', '/sbin/shutdown', param, 'now'],
36+
capture_output=True,
37+
text=True,
38+
check=True)
3639
except subprocess.CalledProcessError as e:
3740
raise ShutdownError(e) from e
3841
if 'failed' in result.stderr.lower():

‎app/network.py‎

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -101,15 +101,18 @@ def inspect_interface(interface_name):
101101
status = InterfaceStatus(interface_name, False, None, None)
102102

103103
try:
104-
ip_cmd_out_raw = subprocess.check_output([
105-
'ip',
106-
'-json',
107-
'address',
108-
'show',
109-
interface_name,
110-
],
111-
stderr=subprocess.STDOUT,
112-
universal_newlines=True)
104+
# The command arguments are trusted because they aren't based on user
105+
# input.
106+
ip_cmd_out_raw = subprocess.check_output( # noqa: S603
107+
[
108+
'/usr/bin/ip',
109+
'-json',
110+
'address',
111+
'show',
112+
interface_name,
113+
],
114+
stderr=subprocess.STDOUT,
115+
universal_newlines=True)
113116
except subprocess.CalledProcessError as e:
114117
logger.error('Failed to run `ip` command: %s', str(e))
115118
return status
@@ -148,7 +151,8 @@ def determine_wifi_settings():
148151
# We cannot read the wpa_supplicant.conf file directly, because it is
149152
# owned by the root user.
150153
config_lines = subprocess.check_output([
151-
'sudo', '/opt/tinypilot-privileged/scripts/print-marker-sections',
154+
'/usr/bin/sudo',
155+
'/opt/tinypilot-privileged/scripts/print-marker-sections',
152156
'/etc/wpa_supplicant/wpa_supplicant.conf'
153157
],
154158
stderr=subprocess.STDOUT,
@@ -181,19 +185,22 @@ def enable_wifi(wifi_settings):
181185
Raises:
182186
NetworkError
183187
"""
188+
# The command arguments are trusted because the WiFi settings are validated
189+
# by the caller.
184190
args = [
185-
'sudo', '/opt/tinypilot-privileged/scripts/enable-wifi', '--country',
186-
wifi_settings.country_code, '--ssid', wifi_settings.ssid
191+
'/usr/bin/sudo', '/opt/tinypilot-privileged/scripts/enable-wifi',
192+
'--country', wifi_settings.country_code, '--ssid', wifi_settings.ssid
187193
]
188194
try:
189195
# Ignore pylint since we're not managing the child process.
190196
# pylint: disable=consider-using-with
191197
if wifi_settings.psk:
192198
args.append('--psk')
193-
process = subprocess.Popen(args, stdin=subprocess.PIPE, text=True)
199+
process = subprocess.Popen( # noqa: S603
200+
args, stdin=subprocess.PIPE, text=True)
194201
process.communicate(input=wifi_settings.psk)
195202
else:
196-
subprocess.Popen(args)
203+
subprocess.Popen(args) # noqa: S603
197204

198205
except subprocess.CalledProcessError as e:
199206
raise NetworkError(str(e.output).strip()) from e
@@ -212,7 +219,7 @@ def disable_wifi():
212219
# Ignore pylint since we're not managing the child process.
213220
# pylint: disable=consider-using-with
214221
subprocess.Popen([
215-
'sudo',
222+
'/usr/bin/sudo',
216223
'/opt/tinypilot-privileged/scripts/disable-wifi',
217224
])
218225
except subprocess.CalledProcessError as e:

‎app/update/launcher.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,4 @@ def start_async():
3636
# Ignore pylint since we're not managing the child process.
3737
# pylint: disable=consider-using-with
3838
subprocess.Popen(
39-
('sudo', '/usr/sbin/service', 'tinypilot-updater', 'start'))
39+
('/usr/bin/sudo', '/usr/sbin/service', 'tinypilot-updater', 'start'))

‎app/update/launcher_test.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ def test_launches_update_when_no_update_is_running(self, mock_status_get,
2121

2222
mock_clear.assert_called()
2323
mock_popen.assert_called_once_with(
24-
('sudo', '/usr/sbin/service', 'tinypilot-updater', 'start'))
24+
('/usr/bin/sudo', '/usr/sbin/service', 'tinypilot-updater',
25+
'start'))
2526

2627
@mock.patch.object(launcher.subprocess, 'Popen')
2728
@mock.patch.object(launcher.update.result_store, 'clear')
@@ -34,7 +35,8 @@ def test_launches_update_when_previous_update_succeeded(
3435

3536
mock_clear.assert_called()
3637
mock_popen.assert_called_once_with(
37-
('sudo', '/usr/sbin/service', 'tinypilot-updater', 'start'))
38+
('/usr/bin/sudo', '/usr/sbin/service', 'tinypilot-updater',
39+
'start'))
3840

3941
@mock.patch.object(launcher.subprocess, 'Popen')
4042
@mock.patch.object(launcher.update.result_store, 'clear')
@@ -48,7 +50,8 @@ def test_launches_update_when_previous_update_failed(
4850

4951
mock_clear.assert_called_once()
5052
mock_popen.assert_called_once_with(
51-
('sudo', '/usr/sbin/service', 'tinypilot-updater', 'start'))
53+
('/usr/bin/sudo', '/usr/sbin/service', 'tinypilot-updater',
54+
'start'))
5255

5356
@mock.patch.object(launcher.subprocess, 'Popen')
5457
@mock.patch.object(launcher.update.result_store, 'clear')

‎app/update_logs.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,12 @@ def read():
3333
"""
3434
logger.info('Running read-update-log')
3535
try:
36-
output = subprocess.check_output(['sudo', _READ_SCRIPT_PATH],
37-
stderr=subprocess.STDOUT,
38-
universal_newlines=True)
36+
# The command arguments are trusted because they aren't based on user
37+
# input.
38+
output = subprocess.check_output( # noqa: S603
39+
['/usr/bin/sudo', _READ_SCRIPT_PATH],
40+
stderr=subprocess.STDOUT,
41+
universal_newlines=True)
3942
except subprocess.CalledProcessError as e:
4043
raise UpdateLogsReadError(str(e.output).strip()) from e
4144
logger.info('read-update-log completed successfully')

‎app/version.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,8 @@ def latest_version():
8383
to the Gatekeeper API.
8484
"""
8585
try:
86-
with urllib.request.urlopen(
86+
# The URL is trusted because it's not based on user input.
87+
with urllib.request.urlopen( # noqa: S310
8788
f'{env.GATEKEEPER_BASE_URL}/community/available-update',
8889
timeout=10) as response:
8990
response_bytes = response.read()

‎app/video_service.py‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ def _restart_ustreamer():
3333
logger.info('Triggering ustreamer restart...')
3434
try:
3535
subprocess.check_output(
36-
['sudo', '/usr/sbin/service', 'ustreamer', 'restart'],
36+
['/usr/bin/sudo', '/usr/sbin/service', 'ustreamer', 'restart'],
3737
stderr=subprocess.STDOUT,
3838
universal_newlines=True)
3939
except subprocess.CalledProcessError as e:
@@ -53,18 +53,19 @@ def _restart_janus():
5353
"""
5454
logger.info('Writing janus configuration...')
5555
try:
56-
subprocess.check_output(
57-
['sudo', '/opt/tinypilot-privileged/scripts/configure-janus'],
58-
stderr=subprocess.STDOUT,
59-
universal_newlines=True)
56+
subprocess.check_output([
57+
'/usr/bin/sudo', '/opt/tinypilot-privileged/scripts/configure-janus'
58+
],
59+
stderr=subprocess.STDOUT,
60+
universal_newlines=True)
6061
except subprocess.CalledProcessError as e:
6162
logger.error('Failed to configure janus: %s', e)
6263
return
6364

6465
logger.info('Triggering janus restart...')
6566
try:
6667
subprocess.check_output(
67-
['sudo', '/usr/sbin/service', 'janus', 'restart'],
68+
['/usr/bin/sudo', '/usr/sbin/service', 'janus', 'restart'],
6869
stderr=subprocess.STDOUT,
6970
universal_newlines=True)
7071
except subprocess.CalledProcessError as e:

0 commit comments

Comments
 (0)