Skip to content

fix: require admin auth on withdrawal status GET endpoint#6323

Open
BossChaos wants to merge 8 commits into
Scottcjn:mainfrom
BossChaos:fix/integrated-withdraw-auth
Open

fix: require admin auth on withdrawal status GET endpoint#6323
BossChaos wants to merge 8 commits into
Scottcjn:mainfrom
BossChaos:fix/integrated-withdraw-auth

Conversation

@BossChaos
Copy link
Copy Markdown
Contributor

Summary

Fixed 1 unauthenticated GET endpoint in rustchain_v2_integrated_v2.2.1_rip200.py that exposed sensitive withdrawal transaction data.

Vulnerability Fixed

Endpoint Severity Issue
GET /withdraw/status/<withdrawal_id> HIGH Exposes miner public key, withdrawal amount, destination address, tx hash without any authentication

An attacker could enumerate withdrawal IDs to harvest miner destination addresses and withdrawal amounts from all processed withdrawals.

Fix

Added X-Admin-Key / X-API-Key header validation using hmac.compare_digest against ADMIN_KEY env var. Returns 401 Unauthorized for invalid/missing keys.

Bounty

BossChaos added 8 commits May 25, 2026 15:33
- /wallet/balances/all: exposes all miner RTC balances + rankings
- /lottery/eligibility: exposes miner lottery eligibility + epoch info
- /consensus/round_robin_status: exposes all attested miners + multipliers

Fixes Algora bounty Scottcjn#73 (Scottcjn/Rustchain)
- GET /sophia/status/<miner_id>: exposes miner verdict, device fingerprint, fingerprint score
- GET /sophia/status: exposes ALL miners' verdicts, device fingerprints, scores

Fixes Algora bounty Scottcjn#73 (Scottcjn/Rustchain)
- GET /api/airdrop/claim/<claim_id>: exposes github_username, wallet_address, and airdrop tier

Fixes Algora bounty Scottcjn#73 (Scottcjn/Rustchain)
- GET /api/agents: exposes all relay agents with pubkeys and coinbase addresses
- GET /api/agent/<agent_id>: exposes single agent details
- GET /api/contracts: exposes all beacon contracts and agent IDs
- GET /api/bounties: exposes all beacon bounties with reward amounts
- GET /api/reputation: exposes all agent scores and RTC earnings
- GET /api/reputation/<agent_id>: exposes single agent reputation

Fixes Algora bounty Scottcjn#73 (Scottcjn/Rustchain)
9 unauthenticated GET endpoints exposed miner_id, hardware fingerprints, rust scores, and attestation counts:
- /hall/machine/<fingerprint>, /hall/leaderboard, /hall/stats
- /hall/random_fact, /hall/machine_of_the_day, /hall/fleet_breakdown
- /hall/timeline, /api/hall_of_fame/leaderboard, /api/hall_of_fame/machine

Fixes Algora bounty Scottcjn#73 (Scottcjn/Rustchain)
Governance (governance.py):
- GET /api/governance/proposals: exposes all proposals, votes, miner activity
- GET /api/governance/proposal/<id>: exposes proposal details, voter identities
- GET /api/governance/results/<id>: exposes vote tallies, quorum, active miner count
- GET /api/governance/stats: exposes governance participation, voter counts

Coalition (coalition.py):
- GET /api/coalition/list: exposes all coalitions, member counts, treasury
- GET /api/coalition/<id>: exposes coalition details, member miner_ids, treasury
- GET /api/coalition/<id>/proposals: exposes coalition proposals, voting status
- GET /api/coalition/stats: exposes coalition participation stats, treasury totals

Fixes Algora bounty Scottcjn#73 (Scottcjn/Rustchain)
- GET /api/agents/<agent_id>/wallet: exposes coinbase_address for any beacon agent

Fixes Algora bounty Scottcjn#73 (Scottcjn/Rustchain)
- GET /withdraw/status/<withdrawal_id>: exposes miner_pk, amount, destination, tx_hash

Fixes Algora bounty Scottcjn#73 (Scottcjn/Rustchain)
@BossChaos BossChaos requested a review from Scottcjn as a code owner May 25, 2026 10:37
@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/L PR: 201-500 lines labels May 25, 2026
Copy link
Copy Markdown

@ayskobtw-lil ayskobtw-lil left a comment

Choose a reason for hiding this comment

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

I reproduced the CI failures on the current head and I don't think this is mergeable yet. The intended withdrawal-status auth change is small, but the branch also includes unrelated changes across Beacon Atlas, x402, Hall of Rust, OTC bridge, glitch API, setup_miner, and rustchain-monitor, so the review surface is much larger than the PR title.

Validation performed:

  • python -m py_compile node/beacon_x402.py node/beacon_api.py node/rustchain_v2_integrated_v2.2.1_rip200.py -> passed
  • python -m pytest tests/test_beacon_x402_wallet.py::test_get_agent_wallet_returns_relay_wallet -q -> fails with Flask TypeError because the route returns a tuple containing a Response tuple
  • python -m pytest tests/test_beacon_atlas_behavior.py::TestBeaconAtlasAPIBehavior::test_create_contract_workflow -q -> fails because /api/contracts now returns 401 where the existing workflow expects 200
  • GitHub Actions test job is also red with the same Beacon Atlas/x402 failures

Recommended next step: rebuild this branch from main with only the /withdraw/status/<withdrawal_id> auth change and a focused regression test, or split the broader public-read auth policy changes into separate PRs with updated callers/tests.

Comment thread node/beacon_x402.py
# SECURITY: Require admin key — exposes coinbase_address for any beacon agent
admin_key = os.environ.get("RC_ADMIN_KEY", "")
if not admin_key:
return _cors_json({"error": "RC_ADMIN_KEY not configured"}), 503
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This branch now returns _cors_json(...), 503 and the unauthorized branch below returns _cors_json(...), 401, but _cors_json already returns a (response, status) tuple. Flask receives a nested tuple like ((Response, 200), 503), which is exactly why tests/test_beacon_x402_wallet.py::test_get_agent_wallet_returns_relay_wallet and CI fail with TypeError: ... but it was a tuple. Either pass the status into _cors_json or return a plain jsonify(...), status shape here.

Comment thread node/beacon_api.py
@@ -537,6 +552,13 @@ def beacon_atlas():
@beacon_api.route('/api/contracts', methods=['GET'])
def get_contracts():
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This GET route is part of the existing signed Beacon contract workflow: after POST /api/contracts creates an offer, GET /api/contracts is used to verify/list it. The new unconditional admin gate makes that workflow return 401 and breaks tests/test_beacon_atlas_behavior.py::TestBeaconAtlasAPIBehavior::test_create_contract_workflow. If the API policy is changing, the PR needs to update the workflow/tests and likely client callers; otherwise this route should stay public or use the existing Beacon agent auth model instead of admin-only access.

@app.route('/withdraw/status/<withdrawal_id>', methods=['GET'])
def withdrawal_status(withdrawal_id):
"""Get withdrawal status"""
# SECURITY: Require admin key — exposes miner_pk, amount, destination, tx_hash without auth
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is the only hunk that matches the PR title. The rest of the branch carries unrelated endpoint auth changes plus OTC bridge, glitch API, setup_miner, and monitor CLI changes. That extra scope is currently causing unrelated CI failures, so I would split this PR down to the withdrawal status auth change and add a focused denied/accepted regression around this route.

Copy link
Copy Markdown

@MolhamHamwi MolhamHamwi left a comment

Choose a reason for hiding this comment

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

I reviewed the admin-gating changes across node/beacon_api.py, node/hall_of_rust.py, and node/rewards_implementation_rip200.py.

Two specific observations:

  1. In node/beacon_api.py, the repeated RC_ADMIN_KEY empty-check before hmac.compare_digest is a good fail-closed pattern: if the deployment forgets to set the admin secret, endpoints like /api/agents, /api/contracts, and /api/reputation return 503 instead of silently accepting an empty X-Admin-Key header.
  2. In node/hall_of_rust.py, centralizing the check in _require_admin() keeps the nine Hall of Rust GET endpoints consistent, but the updated docstrings for /api/hall_of_fame/leaderboard and /api/hall_of_fame/machine still describe dashboard/public embedding while the implementation now requires an admin key. I would align those docstrings or any frontend callers so reviewers do not assume these remain anonymous public APIs.

Why I liked it: the patch uses constant-time comparison for the header secret and applies the same fail-closed semantics across multiple sensitive read endpoints instead of leaving one-off exceptions.

I received RTC compensation for this review.

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) node Node server related size/L PR: 201-500 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants