platformsh / legacy-cli

This is the legacy version of Platform.sh's command-line interface. The new version is at: https://github.com/platformsh/cli
https://docs.platform.sh/administration/cli.html
MIT License
221 stars 120 forks source link

Support IPV6 ip address validation #1405

Closed hanoii closed 7 months ago

hanoii commented 7 months ago

I ended up testing this properly using the cli and all.

--

I haven't did a full tests of the project, I played with the code on

https://codesandbox.io/p/devbox/blissful-hill-56v4n9?file=%2Findex.php%3A33%2C1

pjcdawkins commented 7 months ago

Thanks @hanoii

Closes #1399

pjcdawkins commented 7 months ago

Would we also need to fix this bit above?

        // Normalize the address so that we can compare accurately with the
        // current value returned from the API.
        if ($address == 'any') {
            $address = '0.0.0.0/0';
        } elseif ($address && !strpos($address, '/')) {
            $address .= '/32';
        }
hanoii commented 7 months ago

I reviewed that code, and from what I gathered from our chats and my experience with the commands I think not.

That code only validates what comes from the API, and because your API doesn't truly expect an ipv6 address if you only specify the ipv6 ip it will come back with /32. Thinking a bit more, maybe it would be best to validate it against /128 and at least detect it. but I think this should be handled before sending it to the server than after, I'll check if I can suggest something else.

hanoii commented 7 months ago

I was wrong, fixing that too.

hanoii commented 7 months ago

ci fails, but I assume that's not related to this PR, ready for review.

pjcdawkins commented 7 months ago

I just realised the CI failures are because this is targeting master, it should be main - @hanoii would you mind rebasing and changing the PR target?

hanoii commented 7 months ago

I just realised the CI failures are because this is targeting master, it should be main - @hanoii would you mind rebasing and changing the PR target?

Done!

hanoii commented 7 months ago

Is there a rough timeline into when 4.14.2 will get into a new official 5.x cli release?

pjcdawkins commented 7 months ago

It ended up in release 5.0.10 last week

https://github.com/platformsh/cli/releases

Thanks again @hanoii