Skip to content

Conversation

@omBratteng
Copy link
Member

Add tests for #3230

@omBratteng omBratteng requested review from a team and capJavert as code owners October 27, 2025 17:38
@omBratteng omBratteng requested review from AmarTrebinjac, ilasw, nensidosari, rebelchris and sshanzel and removed request for a team October 27, 2025 17:38
@pulumi
Copy link

pulumi bot commented Oct 27, 2025

🍹 The Update (preview) for dailydotdev/api/prod (at 628f63a) was successful.

Resource Changes

    Name                                                   Type                           Operation
+   vpc-native-api-db-migration-907c261c                   kubernetes:batch/v1:Job        create
~   vpc-native-update-source-public-threshold-cron         kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-users-cron                     kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-stale-user-transactions-cron          kubernetes:batch/v1:CronJob    update
+   vpc-native-api-clickhouse-migration-907c261c           kubernetes:batch/v1:Job        create
~   vpc-native-clean-zombie-user-companies-cron            kubernetes:batch/v1:CronJob    update
~   vpc-native-calculate-top-readers-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-update-tags-str-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-update-source-tag-view-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-check-analytics-report-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-gifted-plus-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-generate-search-invites-cron                kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-history-day-clickhouse-cron  kubernetes:batch/v1:CronJob    update
~   vpc-native-update-tag-recommendations-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-private-deployment                          kubernetes:apps/v1:Deployment  update
~   vpc-native-update-views-cron                           kubernetes:batch/v1:CronJob    update
+-  vpc-native-debezium-props                              kubernetes:core/v1:Secret      create-replacement
~   vpc-native-hourly-notification-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-clickhouse-cron              kubernetes:batch/v1:CronJob    update
+-  vpc-native-debezium-deployment                         kubernetes:apps/v1:Deployment  create-replacement
~   vpc-native-update-current-streak-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-temporal-deployment                         kubernetes:apps/v1:Deployment  update
~   vpc-native-update-trending-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-images-cron                    kubernetes:batch/v1:CronJob    update
+-  vpc-native-k8s-secret                                  kubernetes:core/v1:Secret      create-replacement
~   vpc-native-personalized-digest-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-daily-digest-cron                           kubernetes:batch/v1:CronJob    update
~   vpc-native-sync-subscription-with-cio-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-bg-deployment                               kubernetes:apps/v1:Deployment  update
-   vpc-native-api-clickhouse-migration-aacc3bb8           kubernetes:batch/v1:Job        delete
~   vpc-native-update-highlighted-views-cron               kubernetes:batch/v1:CronJob    update
~   vpc-native-ws-deployment                               kubernetes:apps/v1:Deployment  update
~   vpc-native-deployment                                  kubernetes:apps/v1:Deployment  update
-   vpc-native-api-db-migration-aacc3bb8                   kubernetes:batch/v1:Job        delete
~   vpc-native-personalized-digest-deployment              kubernetes:apps/v1:Deployment  update
~   vpc-native-generic-referral-reminder-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-validate-active-users-cron                  kubernetes:batch/v1:CronJob    update

@omBratteng
Copy link
Member Author

@claude review this pl0x as a 1337 h3x0r

@claude
Copy link

claude bot commented Oct 28, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@claude
Copy link

claude bot commented Oct 28, 2025

Claude finished @omBratteng's task —— View job


Security-Focused Code Review 🛡️

Reviewing this PR with a security perspective as requested:

Todo List

  • Read and analyze the redirector route changes
  • Review the test file additions
  • Check .env changes
  • Analyze for security vulnerabilities:
    • URL validation and sanitization
    • Open redirect vulnerabilities
    • Input validation issues
    • Injection attack vectors
  • Review test coverage for security scenarios
  • Provide comprehensive security assessment

Security Analysis

