Skip to content

Security: Fix unauthenticated endpoints in registry and payout ledger#5011

Closed
Stevenn28 wants to merge 1 commit into
Scottcjn:mainfrom
Stevenn28:fix-auth-vulnerabilities
Closed

Security: Fix unauthenticated endpoints in registry and payout ledger#5011
Stevenn28 wants to merge 1 commit into
Scottcjn:mainfrom
Stevenn28:fix-auth-vulnerabilities

Conversation

@Stevenn28
Copy link
Copy Markdown

Description\n\nCloses #4911\nCloses #4902\n\nThis PR fixes two critical security vulnerabilities related to unauthenticated API endpoints:\n\n1. Payout Ledger API (#4902): Added an @admin_required decorator using the ADMIN_KEY environment variable to secure the POST /api/ledger (create payout) and PATCH /api/ledger/<id>/status (update status) endpoints. This prevents anyone from creating fake payout records or marking them as paid.\n\n2. Contributor Registry (#4911):\n - Secured the /approve/<username> route with the @admin_required decorator to prevent anyone from approving pending contributors.\n - Added format validation for the rtc_wallet field during registration (must start with 0x or RTC, length > 10).\n - Masked wallet addresses in the /api/contributors public JSON response to prevent scraping and data leaks.

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Your PR has a BCOS-L1 or BCOS-L2 label
  • New code files include an SPDX license header
  • You've tested your changes against the live node

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!

@github-actions github-actions Bot added size/S PR: 11-50 lines BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) and removed size/S PR: 11-50 lines labels May 13, 2026
Copy link
Copy Markdown
Contributor

@loganoe loganoe left a comment

Choose a reason for hiding this comment

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

The security direction is right, but this PR needs changes before merge because it leaves the existing test suite red and has a diff hygiene failure.

Findings:

  • tests/test_contributor_registry.py::TestRegisterRoute::test_register_duplicate_username now fails because the test posts RTC0dup, which is rejected by the new wallet validation before the duplicate-username branch runs. The PR should update that fixture/input to a valid RTC wallet if the duplicate handling is still expected behavior.
  • tests/test_contributor_registry.py::TestApproveRoute::test_approve_pending_contributor still asserts unauthenticated GET /approve/<username> succeeds, but this PR intentionally requires admin auth. The test suite needs to cover the new behavior: missing key denied, wrong key denied, correct X-Admin-Key accepted.
  • contributor_registry.py:162 leaves trailing whitespace; git diff --check origin/main...HEAD fails.

Validation run:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --no-project --with pytest --with flask --with flask-cors --with pyyaml --with pycryptodome --with prometheus-client python -m pytest tests/test_contributor_registry.py tests/test_payout_ledger_migration.py -q -> 2 failed, 12 passed
  • python3 -m py_compile contributor_registry.py payout_ledger.py -> passed
  • uv run --no-project --with ruff ruff check contributor_registry.py payout_ledger.py --select E9,F821,F811,F841 --output-format=concise -> passed
  • git diff --check origin/main...HEAD -> trailing whitespace failure

Please update the tests for the new auth/validation contract and clean up the whitespace.

Copy link
Copy Markdown

@TJCurnutte TJCurnutte left a comment

Choose a reason for hiding this comment

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

The auth hardening works for the no-header/bad-header cases, but I am requesting changes because the new admin guard also accepts the admin secret in the URL query string.

Validation run from the PR head:

  • git diff --check origin/main...HEAD -- contributor_registry.py payout_ledger.py
    • failed on contributor_registry.py:162: trailing whitespace.
  • python3 -B -m py_compile contributor_registry.py payout_ledger.py
    • passed.
  • Focused Flask auth probe with ADMIN_KEY=sekrit:
    • contributor registry: no header 401, bad header 401, good X-Admin-Key 302.
    • payout ledger: unauthenticated POST 401, bad-header POST 401, good-header POST 201, unauthenticated PATCH 401, good-header PATCH 200.
  • Query-string probe with the same admin key:
    • GET /approve/bob?admin_key=sekrit returned 302.
    • POST /api/ledger?admin_key=sekrit returned 201.
    • PATCH /api/ledger/<id>/status?admin_key=sekrit returned 200.

Both new admin_required helpers use request.args.get("admin_key") as an alternative to X-Admin-Key. That puts the admin credential in URL material that commonly lands in access logs, browser history, analytics, and referrers. Since this PR is specifically hardening admin endpoints, please make the guard header-only (or use an established non-URL auth channel) before merge, and clean the trailing whitespace.

Copy link
Copy Markdown
Contributor

@himanalot himanalot left a comment

Choose a reason for hiding this comment

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

Requesting changes.

The new admin_required wrappers accept admin_key from the query string and compare with req_key != ADMIN_KEY. Admin secrets should not be accepted in URLs because they end up in access logs, browser history, analytics, and referrers, and the new comparison is not constant-time.

Also, /approve/<username> remains a GET route that mutates contributor state. After adding admin auth, this should be moved to a non-GET method (for example POST) with the key in a header so approval cannot be triggered by link prefetching/crawlers and the admin secret is not exposed in the URL.

@Scottcjn
Copy link
Copy Markdown
Owner

Paid 10 RTC of 40 RTC cluster (PRs #5011 #5012 #5014 #5080).

tx_hash: 0ff9c8d19252e2edca197859e66845ad | pending #1510 | confirms in 24h

Closing as merge-conflict. All 4 PRs add overlapping admin_required decorators on the same files (memory_routes.py + machine_passport_api.py), so they conflict with each other and main has moved. The security direction is correct (auth-hardening unauthenticated Flask endpoints) and the diffs are clean — paying for the audit work even though we can't cleanly merge the 4-way overlap.

To unblock: rebase a single consolidated PR off latest main with all 4 hardenings in one diff, and we'll merge it as a follow-on (no additional bounty — the work is already paid).

— triage 2026-05-14

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 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!

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

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants