fix(gateway): improve device signature validation diagnostics and make clock skew configurable#28993
fix(gateway): improve device signature validation diagnostics and make clock skew configurable#28993dingn42 wants to merge 3 commits intoopenclaw:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbb420b2c7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * Device signature timestamp skew tolerance in milliseconds. | ||
| * Signatures older than this are rejected as stale. Default: 120000 (2 minutes). | ||
| */ | ||
| deviceSignatureSkewMs?: number; |
There was a problem hiding this comment.
Adding gateway.deviceSignatureSkewMs to GatewayConfig without updating the strict Zod schema makes the new option unusable and can break startup for users who set it. loadConfig() validates via validateConfigObjectWithPlugins, and the gateway object in src/config/zod-schema.ts is .strict() and currently does not include this key, so a config containing gateway.deviceSignatureSkewMs is rejected as invalid instead of overriding the default skew.
Useful? React with 👍 / 👎.
Greptile SummaryAdded configurable clock skew tolerance ( Issues found:
Note: The core logic is sound and the feature works as intended, but the config validation infrastructure is incomplete. Without Zod validation, users could set invalid values (negative numbers, non-integers, etc.) that won't be caught until runtime. Confidence Score: 3/5
Last reviewed commit: bbb420b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** | ||
| * Device signature timestamp skew tolerance in milliseconds. | ||
| * Signatures older than this are rejected as stale. Default: 120000 (2 minutes). | ||
| */ | ||
| deviceSignatureSkewMs?: number; |
There was a problem hiding this comment.
Missing Zod schema validation for this new config field. Similar fields like channelHealthCheckMinutes have validation in src/config/zod-schema.ts. Add validation to ensure the value is a valid number:
// In src/config/zod-schema.ts around line 570, after channelHealthCheckMinutes:
deviceSignatureSkewMs: z.number().int().min(0).optional(),Also consider adding:
- Label in
src/config/schema.labels.ts - Help text in
src/config/schema.help.ts - Tests in
src/config/config-misc.test.ts
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/types.gateway.ts
Line: 365-369
Comment:
Missing Zod schema validation for this new config field. Similar fields like `channelHealthCheckMinutes` have validation in `src/config/zod-schema.ts`. Add validation to ensure the value is a valid number:
```typescript
// In src/config/zod-schema.ts around line 570, after channelHealthCheckMinutes:
deviceSignatureSkewMs: z.number().int().min(0).optional(),
```
Also consider adding:
- Label in `src/config/schema.labels.ts`
- Help text in `src/config/schema.help.ts`
- Tests in `src/config/config-misc.test.ts`
How can I resolve this? If you propose a fix, please make it concise.| rejectDeviceAuthInvalid("device-signature-stale", "device signature expired"); | ||
| const serverNowMs = Date.now(); | ||
| const deltaMs = typeof signedAt === "number" ? Math.abs(serverNowMs - signedAt) : undefined; | ||
| if (typeof signedAt !== "number" || deltaMs! > deviceSignatureSkewMs) { |
There was a problem hiding this comment.
Non-null assertion (deltaMs!) is unnecessary with proper restructuring. While technically safe due to short-circuit evaluation, it's a code smell. Consider refactoring:
| if (typeof signedAt !== "number" || deltaMs! > deviceSignatureSkewMs) { | |
| if (typeof signedAt !== "number" || (deltaMs !== undefined && deltaMs > deviceSignatureSkewMs)) { |
Or split into two separate checks for clarity.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server/ws-connection/message-handler.ts
Line: 650
Comment:
Non-null assertion (`deltaMs!`) is unnecessary with proper restructuring. While technically safe due to short-circuit evaluation, it's a code smell. Consider refactoring:
```suggestion
if (typeof signedAt !== "number" || (deltaMs !== undefined && deltaMs > deviceSignatureSkewMs)) {
```
Or split into two separate checks for clarity.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
…e clock skew configurable - Add gateway.deviceSignatureSkewMs config field (default: 120000ms / 2 minutes) - Add Zod schema validation, config labels, and help text - Log warning with full diagnostics on signature stale rejection - Include clock delta in client-facing error message - Fix non-null assertion code smell - Fix unrelated type error in extraparams test mock Closes openclaw#24455, openclaw#28209 Co-Authored-By: Claude Opus 4.6 <[email protected]>
2 participants
Summary
Improves device signature validation with better diagnostics and a configurable clock skew tolerance.
Problem
When a node host fails to connect due to clock skew, the error message is just
"device signature invalid"or"device signature expired"with no indication of how far off the clock is. Users have no way to diagnose whether the issue is a 1-second drift or a 10-minute gap. The 2-minute tolerance is also hardcoded with no way to adjust it for environments with known clock drift (VMs, Raspberry Pi, mobile devices waking from sleep).Relates to #24455, #28209.
Changes
Configurable skew tolerance: Added
gateway.deviceSignatureSkewMsconfig field (default: 120000ms / 2 minutes). Zero-config users are unaffected.Diagnostic logging: On clock skew rejection, logs a
warnwithdeviceId,signedAtMs,serverNowMs,deltaMs, andallowedSkewMs.Actionable client error: Changed the client-facing close reason from
"device signature expired"to"device signature expired (clock delta: Xms, allowed: Yms)"so users can immediately see the problem.The existing distinction between
"device-signature-stale"(clock skew) and"device-signature"(bad crypto) is preserved and enhanced.Files changed
src/config/types.gateway.ts— AddeddeviceSignatureSkewMstoGatewayConfigsrc/gateway/server/ws-connection/message-handler.ts— Read config, add logging, improve error message2 files, +17 / -6 lines.
Testing
pnpm buildpasses