Good Security Practices Found:

  1. URL Sanitization: src/routes/redirector.ts:44

    • Uses encodeURI() to properly encode URLs before redirecting
    • Constructs URLs using the URL constructor for proper parsing
  2. UUID Validation: src/routes/redirector.ts:81

    • Validates referral IDs using z.uuidv4().safeParse() to ensure proper UUID format
    • Gracefully handles invalid UUIDs with logging instead of failing
  3. IP-based Fraud Prevention: src/routes/redirector.ts:123

    • Uses HMAC-based IP hashing to prevent self-referral abuse
    • Prevents users from visiting their own referral links
  4. Referrer Validation: src/routes/redirector.ts:107

    • Validates that referrer starts with https://www.linkedin.com/
    • Prevents abuse from non-LinkedIn sources
  5. Authentication Checks: src/routes/redirector.ts:90

    • Skips referral tracking for logged-in users to prevent double-counting

⚠️ Potential Security Concerns:

  1. Limited Referrer Validation: src/routes/redirector.ts:107

    if (referrer.startsWith('https://www.linkedin.com/') === false)
    • Issue: Only checks the prefix, could be bypassed with URLs like https://www.linkedin.com.evil.com/
    • Recommendation: Use proper URL parsing and validate the hostname:
    try {
      const referrerUrl = new URL(referrer);
      if (referrerUrl.hostname !== 'www.linkedin.com') {
        // reject
      }
    } catch {
      // invalid URL, reject
    }
  2. XSS in HTML Response: src/routes/redirector.ts:66-68

    `<html><head><meta name="robots" content="noindex,nofollow"><meta http-equiv="refresh" content="0;URL=${encodedUri}${
      req.query.a ? `#${req.query.a}` : ''
    }"></head></html>`
    • Issue: The req.query.a parameter is directly inserted into HTML without encoding
    • Risk: Could lead to XSS if an attacker crafts a URL with malicious content in the a parameter
    • Recommendation: HTML-encode the query parameter or validate it more strictly
  3. Database Error Information Leakage: src/routes/redirector.ts:137-142

    • Issue: Database errors are logged but could potentially expose sensitive information
    • Current Implementation: Good - errors are logged at debug level, not exposed to users

🧪 Test Coverage Assessment:

Excellent security test coverage includes:

  • Invalid UUID handling (__tests__/redirector.ts:174)
  • Self-referral prevention (__tests__/redirector.ts:289)
  • Non-LinkedIn referrer blocking (__tests__/redirector.ts:240)
  • Already visited referral handling (__tests__/redirector.ts:312)
  • Status validation (__tests__/redirector.ts:332)

Missing security tests:

  • XSS vulnerability in the a query parameter
  • Referrer validation bypass attempts
  • HTML injection in redirect content

📝 Recommendations:

  1. Fix Referrer Validation (High Priority):

    const referrerUrl = new URL(referrer);
    if (referrerUrl.hostname !== 'www.linkedin.com') {
      // reject
    }
  2. Fix XSS Vulnerability (High Priority):

    req.query.a ? `#${encodeURIComponent(req.query.a)}` : ''
  3. Add Security Tests:

    • Test XSS prevention in a parameter
    • Test referrer validation bypass attempts

🔒 Overall Security Assessment:

The redirector implementation shows good security awareness with proper UUID validation, IP-based fraud prevention, and referrer checks. However, there are two notable vulnerabilities that should be addressed before merge:

  1. Weak referrer validation that could be bypassed
  2. XSS vulnerability in the redirect HTML response

The test coverage is comprehensive for business logic but could be enhanced with more security-focused test cases.


@omBratteng
Copy link
Member Author

  1. No, because I explicitly check for https://www.linkedin.com/ so it's never going to match https://www.linkedin.com.evil.com/, since it would have to be https://www.linkedin.com/evil.com, which is still valid for LinkedIn.
  2. Not related to this route
  3. No sensitive information

Copy link
Contributor

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@omBratteng omBratteng merged commit 84f2e15 into feat-linkedin-button Oct 28, 2025
8 checks passed
@omBratteng omBratteng deleted the test-linkedin-button branch October 28, 2025 12:42
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.

3 participants