Skip to content

Tg identities#466

Closed
cwilvx wants to merge 18 commits intotestnetfrom
tg_identities
Closed

Tg identities#466
cwilvx wants to merge 18 commits intotestnetfrom
tg_identities

Conversation

@cwilvx
Copy link
Contributor

@cwilvx cwilvx commented Sep 8, 2025

PR Type

Enhancement


Description

  • Add comprehensive Telegram identity verification system

  • Implement anti-replay protection with challenge hash embedding

  • Integrate Telegram points management in incentive system

  • Update point values for Twitter and Telegram linking


Diagram Walkthrough

flowchart LR
  A["User Request"] --> B["Challenge Generation"]
  B --> C["Bot Attestation"]
  C --> D["Dual Signature Verification"]
  D --> E["Transaction Creation"]
  E --> F["Challenge Hash Embedding"]
  F --> G["Identity Binding"]
Loading

File Walkthrough

Relevant files
Enhancement
9 files
PointSystem.ts
Add Telegram points management and update values                 
+135/-4 
index.ts
Add Telegram proof parser integration                                       
+23/-2   
telegram.ts
Implement Telegram proof parser with validation                   
+115/-0 
GCRIdentityRoutines.ts
Add Telegram identity linking and unlinking support           
+37/-3   
IncentiveManager.ts
Add Telegram incentive management hooks                                   
+21/-1   
identityManager.ts
Update connection filtering for Telegram accounts               
+5/-3     
telegram.ts
Implement comprehensive Telegram verification system         
+476/-0 
handleIdentityRequest.ts
Add transaction parameter for Telegram validation               
+1/-0     
server_rpc.ts
Add Telegram challenge and verification endpoints               
+74/-0   
Formatting
1 files
validateUint8Array.ts
Fix code formatting consistency                                                   
+2/-2     
Documentation
2 files
TG_INTEGRATION.md
Add comprehensive Telegram integration guide                         
+486/-0 
TG_FLOW.md
Add Telegram verification system documentation                     
+202/-0 

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 8, 2025

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tg_identities

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Replay protection:
New anti-replay relies on embedding SHA256 challenge hash in transaction and matching during validation. Ensure transaction.payload.attestation_id cannot be altered post-signing and is covered by the user's signature. Also verify that authorized bot list derived from genesis balances is correct and cannot be spoofed; parsing genesis content should be hardened against unexpected shapes.

⚡ Recommended focus areas for review

Possible Issue

