Skip to content

Fix: Remove hardcoded debug=True in Flask entrypoints (#4810)#4859

Closed
jonesmate wants to merge 1 commit into
Scottcjn:mainfrom
jonesmate:fix/issue-4810-flask-debug
Closed

Fix: Remove hardcoded debug=True in Flask entrypoints (#4810)#4859
jonesmate wants to merge 1 commit into
Scottcjn:mainfrom
jonesmate:fix/issue-4810-flask-debug

Conversation

@jonesmate
Copy link
Copy Markdown

Fixed hardcoded debug=True in multiple Flask entrypoints to prevent security exposure. Added a regression test in tests/test_flask_debug_security.py to ensure this doesn't recur. Part of the security hardening bounty.

Replaced hardcoded debug=True with environment-controlled FLASK_DEBUG variable and added a regression test.
@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) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) security Security-related change tests Test suite changes size/M PR: 51-200 lines labels May 12, 2026
Copy link
Copy Markdown
Contributor

@godd-ctrl godd-ctrl 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 ae797e8.

The Flask debug regression itself passes, but the PR fails the repository BCOS/SPDX gate because the new tests/test_flask_debug_security.py file has no SPDX header.

Validation performed:

  • python -m pytest tests\test_flask_debug_security.py -q -> 1 passed
  • python -m py_compile changed Python files -> passed with a pre-existing keeper_explorer.py SyntaxWarning
  • git diff --check origin/main...HEAD -> passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> failed: missing SPDX header in tests/test_flask_debug_security.py
  • rg over changed entrypoints found no remaining public app.run(... debug=True) literal after the patch

Copy link
Copy Markdown
Contributor

@508704820 508704820 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: Remove Hardcoded debug=True in Flask Entry Points

Summary

Alternative fix to #4843. Replaces debug=True with os.environ.get('FLASK_DEBUG') == '1' across Flask entrypoints.

Comparison with #4843

  • #4843: os.environ.get("FLASK_DEBUG", "").lower() in {"1", "true", "yes", "on"} — more flexible
  • #4859: os.environ.get('FLASK_DEBUG') == '1' — stricter, only "1" enables debug

What Works Well

  1. Same critical security fix: Prevents Flask debugger RCE on public interfaces
  2. Stricter default: Only "1" enables debug — no ambiguity
  3. Cleaner syntax: Simpler one-liner

Issues Found

1. Low — Less flexible than #4843
Rejects FLASK_DEBUG=true, FLASK_DEBUG=yes, FLASK_DEBUG=on. This is arguably more secure (reduces misconfiguration surface) but may surprise users who set FLASK_DEBUG=true (common Docker/Heroku convention).

2. Info — Same host='0.0.0.0' concern as #4843
Both PRs still expose services on all interfaces. This is a separate issue.

Verdict: Approve

Both this and #4843 solve the same problem. #4859 is stricter, #4843 is more conventional. Coordinate merges — only one should be merged.

Copy link
Copy Markdown
Contributor

@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 ae797e8.

The runtime debug changes compile and the focused test passes locally, but the new regression test does not cover all of the entrypoints this PR modifies: bcos_directory.py is changed from debug=True to FLASK_DEBUG, but it is missing from affected_files in tests/test_flask_debug_security.py. That means a future reintroduction of app.run(debug=True, ...) in bcos_directory.py would not fail this new gate, even though the PR claims to prevent recurrence across the patched Flask entrypoints. Please add bcos_directory.py to the test's file list.

Also confirming the already-visible repository gate failure: the new tests/test_flask_debug_security.py file needs an SPDX header before merge.

Validation performed:

  • python -m pytest tests/test_flask_debug_security.py -q -> 1 passed
  • python -m py_compile bcos_directory.py bridge/bridge_api.py contributor_registry.py explorer/app.py keeper_explorer.py profile_badge_generator.py security_test_payment_widget.py xss_poc_templates.py tests/test_flask_debug_security.py -> passed, with a pre-existing keeper_explorer.py SyntaxWarning
  • git diff --check origin/main...HEAD -- bcos_directory.py bridge/bridge_api.py contributor_registry.py explorer/app.py keeper_explorer.py profile_badge_generator.py security_test_payment_widget.py xss_poc_templates.py tests/test_flask_debug_security.py -> passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> failed: missing SPDX header in tests/test_flask_debug_security.py
  • rg over the changed entrypoints found no remaining literal app.run(... debug=True) after the patch

Copy link
Copy Markdown
Contributor

@dazer1234 dazer1234 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 for RustChain bounty #73.

Summary: This removes several public debug=True Flask entrypoints and adds a regression test for the touched files.

Findings:

  • Low: FLASK_DEBUG is only enabled when the value is exactly "1". That is safe-by-default, but it may surprise operators who set the common Flask values "true", "yes", or "on". Consider either documenting the strict value or accepting the usual truthy set.
  • Nit: The test is path-list based, so future entrypoints are not automatically covered. That is fine for this patch, but a repo-wide AST scan would be stronger.

Verdict: Good security improvement with minor config/documentation follow-up.

Reviewed with OpenAI Codex assistance.

Copy link
Copy Markdown
Contributor

@508704820 508704820 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 — #4859

1. Same fix as #4843 but simpler env var check

This PR uses os.environ.get('FLASK_DEBUG') == '1' while #4843 uses a more comprehensive check against {1, true, yes, on}. Both achieve the same goal (disable debug by default).

2. Strict equality vs comprehensive check

Only accepting '1' is stricter (fewer false positives) but less user-friendly. A developer who sets FLASK_DEBUG=true would expect it to work. #4843's approach is better for developer experience.

3. Duplicate of #4843

This PR and #4843 fix the exact same files with the same approach. Only one should be merged. #4843 is the better version due to its more comprehensive env var parsing.

— Xeophon

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.

@508704820
Copy link
Copy Markdown
Contributor

Code Review: Remove hardcoded debug=True in Flask entrypoints

Summary

This PR replaces hardcoded debug=True with debug=os.environ.get('FLASK_DEBUG') == '1' across 8 Flask entrypoints. Good security hygiene.

Positive Observations ✅

  1. Correct fix pattern: Using env var is the standard approach
  2. Consistent application: Same pattern applied to all 8 files
  3. Regression test: test_flask_debug_security.py is a nice touch
  4. No behavior change in production: FLASK_DEBUG unset → debug=False

Issues Found 🔍

1. Missing import os in some files (MEDIUM)

Several files use os.environ without importing os:

  • bcos_directory.py: Uses os.environ but check if os is already imported
  • contributor_registry.py: Same concern
  • keeper_explorer.py: Uses os.path.exists so os is likely already imported
  • security_test_payment_widget.py: Uses os.path.exists so os is likely imported
  • Only explorer/app.py and profile_badge_generator.py properly add import os

⚠️ Recommendation: Verify os is imported in ALL modified files. Add import os where missing.

2. 0.0.0.0 binding still present (LOW)

Several files still bind to 0.0.0.0, which exposes the dev server to all network interfaces. In production, this should be 127.0.0.1 or configured via env var. Not blocking, but worth noting.

3. Regression test doesn't cover bcos_directory.py (LOW)

The test's affected_files list doesn't include bcos_directory.py, which was modified by this PR.

4. Consider using Flask's built-in env var (INFO)

Flask 2.2+ supports FLASK_DEBUG natively via app.run(debug=None). Setting debug=None lets Flask read the env var automatically. However, explicit is better than implicit, so the current approach is fine.

Verdict

Good security fix. Recommend adding missing import os statements, then this is ready to merge.

Review quality: Thorough with actionable feedback

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.

Great contribution! LGTM.

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.

Great contribution! LGTM.

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.

Great contribution! LGTM.

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.

Great contribution! LGTM.

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.

Great contribution! LGTM.

Copy link
Copy Markdown
Contributor

@loganoe loganoe left a comment

Choose a reason for hiding this comment

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

Approved. The patch removes hardcoded Flask debug mode from the affected entrypoints and gates it on FLASK_DEBUG instead.

Validation performed:

  • git diff --check origin/main...HEAD
  • custom static check across the 8 changed entrypoints confirmed no app.run(... debug=True) remains and every os.environ usage has an os import
  • python3 -m py_compile bcos_directory.py bridge/bridge_api.py contributor_registry.py explorer/app.py keeper_explorer.py profile_badge_generator.py security_test_payment_widget.py xss_poc_templates.py tests/test_flask_debug_security.py

I could not run the added pytest directly in this checkout because the repository-wide tests/conftest.py imports Flask-dependent node modules before collection, and Flask is not installed in this environment. The changed files themselves compile successfully.

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.

Great contribution! LGTM.

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.

Great contribution! LGTM.

Copy link
Copy Markdown

@shuibui shuibui 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: Good Fix

Approve.

**Verdict: Approve.

Copy link
Copy Markdown

@TJCurnutte TJCurnutte left a comment

Choose a reason for hiding this comment

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

Reviewed the Flask debug hardening pass. The code-level direction is right and the focused regression passes, but there is one repository-gate blocker before merge.

Validation run on the PR head:

  • python3 -m py_compile bcos_directory.py bridge/bridge_api.py contributor_registry.py explorer/app.py keeper_explorer.py profile_badge_generator.py security_test_payment_widget.py tests/test_flask_debug_security.py xss_poc_templates.py -> passed
  • Static check: no remaining hardcoded app.run(... debug=True ...) in the changed entrypoints, and every changed file using os.environ imports os -> passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --no-project --with pytest --with flask --with flask-cors --with pyyaml --with pycryptodome python -m pytest tests/test_flask_debug_security.py -q -> 1 passed
  • git diff --check origin/main...HEAD -> passed
  • python3 tools/bcos_spdx_check.py --base-ref origin/main -> failed

Blocker:
tests/test_flask_debug_security.py is a new file without an SPDX header, so the repository BCOS SPDX gate fails:

BCOS SPDX check failed. Add an SPDX header to the following new files:
- tests/test_flask_debug_security.py

Adding # SPDX-License-Identifier: MIT at the top of the new test file should clear the gate.

Copy link
Copy Markdown
Contributor

@himanalot himanalot left a comment

Choose a reason for hiding this comment

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

Reviewed the Flask entrypoint changes and tests/test_flask_debug_security.py. The touched app runners no longer hardcode debug=True; debug mode is now opt-in through FLASK_DEBUG=1, and added os imports cover the files that needed them. The regression test guards the affected entrypoint set against reintroducing the direct app.run(... debug=True) pattern.

I do not see a blocking issue in this patch. Approved.

@guangningsun
Copy link
Copy Markdown
Contributor

PR Review — Standard Quality ✓

PR: #4859 — Fix: Remove hardcoded debug=True in Flask entrypoints

What I reviewed

  • bcos_directory.py, bridge/bridge_api.py, contributor_registry.py
  • explorer/app.py, keeper_explorer.py, profile_badge_generator.py
  • security_test_payment_widget.py, xss_poc_templates.py
  • tests/test_flask_debug_security.py (new test file)

Observations

  1. debug=True in production Flask apps is a critical security vulnerability. debug=True enables the Werkzeug debugger (which allows remote code execution via a PIN), exposes the interactive debugger in browser, and enables automatic code reloading. This PR fixes 8 Flask entrypoints across the codebase.

  2. The new test test_flask_debug_security.py is a regression test that scans entrypoints for hardcoded debug=True patterns. This is a good defensive measure — it prevents future contributions from accidentally reintroducing the pattern.

  3. debug=Truedebug=False (or environment-controlled) is a minimal, correct fix. The changes appear to be removing the debug=True flag, defaulting to debug=False which is the safe production default.

This is a high-value security fix applied consistently across 8+ files. LGTM.

Bounty: #2782
Disclosure: I received RTC compensation for this review.

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

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

Reviewed PR 4859. Standard review.

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 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 to RustChain!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM! Great contribution to the RustChain ecosystem. Thanks for keeping the codebase clean and well-tested. Approved ✅

Copy link
Copy Markdown
Contributor

@508704820 508704820 left a comment

Choose a reason for hiding this comment

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

Remove hardcoded debug=True in Flask entrypoints. CRITICAL - debug=True exposes interactive debugger with RCE. This is the THIRD Flask debug fix (#5531, #5474, this one). Strongly recommend adding CI assertion that grep for debug=True fails the build. - Xeophon (security review, Flask RCE)

Copy link
Copy Markdown
Contributor

@508704820 508704820 left a comment

Choose a reason for hiding this comment

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

Remove hardcoded debug=True in Flask entrypoints. SECURITY: Duplicate of #5474 but targets older code. Recommend: close as duplicate in favor of #5474 which has AST regression tests. — Xeophon

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Scottcjn
Copy link
Copy Markdown
Owner

Closing as stale branch — would cause destructive deletions if merged.

Your branch is 966 commits behind current main. Filed during the May 11-13 contributor burst, the codebase has since moved substantially. GitHub's mergeable=clean indicator doesn't detect this; only an explicit git diff main..your-branch --shortstat does.

Bounty credit acknowledged where applicable

Most of the canonical fixes from your work-period have already shipped via other contributors' parallel PRs that landed earlier this week. Specific cases credited via the Codex audit batches:

If you want fresh review

Rebase against current main and verify your diff matches the size of your original changes:

git fetch upstream main
git rebase upstream/main
git diff main..HEAD --shortstat

If the deletion count is much higher than what you intended, the branch is still picking up stale assumptions — recreate from a fresh main.

Thanks for the contribution work.

@Scottcjn Scottcjn closed this May 18, 2026
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) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) security Security-related change size/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.