Skip to content

fix: require admin key for contributor approval#4723

Open
saim256 wants to merge 1 commit into
Scottcjn:mainfrom
saim256:fix/contributor-approval-admin-key
Open

fix: require admin key for contributor approval#4723
saim256 wants to merge 1 commit into
Scottcjn:mainfrom
saim256:fix/contributor-approval-admin-key

Conversation

@saim256
Copy link
Copy Markdown

@saim256 saim256 commented May 12, 2026

/claim #4714

Summary

Fixes the public contributor registry approval authorization issue reported in #4714.

The approval route now:

  • accepts only POST /approve/<username> instead of state-changing GET requests,
  • fails closed unless CONTRIBUTOR_ADMIN_KEY is configured,
  • requires the configured key through X-Admin-Key or X-API-Key,
  • uses hmac.compare_digest() for key comparison,
  • keeps pending contributors unchanged for unconfigured, missing, wrong, and GET approval attempts.

I also changed the contributor registry DB access and focused tests to explicitly close SQLite connections. That keeps the regression suite reliable on Windows, where open handles prevent temporary DB cleanup.

Validation

  • python -m pytest tests\test_contributor_registry.py -q -> 15 passed, 1 warning
  • python -m py_compile contributor_registry.py tests\test_contributor_registry.py -> passed
  • python -m ruff check contributor_registry.py tests\test_contributor_registry.py --select F821,F401,F811 --output-format=concise -> passed
  • git diff --check -- contributor_registry.py tests\test_contributor_registry.py -> passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> passed

Wallet/miner ID: RTC253255d034065a839cd421811ec589ae5b694ffc

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes size/M PR: 51-200 lines and removed BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes labels May 12, 2026
Copy link
Copy Markdown

@strongkeep-debug strongkeep-debug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved current head f096720.

The reviewed path now fails closed for contributor approval and limits the state-changing route to authenticated POST requests. The tests cover the key authorization boundaries: GET no longer mutates state, an unset admin key returns 503, missing or wrong keys return 403, and a valid X-Admin-Key approves the pending contributor.

Check Result
uv run --no-project --with pytest --with flask python -m pytest tests\test_contributor_registry.py -q 15 passed, 1 warning
python -m py_compile contributor_registry.py tests\test_contributor_registry.py passed
uv run --no-project --with ruff python -m ruff check contributor_registry.py tests\test_contributor_registry.py --select F821,F401,F811 --output-format=concise All checks passed!
git diff --check origin/main...HEAD -- contributor_registry.py tests\test_contributor_registry.py passed
python tools\bcos_spdx_check.py --base-ref origin/main BCOS SPDX check: OK

No merge blocker found in the contributor approval authorization scope.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for contributing. Approved.

@CookUpSunny
Copy link
Copy Markdown

There is a regression risk from replacing with sqlite3.connect(...) as conn: with with closing(sqlite3.connect(...)) as conn: throughout the tests and app.

sqlite3.Connection is itself a context manager: exiting the with sqlite3.connect(...) block commits on success and rolls back on error. contextlib.closing(...) only calls close(); it does not preserve that commit/rollback behavior. That means writes without an explicit conn.commit() can be rolled back when the connection closes.

The immediate breakage appears to be the seed_contributor fixture: it inserts the seeded row but does not call conn.commit(). With closing(...), tests depending on that fixture, such as the API contributor fields test, may no longer see the seeded contributor.

I would either keep the original with sqlite3.connect(...) as conn: pattern and let sqlite manage transactions, or add explicit commits everywhere a closing(...) connection performs writes. For this PR, keeping the original pattern seems safer and avoids mixing connection-closing cleanup with transaction semantics.

Copy link
Copy Markdown
Contributor

@godd-ctrl godd-ctrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after focused security review of the contributor approval route. The change makes approval POST-only, fails closed when CONTRIBUTOR_ADMIN_KEY is missing, compares X-Admin-Key or legacy X-API-Key with hmac.compare_digest before any SQLite status update, and the regression tests assert that GET, missing config, missing key, and wrong key attempts leave pending contributors unchanged. Validation run locally on this head: uv run --no-project --with pytest --with flask python -m pytest tests\test_contributor_registry.py -q passed with 15 passed and 1 warning. python -m py_compile contributor_registry.py tests\test_contributor_registry.py passed. uv run --no-project --with ruff python -m ruff check contributor_registry.py tests\test_contributor_registry.py --select F821,F401,F811 --output-format=concise passed. git diff --check origin/main...HEAD for the touched files passed, and python tools\bcos_spdx_check.py --base-ref origin/main returned BCOS SPDX check OK. No blocker found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M PR: 51-200 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants