Skip to content

Conversation

@dschom
Copy link
Contributor

@dschom dschom commented Dec 12, 2025

Because

  • X-forward-for address can contain multiple ips

This pull request

  • Handles a multi ip scenario
  • Picks the first IP in the list, which belongs to the client

Issue that this pull request solves

Closes: FXA-12755

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@dschom dschom requested a review from a team as a code owner December 12, 2025 20:26
@nshirley nshirley self-assigned this Dec 12, 2025
Copy link
Contributor

@nshirley nshirley left a comment

Choose a reason for hiding this comment

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

Just a question about v4 vs v6 syntax - though maybe out of scope for the changes since, the bug looks to be resolved!

describe('sanitizes ip address', () => {
it('handles multiple ips', () => {
const ip = sanitizeIp(' 127.0.0.1, 127.0.0.2');
assert.equal(ip, '::127.0.0.1');
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess related to my question above about v4 and v6 should this start with a double ::? From what I read the :: is valid in v6, but only when compressing a 16-bit group of zeros (0:0:0:0:0:0). Feels like we don't really need to "prefix" the :: on there at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My research showed it's valid, but 'deprecated'. I mostly just didn't want to change the logic here. I feel like if we want to do this it should be a separate PR just on the off chance we need to roll it back.

@dschom dschom merged commit 81e8ab0 into main Dec 15, 2025
19 checks passed
@dschom dschom deleted the FXA-12755 branch December 15, 2025 22:45
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