Skip to content

fix: fail closed integrated admin routes#4684

Open
yejingyan wants to merge 3 commits into
Scottcjn:mainfrom
yejingyan:fix/integrated-admin-fail-closed
Open

fix: fail closed integrated admin routes#4684
yejingyan wants to merge 3 commits into
Scottcjn:mainfrom
yejingyan:fix/integrated-admin-fail-closed

Conversation

@yejingyan
Copy link
Copy Markdown

Summary

  • routes the integrated-node transfer and pending-transfer admin endpoints through the existing fail-closed is_admin(request) helper
  • prevents missing RC_ADMIN_KEY plus an empty admin header from passing hmac.compare_digest("", "")
  • adds regression coverage for /wallet/transfer and /pending/confirm when the runtime admin key is absent

Fixes #4588.

Validation

  • python3 -m pytest node/tests/test_integrated_admin_fail_closed.py -q -> 2 passed
  • python3 -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_integrated_admin_fail_closed.py
  • git diff --check HEAD~1..HEAD

Wallet/address: RTC6a5325fd2708469d4625ad15e70a807b127846bc

@yejingyan yejingyan requested a review from Scottcjn as a code owner May 12, 2026 01:01
@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 BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related tests Test suite changes size/M PR: 51-200 lines labels May 12, 2026
Copy link
Copy Markdown

@saim256 saim256 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 on current head f0f5f5c5197d46fd99577dc006c17e940ad7bfd1.

The production direction is right: routing these integrated-node admin endpoints through the existing fail-closed is_admin(request) helper fixes the empty-header / missing-RC_ADMIN_KEY compare_digest("", "") issue for /wallet/transfer, /pending/list, /pending/void, and /pending/confirm.

Two repo-gate blockers need fixing before merge:

  1. The new regression file is missing the required SPDX header, so the repository BCOS SPDX gate fails:
python tools\bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check failed. Add an SPDX header to:
# - node/tests/test_integrated_admin_fail_closed.py
  1. The focused tests fail on Windows during teardown because the temporary SQLite DB remains open. The endpoint assertions reach the expected unauthorized behavior, but both test cases fail deleting admin_fail_closed.db in TemporaryDirectory.cleanup() with PermissionError: [WinError 32] The process cannot access the file because it is being used by another process.

Validation run locally:

python -m pytest node\tests\test_integrated_admin_fail_closed.py -q
# 2 failed
# both failures are teardown PermissionError deleting admin_fail_closed.db

python -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py node\tests\test_integrated_admin_fail_closed.py
# passed

git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_integrated_admin_fail_closed.py
# passed

python tools\bcos_spdx_check.py --base-ref origin/main
# failed: missing SPDX header on the new test file

Please add the SPDX header and make the test harness close/release any integrated-node SQLite handle before cleaning the temporary directory. Once those gates pass, I do not see a blocker in the endpoint auth change itself.

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.

Requesting changes because the fail-closed sweep misses one adjacent pending-admin route.

Area Evidence Expected change
/pending/integrity On current head f0f5f5c, with RC_ADMIN_KEY unset and no auth header, a local Flask smoke test returns 200 from /pending/integrity. The route still compares admin_key to os.environ.get("RC_ADMIN_KEY", ""), so empty input authenticates against an absent runtime key. Route this endpoint through is_admin(request) too, and add a regression that proves it returns 401 when the runtime admin key is absent.
New regression suite on Windows uv run --no-project --with pytest --with flask python -m pytest node\tests\test_integrated_admin_fail_closed.py -q returns 1 failed, 1 passed because admin_fail_closed.db remains locked during TemporaryDirectory.cleanup(). Make the new test file release the Flask/SQLite resources before temp-dir teardown so the claimed validation is reproducible on Windows.

Validation I ran locally:

uv run --no-project --with pytest --with flask python -m pytest node\tests\test_integrated_admin_fail_closed.py -q
1 failed, 1 passed

python -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py node\tests\test_integrated_admin_fail_closed.py
passed

git diff --check origin/main...HEAD
passed

inline Flask smoke, RC_ADMIN_KEY unset, no auth header, GET /pending/integrity
status 200

The production fix looks small: the same helper already used in this patch should cover /pending/integrity as well. After that, the regression tests should cover the missed route and run cleanly on Windows.

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.

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.

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.

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.

@yejingyan
Copy link
Copy Markdown
Author

Addressed the pending-admin review by routing /pending/integrity through the shared is_admin(request) fail-closed helper as well.

Validation run:

  • python3 -m pytest node/tests/test_integrated_admin_fail_closed.py -q -> 3 passed
  • python3 -m compileall -q node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_integrated_admin_fail_closed.py -> passed
  • git diff --check -> passed

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.

Copy link
Copy Markdown

@saim256 saim256 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 on current head 85684ef098b0c736e3e6a6c692a7a1331daa84dd.

The production route change now includes /pending/integrity, which addresses the missed-route blocker from the earlier review. However, two repo-gate blockers are still present on this head:

  1. node/tests/test_integrated_admin_fail_closed.py is still missing the required SPDX header. The repository BCOS SPDX gate fails on the new test file.

  2. The new focused test file still fails on Windows teardown because admin_fail_closed.db remains open when TemporaryDirectory.cleanup() runs. All three test cases fail with PermissionError: [WinError 32] The process cannot access the file because it is being used by another process. That means the claimed Windows cleanup fix is not reproducible in this environment.

Validation run locally:

  • python -m pytest node\tests\test_integrated_admin_fail_closed.py -q -> 3 failed, all from teardown PermissionError deleting admin_fail_closed.db
  • python -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py node\tests\test_integrated_admin_fail_closed.py -> passed
  • git diff --check origin/main...review-pr4684 -- node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_integrated_admin_fail_closed.py -> passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> failed: missing SPDX header in node/tests/test_integrated_admin_fail_closed.py
  • Static check confirms /wallet/transfer, /pending/list, /pending/void, /pending/confirm, and /pending/integrity now use is_admin(request) on this branch

Please add # SPDX-License-Identifier: MIT to the new test file and explicitly release/close the integrated app/database resources before temporary-directory cleanup on Windows. Once those gates pass, I do not see another blocker in the fail-closed route wiring itself.

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.

Code review: LGTM! Thanks for contributing to RustChain. Approved.

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.

Code review: LGTM! Thanks for contributing to RustChain. Approved.

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.

Code review: LGTM! Thanks for contributing to RustChain. Approved.

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.

Code review: LGTM! Thanks for contributing to RustChain. Approved.

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.

Code review: LGTM! Thanks for contributing to RustChain. Approved.

@yejingyan
Copy link
Copy Markdown
Author

Addressed the remaining repo-gate blockers on the PR branch.

Updates:

  • Added the required # SPDX-License-Identifier: MIT header to node/tests/test_integrated_admin_fail_closed.py.
  • Changed the focused regression test SQLite connections to use contextlib.closing(...), so the handles are explicitly closed before TemporaryDirectory.cleanup() runs. This targets the Windows PermissionError teardown blocker.

Validation rerun locally on the updated head 0b3d8f2:

python3 -m pytest node/tests/test_integrated_admin_fail_closed.py -q
# 3 passed in 0.71s

python3 -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_integrated_admin_fail_closed.py
# passed

git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_integrated_admin_fail_closed.py
# passed

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

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.

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.

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.

Copy link
Copy Markdown

@saim256 saim256 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 on current head 0b3d8f29b85c9e4280d48e81cfe718b229495f8e.

The production route wiring looks complete now: /wallet/transfer, /pending/list, /pending/void, /pending/confirm, and /pending/integrity all route through is_admin(request), and the new test file now has the SPDX header.

One Windows repo-gate blocker remains. The focused regression file still leaves the import-time SQLite database open, so TemporaryDirectory.cleanup() fails in tearDownClass when deleting import.db:

python -m pytest node\tests\test_integrated_admin_fail_closed.py -q
# 3 passed, 1 error
# PermissionError: [WinError 32] ... import.db is being used by another process

The per-test admin_fail_closed.db cleanup issue was fixed with contextlib.closing(...), but the class-level import DB created via RUSTCHAIN_DB_PATH is still locked by the imported integrated app/module. Please explicitly close/release the module-level SQLite resource or move the import DB out of a TemporaryDirectory that is deleted while the app still holds it.

Validation run locally:

  • python -m pytest node\tests\test_integrated_admin_fail_closed.py -q -> 3 passed, 1 teardown error deleting import.db
  • python -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py node\tests\test_integrated_admin_fail_closed.py -> passed
  • git diff --check origin/main...HEAD -- node\rustchain_v2_integrated_v2.2.1_rip200.py node\tests\test_integrated_admin_fail_closed.py -> passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> OK

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/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SECURITY] Integrated admin endpoints can authorize empty keys after runtime key loss

4 participants