Add tlsn identity assign and remove logics#664
Conversation
Review Summary by QodoAdd TLSNotary identity verification and management
WalkthroughsDescription• Add TLSNotary identity verification for GitHub, Discord, Telegram • Implement identity add/remove operations via TLSN proof verification • Create server-side proof structure validation module with WASM support • Integrate TLSN identity handling into GCR identity routines and request handlers Diagramflowchart LR
A["TLSNotary Proof"] -->|verifyTLSNotaryPresentation| B["Proof Validation"]
B -->|parseHttpResponse| C["Extract User Data"]
C -->|extractUser| D["Verified Identity"]
D -->|applyTLSNIdentityAdd| E["GCR Storage"]
E -->|IncentiveManager| F["Award Incentives"]
G["Remove Request"] -->|applyTLSNIdentityRemove| H["GCR Update"]
H -->|IncentiveManager| I["Rollback Incentives"]
File Changes1. src/libs/tlsnotary/verifier.ts
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Node-side TLSNotary verification and exports, integrates tlsnadd/tlsnremove into GCR identity routines and network handlers, persists TLSNotary identity records, links Telegram TLSN incentives, updates tooling/install scripts, adds docs, and bumps a demo SDK dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as Client
participant Handler as "Identity Request\nHandler"
participant Verifier as "TLSNotary\nVerifier"
participant Parser as "HTTP Response\nParser"
participant GCR as "GCR Identity\nRoutines"
participant DB as "GCR Database"
participant Incentive as "Incentive\nManager"
Client->>Handler: POST tlsn_identity_assign (payload)
Handler->>Verifier: verifyTLSNProof(payload)
Verifier->>Parser: parseHttpResponse(revealedRecv)
Parser-->>Verifier: {statusLine, headers, body}
Verifier->>Verifier: extractUser(context, body)
Verifier-->>Handler: {success, extractedUsername, extractedUserId}
Handler->>GCR: applyTLSNIdentityAdd(editOperation, simulate?)
GCR->>GCR: validate, dedupe, compute proofHash
GCR->>DB: persist identity record
GCR->>Incentive: telegramTLSNLinked(userId, telegramUserId, referralCode?)
Incentive-->>GCR: award result
GCR-->>Handler: GCRResult
Handler-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
Code Review by Qodo
✅ 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| log.info( | ||
| `[TLSN Identity] Verifying proof for ${context} identity: ${username}`, | ||
| ) |
There was a problem hiding this comment.
1. Pii in tlsn logs 📘 Rule violation ⛨ Security
The new TLSN identity flow logs username and userId, which can be considered personal user information and must not appear in application logs. These logs are also plain interpolated strings, making them harder to audit/parse consistently.
Agent Prompt
## Issue description
TLSN identity-related log messages include personal user information (`username`, `userId`) and are emitted as unstructured interpolated strings.
## Issue Context
Compliance requires logs to avoid sensitive/personal user data and be useful for auditing. Identity operations are security-sensitive, so logging must be carefully redacted and ideally structured.
## Fix Focus Areas
- src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts[1231-1331]
- src/libs/tlsnotary/verifier.ts[326-328]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (!verified.success) { | ||
| log.warn( | ||
| `[TLSN Identity] Proof verification failed: ${verified.error}`, | ||
| ) | ||
| return { | ||
| success: false, | ||
| message: `Proof verification failed: ${verified.error}`, | ||
| } |
There was a problem hiding this comment.
2. Internal errors returned to clients 📘 Rule violation ⛨ Security
Several TLSN verification failures return verified.error and other internal details (e.g., expected/actual server names) in the message, which can leak implementation details to end-users. Detailed failure context should be kept in internal logs, while client-facing messages remain generic.
Agent Prompt
## Issue description
TLSN-related APIs return internal verification details (`verified.error`, server mismatch specifics) in response messages, risking information leakage.
## Issue Context
Client-facing errors should be generic per secure error-handling requirements; detailed context should be logged internally for debugging.
## Fix Focus Areas
- src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts[1238-1260]
- src/libs/tlsnotary/verifier.ts[317-324]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // WASM verification disabled - trust claimed data with warning | ||
| // NOTE: This is less secure but allows operation until WASM works in Node.js | ||
| log.warn( | ||
| `[TLSN Identity] WASM disabled - trusting claimed data for ${context}: ${username} (${userId})`, | ||
| ) | ||
| extractedUser = { username, userId: String(userId) } | ||
| } |
There was a problem hiding this comment.
3. Tlsn add trusts client claims 📘 Rule violation ⛨ Security
When operating in structure-validation-only mode (or when verified.recv is absent), the code proceeds by trusting client-provided username/userId rather than deriving them from a verified proof. This is insufficient validation for an externally supplied identity claim and can enable identity spoofing.
Agent Prompt
## Issue description
TLSN identity assignment can succeed without server-side cryptographic verification by trusting client claims when running in structure-only verification mode.
## Issue Context
Identity binding is security-sensitive. Validating only proof structure is insufficient to authenticate the claimed `username`/`userId`.
## Fix Focus Areas
- src/libs/tlsnotary/verifier.ts[1-167]
- src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts[1230-1332]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (!simulate) { | ||
| await gcrMainRepository.save(accountGCR) | ||
|
|
||
| // Trigger incentive rollback if applicable | ||
| if (context === "github" && identity.userId) { | ||
| await IncentiveManager.githubUnlinked( | ||
| editOperation.account, | ||
| identity.userId, | ||
| ) | ||
| } else if (context === "discord") { | ||
| await IncentiveManager.discordUnlinked(editOperation.account) | ||
| } else if (context === "telegram") { | ||
| await IncentiveManager.telegramUnlinked(editOperation.account) | ||
| } | ||
| } | ||
|
|
||
| return { success: true, message: "TLSN identity removed successfully" } |
There was a problem hiding this comment.
4. Tlsn remove lacks audit log 📘 Rule violation ✧ Quality
The TLSN identity removal operation performs a write (gcrMainRepository.save) without emitting an audit log capturing who performed the action, what changed, and the outcome. This makes it harder to reconstruct identity changes during incident response or compliance reviews.
Agent Prompt
## Issue description
TLSN identity removal writes state without an accompanying audit log entry describing the action and outcome.
## Issue Context
Identity add/remove operations are critical security events and should be traceable without exposing sensitive user data.
## Fix Focus Areas
- src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts[1422-1473]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const { context, username } = editOperation.data | ||
|
|
||
| if (!context || !username) { | ||
| return { | ||
| success: false, | ||
| message: "Invalid payload: missing context or username", | ||
| } | ||
| } | ||
|
|
||
| const accountGCR = await ensureGCRForUser(editOperation.account) | ||
|
|
||
| accountGCR.identities.web2 = accountGCR.identities.web2 || {} | ||
| accountGCR.identities.web2[context] = | ||
| accountGCR.identities.web2[context] || [] | ||
|
|
||
| // Find the identity to remove | ||
| const identity = accountGCR.identities.web2[context].find( | ||
| (id: Web2GCRData["data"]) => id.username === username, | ||
| ) | ||
|
|
||
| if (!identity) { | ||
| return { success: false, message: "Identity not found" } | ||
| } | ||
|
|
||
| // Filter out the identity | ||
| accountGCR.identities.web2[context] = accountGCR.identities.web2[ | ||
| context | ||
| ].filter((id: Web2GCRData["data"]) => id.username !== username) | ||
|
|
There was a problem hiding this comment.
5. Tlsn remove deletes non-tlsn 🐞 Bug ✓ Correctness
applyTLSNIdentityRemove deletes identities purely by context+username and does not check that the record was added via TLSN (proofType). This allows a tlsn_remove operation to remove legacy web2 identities and trigger incentive rollback unexpectedly.
Agent Prompt
### Issue description
`applyTLSNIdentityRemove` removes any web2 identity matching `username` without checking `proofType === "tlsn"`, and without validating `context` is one of the TLSN-supported platforms. This can delete non-TLSN identities.
### Issue Context
TLSN identities are stored in the same `accountGCR.identities.web2[context]` arrays used for regular web2 identities.
### Fix Focus Areas
- src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts[1422-1470]
- src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts[197-415]
### Suggested changes
1. Validate `context` in remove (e.g., only `github|discord|telegram`).
2. When finding/filtering, require the record to be TLSN-added, e.g. `(id as any).proofType === "tlsn"`.
3. Prefer removing by `userId` (or both `userId` and `username`) to avoid removing the wrong record when usernames collide or change.
4. Only trigger TLSN incentive rollback when the removed record is confirmed TLSN provenance.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libs/omniprotocol/protocol/handlers/gcr.ts (1)
46-49:⚠️ Potential issue | 🟡 MinorStale doc comment — doesn't mention
nomisortlsncontexts.Line 49 says "apply identity changes (xm, web2, pqc, ud)" but should now include
nomisandtlsn.- * Uses GCRIdentityRoutines to apply identity changes (xm, web2, pqc, ud). + * Uses GCRIdentityRoutines to apply identity changes (xm, web2, pqc, ud, nomis, tlsn).
🤖 Fix all issues with AI agents
In `@src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts`:
- Around line 1165-1173: The JSDoc for the "Add an identity via TLSNotary proof
verification" routine is inaccurate: the code path currently falls back to
trusting client-provided claims because verifyTLSNotaryPresentation returns no
recv data, so cryptographic verification is not performed. Update the comment on
the method in GCRIdentityRoutines.ts to state the current behavior: that
verifyTLSNotaryPresentation does not yield cryptographically-extracted recv data
and the implementation therefore accepts client-supplied claims in the fallback
branch (where the code uses the client-provided values), and remove or soften
any absolute statements like "never trusting client-provided claims" and
"performs cryptographic verification."
- Around line 1267-1332: The code block that depends on verified.recv is dead
because verifyTLSNotaryPresentation never returns recv; update the TLSN
verification flow in GCRIdentityRoutines.verify... to avoid implying proof
verification: add a top-of-function TODO comment stating "identity claims are
currently unverified — WASM recv path disabled" and either remove/disable the
entire recv-based branch (the if (verified.recv) block that calls
parseHttpResponse, extractUser, and performs username/userId checks) or gate it
explicitly behind a clearly named feature flag (e.g., ENABLE_WASM_RECV) so it
cannot run accidentally; also update the log messages (the else branch and any
success logs) to make it explicit they are trusting client claims, and keep
references to verifyTLSNotaryPresentation, verified.recv, parseHttpResponse,
extractUser, and TLSNIdentityContext to locate the code.
- Around line 1422-1449: The applyTLSNIdentityRemove function currently calls
ensureGCRForUser which can create a new GCR record; change it to load the record
with gcrMainRepository.findOneBy({ account: editOperation.account }) (same
pattern as applyXmIdentityRemove / applyNomisIdentityRemove), and if no record
is found return { success: false, message: "Account not found" } instead of
proceeding; keep the rest of the logic that accesses accountGCR.identities.web2
the same so you only avoid creating empty GCRs when removing an identity.
- Around line 1180-1212: The destructured fields proofString, username, and
userId (from editOperation.data.data) are not validated and can be undefined;
add explicit checks in the GCRIdentityRoutines method after parsing proof
(and/or immediately after destructuring) to ensure username and userId are
present and non-empty (and that proof was successfully parsed), returning a
failure object with a clear message (e.g., "Missing required field: username" or
"Missing required field: userId") if they are missing; keep existing context
validation (this.TLSN_EXPECTED_ENDPOINTS[context]) but perform these new
validations before any use of username/userId (e.g., before logging or storing
at later lines) to prevent downstream errors.
- Around line 1396-1410: The TLSN branch calling IncentiveManager.telegramLinked
is missing the attestation argument; update the call in GCRIdentityRoutines
(where isFirstConnection is used) to pass data.proof as the fourth parameter so
the call becomes IncentiveManager.telegramLinked(editOperation.account,
String(userId), referralCode, data.proof), matching the Web2 path and allowing
awardTelegramPoints to inspect attestation.payload.group_membership.
In `@src/libs/omniprotocol/protocol/handlers/gcr.ts`:
- Around line 100-107: Update the stale doc comment that enumerates allowed
context types to include "nomis" and "tlsn" and add a short TODO noting the
temporary nature of the `as any` cast; specifically, update the comment near the
use of gcrIdentityRoutines.apply to list "xm, web2, pqc, ud, nomis, tlsn" and
add a note that the editOperation is currently cast to any due to a missing
"tlsn" union member in the SDK's GCREdit type; keep the `as any` for now but add
a TODO referencing `@kynesyslabs/demosdk` and instruct to remove the cast and
restore proper typing of editOperation to GCREdit once the SDK is updated.
In `@src/libs/tlsnotary/index.ts`:
- Around line 1-6: Update the module doc comment to remove the incorrect "using
WASM" claim and clearly state that this TLSNotary Verification Module performs
server-side, structure-only verification without WASM support; reference the
verifier implementation (verifier.ts) and its structure-only verification mode
so readers know where the non-WASM logic lives and that WASM is intentionally
unsupported.
In `@src/libs/tlsnotary/verifier.ts`:
- Around line 264-277: The extractUser logic in extractUser (case "telegram")
currently returns an empty string when both user.username and user.first_name
are missing, which can silently match in applyTLSNIdentityAdd's username
comparison; update extractUser to treat a missing username as an extraction
failure by checking that either user.username or user.first_name exists before
returning, and if neither exists log a clear warning/error (include context like
"Telegram response missing username/first_name") and return null instead of
username:""; ensure callers like applyTLSNIdentityAdd rely on null to indicate
extraction failure.
- Around line 301-335: The function verifyTLSNProof currently returns
caller-supplied username/userId as extractedUsername/extractedUserId which is
misleading; update verifyTLSNProof to either (A) rename the return fields to
claimedUsername and claimedUserId and adjust callers (e.g.,
handleIdentityRequest.ts handling of tlsn_identity_assign) to reflect they are
unverified claims, or (B) implement proof-derived extraction by parsing the
verified proof inside verifyTLSNProof (similar to applyTLSNIdentityAdd) and
compare those extracted values with the supplied username/userId, returning the
extracted values only when they match; choose one approach and update all uses
of extractedUsername/extractedUserId accordingly to avoid implying proof-derived
trust.
🧹 Nitpick comments (4)
src/libs/tlsnotary/verifier.ts (2)
138-152: Consider adding an upper bound ondatalength before running the regex test.
/^[0-9a-fA-F]+$/.test(presentationJSON.data)will scan the entire string. Ifdatais extremely large (megabytes of hex), this could be expensive. A length ceiling before the regex would be a simple safeguard.+ // Upper bound check to prevent processing excessively large payloads + if (presentationJSON.data.length > 10_000_000) { + return { + success: false, + error: "Invalid presentation: 'data' field exceeds maximum allowed size", + } + } + // Validate data is hex-encoded (basic check) if (!/^[0-9a-fA-F]+$/.test(presentationJSON.data)) {
82-95:initTLSNotaryVerifierandisVerifierInitializedare no-ops — document that clearly or remove the initialization ceremony.These functions exist "for API compatibility" but serve no purpose. Any caller checking
isVerifierInitialized()will always gettrue, which masks the fact that no real verification infrastructure is running. If these are meant to be a future extension point, the doc comment onisVerifierInitializedshould warn that it does not indicate cryptographic readiness.src/libs/network/routines/transactions/handleIdentityRequest.ts (1)
102-104: Verify the shape ofpayload.payloadmatchesTLSNIdentityPayloadat runtime.The cast
payload.payload as TLSNIdentityPayloadassumes the payload containscontext,proof,username, anduserIdfields. If a malformed request omits any of these,verifyTLSNProofwill proceed withundefinedvalues — the context validation on line 310 ofverifier.tswould catch an invalid context, butundefinedproofwould still pass thetypeof proof !== "object"check (sincetypeof undefined === "undefined"), leading to a structure validation failure. This works incidentally but is fragile.Consider adding explicit field presence checks before the cast, consistent with how other cases validate their payloads.
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (1)
1350-1359: Storing the full TLSNotary proof object in the JSONB column could significantly bloat storage.TLSNotary proofs can contain large hex-encoded data fields. Storing the entire proof per identity in the
web2JSONB column could degrade query performance over time. Consider storing only theproofHashand persisting the full proof in a separate table or blob store if re-verification is needed.
| // Apply the identity operation (simulate = false for actual execution) | ||
| // Type assertion needed: local IdentityAssignRequest includes "tlsn" context | ||
| // but the GCREdit type from SDK package may not have it yet | ||
| const result = await gcrIdentityRoutines.apply( | ||
| editOperation, | ||
| editOperation as any, | ||
| gcrMainRepository, | ||
| false, // simulate = false (actually apply changes) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its size
wc -l src/libs/omniprotocol/protocol/handlers/gcr.tsRepository: kynesyslabs/node
Length of output: 110
🏁 Script executed:
# Read the relevant sections of the file
sed -n '40,60p' src/libs/omniprotocol/protocol/handlers/gcr.tsRepository: kynesyslabs/node
Length of output: 834
🏁 Script executed:
# Read the section with the type assertion
sed -n '95,115p' src/libs/omniprotocol/protocol/handlers/gcr.tsRepository: kynesyslabs/node
Length of output: 993
🏁 Script executed:
# Check package.json for current SDK version
cat package.json | grep -A 2 -B 2 "demosdk"Repository: kynesyslabs/node
Length of output: 656
🌐 Web query:
@kynesyslabs/demosdk npm package latest version GCREdit type
💡 Result:
- Latest npm version of
@kynesyslabs/demosdk:2.3.22(shown as the current version on npm). [1] GCREdittype: I could not find any published API-reference entry or docs page mentioning aGCREdittype for@kynesyslabs/demosdk. [2]
To confirm whether GCREdit exists in the installed typings (and whether it’s exported), search your local install:
npm i `@kynesyslabs/demosdk`@latest
grep -R "GCREdit" node_modules/@kynesyslabs/demosdk -nSources: [1] [2]
🏁 Script executed:
# Search for GCREdit type in node_modules
find node_modules/@kynesyslabs/demosdk -name "*.d.ts" 2>/dev/null | head -20Repository: kynesyslabs/node
Length of output: 42
🏁 Script executed:
# Search for GCREdit in the SDK
grep -r "GCREdit" node_modules/@kynesyslabs/demosdk 2>/dev/null | head -10Repository: kynesyslabs/node
Length of output: 42
🏁 Script executed:
# Also check if we can find the actual IdentityAssignRequest type in the codebase
rg "IdentityAssignRequest" src/ -A 5 -B 2Repository: kynesyslabs/node
Length of output: 2041
🏁 Script executed:
# Check what contexts are validated in the code (around line 74-75 mentioned in scratchpad)
sed -n '70,80p' src/libs/omniprotocol/protocol/handlers/gcr.tsRepository: kynesyslabs/node
Length of output: 635
Update stale doc comment to include new context types, and track as any cast as tech debt.
The doc comment on line 49 lists only "xm, web2, pqc, ud" but should include the newly added "nomis" and "tlsn" contexts. The as any cast on line 104 is a pragmatic workaround for SDK type misalignment—once @kynesyslabs/demosdk is updated to include "tlsn" in GCREdit, remove this assertion and update the comment accordingly.
🤖 Prompt for AI Agents
In `@src/libs/omniprotocol/protocol/handlers/gcr.ts` around lines 100 - 107,
Update the stale doc comment that enumerates allowed context types to include
"nomis" and "tlsn" and add a short TODO noting the temporary nature of the `as
any` cast; specifically, update the comment near the use of
gcrIdentityRoutines.apply to list "xm, web2, pqc, ud, nomis, tlsn" and add a
note that the editOperation is currently cast to any due to a missing "tlsn"
union member in the SDK's GCREdit type; keep the `as any` for now but add a TODO
referencing `@kynesyslabs/demosdk` and instruct to remove the cast and restore
proper typing of editOperation to GCREdit once the SDK is updated.
| /** | ||
| * TLSNotary Verification Module | ||
| * | ||
| * Provides server-side verification of TLSNotary proofs using WASM. | ||
| * Used by GCR identity routines to verify TLSN-based identity claims. | ||
| */ |
There was a problem hiding this comment.
Module doc comment is misleading — says "using WASM" but this module explicitly does not use WASM.
Line 4 states "Provides server-side verification of TLSNotary proofs using WASM" but the underlying verifier.ts operates in "structure-only verification mode" with WASM explicitly noted as unsupported. This will confuse future readers.
/**
* TLSNotary Verification Module
*
- * Provides server-side verification of TLSNotary proofs using WASM.
+ * Provides server-side structural validation of TLSNotary proofs.
+ * NOTE: Full cryptographic verification via WASM is not yet supported in Node.js.
* Used by GCR identity routines to verify TLSN-based identity claims.
*/📝 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.
| /** | |
| * TLSNotary Verification Module | |
| * | |
| * Provides server-side verification of TLSNotary proofs using WASM. | |
| * Used by GCR identity routines to verify TLSN-based identity claims. | |
| */ | |
| /** | |
| * TLSNotary Verification Module | |
| * | |
| * Provides server-side structural validation of TLSNotary proofs. | |
| * NOTE: Full cryptographic verification via WASM is not yet supported in Node.js. | |
| * Used by GCR identity routines to verify TLSN-based identity claims. | |
| */ |
🤖 Prompt for AI Agents
In `@src/libs/tlsnotary/index.ts` around lines 1 - 6, Update the module doc
comment to remove the incorrect "using WASM" claim and clearly state that this
TLSNotary Verification Module performs server-side, structure-only verification
without WASM support; reference the verifier implementation (verifier.ts) and
its structure-only verification mode so readers know where the non-WASM logic
lives and that WASM is intentionally unsupported.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/libs/tlsnotary/verifier.ts`:
- Around line 102-140: The fallback extraction in maybeParseJsonText is too
greedy because it uses indexOf("{") and lastIndexOf("}") (and similarly for
"["/"]"); change this to find a properly balanced JSON span by scanning forward
from the first opening brace/bracket and counting nested braces/brackets until
the matching closing character is found (so you return the minimal balanced
substring), and apply the same balanced-scan logic for arrays as well; update
maybeParseJsonText to return the balanced object or array substring (or null if
no matching closer is found) instead of slicing to the last closing character.
- Around line 161-198: The current extractUserFromRawText fallback uses separate
regexes that can match "id" and "username/login" from different JSON objects in
the same text; update extractUserFromRawText (and the github/discord/telegram
branches) to ensure username/login and id are captured from the same JSON object
by either: 1) using a single regex that matches a JSON object/block containing
both fields (e.g. match a { ... "login": ... ... "id": ... } or similar
non-greedy bracketed pattern) and capture both groups, or 2) attempt to parse
the text for JSON objects (split by braces or try JSON.parse on found
substrings) and extract username+id from the same parsed object; replace the
separated regexes (e.g. /"login".../ and /"id".../) with a proximity-scoped
match so you never combine fields from different objects.
🧹 Nitpick comments (2)
src/features/incentive/PointSystem.ts (1)
935-1022:awardTelegramTLSNPointsduplicates ~90% ofawardTelegramPoints.The only behavioral difference is the absence of the
group_membershipattestation check. Consider extracting the shared logic into a private helper (e.g.,_awardTelegramPointsCore) and calling it from both methods, passing a flag or predicate for the group-membership gate.documentation/tlsn-oauth-flow.md (1)
3-3: Minor: capitalize platform names in prose."github" → "GitHub", "discord" → "Discord" per their official names, as flagged by static analysis.
…d-tlsn-identity-assign-and-remove-logics
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts`:
- Around line 1663-1669: The call to IncentiveManager.telegramTLSNLinked
currently passes (editOperation.account, String(userId), referralCode) but the
method signature is (userId: string, telegramUserId: string, referralCode?:
string); swap the first two arguments so the app userId is passed first and the
telegram account second (e.g. call with String(userId), editOperation.account,
referralCode), and ensure referralCode is passed only when present and types
match the function signature; verify this aligns with the intended semantics in
GCRIdentityRoutines.
In `@src/libs/tlsnotary/verifier.ts`:
- Around line 724-817: The recvHash comparison in verifyTLSNProof
(computedRecvHash vs recvHash after decodeRevealedRecv) is self-referential
because both values come from the client; add a clear inline comment at that
comparison site (near computedRecvHash.toLowerCase() !== recvHash.toLowerCase())
stating this check only ensures internal consistency of the supplied payload and
is NOT a cryptographic authenticity check, and reference the existing TODO about
binding recvHash to the TLSNotary proof.data via WASM verification
(verifyTLSNotaryPresentation / proof.data) so future readers don't treat it as a
security boundary.
🧹 Nitpick comments (4)
src/libs/tlsnotary/verifier.ts (2)
102-164: DRY:findBalancedJsonValueandfindBalancedJsonValueAtshare identical balanced-brace parsing logic.
findBalancedJsonValueiterates over start positions and delegates to the same brace-matching algorithm duplicated infindBalancedJsonValueAt. Consider implementingfindBalancedJsonValuein terms offindBalancedJsonValueAt:♻️ Proposed refactor
function findBalancedJsonValue(text: string): string | null { for (let start = 0; start < text.length; start++) { - const first = text[start] - if (first !== "{" && first !== "[") { + if (text[start] !== "{" && text[start] !== "[") { continue } - - const stack: string[] = [first] - let inString = false - let escaped = false - - for (let i = start + 1; i < text.length; i++) { - const ch = text[i] - // ... ~60 lines of duplicated logic ... - } + const match = findBalancedJsonValueAt(text, start) + if (match) { + return match.value + } } - return null }Also applies to: 166-229
305-490: Consider extracting per-context extraction logic into a helper map to reduce repetition.The github/discord/telegram branches are repeated three times (structured parse, regex on candidate, final fallback). A context-to-field-names mapping could reduce ~180 lines to ~60, but this is optional given the code works correctly.
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (2)
1450-1460:TLSN_EXPECTED_ENDPOINTSvalues (server,pathPrefix) are never used.The map is only accessed to check key existence (line 1548:
this.TLSN_EXPECTED_ENDPOINTS[context]). TheserverandpathPrefixvalues are never validated against the actual proof content. Either remove the unused values (replace with aSet<string>) or implement the endpoint validation.
1596-1620: TLSN identities co-stored with OAuth identities inweb2— document this design choice.TLSN-verified identities are stored in the same
identities.web2[context]array as OAuth-verified identities, distinguished only byproofType: "tlsn". This works but is a non-obvious design decision. A brief comment at line 1596 explaining why TLSN identities live inweb2(e.g., "TLSN identities share the web2 store so isFirstConnection and incentive deduplication work across both verification paths") would help future maintainers.
| if (isFirst) { | ||
| await IncentiveManager.telegramTLSNLinked( | ||
| editOperation.account, | ||
| String(userId), | ||
| referralCode, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for telegramTLSNLinked definition in IncentiveManager
rg -nP 'telegramTLSNLinked' --type=ts -C5Repository: kynesyslabs/node
Length of output: 2101
Fix parameter mapping for IncentiveManager.telegramTLSNLinked call.
The method exists but has a signature mismatch. The actual definition expects (userId: string, telegramUserId: string, referralCode?: string), but the call at lines 1664–1668 passes arguments as if the signature were (account, userId, referralCode). The first parameter editOperation.account is passed where userId is expected, and String(userId) is passed where telegramUserId is expected. Verify the parameter order and semantic intent match the method's design.
🤖 Prompt for AI Agents
In `@src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts` around lines
1663 - 1669, The call to IncentiveManager.telegramTLSNLinked currently passes
(editOperation.account, String(userId), referralCode) but the method signature
is (userId: string, telegramUserId: string, referralCode?: string); swap the
first two arguments so the app userId is passed first and the telegram account
second (e.g. call with String(userId), editOperation.account, referralCode), and
ensure referralCode is passed only when present and types match the function
signature; verify this aligns with the intended semantics in
GCRIdentityRoutines.
| export async function verifyTLSNProof(payload: TLSNIdentityPayload): Promise<{ | ||
| success: boolean | ||
| message: string | ||
| extractedUsername?: string | ||
| extractedUserId?: string | ||
| }> { | ||
| const { context, proof, recvHash, revealedRecv, username, userId } = payload | ||
|
|
||
| // Validate context | ||
| if (!["github", "discord", "telegram"].includes(context)) { | ||
| return { | ||
| success: false, | ||
| message: `Unsupported TLSN context: ${context}`, | ||
| } | ||
| } | ||
|
|
||
| if (typeof recvHash !== "string" || !/^[0-9a-fA-F]{64}$/.test(recvHash)) { | ||
| return { | ||
| success: false, | ||
| message: "Invalid TLSN recvHash: expected 64-char hex sha256", | ||
| } | ||
| } | ||
|
|
||
| // Verify the proof structure | ||
| const verified = await verifyTLSNotaryPresentation(proof) | ||
| if (!verified.success) { | ||
| return { | ||
| success: false, | ||
| message: `Proof verification failed: ${verified.error}`, | ||
| } | ||
| } | ||
|
|
||
| const recvBytes = decodeRevealedRecv(revealedRecv) | ||
| if (!recvBytes) { | ||
| return { | ||
| success: false, | ||
| message: "Invalid TLSN revealedRecv: expected byte array (0-255)", | ||
| } | ||
| } | ||
|
|
||
| if (recvBytes.length === 0) { | ||
| return { | ||
| success: false, | ||
| message: "Invalid TLSN revealedRecv: empty payload", | ||
| } | ||
| } | ||
|
|
||
| const computedRecvHash = Hashing.sha256Bytes(recvBytes) | ||
| if (computedRecvHash.toLowerCase() !== recvHash.toLowerCase()) { | ||
| return { | ||
| success: false, | ||
| message: | ||
| "recvHash mismatch: provided hash does not match disclosed recv bytes", | ||
| } | ||
| } | ||
|
|
||
| const responseBody = parseDisclosedRecvBody(recvBytes) | ||
| const rawText = new TextDecoder().decode(recvBytes) | ||
| const extractedUser = | ||
| (responseBody ? extractUser(context, responseBody) : null) || | ||
| extractUserFromRawText(context, rawText) | ||
| if (!extractedUser) { | ||
| return { | ||
| success: false, | ||
| message: `Failed to extract user from ${context} revealedRecv payload`, | ||
| } | ||
| } | ||
|
|
||
| if (extractedUser.username !== username) { | ||
| return { | ||
| success: false, | ||
| message: `Username mismatch: claimed '${username}', proof contains '${extractedUser.username}'`, | ||
| } | ||
| } | ||
|
|
||
| if (extractedUser.userId !== String(userId)) { | ||
| return { | ||
| success: false, | ||
| message: `UserId mismatch: claimed '${String( | ||
| userId, | ||
| )}', proof contains '${extractedUser.userId}'`, | ||
| } | ||
| } | ||
|
|
||
| log.info( | ||
| `[TLSNotary Verifier] ${context} proof and recvHash validated for userId=${userId}`, | ||
| ) | ||
|
|
||
| return { | ||
| success: true, | ||
| message: "Proof and recvHash verified", | ||
| extractedUsername: extractedUser.username, | ||
| extractedUserId: extractedUser.userId, | ||
| } |
There was a problem hiding this comment.
recvHash integrity check is self-referential — both recvHash and revealedRecv are client-supplied.
The function hashes revealedRecv and compares it to recvHash (line 772), but both values come from the same untrusted client payload. A malicious client can submit any revealedRecv with a matching recvHash. Without cryptographic binding of recvHash to the TLSNotary proof data field (which requires WASM verification), this check only validates internal consistency of the client's submission, not authenticity.
This is already acknowledged in the file header's TODO, but consider adding an inline comment at the hash-comparison site so future readers don't mistake this for a security boundary.
🤖 Prompt for AI Agents
In `@src/libs/tlsnotary/verifier.ts` around lines 724 - 817, The recvHash
comparison in verifyTLSNProof (computedRecvHash vs recvHash after
decodeRevealedRecv) is self-referential because both values come from the
client; add a clear inline comment at that comparison site (near
computedRecvHash.toLowerCase() !== recvHash.toLowerCase()) stating this check
only ensures internal consistency of the supplied payload and is NOT a
cryptographic authenticity check, and reference the existing TODO about binding
recvHash to the TLSNotary proof.data via WASM verification
(verifyTLSNotaryPresentation / proof.data) so future readers don't treat it as a
security boundary.
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores