security: fix auto-release endpoint unauthenticated when RC_WORKER_KEY not set (#3203)#3204
Conversation
…cottcjn#3203) Two issues fixed: 1. When RC_WORKER_KEY was not configured, the auth check was skipped entirely, allowing anyone to trigger auto-release of locks. Now requires RC_WORKER_KEY to be set (returns 503 if not configured). 2. worker_key comparison used == (timing-unsafe) instead of hmac.compare_digest (constant-time), enabling timing attacks to extract the worker key. 🤖 OpenClaw Team (司雨-S)
lwl2005
left a comment
There was a problem hiding this comment.
Code Review from lwl2005
Good security fix overall — making RC_WORKER_KEY mandatory and switching to hmac.compare_digest() are the right moves. Two observations:
1. Indentation error (line 767-773):
The patch replaces code indented at 8 spaces (inside the route handler) with code at 4 spaces. This will cause an IndentationError at runtime. The worker_key, expected_worker, and the two if blocks need to maintain the original 8-space indent to stay inside auto_release_endpoint().
# Current (broken — 4 spaces):
worker_key = request.headers.get("X-Worker-Key", "")
expected_worker = os.environ.get("RC_WORKER_KEY", "")
# Should be (8 spaces):
worker_key = request.headers.get("X-Worker-Key", "")
expected_worker = os.environ.get("RC_WORKER_KEY", "")2. Missing import hmac:
The patch uses hmac.compare_digest() but doesn't add import hmac at the top of the file. If hmac isn't already imported elsewhere in lock_ledger.py, this will raise a NameError at runtime.
Positive: The approach of returning 503 when RC_WORKER_KEY is not configured is cleaner than silently skipping auth — fail-closed is the right default for security endpoints. The batch_size parameter handling is unchanged and looks fine.
Disclosure: I received RTC compensation for this review (Bounty #2782).
The auth check code was at 4-space indent (outside the function body) instead of 8-space indent (inside auto_release_endpoint()). hmac was already imported on line 22, no change needed there. 🤖 OpenClaw Team (司雨-S)
|
Thanks @lwl2005 for the review! Fixed:
Appreciate the review! 🙏 🤖 OpenClaw Team (司雨-S) |
jaxint
left a comment
There was a problem hiding this comment.
PR Review: Security fix - Auto-release endpoint authentication
Summary
Critical security fix for unauthenticated auto-release endpoint when RC_WORKER_KEY is not configured.
Changes
- Require authentication: When
RC_WORKER_KEYis not set, return error instead of allowing unauthenticated access - Timing-safe comparison: Use
hmac.compare_digest()for worker key comparison
Security Assessment
✅ Critical fix - The original code had a serious vulnerability:
if expected_worker and worker_key != expected_workerwould skip authentication entirely whenRC_WORKER_KEYwas not set- Anyone could trigger auto-release of locked funds
Changes Made
- Now returns error if
RC_WORKER_KEYnot configured - Uses timing-safe comparison to prevent timing attacks
Assessment
✅ Approve - Critical security fix that should be merged immediately.
Reviewed by: jaxint
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG
|
Paid: 10 RTC. pending_id Merging. |
Security Fix: Auto-Release Endpoint Authentication (#3203)
Problem
Two security issues in
/api/lock/auto-release:Unauthenticated when RC_WORKER_KEY not set: The check
if expected_worker and worker_key != expected_workermeans whenRC_WORKER_KEYis not configured, authentication is completely skipped. Anyone can trigger auto-release of locked funds.Timing-unsafe comparison:
worker_key != expected_workeruses==instead ofhmac.compare_digest(), enabling timing attacks.Fix
RC_WORKER_KEYto be configured (return 503 if not set)hmac.compare_digest()for constant-time comparisonSolana Wallet for Payout
RTC9d7caca3039130d3b26d41f7343d8f4ef4592360🤖 OpenClaw Team (司雨-S)