Skip to content

Add trusted proxy support - #542

Open
BelfordZ wants to merge 1 commit into
devfrom
fix/add-trusted-proxy-ip-configuration
Open

BelfordZ wants to merge 1 commit into
devfrom
fix/add-trusted-proxy-ip-configuration

Conversation

@BelfordZ

@BelfordZ BelfordZ commented May 21, 2025 •

Copy link
Copy Markdown
Contributor

User description

Summary

  • add a trustedProxies config option with env var override
  • implement getClientIp to only honor X-Forwarded-For from trusted sources
  • update injection endpoints and logging to use the new helper
  • document configuring trusted proxies

Testing

  • npm run compile
  • npm test

PR Type

Enhancement, Documentation


Description

  • Add trusted proxy IP support via config and env variable

  • Implement secure getClientIp to validate proxy headers

  • Update endpoints and logging to use trusted proxy logic

  • Document trusted proxy configuration in README and setup guide


Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Add trustedProxies config and env variable support             

src/config/index.ts

  • Add trustedProxies option to server config interface
  • Initialize trustedProxies as empty array in default config
  • Parse TRUSTED_PROXIES env variable to override config
  • +15/-0   
    index.ts
    Use secure client IP extraction with trusted proxies         

    src/index.ts

  • Replace unsafeGetClientIp with new getClientIp using trusted proxies
  • Update endpoints and logging to use secure client IP extraction
  • +4/-4     
    requests.ts
    Implement secure getClientIp with trusted proxy validation

    src/utils/requests.ts

  • Add getClientIp function validating trusted proxies
  • Use proxy-addr for secure proxy header parsing
  • +34/-0   
    Documentation
    README.md
    Document trusted proxy configuration in README                     

    README.md

  • Document TRUSTED_PROXIES env variable and config option
  • Explain usage for deployments behind reverse proxies
  • +4/-0     
    config.json
    Add trustedProxies to example config.json                               

    config.json

    • Add trustedProxies array to example server config
    +1/-0     
    local-environment-setup.md
    Document trusted proxy setup in environment guide               

    local-environment-setup.md

    • Document trusted proxy setup in network configuration section
    +1/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🏅 Score: 88
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Proxy Trust Logic

    The new getClientIp function uses the proxy-addr library to validate trusted proxies and extract the client IP. Ensure that the logic correctly handles edge cases, such as malformed headers, IPv6 addresses, and empty or misconfigured trustedProxies arrays. Also, verify that the fallback to the remote address is secure and reliable.

    export function getClientIp(req, trustedProxies: string[] = []): string {
      if (req == null) {
        return null
      }
    
      const remoteAddress =
        (req.connection ? req.connection.remoteAddress : null) ||
        (req.socket ? req.socket.remoteAddress : null) ||
        null
    
      const forwarded = req.headers['x-forwarded-for']
      if (forwarded && remoteAddress) {
        const trust = proxyaddr.compile(trustedProxies)
        if (trust(remoteAddress)) {
          try {
            return proxyaddr(req, trust) as string
          } catch {
            // fall back to remote address on any error
          }
        }
      }
    
      return remoteAddress as string
    }
    Integration of getClientIp

    The injection endpoints and logging now use getClientIp with the trusted proxies configuration. Confirm that all relevant endpoints and logging statements have been updated and that the function is called with the correct parameters, especially in cases where config.server.trustedProxies may be undefined.

          requestIP = getClientIp(req, config.server.trustedProxies || []) || 'cant-get-ip'
        }
    
        let serviePointSpenders: Map<string, number> = debugServicePointSpendersByType.get(key)
        if (!serviePointSpenders) {
          serviePointSpenders = new Map()
          debugServicePointSpendersByType.set(key, serviePointSpenders)
        }
        if (serviePointSpenders.has(requestIP) === false) {
          serviePointSpenders.set(requestIP, points)
        } else {
          const currentPoints = serviePointSpenders.get(requestIP)
          serviePointSpenders.set(requestIP, currentPoints + points)
        }
        debugTotalServicePointRequests += points
    
        //upate debugServiePointByType
        if (debugServicePointsByType.has(key) === false) {
          debugServicePointsByType.set(key, points)
        } else {
          const currentPoints = debugServicePointsByType.get(key)
          debugServicePointsByType.set(key, currentPoints + points)
        }
      }
    
      //is the new operation too expensive?
      if (totalPoints + points > maxAllowedPoints) {
        nestedCountersInstance.countEvent('shardeum-service-points', 'fail: not enough points available to spend')
        return false
      }
    
      //Add new entry to array
      const newEntry = { points, ts: nowTs }
      servicePointSpendHistory.unshift(newEntry)
    
      nestedCountersInstance.countEvent('shardeum-service-points', 'pass: points available to spend')
      return true
    }
    
    function pruneOldBlocks(): void {
      /* eslint-disable security/detect-object-injection */
      const maxOldBlocksCount = ShardeumFlags.maxNumberOfOldBlocks || 256
      if (latestBlock > maxOldBlocksCount) {
        for (let i = 10; i > 0; i--) {
          const block = latestBlock - maxOldBlocksCount - i
          if (blocks[block]) {
            try {
              const blockHash = readableBlocks[block].hash
              delete blocks[block]
              delete blocksByHash[blockHash]
              delete readableBlocks[block]
              /* prettier-ignore */ if (ShardeumFlags.VerboseLogs) console.log('Lengths of blocks after pruning', Object.keys(blocksByHash).length, Object.keys(readableBlocks).length)
            } catch (e) {
              /* prettier-ignore */ if (logFlags.error) console.log('Error: pruneOldBlocks', e)
            }
          }
        }
      }
      /* eslint-enable security/detect-object-injection */
    }
    
    function convertToReadableBlock(block: Block): ShardeumBlockOverride {

    Comment thread src/utils/requests.ts
    Comment on lines +151 to +174
    export function getClientIp(req, trustedProxies: string[] = []): string {
    if (req == null) {
    return null
    }

    const remoteAddress =
    (req.connection ? req.connection.remoteAddress : null) ||
    (req.socket ? req.socket.remoteAddress : null) ||
    null

    const forwarded = req.headers['x-forwarded-for']
    if (forwarded && remoteAddress) {
    const trust = proxyaddr.compile(trustedProxies)
    if (trust(remoteAddress)) {
    try {
    return proxyaddr(req, trust) as string
    } catch {
    // fall back to remote address on any error
    }
    }
    }

    return remoteAddress as string
    }

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: The function may return non-string values (such as undefined or null) in some cases, which could cause downstream errors. Ensure the return value is always a string or explicitly null. [possible issue, importance: 7]

    Suggested change
    export function getClientIp(req, trustedProxies: string[] = []): string {
    if (req == null) {
    return null
    }
    const remoteAddress =
    (req.connection ? req.connection.remoteAddress : null) ||
    (req.socket ? req.socket.remoteAddress : null) ||
    null
    const forwarded = req.headers['x-forwarded-for']
    if (forwarded && remoteAddress) {
    const trust = proxyaddr.compile(trustedProxies)
    if (trust(remoteAddress)) {
    try {
    return proxyaddr(req, trust) as string
    } catch {
    // fall back to remote address on any error
    }
    }
    }
    return remoteAddress as string
    }
    export function getClientIp(req, trustedProxies: string[] = []): string | null {
    if (req == null) {
    return null
    }
    const remoteAddress =
    (req.connection ? req.connection.remoteAddress : null) ||
    (req.socket ? req.socket.remoteAddress : null) ||
    null
    const forwarded = req.headers['x-forwarded-for']
    if (forwarded && remoteAddress) {
    const trust = proxyaddr.compile(trustedProxies)
    if (trust(remoteAddress)) {
    try {
    const ip = proxyaddr(req, trust)
    return typeof ip === 'string' ? ip : null
    } catch {
    // fall back to remote address on any error
    }
    }
    }
    return typeof remoteAddress === 'string' ? remoteAddress : null
    }

    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.

    1 participant