Backport subdomain validation #477
Conversation
| @@ -1,6 +1,6 @@ | |||
| <?php | |||
There was a problem hiding this comment.
This file does not exist in current master, and a similar issue has already been fixed, can you please try to merge master in here?
There was a problem hiding this comment.
This change is for the v1 branch. It doesn't make sense to merge the current master.
There was a problem hiding this comment.
Thank you for the clarification.
| public function testInvalidSubdomainThrows() | ||
| { | ||
| new Client('...', ''); | ||
| } |
There was a problem hiding this comment.
I'm afraid we would need a more specific test.
There was a problem hiding this comment.
I'm happy to add one. What test would you like to see?
There was a problem hiding this comment.
You're introducing a regex, could you please provide a test that passes the validation and one that throws InvalidArgumentException?
|
Please review my comments. Alternatively, I'll be happy to close this PR as it looks like it is no longer necessary for my cause. |
There was a problem hiding this comment.
Hi @sanmai - thank you very much for your contribution and patience.
I know we've not given a reasonable feedback loop in the past, but moving forward, we are committed to maintaining this library and be nicer to our awesome open-source contributors.
Please keep in mind though, that I personally don't have a deep knowledge of this lib nor PHP, but I'll do my best in a world of conflicting priorities.
With that said, would you be so kind to address the comments?
Thanks a lot, long live open-source ❤️
| @@ -1,6 +1,6 @@ | |||
| <?php | |||
|
|
|||
| namespace Zendesk\API\LiveTests; | |||
| namespace Zendesk\API\UnitTests; | |||
| public function testInvalidSubdomainThrows() | ||
| { | ||
| new Client('...', ''); | ||
| } |
There was a problem hiding this comment.
You're introducing a regex, could you please provide a test that passes the validation and one that throws InvalidArgumentException?
| @@ -1,6 +1,6 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Thank you for the clarification.
This PR backports subdomain validation from #466, necessary to fix CVE-2021-30492.
I also added/fixed tests. Here's how you can run them:
All should be left is to tag a new release, update the Packagist registry, and tweak CVE-2021-30492 to mark this new version as unaffected.