Skip to content

fix: add silent=True and validation to /epoch/enroll JSON parsing#4817

Open
BossChaos wants to merge 1 commit into
Scottcjn:mainfrom
BossChaos:bugfix/enroll-input-validation
Open

fix: add silent=True and validation to /epoch/enroll JSON parsing#4817
BossChaos wants to merge 1 commit into
Scottcjn:mainfrom
BossChaos:bugfix/enroll-input-validation

Conversation

@BossChaos
Copy link
Copy Markdown
Contributor

Security Fix

The /epoch/enroll endpoint used request.get_json() without silent=True, causing a 400 BadRequest on non-JSON Content-Type headers.
Additionally, there was no isinstance check to validate the parsed data is a dict before calling .get() on it.

Impact

  • Sending Content-Type: text/plain with POST /epoch/enroll causes 400
  • Sending a JSON array [] crashes with AttributeError on .get()

Fix

  • Added silent=True to request.get_json()
  • Added isinstance(data, dict) check with proper 400 response

Testing

Verified that the endpoint now returns a proper 400 error for invalid Content-Types and JSON arrays instead of raising unhandled exceptions.


RTC Wallet: RTC6d1f27d28961279f1034d9561c2403697eb55602

SECURITY: The /epoch/enroll endpoint used request.get_json() without
silent=True, causing a 400 BadRequest on non-JSON Content-Type headers.
Additionally, there was no isinstance check to validate the parsed data
is a dict before calling .get() on it.

Impact:
- Sending Content-Type: text/plain with POST /epoch/enroll causes 400
- Sending a JSON array [] crashes with AttributeError on .get()

Fix:
- Added silent=True to request.get_json()
- Added isinstance(data, dict) check with proper 400 response
@BossChaos BossChaos requested a review from Scottcjn as a code owner May 12, 2026 13:02
@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/XS PR: 1-10 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.

Approved current head 7206822.

This is a narrow, correct hardening for /epoch/enroll: the route now uses request.get_json(silent=True) and rejects non-dict payloads before calling .get(), so malformed/non-object JSON no longer reaches the field-access path.

Validation performed locally:

  • git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py
  • python -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py
  • python tools\bcos_spdx_check.py --base-ref origin/main
  • manual line/diff review around /epoch/enroll

Caveats:

  • This overlaps with open PR #4808, which applies the same pattern to /epoch/enroll plus several other JSON parsing routes, so maintainers should merge only one compatible version of this change.
  • python -m ruff check node\rustchain_v2_integrated_v2.2.1_rip200.py --select E9,F821,F811 --output-format=concise still reports pre-existing issues already present on origin/main in this large integrated file (json/send_file redefinitions plus undefined log, miners, and status). This PR does not introduce those findings.

No production node or external endpoint was exercised.

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.

Approved current head 7206822.

I checked the changed /epoch/enroll path with a Flask test client. The new silent=True plus dict guard returns the intended 400 {"error": "Invalid JSON body"} for wrong content type, JSON arrays, and malformed JSON before the later enrollment, signature, or attestation-gate logic runs. The touched hunk is narrow and leaves valid dict payload handling on the existing path.

Validation run locally:

  • python -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py
  • git diff --check origin/main...HEAD -- node\rustchain_v2_integrated_v2.2.1_rip200.py
  • python tools\bcos_spdx_check.py --base-ref origin/main
  • Flask test-client smoke for text/plain, JSON array [], and malformed JSON bodies; all returned the same 400 JSON body.
  • uv run --no-project --with ruff python -m ruff check node\rustchain_v2_integrated_v2.2.1_rip200.py --select E9,F821,F811 --output-format=concise still reports the pre-existing large-file issues outside this diff (json/send_file redefinitions and undefined log/miners/status).

Caveat: this overlaps with #4808, so maintainers should merge only one compatible JSON-hardening version. No live node or external endpoint was exercised.

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.

Approved current head 7206822 after reviewing the /epoch/enroll JSON parsing hardening. The route now calls request.get_json(silent=True) and rejects non-dict payloads before any .get() calls, so malformed JSON, wrong content types, and JSON arrays stay on a controlled 400 Invalid JSON body path instead of reaching enrollment logic. Validation: python -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py -> passed; git diff --check origin/main...HEAD -- node\rustchain_v2_integrated_v2.2.1_rip200.py -> passed; python tools\bcos_spdx_check.py --base-ref origin/main -> OK. Targeted Ruff E9/F821/F811 still reports the same pre-existing integrated-file findings outside this one-line route change. Caveat: this overlaps with #4808, so maintainers should merge only one compatible JSON-hardening version. No live node, wallet, or production endpoint was exercised.

Copy link
Copy Markdown

@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 endpoint now uses request.get_json(silent=True) and rejects non-dict parsed bodies before calling .get(), so both invalid content types and JSON arrays return controlled 400 responses instead of Flask/parser exceptions or AttributeError.

Validation performed: git diff --check origin/main...HEAD, python3 -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py, python3 tools/bcos_spdx_check.py --base-ref origin/main, and a Flask test-client smoke test for text/plain, JSON array, and empty-object /epoch/enroll requests.

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: Add silent=True and Validation to /epoch/enroll JSON Parsing

Adds silent=True to prevent HTTP 500 on malformed JSON, and isinstance(data, dict) validation to reject non-object payloads.

Approve — consistent with #4871 pattern.

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: Approve

Good fix.

**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.

Approved. The endpoint now handles malformed or non-object JSON explicitly instead of relying on Flask's default parse exception or crashing on .get().

Validation performed:

  • Reviewed the focused change in /epoch/enroll.
  • Confirmed request.get_json(silent=True) is used and a non-dict body now returns 400 {"error": "Invalid JSON body"} before any .get() calls.
  • Ran git diff --check origin/main...HEAD on the PR branch: passed.
  • Ran python3 -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py: passed.

This is a reasonable fail-closed input-validation fix for the enroll route.

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/XS PR: 1-10 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants