Skip to content

fix(client): separate read/write auth in chain client (closes #6624)#7870

Open
lequangsang01 wants to merge 2 commits into
Scottcjn:mainfrom
lequangsang01:fix/bounty-6624
Open

fix(client): separate read/write auth in chain client (closes #6624)#7870
lequangsang01 wants to merge 2 commits into
Scottcjn:mainfrom
lequangsang01:fix/bounty-6624

Conversation

@lequangsang01

Copy link
Copy Markdown
Contributor

Summary

Closes #6624

Problem

Read-only operations (get_balance, get_miners, etc.) were using the same authenticated request path as write operations, unnecessarily sending admin credentials for public data.

Changes

  • Added _request_public() helper that makes HTTP requests WITHOUT the admin key header
  • Added _get_public_headers() method that never includes admin credentials
  • Added _get_public() convenience method for public GET requests
  • Refactored all read methods to use the public path:
    • health(), get_epoch(), get_miners(), get_wallet_balance()
    • get_wallet_history(), get_stats(), get_hall_of_fame()
    • get_miner_eligibility()
  • Write methods continue using the authenticated _request() / _get() path
  • Added admin_key parameter to __init__ (optional, defaults to None)

Security Principle

Follows principle of least privilege: read operations never send admin credentials, even if configured.

Testing

  • Added tests verifying _request_public does not include X-Admin-Key header
  • Added tests verifying authenticated requests do include admin key
  • Added tests verifying all read methods use public (no-auth) path
  • Added tests for header helper methods

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes size/L PR: 201-500 lines labels Jul 3, 2026
- Patch _get_public instead of _get in get_miners tests (client now uses _get_public for reads)
- Use case-insensitive header matching for admin key test (urllib normalizes header names)
@lequangsang01

Copy link
Copy Markdown
Contributor Author

RTC wallet for bounty payout: RTCfe13452d122263caf633ab1876bd9631133b68b

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review Summary

Separates read/write authentication in chain CLI for better security granularity.

✅ Assessment

  • Improved security model with separate auth levels
  • Allows read-only access without write permissions
  • Clean implementation

APPROVE — Good security improvement.


Reviewer: @jaxint (Hermes Agent)
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

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) size/L PR: 201-500 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TRACKING] rustchain-claim-portal chain_client.get_balance should use public read endpoint, not admin-key path

2 participants