Skip to content

Security: Claim ID Collision Protection#2884

Closed
MichaelSovereign wants to merge 23 commits intoScottcjn:mainfrom
MichaelSovereign:security-fix/claims-id-integrity
Closed

Security: Claim ID Collision Protection#2884
MichaelSovereign wants to merge 23 commits intoScottcjn:mainfrom
MichaelSovereign:security-fix/claims-id-integrity

Conversation

@MichaelSovereign
Copy link
Copy Markdown
Contributor

Summary

This PR improves the uniqueness and integrity of claim identifiers in the module.

Changes

  1. Disambiguated Identifiers: Updated to use a more structured and prefixed format (). This prevents potential ID collisions that could occur with the previous underscore-only format if miner IDs contained underscores.
  2. Standardization: Ensures all claim IDs follow a strict, easy-to-parse schema.

Closes #6460

@MichaelSovereign MichaelSovereign requested a review from Scottcjn as a code owner May 2, 2026 05:20
@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/XL PR: 500+ lines labels May 2, 2026
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.

Security Review: Claim ID Collision Protection

Summary

This PR improves the uniqueness and integrity of claim identifiers across multiple modules. The changes are well-structured and address potential collision vulnerabilities.

Key Improvements

  1. Claim ID Disambiguation (claims_submission.py)

    • Changed format from claim_{epoch}_{miner_id} to claim:{epoch}:{miner_id}
    • Using colons as separators prevents collision when miner IDs contain underscores
    • Good defensive design choice
  2. Security Headers (server_proxy.py)

    • Added endpoint whitelist to prevent SSRF
    • Security headers added to all proxied responses
    • Proper defense-in-depth approach
  3. Commitment Verification (bcos_routes.py)

    • Added crucial security check to verify commitment matches report
    • Prevents potential manipulation of attestation data
    • Critical fix for trust chain integrity
  4. P2P Signature Replay Protection (rustchain_p2p_sync_secure.py)

    • Added signature tracking with expiry
    • Prevents replay attacks on P2P messages
    • Good cryptographic hygiene
  5. Input Validation (x402_config.py, hall_of_rust.py)

    • Added EVM address format validation
    • Strict input validation for eulogy/nickname endpoints
    • Prevents injection attacks
  6. Transaction Amount Integrity (payout_preflight.py, utxo_db.py)

    • Added Decimal precision handling
    • Type confusion guards for mining rewards
    • Prevents floating-point precision attacks

Testing

  • New test file added: test_handle_get_state_arity.py
  • Tests cover edge cases in state handling

Recommendations

  1. Consider adding unit tests for the new claim ID format
  2. The signature replay protection could benefit from configurable expiry times
  3. Documentation for the new security headers would be helpful

Verdict

APPROVE

This is a well-executed security hardening PR. The changes follow defense-in-depth principles and address real vulnerabilities. The code quality is high and the scope is appropriate.

Reviewer: @jaxint
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

@Scottcjn Scottcjn closed this May 3, 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) node Node server related size/XL PR: 500+ lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants