security: harden contribute.py with token validation, error sanitization, and retry/backoff (fixes #288)#398
Conversation
…ling, and retry logic (fixes ControlCore-Project#288)
There was a problem hiding this comment.
Pull request overview
This PR attempts to harden contribute.py by adding token validation, improving error handling, and adding retry logic for GitHub API interactions. The changes address security concerns raised in issue #288 including empty token validation, token format checking, PR URL corrections, and removing unused code.
Changes:
- Added early validation for missing or malformed GitHub tokens (lines 10-18)
- Introduced a
github_request()wrapper function with retry/backoff logic (lines 38-55) - Replaced generic exception handlers with specific
requests.exceptionstypes throughout the file - Fixed PR URL construction from
pulls/{pr.number}topull/{pr.number}(line 59) - Removed unused
decode_token()function (line 140) - Added
requestsandremodule imports
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eption, expand token regex, remove unused requests import, fix spelling (fixes ControlCore-Project#288)
| # Initializing the Variables | ||
| BOT_TOKEN = os.environ.get('CONCORE_BOT_TOKEN', '') | ||
|
|
||
| # Fix 1: Fail fast if token is missing |
There was a problem hiding this comment.
Can you remove the "Fix 1" "Fix 2" "Fix 3" ... counters please?
Just noting, "Fail fast if token is missing" is sufficient.
The counter of errors indeed help with reviewing the PR. But it shouldn't live in the code itself.
There was a problem hiding this comment.
ok @pradeeban sir ill start working on it immediately....
There was a problem hiding this comment.
Thank you for the feedback, @pradeeban Sir You're absolutely right the "Fix 1:", "Fix 2:", etc. counters were meant for PR traceability but don't belong in the code itself.
I've removed all the counter prefixes and kept only the descriptive comments (e.g., # Fail fast if token is missing). Changes pushed in commit 08bd61b.
Please let me know if there's anything else to address.
Hey pradeeban,
Fixes #288.
This PR improves the security and reliability of
contribute.py, mainly around how it handles GitHub API authentication and errors.Previously the script would continue running even if the token was empty, which could lead to confusing failures later. Now the script checks for
CONCORE_BOT_TOKENat startup and exits with a clear error message if it is missing.A simple format check is also added for the token using regex. It supports common GitHub token formats such as
ghp_,github_pat_,ghs_, and the classic 40-character hex tokens. This helps catch obvious configuration mistakes early.Some changes were also made to avoid accidental token exposure. Unsafe prints were removed and exception handling avoids logging the token value.
Other fixes and improvements:
pull/{pr.number}instead ofpulls/{pr.number})with_retry) with exponential backoff for GitHub API calls to handle rate limits and temporary server errorsrequests.exceptions.*handling withgithub.GithubException, which is what PyGithub actually raisese.response.status_codeto the correcte.statusattributedecode_token()functionrequestsimportThese changes are limited to a single file:
contribute.py.No other modules were modified, and existing workflows should continue to work as before.
Testing done locally:
CONCORE_BOT_TOKENis missinggithub.GithubExceptionis caught correctly (e.g., 401)/pull/endpoint