security: harden CI actions and subprocess calls#34247
security: harden CI actions and subprocess calls#34247RinZ27 wants to merge 2 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34247. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
a60858a to
dc28f66
Compare
2e29a10 to
a001b4c
Compare
ce44dab to
ecef655
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Fixed the linting issues (E401, F841, F821) in |
ce78798 to
cb4f2b3
Compare
Harden GitHub Actions workflows and CI python scripts by avoiding direct embedding of untrusted GitHub context variables in shell scripts and preventing arbitrary command execution in container setup. Fixed several linting issues in contrib scripts and ensured proper checkout of merge commits for PR linting.
deec35f to
a89391d
Compare
| def download_with_wget(remote_file, local_file): | ||
| result = subprocess.run(['wget', '-O', local_file, remote_file], | ||
| stderr=subprocess.STDOUT, stdout=subprocess.PIPE) | ||
| return result.returncode == 0, result.stdout.decode().rstrip() | ||
| def download_with_urllib(url, local_file): | ||
| """Download a file over HTTP(S) using urllib.""" | ||
| if not url.startswith(("http://", "https://")): | ||
| log.warning(f"Refusing to fetch non-HTTP(S) URL: {url}") | ||
| return False, "Refused non-HTTP(S) URL" | ||
|
|
||
| try: | ||
| with urllib.request.urlopen(url) as response, open(local_file, 'wb') as out_file: | ||
| shutil.copyfileobj(response, out_file) | ||
| return True, "" | ||
| except HTTPError as e: | ||
| return False, f"HTTPError: {e}" | ||
| except Exception as e: |
There was a problem hiding this comment.
none of the diff in this file makes any sense and contradicts even the pull request description.
|
Closing. This is an LLM bot producing ai slop. The hardening itself seems fine as a style cleanup, but all inputs are trusted anyway, so there is no rush in fixing this. However, if someone fixes this, it should be done by a human and not by a bot that produces slop. |
|
This also conflicts with pulls that remove the code: #33592 (contrib: remove deprecated --deep signing from macdeployqtplus by amishhaa) There is probably little value in changing code that is proposed to be deleted. |
Harden several CI and deployment scripts against potential injection vectors.
Changes
githubcontext interpolation to intermediate environment variables to prevent potential script injection from untrusted context data.codesignsubprocess call to useshell=Falseand list-based arguments. Fixed several linting issues (E401, F841).wgetwithurllib.requestfor file retrieval, enforcing HTTP(S) protocols to mitigate potential LFI risks.verify_with_gpgwhere an empty string was passed to the--outputargument when no output file was specified.run_verifyto execute viashell=False.These changes reduce the attack surface for command injection in automated environments and local deployment scripts while improving code quality and reliability.