Challenge parsing relies on splitting by underscores with strict length=6; if future format or unexpected underscores appear, valid challenges may be rejected. Consider a more robust parser or regex anchored to exact format to avoid subtle failures.

} | null {
    const parts = challenge.split("_")
    // Expected format: DEMOS_TG_BIND_<demos_address>_<timestamp>_<nonce>
    if (parts.length !== 6 || parts[0] !== "DEMOS" || parts[1] !== "TG" || parts[2] !== "BIND") {
        return null
    }
    const demosAddress = parts[3]
    const timestamp = parseInt(parts[4])
    const nonce = parts[5]
    if (isNaN(timestamp)) {
        return null
    }
API Contract Risk

verifyWeb2Proof now optionally requires a Transaction for Telegram to pass into parser; ensure all call sites that handle Telegram provide this, or define a clear fallback path and typings so non-Telegram contexts aren’t impacted at runtime.

export async function verifyWeb2Proof(
    payload: Web2CoreTargetIdentityPayload,
    sender: string,
    transaction?: Transaction,
) {
    let parser: typeof TwitterProofParser | typeof GithubProofParser | typeof TelegramProofParser

    switch (payload.context) {
        case "twitter":
            parser = TwitterProofParser
            break
        case "github":
            parser = GithubProofParser
            break
Type Safety

Access to socialAccounts uses 'as any' for telegram checks which can mask bugs. Prefer refining the breakdown type to include 'telegram' and guarding absent structures to avoid runtime errors.

// Check if user already has Telegram points specifically
if ((userPointsWithIdentities.breakdown.socialAccounts as any).telegram > 0) {
    return {
        result: 200,
        response: {
            pointsAwarded: 0,
            totalPoints: userPointsWithIdentities.totalPoints,
            message: "Telegram points already awarded",
        },
        require_reply: false,
        extra: {},
    }

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Replace genesis-balances bot whitelist

Authorizing Telegram bots by deriving keys from genesis balances is a weak trust
anchor—any genesis-funded key becomes an attester and rotation requires a hard
fork. Introduce a dedicated, explicit whitelist (e.g., a genesis
"authorized_bots" field or on-chain/system registry) and enforce verification
strictly against that list, with support for safe key rotation. This prevents
unauthorized entities from issuing valid Telegram attestations and strengthens
the system’s security model.

Examples:

src/libs/identity/tools/telegram.ts [95-97]
            this.authorizedBots = genesisData.balances?.map((balance: [string, string]) => 
                balance[0].toLowerCase(),
            ) || []

Solution Walkthrough:

Before:

// src/libs/identity/tools/telegram.ts
class Telegram {
    async getAuthorizedBots(): Promise<string[]> {
        // ...
        const genesisBlock = await Chain.getGenesisBlock();
        // ... parse genesisData ...

        // All addresses from the genesis balances are considered authorized bots.
        this.authorizedBots = genesisData.balances?.map((balance: [string, string]) => 
            balance[0].toLowerCase(),
        ) || [];
        
        return this.authorizedBots;
    }
}

After:

// src/libs/identity/tools/telegram.ts
class Telegram {
    async getAuthorizedBots(): Promise<string[]> {
        // ...
        const genesisBlock = await Chain.getGenesisBlock();
        // ... parse genesisData ...

        // A dedicated, explicit list of bots is read from genesis.
        // This could also point to an on-chain registry for updatability.
        this.authorizedBots = genesisData.authorized_telegram_bots?.map(addr => 
            addr.toLowerCase()
        ) || [];
        
        return this.authorizedBots;
    }
}
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical security flaw in the bot authorization logic, where any genesis-funded account is implicitly trusted, and proposes a much more secure and flexible explicit whitelist mechanism.

High
General
Add JSON parsing error handling

The code doesn't handle JSON parsing errors that could occur if the request body
is malformed. This could cause the server to crash or return unclear error
messages.

src/libs/network/server_rpc.ts [398-405]

-const payload = await req.json()
+let payload;
+try {
+    payload = await req.json()
+} catch (error) {
+    return jsonResponse({ 
+        error: "Invalid JSON in request body", 
+    }, 400)
+}
 
 // Validate request structure
 if (!payload.demos_address || typeof payload.demos_address !== "string") {
     return jsonResponse({ 
         error: "Invalid request: demos_address is required", 
     }, 400)
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a missing error handling case for JSON parsing, which enhances the server's robustness against malformed requests.

Medium
Fix cache validation logic

The cache check only validates that bots array is not empty, but doesn't verify
if the cache is actually valid. An empty authorized bots list could be a
legitimate state that should be cached too.

src/libs/identity/tools/telegram.ts [62-64]

-if (Date.now() - this.lastGenesisCheck < 3600000 && this.authorizedBots.length > 0) {
+if (Date.now() - this.lastGenesisCheck < 3600000) {
     return this.authorizedBots
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that an empty list of authorized bots is a valid state that should be cached, improving the caching logic's correctness.

Low
Remove unsafe type assertion

Using type assertion as any bypasses TypeScript safety and could cause runtime
errors if the property doesn't exist. The code should safely check for the
property existence before accessing it.

src/features/incentive/PointSystem.ts [639]

-if ((userPointsWithIdentities.breakdown.socialAccounts as any).telegram > 0) {
+const telegramPoints = userPointsWithIdentities.breakdown.socialAccounts?.telegram || 0;
+if (telegramPoints > 0) {
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly replaces an unsafe as any type assertion with safe optional chaining, improving code quality and preventing potential runtime errors.

Low
  • More

@tcsenpai
Copy link
Contributor

I think this is superseeded by the #468, please @cwilvx take a look at that (i rewrote that based on the latest modifications in https://github.com/kynesyslabs/tg_verification_bot). If that's ok let's close this one.

@tcsenpai tcsenpai closed this Sep 17, 2025
@tcsenpai tcsenpai deleted the tg_identities branch December 4, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants