fix: add admin authorization for contributor approval (closes #4714)#4753
fix: add admin authorization for contributor approval (closes #4714)#4753wukong921 wants to merge 4 commits into
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
saim256
left a comment
There was a problem hiding this comment.
Requesting changes on current head 43f6ac1.
This now edits the live contributor_registry.py route, which is the right file, but the new auth paths will raise NameError before returning the intended controlled responses because the file does not import logging.
Blocking details:
- When CONTRIBUTOR_ADMIN_KEY is unset, the code executes logging.error(...) before returning the 503 JSON response. Since logging is not imported in this module, the fail-closed path becomes a 500 instead of the documented disabled response.
- With a wrong or missing admin key, logging.warning(...) has the same problem, so the unauthorized path also 500s instead of returning 401.
- On the success path, logging.info(...) can also raise after committing the approval, which means the database state may change but the request still fails with a server error.
Other review notes:
- make_response is imported but unused.
- The unrelated README footer lines from the first commit should be removed from this security fix.
- Please add regression tests for the unset-key, wrong-key, and valid-key paths; this bug would be caught immediately by exercising those responses.
PR #4723 already has a narrower green patch for the same live route with auth-path regression coverage, so this PR should either be fixed to equivalent quality or closed in favor of the focused PR.
The previous commit missed `import logging`, causing NameError when the code tries to log messages. Fixes the CHANGES_REQUESTED review from @saim256.
Asti1982
left a comment
There was a problem hiding this comment.
Requesting changes on current head 2dc6879.
The logging import fix addresses the earlier immediate 500, but the PR still leaves the contributor registry regression surface inconsistent with the route change.
Blocking issue:
tests/test_contributor_registry.py::TestApproveRoute::test_approve_pending_contributorstill exercisesGET /approve/<username>and expects a 200 plus an approval state change. Current head intentionally changes the route toPOSTonly, so the existing test now fails with 405. That means this PR changes the public route contract without updating the repo's own coverage to encode the new expected behavior: unset key -> 503, wrong key -> 401, valid admin key -> approval, GET -> no mutation.
Additional notes:
- The current branch still includes unrelated README footer noise from the first commit; this security fix should stay limited to the auth route and its tests.
- On Windows, the existing contributor registry test fixture also leaves temporary SQLite files locked during teardown. That may predate this PR, but it means the contributor-registry suite is not a clean validation signal on Windows until the fixture closes all DB handles or the new targeted tests avoid the lock pattern.
Validation I ran locally:
python -m py_compile contributor_registry.py
# passed
python -m pytest tests\test_contributor_registry.py -q
# 1 failed, 11 passed, 12 teardown errors on Windows
# primary assertion failure: GET /approve/pendinguser returned 405, expected 200 in the unchanged testI also ran a focused Flask-client smoke against the new route behavior on a temporary DB:
POST /approve/nomaduser with no CONTRIBUTOR_ADMIN_KEY -> 503
POST /approve/nomaduser with wrong X-Admin-Key -> 401
GET /approve/nomaduser -> 405
POST /approve/nomaduser with valid X-Admin-Key -> 302 and DB status approved
The auth direction is right, but the PR needs updated regression tests and the unrelated README edit removed before merge.
Update test_approve_pending_contributor to test POST instead of GET. Test cases: - No admin key → 503 - Wrong admin key → 401 - Valid admin key → 200 (approve) - GET method → 405 Fixes CHANGES_REQUESTED review from @Asti1982.
Summary
Fix security vulnerability in contributor approval endpoint.
Problem
The
/approve/<username>endpoint had no authentication, allowing anyone to approve contributors without authorization.Solution
X-Admin-Keyheader oradmin_keyform fieldCONTRIBUTOR_ADMIN_KEYenvironment variablesecrets.compare_digest()for constant-time key comparisonTesting
test_contributor_approval_security.pywith test casesSecurity Impact
/approve/<username>Fixes #4714
@saim256 @SimoneMariaRomeo Please review.