Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IPv6 parsing in API access methods and custom DNS #6852

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Sep 24, 2024

Acceptance criteria:

  • Adding IPv6 addresses should work, whilst the app should still reject any arbitrary unsigned 32 bit integer instead of interpreting that as an IPv4 address.
  • Custom DNS and API access method code should use the same validation for IP addresses.
  • Using IPv6 addresses in custom API methods and custom DNS works.
  • The validation code must be unit tested.
  • When this is fixed, it should be mentioned in the changelog.

This change is Reviewable

@rablador rablador added the iOS Issues related to iOS label Sep 24, 2024
@rablador rablador self-assigned this Sep 24, 2024
Copy link

linear bot commented Sep 24, 2024

@rablador rablador force-pushed the ipv6-parsing-fails-in-api-connection-methods-and-custom-dns-ios-696 branch from 7f98e1f to 7d53151 Compare September 24, 2024 08:06
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @rablador)


ios/MullvadVPNTests/MullvadTypes/AnyIPAddressTests.swift line 25 at r1 (raw file):

        XCTAssertNotNil(AnyIPAddress("2a03:1b20:1:f011::bb09"))
        XCTAssertNotNil(AnyIPAddress("FE80:0000:0000:0000:0202:B3FF:FE1E:8329"))
    }

I suggest to add more IPs to be tested out:

Code snippet:

        XCTAssertNotNil(AnyIPAddress("2001:0db8:0000:0042:0000:8a2e:0370:7334"))
        XCTAssertNotNil(AnyIPAddress("2001:db8::42:0:8a2e:370:7334"))
        XCTAssertNotNil(AnyIPAddress("::1"))
        XCTAssertNotNil(AnyIPAddress("::"))
        XCTAssertNotNil(AnyIPAddress("::ffff:192.168.1.1"))
        XCTAssertNotNil(AnyIPAddress("fe80::1ff:fe23:4567:890a"))
        XCTAssertNotNil(AnyIPAddress("ff02::1"))
        XCTAssertNotNil(AnyIPAddress("2001:0db8:85a3::8a2e:0370:7334"))
        XCTAssertNotNil(AnyIPAddress("fc00::1234:5678:abcd"))

mojganii
mojganii previously approved these changes Sep 25, 2024
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @rablador)

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @mojganii)


ios/MullvadVPNTests/MullvadTypes/AnyIPAddressTests.swift line 25 at r1 (raw file):

Previously, mojganii wrote…

I suggest to add more IPs to be tested out:

Done.

@rablador rablador force-pushed the ipv6-parsing-fails-in-api-connection-methods-and-custom-dns-ios-696 branch from 7d53151 to 1fd4bf2 Compare September 26, 2024 08:04
buggmagnet
buggmagnet previously approved these changes Sep 26, 2024
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @mojganii)

@rablador rablador force-pushed the ipv6-parsing-fails-in-api-connection-methods-and-custom-dns-ios-696 branch 2 times, most recently from 711b744 to f3d2fd4 Compare September 27, 2024 08:16
@pinkisemils pinkisemils force-pushed the ipv6-parsing-fails-in-api-connection-methods-and-custom-dns-ios-696 branch from f3d2fd4 to fe8062e Compare September 30, 2024 09:19
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

@pinkisemils pinkisemils force-pushed the ipv6-parsing-fails-in-api-connection-methods-and-custom-dns-ios-696 branch from fe8062e to d33909e Compare September 30, 2024 11:23
@pinkisemils pinkisemils merged commit 7adb5e8 into main Sep 30, 2024
10 of 11 checks passed
@pinkisemils pinkisemils deleted the ipv6-parsing-fails-in-api-connection-methods-and-custom-dns-ios-696 branch September 30, 2024 11:25
Copy link

🚨 End to end tests failed. Please check the failed workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants