fix: strip port from X-Forwarded-For in request.ip (#827) by guoyangzhen · Pull Request #1943 · koajs/koa

guoyangzhen · 2026-03-18T09:14:58Z

Summary

Fixes #827

When X-Forwarded-For contains IP:port format (e.g., from Azure: 23.243.1.1:38242), request.ip should return only the IP address without the port.

Problem

On Azure and other environments, X-Forwarded-For includes the port:

'x-forwarded-for': '23.243.1.1:38242'

Previously request.ip would return 23.243.1.1:38242, which is not a valid IP address.

Fix

Strip port from the IP in the ip getter:

IPv4 with port: 1.2.3.4:8080 → 1.2.3.4
IPv6 with port: [::1]:8080 → ::1
Plain IPv4/IPv6 (no port): unchanged

Note

There was an earlier attempt (#1205) from 2018 that is no longer mergeable due to conflicts. This is a clean implementation targeting the current codebase.

When X-Forwarded-For contains IP:port (e.g., from Azure), request.ip should return only the IP address. Handles both IPv4 (1.2.3.4:8080) and IPv6 ([::1]:8080) formats. Fixes koajs#827

codecov · 2026-03-18T09:16:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (e0ba8ef) to head (3bb63fe).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1943   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files           9        9           
  Lines        2105     2133   +28     
=======================================
+ Hits         2103     2131   +28     
  Misses          2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

guoyangzhen · 2026-03-28T14:02:56Z

Hi! This PR has been open for about 10 days. The fix addresses a real bug (#827) where X-Forwarded-For includes port numbers. Tests pass. Would appreciate a review when you have time.

fengmk2 · 2026-03-28T14:45:45Z

@guoyangzhen Can you directly use the https://github.com/jshttp/forwarded library to parse it?
And add a unit test to verify that the port is removed.

Use the jshttp/forwarded library to properly parse X-Forwarded-For headers instead of manual string manipulation. Changes: - Add forwarded dependency (^0.2.0) from jshttp ecosystem - Use forwarded() to parse XFF chain in ip getter - Add stripPort() helper for IPv4/IPv6 port removal - Add unit tests for port stripping (IPv4, IPv6, with/without port) Fixes koajs#827

guoyangzhen · 2026-04-01T03:59:16Z

Updated the PR to use the forwarded library as requested.

Changes:

Added forwarded (^0.2.0) from the jshttp ecosystem as a dependency
Replaced manual string parsing with forwarded(this.req) to properly parse the X-Forwarded-For chain
Added a stripPort() helper that handles both IPv4 (1.2.3.4:8080) and bracketed IPv6 ([::1]:8080)
Added unit tests for port stripping with both IPv4 and IPv6 addresses

The forwarded library returns \[socketAddr, ...xffEntriesReversed\], so the original client IP (last in the XFF chain) is at addrs[addrs.length - 1]. We also strip the port from this.ips[0]\] as a fallback for custom proxyIpHeader` configurations.

fengmk2 · 2026-04-01T05:53:58Z

@codex review

Copilot

Pull request overview

This PR fixes request.ip returning an invalid value when X-Forwarded-For entries include a port (e.g. 1.2.3.4:8080), by stripping the port portion and adding tests to cover IPv4/IPv6 cases.

Changes:

Add forwarded dependency and use it in request.ip resolution.
Introduce stripPort() helper to remove :port (and IPv6 brackets) from forwarded addresses.
Add a new test suite covering X-Forwarded-For values containing ports.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
package.json	Adds the `forwarded` dependency used for parsing `X-Forwarded-For`.
lib/request.js	Updates `request.ip` logic to strip ports and leverages `forwarded()` output.
tests/request/ip-port.test.js	Adds test cases for IPv4/IPv6 `X-Forwarded-For` entries that include ports.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-01T05:57:00Z

+      // Use forwarded() to properly parse the X-Forwarded-For chain.
+      // forwarded() returns [socketAddr, ...xffEntriesReversed], so the
+      // first XFF entry (original client) is at the end of the array.
+      // See: https://github.com/koajs/koa/issues/827
+      const addrs = forwarded(this.req)
+      const addr = stripPort(this.ips[0]) || stripPort(addrs[addrs.length - 1]) || addrs[0] || ''


ip now calls forwarded(this.req) unconditionally. This means a spoofed X-Forwarded-For can influence request.ip even when app.proxy is false (since this.ips[0] will be empty and the code falls back to forwarded() output). It can also inadvertently fall back to X-Forwarded-For when a non-default app.proxyIpHeader is configured but missing on a request. Please preserve the previous trust model: only read forwarded headers when app.proxy is true (and ideally only via the configured proxyIpHeader), otherwise use socket.remoteAddress; then apply stripPort() to the selected value.

Suggested change

// Use forwarded() to properly parse the X-Forwarded-For chain.

// forwarded() returns [socketAddr, ...xffEntriesReversed], so the

// first XFF entry (original client) is at the end of the array.

// See: https://github.com/koajs/koa/issues/827

const addrs = forwarded(this.req)

const addr = stripPort(this.ips[0]) || stripPort(addrs[addrs.length - 1]) || addrs[0] || ''

let addr

// When behind a proxy, trust only the configured proxyIpHeader via this.ips.

if (this.app && this.app.proxy) {

const ips = this.ips

if (ips && ips.length > 0) {

addr = stripPort(ips[0])

}

}

// Fallback to the remote socket address when not behind a proxy

// or when no trusted forwarded header is available.

if (!addr) {

const socket = this.req && this.req.socket

const remoteAddress =

socket &&

(socket.remoteAddress ||

(socket.socket && socket.socket.remoteAddress))

addr = stripPort(remoteAddress || '')

}

Copilot · 2026-04-01T05:57:00Z

    "destroy": "^1.2.0",
    "encodeurl": "^2.0.0",
    "escape-html": "^1.0.3",
+    "forwarded": "^0.2.0",


Adding the forwarded dependency requires updating package-lock.json as well; otherwise npm ci/locked installs will fail in CI because the lockfile no longer matches package.json. Please regenerate and commit the updated lockfile.

Suggested change

"forwarded": "^0.2.0",

Copilot · 2026-04-01T05:57:01Z

+  describe('when XFF contains IPv4 with port', () => {
+    it('should strip the port', () => {
+      const req = request()
+      req.app.proxy = true
+      req.header['x-forwarded-for'] = '1.2.3.4:8080, 5.6.7.8'


request() is not defined in this file (you imported the helper as Request). As written, the tests will throw a ReferenceError before assertions run. Rename the import to request or update the calls to use the imported helper consistently.

Copilot · 2026-04-01T05:57:01Z

+const Stream = require('stream')
+const Koa = require('../..')
+const Request = require('../../test-helpers/context').request


Stream, Koa, and Request are currently unused in this test file, which will fail the standard linter (and Request is also inconsistent with other request tests that import the helper as request). Please remove unused imports and/or use the same helper pattern as the existing request tests.

Suggested change

const Stream = require('stream')

const Koa = require('../..')

const Request = require('../../test-helpers/context').request

const request = require('../../test-helpers/context').request

- Fix test imports: rename Request to request, remove unused Stream/Koa - Regenerate package-lock.json with forwarded dependency - Fix missing newlines at end of files

…ion parens

guoyangzhen

Copilot raised valid points. The core issue is that forwarded(this.req) is called unconditionally in the ip getter, which means a spoofed X-Forwarded-For header can influence request.ip even when app.proxy is false.

The original fallback was this.socket.remoteAddress, but the PR changed it to forwarded(this.req) output — that's a security regression.

Suggested fix — the forwarded import and stripPort are sufficient; the ip getter just needs to be simplified:

get ip () {
  if (!this[IP]) {
    this[IP] = this.app.proxy
      ? (stripPort(this.ips[0]) || this.socket.remoteAddress || '')
      : (this.socket.remoteAddress || '')
  }
  return this[IP]
}

This way:

When app.proxy = true: strip port from this.ips[0] (which already parses XFF via this.ips getter), fall back to socket.remoteAddress
When app.proxy = false: use socket.remoteAddress directly, never touch XFF
No need for the extra forwarded() call — this.ips getter already handles XFF parsing

Also consider adding a test that verifies app.proxy = false ignores XFF even when port is present.

fix: strip port from X-Forwarded-For in request.ip

5059663

When X-Forwarded-For contains IP:port (e.g., from Azure), request.ip should return only the IP address. Handles both IPv4 (1.2.3.4:8080) and IPv6 ([::1]:8080) formats. Fixes koajs#827

fengmk2 self-assigned this Mar 28, 2026

fengmk2 requested a review from Copilot April 1, 2026 05:53

Merge branch 'master' into fix/x-forwarded-for-port

3776120

Copilot started reviewing on behalf of fengmk2 April 1, 2026 05:54 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

guoyangzhen and others added 2 commits April 1, 2026 21:15

fix: address Copilot review feedback

2514803

- Fix test imports: rename Request to request, remove unused Stream/Koa - Regenerate package-lock.json with forwarded dependency - Fix missing newlines at end of files

fix: lint errors - remove extra blank line and add space before funct…

3bb63fe

…ion parens

guoyangzhen commented Apr 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: strip port from X-Forwarded-For in request.ip (#827)#1943

fix: strip port from X-Forwarded-For in request.ip (#827)#1943
guoyangzhen wants to merge 5 commits intokoajs:masterfrom
guoyangzhen:fix/x-forwarded-for-port

guoyangzhen commented Mar 18, 2026

Uh oh!

codecov Bot commented Mar 18, 2026 •

edited

Loading

Uh oh!

guoyangzhen commented Mar 28, 2026

Uh oh!

fengmk2 commented Mar 28, 2026

Uh oh!

guoyangzhen commented Apr 1, 2026

Uh oh!

fengmk2 commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

guoyangzhen left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-      // Use forwarded() to properly parse the X-Forwarded-For chain.
-      // forwarded() returns [socketAddr, ...xffEntriesReversed], so the
-      // first XFF entry (original client) is at the end of the array.
-      // See: https://github.com/koajs/koa/issues/827
-      const addrs = forwarded(this.req)
-      const addr = stripPort(this.ips[0]) || stripPort(addrs[addrs.length - 1]) || addrs[0] || ''
+      let addr
+      // When behind a proxy, trust only the configured proxyIpHeader via this.ips.
+      if (this.app && this.app.proxy) {
+        const ips = this.ips
+        if (ips && ips.length > 0) {
+          addr = stripPort(ips[0])
+        }
+      }
+      // Fallback to the remote socket address when not behind a proxy
+      // or when no trusted forwarded header is available.
+      if (!addr) {
+        const socket = this.req && this.req.socket
+        const remoteAddress =
+          socket &&
+          (socket.remoteAddress ||
+            (socket.socket && socket.socket.remoteAddress))
+        addr = stripPort(remoteAddress || '')
+      }

Uh oh!

Conversation

guoyangzhen commented Mar 18, 2026

Summary

Problem

Fix

Note

Uh oh!

codecov Bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

guoyangzhen commented Mar 28, 2026

Uh oh!

fengmk2 commented Mar 28, 2026

Uh oh!

guoyangzhen commented Apr 1, 2026

Uh oh!

fengmk2 commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

guoyangzhen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Mar 18, 2026 •

edited

Loading