cross-node auth tokens#1404
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds cross-node delegated JWT token validation via a new P2P VALIDATE_AUTH_TOKEN command. Auth now derives fallback verification remotely through configurable-timeout P2P calls, signature verification is refactored into a reusable helper, and consumerAddress is propagated from auth results into persistent storage handlers instead of decoding tokens directly. ChangesDelegated auth token validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LocalNode as Auth (local node)
participant P2PNode as OceanP2P
participant IssuerNode as Issuer node
participant ValidateHandler as ValidateAuthTokenHandler
Client->>LocalNode: validateToken(token)
LocalNode->>LocalNode: check local AuthTokenDatabase
alt token found locally
LocalNode-->>Client: AuthToken (local)
else not found locally
LocalNode->>LocalNode: validateRemoteToken(token)
LocalNode->>LocalNode: decode JWT, verifyConsumerSignature
LocalNode->>LocalNode: parse issuerPeerId, ensure not self
LocalNode->>P2PNode: sendTo(issuerPeerId, VALIDATE_AUTH_TOKEN, timeoutMs)
P2PNode->>IssuerNode: dial + stream request
IssuerNode->>ValidateHandler: handle(ValidateAuthTokenCommand)
ValidateHandler->>ValidateHandler: verify JWT + fetch local token
ValidateHandler-->>P2PNode: { valid, validUntil }
P2PNode-->>LocalNode: streamed verdict
alt verdict.valid
LocalNode-->>Client: AuthToken (created, validUntil)
else invalid
LocalNode-->>Client: null / rejected
end
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
The PR introduces cross-node token verification but contains a critical security flaw. The implementation uses jwt.decode() to parse remote tokens, bypassing cryptographic signature verification. Since the user's embedded signature only authenticates the address, nonce, and command, an attacker can forge the JWT payload to alter or remove the validUntil field, creating tokens that never expire. Additionally, there are bypass vulnerabilities in how validUntil and createdAt are validated.
Comments:
• [ERROR][security] Using jwt.decode() evaluates the payload without verifying its cryptographic signature (CWE-345). Because the embedded user signature (signatureMatchesAddress) only covers address, nonce, and command, the validUntil and createdAt fields in the JWT payload are completely unauthenticated.
An attacker can take a legitimate token, extract the address, nonce, and signature, and generate a forged JWT payload with an altered or missing validUntil field. The validateRemoteToken method will incorrectly accept this forged token.
Recommendations for architectural fix:
- Cryptographic Trust: If nodes share trust, verify the JWT using
jwt.verify()with a shared secret or the issuing node's public key (JWKS). - Stateless Verification (Client-Signed Expiration): If validation must be stateless and trustless, the user's original signature MUST include the expiration time (e.g., signing
address + nonce + validUntil + command). - Nonce Freshness: If the
nonceis guaranteed to be a timestamp, the server must enforce a strict maximum time-to-live against it (e.g., rejecting ifDate.now() - Number(nonce) > MAX_TTL), and completely ignore the unauthenticatedvalidUntilfield in the payload.
• [ERROR][security] The expiration check is flawed and can be bypassed. An attacker can omitvalidUntilfrom a forged JWT payload (making itnullorundefined), or pass a non-numeric string (e.g.,"infinity") which causesNumber(validUntil)to evaluate toNaN. BecauseDate.now() >= NaNevaluates tofalse, the token will be considered valid indefinitely.
Require validUntil to be a valid number and reject tokens that fail this check.
- if (validUntil != null && Date.now() >= Number(validUntil)) {
+ const expTime = Number(validUntil)
+ if (validUntil == null || Number.isNaN(expTime) || Date.now() >= expTime) {
return null
}• [WARNING][bug] Similar to validUntil, the createdAt value is pulled from an unverified payload. If it is omitted or a non-numeric string is provided, Number(createdAt) will be NaN, resulting in an invalid Date object being returned inside the AuthToken. This could lead to downstream crashes if other components expect a valid Date object.
Ensure createdAt is validated before constructing the Date object.
+ const createdTime = Number(createdAt)
+ if (Number.isNaN(createdTime)) {
+ return null
+ }
return {
token,
address,
- created: new Date(Number(createdAt)),
+ created: new Date(createdTime),
validUntil: validUntil != null ? new Date(Number(validUntil)) : null,
isValid: true
}• [INFO][style] Good use of falling back to verify both the hex string and the raw byte array with toBeArray. This ensures compatibility with different client-side signing implementations (e.g., clients that sign the raw bytes vs. clients that sign the literal hex representation).
|
/run-security-scan |
|
@coderabbitai review |
✅ Action performedReview finished.
|
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This PR implements cross-node authentication token validation. While the feature is well-structured, there are critical security vulnerabilities that need to be addressed. The primary issue is an authentication bypass in the remote token validation logic due to trusting the issuerPeerId from an unverified JWT. Additionally, there is an IDOR/privilege escalation flaw in the Persistent Storage handlers, as well as potential DDoS vectors via unbounded stream reading and long network timeouts.
Comments:
• [ERROR][security] Critical Authentication Bypass: jwt.decode is used without cryptographic verification of the issuing node. An attacker who intercepts or observes a victim's valid CREATE_AUTH_TOKEN signature (which is node-agnostic) can craft a forged JWT containing the victim's address and signature, but setting issuerPeerId to the attacker's own malicious node. Node B will successfully verify the victim's consumer signature locally, then query the attacker's node. The attacker's node will reply {"valid": true}, causing Node B to fully authenticate the attacker as the victim. To resolve this, the issuing node MUST sign the JWT asymmetrically (e.g., using its libp2p private key), and Node B MUST verify the token's signature against the issuerPeerId's public key before making any P2P calls.
• [ERROR][security] Privilege Escalation / IDOR: By using task.consumerAddress ? getAddress(task.consumerAddress) : ..., you allow an authenticated user to act on behalf of another user. An attacker can supply their own valid token in task.authorization (which sets isAuthRequestValid.consumerAddress to the attacker's address) but pass a victim's address in task.consumerAddress. Since task.consumerAddress is truthy, the system will use it and grant access to the victim's storage. You should strictly use the authenticated identity:
- ownerNormalized = task.consumerAddress
- ? getAddress(task.consumerAddress)
- : getAddress(isAuthRequestValid.consumerAddress)
+ ownerNormalized = getAddress(isAuthRequestValid.consumerAddress)Note: This issue is also present in other PersistentStorage handlers (e.g., lines 163, 271, 315, 373, 423). Please update all occurrences.
• [WARNING][security] Unbounded Stream Reading: streamToString consumes the entire stream into memory. A malicious peer could exploit this by sending an endless stream of data in response to VALIDATE_AUTH_TOKEN, causing an Out-Of-Memory (OOM) crash on the node. You should enforce a maximum byte limit (e.g., 1024 bytes) when reading this JSON response.
• [WARNING][performance] A 30-second timeout for remote token validation (VALIDATE_TOKEN_DIAL_TIMEOUT_MS = 30_000) is very long. Since this validation blocks incoming API requests, it can lead to resource exhaustion if an attacker supplies a slow or unresponsive issuerPeerId. Consider reducing this to a much shorter timeout (e.g., 5000ms).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/core/handler/persistentStorage.ts (1)
106-110: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the derived consumer address before the access-list check.
For token-only delegated auth,
task.consumerAddresscan be absent whileisAuthRequestValid.consumerAddressis valid. The create-bucket access-list check still usestask.consumerAddress, so valid requests can be denied before Line 110’s fallback is applied.🔧 Proposed fix
+ let ownerNormalized: string + try { + const effectiveConsumerAddress = + task.consumerAddress || isAuthRequestValid.consumerAddress + ownerNormalized = getAddress(effectiveConsumerAddress) + } catch { + return { + stream: null, + status: { httpStatus: 400, error: 'Invalid parameter: "consumerAddress"' } + } + } + // if we have access lists,check them. if ( config.persistentStorage?.accessLists && config.persistentStorage?.accessLists.length > 0 ) { const isAllowedCreate = await checkAddressOnAccessList( - task.consumerAddress, + ownerNormalized, config.persistentStorage?.accessLists, node ) @@ - let ownerNormalized: string - try { - ownerNormalized = task.consumerAddress - ? getAddress(task.consumerAddress) - : getAddress(isAuthRequestValid.consumerAddress) - } catch { - return { - stream: null, - status: { httpStatus: 400, error: 'Invalid parameter: "consumerAddress"' } - } - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/core/handler/persistentStorage.ts` around lines 106 - 110, The access-list check is still using task.consumerAddress even when the derived consumer address should come from isAuthRequestValid.consumerAddress for token-only delegated auth. Update the create-bucket authorization path in persistentStorage.ts to resolve the consumer address once into ownerNormalized (or a similarly named local) and use that derived value for the access-list check instead of the possibly undefined task.consumerAddress.
🧹 Nitpick comments (3)
src/test/unit/auth/token.test.ts (3)
106-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHard-coded command string risks silent drift from
PROTOCOL_COMMANDS.CREATE_AUTH_TOKEN.
mintRemoteTokensigns with the literal string'createAuthToken'instead of importingPROTOCOL_COMMANDS.CREATE_AUTH_TOKEN, which is whatAuth.validateRemoteTokenactually passes toverifyConsumerSignature. If that constant's value ever changes, these tests will keep passing while production code silently breaks (or vice versa) since there is no compile-time link between the two.♻️ Proposed fix
+import { PROTOCOL_COMMANDS } from '../../../utils/constants.js' ... const signature = await safeSign( signWith, - createHashForSignature(claimAddress, nonce, 'createAuthToken') + createHashForSignature(claimAddress, nonce, PROTOCOL_COMMANDS.CREATE_AUTH_TOKEN) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/unit/auth/token.test.ts` around lines 106 - 124, Update mintRemoteToken in token.test.ts to use PROTOCOL_COMMANDS.CREATE_AUTH_TOKEN instead of the hard-coded 'createAuthToken' string when calling createHashForSignature, so the test stays aligned with Auth.validateRemoteToken and verifyConsumerSignature. Import or reference the shared command constant directly in the test helper to avoid drift if the protocol command value changes.
81-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sendTomock never asserts the outgoing message payload.The mock tracks call count but the tests never verify
_msgcontains the expected{ command: PROTOCOL_COMMANDS.VALIDATE_AUTH_TOKEN, token }shape. This leaves the actual wire contract betweenAuth.validateRemoteTokenand the P2P layer unverified — a regression that mangles the request body would go undetected as long as call counts match.Consider capturing
_msgon the tracker and asserting its shape in at least the "accepts" test.Also applies to: 133-244
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/unit/auth/token.test.ts` around lines 81 - 104, The `makeFakeP2P` test double in `token.test.ts` only counts `sendTo` calls and does not verify the request body sent by `Auth.validateRemoteToken`. Update the mock to capture the `_msg` payload (alongside `tracker.calls`) and add assertions in the remote-token tests, especially the “accepts” case, that the outgoing message contains `{ command: PROTOCOL_COMMANDS.VALIDATE_AUTH_TOKEN, token }`. This keeps the wire contract between `validateRemoteToken` and `OceanP2P.sendTo` covered.
133-244: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTest cases correctly mirror
Auth.validateRemoteTokencontrol flow.Verified each scenario (local hit, valid/invalid verdict, unreachable issuer, missing signature, missing issuerPeerId, self-issuer, loop prevention via
getLocalToken) againstsrc/components/Auth/index.ts'svalidateRemoteToken/getLocalTokenimplementation — the assertions ontracker.callsand return values line up with the guard order (signature check → issuerPeerId presence → self-peer check → P2P dial → verdict parsing).One gap: no case exercises a malformed
issuerPeerIdthat failspeerIdFromString. Optional to add for completeness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/unit/auth/token.test.ts` around lines 133 - 244, Add a unit test in the Auth token suite to cover a malformed issuerPeerId that causes peerIdFromString to fail in validateRemoteToken. Use the existing Auth, makeFakeP2P, and mintRemoteToken helpers as the reference points, and assert that validateToken returns null without delegating to the issuer (tracker.calls stays 0). Keep the new case aligned with the current control-flow assertions around validateRemoteToken/getLocalToken so it fits the existing guard-order coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Auth/index.ts`:
- Around line 145-150: The P2P auth validation path in `Auth` is sending a raw
JWT through `p2p.sendTo`, and `OceanP2P.sendTo()` logs the full message payload.
Update `sendTo` to redact or omit token-bearing fields before logging, or stop
logging the raw `message` for `VALIDATE_AUTH_TOKEN` requests so `token` never
appears in logs.
- Around line 155-157: The auth verdict parsing in Auth/index.ts currently reads
the entire remote response with streamToString() before JSON.parse, which can
hang indefinitely or consume too much memory. Update the verdict-fetch path
around the verdict assignment to enforce a maximum byte limit and a read timeout
on response.stream before parsing. Use the existing auth validation flow and
streamToString-related handling to ensure oversized or slow responses are
rejected cleanly before JSON.parse runs.
In `@src/components/core/handler/authHandler.ts`:
- Around line 140-142: The new ValidateAuthTokenCommand path can leak bearer
tokens because validateCommandParameters() logs the raw command payload. Update
authHandler’s validate() flow to redact the token before any shared command
logging, using a VALIDATE_AUTH_TOKEN redaction path in the same
validation/logging path so the token is never emitted. Keep the fix localized
around validateCommandParameters and the ValidateAuthTokenCommand handling.
In `@src/components/core/handler/handler.ts`:
- Around line 208-209: The success response in the handler’s P2PCommandResponse
is missing the error field, causing the response shape to drift from the rest of
the handler path. Update the success return in handler.ts to keep the full
P2PCommandResponse contract by including error: null alongside status and
consumerAddress, matching the other response branches in this handler.
---
Outside diff comments:
In `@src/components/core/handler/persistentStorage.ts`:
- Around line 106-110: The access-list check is still using task.consumerAddress
even when the derived consumer address should come from
isAuthRequestValid.consumerAddress for token-only delegated auth. Update the
create-bucket authorization path in persistentStorage.ts to resolve the consumer
address once into ownerNormalized (or a similarly named local) and use that
derived value for the access-list check instead of the possibly undefined
task.consumerAddress.
---
Nitpick comments:
In `@src/test/unit/auth/token.test.ts`:
- Around line 106-124: Update mintRemoteToken in token.test.ts to use
PROTOCOL_COMMANDS.CREATE_AUTH_TOKEN instead of the hard-coded 'createAuthToken'
string when calling createHashForSignature, so the test stays aligned with
Auth.validateRemoteToken and verifyConsumerSignature. Import or reference the
shared command constant directly in the test helper to avoid drift if the
protocol command value changes.
- Around line 81-104: The `makeFakeP2P` test double in `token.test.ts` only
counts `sendTo` calls and does not verify the request body sent by
`Auth.validateRemoteToken`. Update the mock to capture the `_msg` payload
(alongside `tracker.calls`) and add assertions in the remote-token tests,
especially the “accepts” case, that the outgoing message contains `{ command:
PROTOCOL_COMMANDS.VALIDATE_AUTH_TOKEN, token }`. This keeps the wire contract
between `validateRemoteToken` and `OceanP2P.sendTo` covered.
- Around line 133-244: Add a unit test in the Auth token suite to cover a
malformed issuerPeerId that causes peerIdFromString to fail in
validateRemoteToken. Use the existing Auth, makeFakeP2P, and mintRemoteToken
helpers as the reference points, and assert that validateToken returns null
without delegating to the issuer (tracker.calls stays 0). Keep the new case
aligned with the current control-flow assertions around
validateRemoteToken/getLocalToken so it fits the existing guard-order coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf6185ff-2be9-4790-b4d4-5199d074ca3b
📒 Files selected for processing (9)
src/components/Auth/index.tssrc/components/P2P/index.tssrc/components/core/handler/authHandler.tssrc/components/core/handler/coreHandlersRegistry.tssrc/components/core/handler/handler.tssrc/components/core/handler/persistentStorage.tssrc/components/core/utils/nonceHandler.tssrc/test/unit/auth/token.test.tssrc/utils/constants.ts
| const response = await p2p.sendTo( | ||
| issuerPeerId, | ||
| JSON.stringify({ command: PROTOCOL_COMMANDS.VALIDATE_AUTH_TOKEN, token }), | ||
| undefined, | ||
| undefined, | ||
| VALIDATE_TOKEN_DIAL_TIMEOUT_MS |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact auth tokens before they hit P2P logs.
This sends the raw JWT in the P2P message, and OceanP2P.sendTo() logs the full message string. That leaks reusable auth tokens into logs; add redaction in sendTo or avoid logging token-bearing payloads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Auth/index.ts` around lines 145 - 150, The P2P auth validation
path in `Auth` is sending a raw JWT through `p2p.sendTo`, and
`OceanP2P.sendTo()` logs the full message payload. Update `sendTo` to redact or
omit token-bearing fields before logging, or stop logging the raw `message` for
`VALIDATE_AUTH_TOKEN` requests so `token` never appears in logs.
| let verdict: { valid?: boolean; validUntil?: number | string | null } | ||
| try { | ||
| verdict = JSON.parse(await streamToString(response.stream as Readable)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the remote verdict stream read.
streamToString() buffers until EOF, so a remote peer can keep auth validation hanging or stream an oversized body. Cap bytes and apply a response-read timeout before JSON.parse.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Auth/index.ts` around lines 155 - 157, The auth verdict
parsing in Auth/index.ts currently reads the entire remote response with
streamToString() before JSON.parse, which can hang indefinitely or consume too
much memory. Update the verdict-fetch path around the verdict assignment to
enforce a maximum byte limit and a read timeout on response.stream before
parsing. Use the existing auth validation flow and streamToString-related
handling to ensure oversized or slow responses are rejected cleanly before
JSON.parse runs.
| validate(command: ValidateAuthTokenCommand): ValidateParams { | ||
| return validateCommandParameters(command, ['token']) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact validation tokens before shared command logging.
validateCommandParameters() logs the command payload, so this new command can write bearer tokens to logs. Add a VALIDATE_AUTH_TOKEN redaction path before logging.
🛡️ Proposed redaction
+ } else if (commandStr === PROTOCOL_COMMANDS.VALIDATE_AUTH_TOKEN && commandData.token) {
+ logCommandData.token = '[REDACTED]'
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/core/handler/authHandler.ts` around lines 140 - 142, The new
ValidateAuthTokenCommand path can leak bearer tokens because
validateCommandParameters() logs the raw command payload. Update authHandler’s
validate() flow to redact the token before any shared command logging, using a
VALIDATE_AUTH_TOKEN redaction path in the same validation/logging path so the
token is never emitted. Keep the fix localized around validateCommandParameters
and the ValidateAuthTokenCommand handling.
| status: { httpStatus: 200 }, | ||
| consumerAddress: isAuthRequestValid.address |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep the success response contract complete.
Line 208 drops error: null, unlike the other P2PCommandResponse statuses in this handler path. Add it back to avoid response-shape drift. As per coding guidelines, “All handler responses must use P2PCommandResponse format with httpStatus, body, stream, and error properties”.
🔧 Proposed fix
- status: { httpStatus: 200 },
+ status: { httpStatus: 200, error: null },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| status: { httpStatus: 200 }, | |
| consumerAddress: isAuthRequestValid.address | |
| status: { httpStatus: 200, error: null }, | |
| consumerAddress: isAuthRequestValid.address |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/core/handler/handler.ts` around lines 208 - 209, The success
response in the handler’s P2PCommandResponse is missing the error field, causing
the response shape to drift from the rest of the handler path. Update the
success return in handler.ts to keep the full P2PCommandResponse contract by
including error: null alongside status and consumerAddress, matching the other
response branches in this handler.
Source: Coding guidelines
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This PR implements cross-node verifiable JWT authentication by embedding the original authentication signature and a validUntil timestamp into the token payload. The architecture allows nodes to validate tokens statelessly while maintaining strong cryptographic guarantees. The refactoring to reuse consumerAddress in storage handlers is also excellent. A few security hardening suggestions are provided, specifically around enforcing a max token TTL during stateless validation to prevent permanently valid tokens, and adding explicit type checking for the decoded JWT payload.
Comments:
• [WARNING][security] Cross-node tokens bypass the MAX_AUTH_TOKEN_TTL_MS limit that is normally enforced during token creation. A malicious dApp could prompt a user to sign a token valid for many years, construct the JWT locally, and use it indefinitely across nodes since this stateless validation lacks TTL bounds (and stateless tokens cannot be easily revoked by a single node). Consider exporting MAX_AUTH_TOKEN_TTL_MS from authHandler.ts and enforcing it here as well:
- if (!Number.isFinite(validUntilNum) || Date.now() >= validUntilNum) {
+ if (
+ !Number.isFinite(validUntilNum) ||
+ Date.now() >= validUntilNum ||
+ validUntilNum - Date.now() > MAX_AUTH_TOKEN_TTL_MS
+ ) {
return null
}• [INFO][security] The decoded JWT payload is cast to an object without strict type checking. Although downstream functions (like verifyConsumerSignature) handle non-string values safely by throwing catchable errors in ethers, it is best practice to explicitly validate types to prevent prototype pollution or edge-case coercion bugs.
const { address, nonce, signature, createdAt, validUntil, chainId } = decoded as {
address?: string
nonce?: string
signature?: string
createdAt?: number
validUntil?: number | null
chainId?: string | null
}
- if (!address || !nonce || !signature || validUntil == null) {
+ if (
+ typeof address !== 'string' ||
+ typeof nonce !== 'string' ||
+ typeof signature !== 'string' ||
+ typeof validUntil !== 'number'
+ ) {
return null
}• [INFO][security] Hardcoding PROTOCOL_COMMANDS.CREATE_AUTH_TOKEN during signature verification is an excellent security measure. It ensures that signatures intended for other operational commands (e.g., UPDATE_BUCKET_LABEL) cannot be maliciously repurposed to mint cross-node authentication tokens.
• [INFO][performance] Excellent refactoring here! Reusing the consumerAddress returned from validateTokenOrSignature completely avoids a redundant token database lookup (getAddressFromToken), improving overall performance and keeping the code DRY.
2 participants
Changes proposed in this PR:
Summary by CodeRabbit
New Features
Bug Fixes
Tests