Security: Fix authentication bypass when ADMIN_KEY is unset and secure memory clear endpoint

[Stevenn28]

Description\n\nCloses #4880\nCloses #4878\n\nThis PR fixes two HIGH severity security vulnerabilities related to the ADMIN_KEY:\n\n1. Memory API Clear Endpoint (#4880): Added the @admin_required decorator to the DELETE /api/memory/clear endpoint. Previously, this endpoint had no authentication, allowing anyone to completely wipe an agent's memory. Now it requires a valid X-Admin-Key.\n\n2. Machine Passport API Authentication Bypass (#4878): Changed the admin key check to default-deny. Previously, if the ADMIN_KEY environment variable was not set, the authentication check was skipped entirely, leaving admin endpoints wide open. It now explicitly rejects requests if the ADMIN_KEY is not configured.

[github-actions]

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!

[god-ts]

I am requesting changes because the patch improves a few paths, but it still leaves parts of the claimed security issues open and introduces an unsafe admin-key transport.

Findings:

1. contributor_registry.py:20, payout_ledger.py:27, and bounties/issue-2285/src/memory_routes.py:46 accept admin_key from the URL query string. That means the new admin secret can be written into access logs, browser history, reverse-proxy logs, referrers, and monitoring traces. Since this PR is hardening admin-only destructive/financial routes, please require the key only in a header such as X-Admin-Key / X-API-Key and avoid query-string credentials.

2. payout_ledger.py:196-210 still exposes the payout ledger read endpoints without any authentication. Issue #4902 explicitly lists GET /api/ledger and GET /api/ledger/<id> as part of the data leak because they reveal contributors, wallet addresses, amounts, statuses, PR URLs, tx hashes, and notes. This PR protects create/update, but if it closes #4902 it should either protect those reads too or deliberately redact sensitive fields from public responses.

3. contributor_registry.py:152-174 still lets anyone register any github_username with any wallet, and the form still has no CSRF protection. Adding wallet prefix validation and protecting /approve/<username> helps the final approval step, but it does not close the identity-claim / queue-poisoning part of #4911. At minimum, please do not mark #4911 closed unless registration ownership/CSRF/rate-limit coverage is added or the issue is narrowed.

Validation performed:

python3 -m py_compile contributor_registry.py payout_ledger.py bounties/issue-2285/src/memory_routes.py node/machine_passport_api.py
# passed

git diff --check origin/main...HEAD
# failed: trailing whitespace in contributor_registry.py and node/machine_passport_api.py

python3 tools/bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check: OK

Recommended fix: remove query-parameter admin auth, add focused Flask client regressions for missing/invalid/valid headers on the four protected mutating routes, decide whether payout ledger reads are admin-only or redacted, and either implement GitHub ownership/CSRF/rate-limit protection for registration or stop closing #4911 from this PR.

[508704820]

Code Review: Fix authentication bypass when ADMIN_KEY unset

Summary

Fixes the default-allow pattern in memory_routes and contributor_registry (same bugs as our #4882 and #4912).

Security Issue Found

The admin check uses timing-unsafe comparison:

if req_key != ADMIN_KEY:  # ← TIMING-UNSAFE!
    abort(401)

This should be hmac.compare_digest(req_key, ADMIN_KEY) instead. Direct string comparison (!=) enables timing side-channel attacks where an attacker can determine the key character-by-character by measuring response time differences.

This is the same class of bug as #5000 (bridge API timing attack).

Positive

1. Decorator pattern — @admin_required is clean and reusable
2. Default-deny — aborts when key not configured
3. Applied to both modules — memory_routes and contributor_registry

Suggestion

Replace req_key != ADMIN_KEY with:

import hmac
if not hmac.compare_digest(req_key or "", ADMIN_KEY):
    abort(401)

Needs fix before merge — timing-unsafe comparison is a security vulnerability in a security fix.

Review quality: Security-focused review (20-25 RTC)

[loganoe]

I agree this needs changes. In addition to the existing security review concerns, the affected test suites are red because the PR changes auth behavior without updating the corresponding expectations/setup.

Findings:

• bounties/issue-2285/tests/test_memory_routes.py::MemoryRoutesTestCase::test_clear_memory still calls DELETE /api/memory/clear?agent_id=test-agent without an admin header and now gets 403 instead of the expected 200. This PR should add missing/wrong/correct admin-key route coverage for the destructive clear endpoint.
• tests/test_contributor_registry.py::TestRegisterRoute::test_register_duplicate_username posts RTC0dup, which the new wallet validation rejects before duplicate handling, so the test no longer verifies duplicates. TestApproveRoute::test_approve_pending_contributor also still assumes unauthenticated approval succeeds.
• node/tests/test_machine_passport.py now has three failures: create-without-admin expects 201 but gets 401, and the update auth tests get 404 because the setup POST no longer creates the passport without an admin key. The tests need to seed/create passports under the new default-deny contract before testing updates.
• git diff --check origin/main...HEAD fails on trailing whitespace in contributor_registry.py and node/machine_passport_api.py.

Validation run:

• PYTHONPATH=bounties/issue-2285/src ... pytest bounties/issue-2285/tests/test_memory_routes.py -q -> 1 failed, 29 passed, 11 subtests passed
• ... pytest tests/test_contributor_registry.py tests/test_payout_ledger_migration.py tests/test_machine_passport_event_json_validation.py node/tests/test_machine_passport.py -q -> 5 failed, 40 passed
• python3 -m py_compile contributor_registry.py payout_ledger.py bounties/issue-2285/src/memory_routes.py node/machine_passport_api.py -> passed
• uv run --no-project --with ruff ruff check contributor_registry.py payout_ledger.py bounties/issue-2285/src/memory_routes.py node/machine_passport_api.py --select E9,F821,F811,F841 --output-format=concise -> passed
• python3 tools/bcos_spdx_check.py --base-ref origin/main -> OK

Please update the affected tests for the new auth contract and clean up the diff-check failures before merge.

[TJCurnutte]

I’m requesting changes because the new admin guards still accept the admin secret in the URL on several mutating routes.

Validation I ran:

• git diff --check origin/main...HEAD -- bounties/issue-2285/src/memory_routes.py contributor_registry.py node/machine_passport_api.py payout_ledger.py failed on trailing whitespace at contributor_registry.py:162, node/machine_passport_api.py:212, and node/machine_passport_api.py:313.
• python3 -B -m py_compile bounties/issue-2285/src/memory_routes.py contributor_registry.py node/machine_passport_api.py payout_ledger.py passed.
• Focused Flask probe against payout_ledger.register_ledger_routes() with ADMIN_KEY=sekrit:
  • POST /api/ledger with no key -> 401
  • POST /api/ledger?admin_key=sekrit -> 201 {'status': 'queued'}
  • POST /api/ledger with X-Admin-Key: sekrit -> 201 {'status': 'queued'}
• Static check found request.args.get("admin_key") in bounties/issue-2285/src/memory_routes.py, contributor_registry.py, and payout_ledger.py; those guards use plain !=. node/machine_passport_api.py is the only touched file here using hmac.compare_digest and header-only admin auth.

The patch does close the fully unauthenticated path when ADMIN_KEY is unset, but ?admin_key= on POST/PATCH/DELETE/admin routes still exposes the admin credential through access logs, browser history, proxy logs, and referrers. Please remove the query-string fallback, require a header for the secret, and use constant-time comparison across the new admin guards before merging.

[himanalot]

Requesting changes.

The machine-passport default-deny change is good, but the new admin_required wrappers added in this PR accept request.args.get("admin_key") and compare with req_key != ADMIN_KEY. That puts admin secrets in URLs/logs/referrers and browser history, and the new comparisons are not constant-time.

This affects the newly protected memory clear route and the unrelated contributor/payout mutations included in the same PR. Please make the new admin checks header-only and use hmac.compare_digest (or reuse an existing shared admin-auth helper) before merging.

[Scottcjn]

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

[HCIE2054]

LGTM! Thanks for contributing!