Use subprocess.run instead of os.system in webhook handler - #164
Use subprocess.run instead of os.system in webhook handler#164sumitjhadev wants to merge 1 commit into
Conversation
| try: | ||
| sha_name, signature_hash = signature.split('=', 1) | ||
| if sha_name != 'sha256': | ||
| parts = signature.split('=', 1) |
There was a problem hiding this comment.
I used maxsplit=1 so the signature is split only at the first = into the algorithm name and the digest.
This keeps the parsing limited to the expected sha256=<digest> structure and avoids splitting the remainder unnecessarily.
If you prefer a stricter or simpler parsing approach here, I’m happy to change it.
There was a problem hiding this comment.
Did you confirm that multiple = existed before now? I'm wondering why that'll be the case.
There was a problem hiding this comment.
No, legitimate GitHub signatures never contain multiple = (they are hexadecimal, not base64).
maxsplit=1 was just added defensively for malformed headers. Since the enclosing try...except already catches unpacking errors and safely returns False, signature.split('=') is completely safe.
I can revert to signature.split('=') if you prefer the simpler version!
There was a problem hiding this comment.
I have no issue with your fix, I Just wanted you to articulate why, it'll be great to include that in your commit message. See making commits.
There was a problem hiding this comment.
Thanks so much for pointing me to the making-commits guide, @chimosky it really helped clarify things.
I've reworked the commit message, title, and description around it.
Kindly review it.
3e34744 to
de73688
Compare
The webhook handler built shell command strings from repo data and ran
them with os.system(), letting a malicious repository or branch name
inject arbitrary shell commands. Run git fetch, git reset, and
systemctl restart via subprocess.run with argument lists instead,
using shell=False, check=True, timeout=30, and an explicit
cwd=REPO_PATH_LOCALLY, so input is passed directly to the process and
never interpreted by a shell.
Parse the X-Hub-Signature-256 header with signature.split('=', 1).
Valid GitHub signatures are always sha256=<64-char hex digest> and
never contain more than one '=', so this only matters defensively —
the surrounding exception handler already treats malformed input as
an invalid signature.
Add tests in app/tests/test_webhook.py covering signature validation,
subprocess failure handling (CalledProcessError, TimeoutExpired), and
confirming subprocess.run is never called on authentication failures.
Fixes sugarlabs#135
de73688 to
2243523
Compare
sumitjhadev
changed the title
2 participants
Fixes #135.
Replaces os.system() shell-string execution with parameterized
subprocess.run calls so webhook-triggered commands can't be
manipulated via shell metacharacters. See the commit message for
the reasoning behind the signature-parsing change and test coverage.