Skip to content

fix: parse port in x-forwarded-for (#827)#1205

Open
jasonmacgowan wants to merge 3 commits into
koajs:masterfrom
jasonmacgowan:x-forwarded-for-ip
Open

fix: parse port in x-forwarded-for (#827)#1205
jasonmacgowan wants to merge 3 commits into
koajs:masterfrom
jasonmacgowan:x-forwarded-for-ip

Conversation

@jasonmacgowan

Copy link
Copy Markdown
Contributor

Parses out IPs even if a port is sent in x-forwarded-for for IPv4. Probably fails if port is sent with an IPv6 address, but works otherwise.

fixes #827

@codecov

codecov Bot commented Jun 14, 2018

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (d1c9263) to head (d2a2714).
⚠️ Report is 226 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1205   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          487       494    +7     
  Branches       136       135    -1     
=========================================
+ Hits           487       494    +7     

☔ View full report in Codecov by Harness.
📢 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.

@3imed-jaberi 3imed-jaberi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jasonmacgowan, thank you for your PR 🙏🏻.
Plz, can you resolve the conflicts then use parseurl module over the url module.

Comment thread lib/request.js
Comment on lines +447 to +449
const hostname = new URL(`http://${normalizedHost}`).hostname;

return hostname.replace(/(^\[|\]$)/g, '');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@3imed-jaberi I don't think I can use parseurl module here as it expects a http.IncomingMessage object and here we're working on a header. I see url.parse is now deprecated so I swapped to using the URL constructor.

Problem is this doesn't strip the [] chars from IPv6 hosts so now we gotta do it manually. Regex to the rescue

@jasonmacgowan

Copy link
Copy Markdown
Contributor Author

Looks like @3imed-jaberi is going to be unavailable for a while based on his profile.

Anyone else around to take a look at this?

/cc @jonathanong @dougwilson

Comment thread test/request/ips.js
});
});

describe('and contains IPv6', () => {

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.

I would recommend adding a test for a IPv6 address with a port (i.e [::1]:80) as well as a dns name with a port (i.e. localhost:80).

Comment thread lib/request.js
let normalizedHost = host;

if (net.isIPv6(host)) {
normalizedHost = `[${host}]`;

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.

Because since this already validates it is the IP only, I would suggest simply shortcutting the rest of the processing for this case:

Suggested change
normalizedHost = `[${host}]`;
return host;

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.

request.ip via x-forwarded-for should strip out ports

3 participants