fix: fail closed integrated admin routes#4684
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
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! |
saim256
left a comment
There was a problem hiding this comment.
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:
- 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
- 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.dbinTemporaryDirectory.cleanup()withPermissionError: [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.
strongkeep-debug
left a comment
There was a problem hiding this comment.
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.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
|
Addressed the pending-admin review by routing Validation run:
|
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
saim256
left a comment
There was a problem hiding this comment.
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:
-
node/tests/test_integrated_admin_fail_closed.pyis still missing the required SPDX header. The repository BCOS SPDX gate fails on the new test file. -
The new focused test file still fails on Windows teardown because
admin_fail_closed.dbremains open whenTemporaryDirectory.cleanup()runs. All three test cases fail withPermissionError: [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 teardownPermissionErrordeletingadmin_fail_closed.dbpython -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py node\tests\test_integrated_admin_fail_closed.py-> passedgit diff --check origin/main...review-pr4684 -- node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_integrated_admin_fail_closed.py-> passedpython tools\bcos_spdx_check.py --base-ref origin/main-> failed: missing SPDX header innode/tests/test_integrated_admin_fail_closed.py- Static check confirms
/wallet/transfer,/pending/list,/pending/void,/pending/confirm, and/pending/integritynow useis_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.
jaxint
left a comment
There was a problem hiding this comment.
Code review: LGTM! Thanks for contributing to RustChain. Approved.
jaxint
left a comment
There was a problem hiding this comment.
Code review: LGTM! Thanks for contributing to RustChain. Approved.
jaxint
left a comment
There was a problem hiding this comment.
Code review: LGTM! Thanks for contributing to RustChain. Approved.
jaxint
left a comment
There was a problem hiding this comment.
Code review: LGTM! Thanks for contributing to RustChain. Approved.
jaxint
left a comment
There was a problem hiding this comment.
Code review: LGTM! Thanks for contributing to RustChain. Approved.
|
Addressed the remaining repo-gate blockers on the PR branch. Updates:
Validation rerun locally on the updated head |
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
saim256
left a comment
There was a problem hiding this comment.
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 deletingimport.dbpython -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py node\tests\test_integrated_admin_fail_closed.py-> passedgit diff --check origin/main...HEAD -- node\rustchain_v2_integrated_v2.2.1_rip200.py node\tests\test_integrated_admin_fail_closed.py-> passedpython tools\bcos_spdx_check.py --base-ref origin/main-> OK
Summary
is_admin(request)helperRC_ADMIN_KEYplus an empty admin header from passinghmac.compare_digest("", "")/wallet/transferand/pending/confirmwhen the runtime admin key is absentFixes #4588.
Validation
python3 -m pytest node/tests/test_integrated_admin_fail_closed.py -q-> 2 passedpython3 -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_integrated_admin_fail_closed.pygit diff --check HEAD~1..HEADWallet/address: RTC6a5325fd2708469d4625ad15e70a807b127846bc