Skip to content

security(clawrouter): require configurable owner for /wallet and /stats; set /stats requireAuth true#22

Open
hubofvalley wants to merge 1 commit intoBlockRunAI:mainfrom
hubofvalley:main
Open

security(clawrouter): require configurable owner for /wallet and /stats; set /stats requireAuth true#22
hubofvalley wants to merge 1 commit intoBlockRunAI:mainfrom
hubofvalley:main

Conversation

@hubofvalley
Copy link

@hubofvalley hubofvalley commented Feb 13, 2026

Summary of Changes:

Dynamic Owner List: Added isAllowedUser() and resolveAllowedUsers() to handle configurable owner IDs.
Improved Security: /stats now requires authentication (requireAuth: true) and both /stats and /wallet now enforce the owner check.
Flexible Matching: Supports both exact IDs (e.g., 123) and platform-prefixed IDs (e.g., telegram:123).
Secure Default: If no users are configured, sensitive commands return a clear configuration error instead of potentially exposing data.
How to use: Set the allowed users via environment variable or plugin config:

export CLAWROUTER_ALLOWED_USERS="telegram:123,whatsapp:456"
Or add allowedUsers: ["telegram:123"] to your OpenClaw plugin configuration.

…ts; set /stats requireAuth true

- Add isAllowedUser() and resolveAllowedUsers() guard helpers
- Set /stats requireAuth to true (matches /wallet)
- Implement owner guard in both command handlers
- Resolve allowed IDs from CLAWROUTER_ALLOWED_USERS env var or plugin config
- Improved "secure by default" behavior (blocks if no users configured)
@1bcMax
Copy link
Member

1bcMax commented Feb 14, 2026

Thanks for the security improvement! The direction is right, but I have some concerns before merging:

1. Breaking Change Without Migration Path

This will immediately block all existing users who haven't configured CLAWROUTER_ALLOWED_USERS. Users upgrading will find /stats and /wallet suddenly broken with no warning.

Suggestion: Consider backwards compatibility - either allow unrestricted access when no users are configured (with a deprecation warning), or make this an opt-in feature.

2. Does /stats Need This Level of Protection?

  • /wallet export exposes private keys → must protect
  • /stats is read-only usage data → does it need the same protection?

Consider tiered protection: strict for /wallet, optional for /stats.

3. Bare ID Matching Security Concern

// If "123" is in allowedUsers, it matches ALL platforms:
"telegram:123" 
"whatsapp:123"   // Could be a different person!

Different platforms may have overlapping user IDs. This could be a security hole.

Suggestion: Remove bare ID matching, require full platform-prefixed IDs only.

4. Code Duplication

The owner guard logic is duplicated in both commands. Consider extracting to a helper function.


Happy to discuss further or review a revised version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants