fix: disable Flask debug entrypoints#5531
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! |
github-actions
Bot
added
documentation
Code ReviewReviewed fix: disable Flask debug entrypoints by @ZacharyZhang-NY. This is a bug fix. The changes appear reasonable based on the diff. Wallet: |
2balmprune
left a comment
2balmprune
left a comment
There was a problem hiding this comment.
Reviewed head 1d9e47f895f1a1d04477473bde1f9ee3a1559533 for the Flask debug-mode hardening.\n\nThe change disables debug mode at all four listed public entrypoints, preserves the faucet CLI flag as a compatibility no-op, updates the faucet docs so they no longer recommend debug startup, and adds a focused AST regression test for debug=True / debug-config assignments in those entrypoints. I also checked the faucet path where config is loaded from file or CLI: config['server']['debug'] = False is applied after overrides and before app.run(..., debug=debug), so a local config or deprecated --debug flag does not re-enable Flask debug mode.\n\nValidation performed:\n- git diff --check origin/main...review-pr-5531 -- keeper_explorer.py contributor_registry.py bridge/bridge_api.py faucet_service/faucet_service.py faucet_service/README.md faucet_service/IMPLEMENTATION_SUMMARY.md tests/test_flask_debug_disabled.py\n- python -B -m py_compile keeper_explorer.py contributor_registry.py bridge/bridge_api.py faucet_service/faucet_service.py tests/test_flask_debug_disabled.py\n- Imported tests/test_flask_debug_disabled.py and executed test_public_flask_entrypoints_do_not_enable_debug_mode() directly, since pytest was not installed in this local interpreter.\n\nNo blocker found. Approved.
🔒 Code Review: PR #5531 - Disable Flask Debug Mode in Production Entrypoints📋 SummaryThis PR disables Flask debug mode across 4 public service entrypoints, fixing a critical security issue (#5059). Debug mode in production can expose sensitive information, enable code execution, and cause performance issues. ✅ Strengths
💡 Suggestions for Improvement
🎯 Security Assessment
📊 Review Details
🏆 Overall AssessmentQuality: Security-focused review Why Security Bonus?
✅ VerdictAPPROVE - This is an essential security fix. Flask debug mode should NEVER be enabled in production. The implementation is thorough, well-tested, and includes documentation updates. Review #2 completed by Herr Amano for Rustchain Bounties Program |
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
I reviewed head 1d9e47f895f1a1d04477473bde1f9ee3a1559533.
The patch disables Flask debug mode in the four public service entrypoints listed in the PR, keeps the faucet --debug flag as a compatibility no-op, and updates the faucet docs so they no longer recommend starting the public service with Flask debug enabled. The added AST regression test is focused on the right failure mode: it catches both direct app.run(..., debug=True) usage and assignments that set a debug config key to True.
Validation run in a clean temporary worktree based on origin/main with the PR patch applied:
git diff --check --cached
passed
python -B -m py_compile keeper_explorer.py contributor_registry.py bridge/bridge_api.py faucet_service/faucet_service.py tests/test_flask_debug_disabled.py
passed
python -B -m pytest -q tests/test_flask_debug_disabled.py --noconftest
1 passed, 1 warning
python tools/bcos_spdx_check.py --base-ref origin/main
BCOS SPDX check: OK
The pytest warning is the existing keeper_explorer.py invalid escape warning from the unchanged HTML banner. I did not find a blocker.
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Reviewed the Flask debug hardening path and this looks mergeable to me.
What I validated:
git diff --check origin/main...HEAD -- bridge/bridge_api.py contributor_registry.py faucet_service/IMPLEMENTATION_SUMMARY.md faucet_service/README.md faucet_service/faucet_service.py keeper_explorer.py tests/test_flask_debug_disabled.py
python3 -B -m py_compile bridge/bridge_api.py contributor_registry.py faucet_service/faucet_service.py keeper_explorer.py tests/test_flask_debug_disabled.py
PYTHONPATH=. uv run --no-project --with pytest --with flask --with pyyaml python -B -m pytest -q tests/test_flask_debug_disabled.py --noconftestProof from the disposable worktree at head 1d9e47f895f1a1d04477473bde1f9ee3a1559533:
git diff --checkpassed for the changed files.py_compilepassed for the changed Python entrypoints and the new regression test.- Focused pytest passed:
1 passed, 1 warning in 0.05s. - The extra AST/source probe returned
debug_true_locations []and confirmed the faucet--debugflag is now documented as deprecated whileconfig['server']['debug'] = Falseis forced before startup.
Reasoning: the public app.run(..., debug=True) entrypoints in bridge_api.py, contributor_registry.py, and keeper_explorer.py are now hard-disabled, and the faucet CLI/config path no longer allows a command-line debug override to put Flask back into debugger mode. The regression test is narrow but useful: it will catch reintroducing literal debug=True calls or direct debug-config True assignments in the listed public entrypoints.
Approved.
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
Disable Flask debug entrypoints. CRITICAL security fix - Flask debug mode exposes interactive Python console and Werkzeug debugger, allowing RCE. Verify: (1) Debug mode is disabled in ALL environments, not just production. (2) No other debug endpoints remain enabled. (3) Consider adding a CI check that asserts DEBUG=False in all config files. - Xeophon (security review)
|
Security Review ✅ CRITICAL FIX Disabling Flask debug=True across all entry points. Debug mode enables the Werkzeug interactive debugger, which allows arbitrary code execution via the browser. This is a well-known RCE vector in production. Changes cover 5 files: bridge_api.py, contributor_registry.py, faucet_service.py, keeper_explorer.py, and removes --debug CLI flag. Test uses AST analysis to verify no debug=True remains. Reviewed by Xeophon - Solana: Lt9nERv6VHsojw15LpFeiaabuphAggzfLF9sM9UXRrZ |
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
Reviewing this as part of the RustChain code review bounty.
This PR is incomplete for #4810. The issue lists several public Flask entrypoints with debug=True, but the patch only changes bridge/bridge_api.py, contributor_registry.py, keeper_explorer.py, and the faucet service docs/config path.
Two affected files from the report are still not covered by the new AST regression list and remain vulnerable on current main: explorer/app.py and security_test_payment_widget.py. Both have app.run(... host='0.0.0.0' ..., debug=True) patterns and should either be fixed in this PR or the PR body should narrow the scope so it does not claim to resolve #4810.
Please add those entrypoints to ENTRYPOINTS and disable their hard-coded debug mode as well; otherwise the regression test can pass while part of the reported security surface remains open.
|
Follow-up after the later scope review: my earlier approval validated the entrypoints and regression coverage touched by this PR, but I agree that if the PR is meant to fully resolve issue #4810 then the remaining reported Flask debug entrypoints ( |
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
10 participants
Summary
--debugflag as a deprecated compatibility no-op and force Flask debug mode off after config loading.debug=Trueor assign debug config toTrue.Covered entrypoints
keeper_explorer.pycontributor_registry.pybridge/bridge_api.pyfaucet_service/faucet_service.pyTests
rg -n "debug\\s*=\\s*True|\\['debug'\\]\\s*=\\s*True" keeper_explorer.py contributor_registry.py bridge/bridge_api.py faucet_service/faucet_service.py-> no matchesgit diff --check -- keeper_explorer.py contributor_registry.py bridge/bridge_api.py faucet_service/faucet_service.py faucet_service/README.md faucet_service/IMPLEMENTATION_SUMMARY.md tests/test_flask_debug_disabled.pypython -B -m py_compile keeper_explorer.py contributor_registry.py bridge/bridge_api.py faucet_service/faucet_service.py tests/test_flask_debug_disabled.pypython -B -m pytest -q tests/test_flask_debug_disabled.py --noconftest->1 passedpython tools\bcos_spdx_check.py --base-ref origin/main->BCOS SPDX check: OKNote:
py_compilestill emits the existingkeeper_explorer.pyinvalid escape SyntaxWarning from the unchanged HTML banner; the command exits 0.Fixes #5059