-
Notifications
You must be signed in to change notification settings - Fork 259
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
Backport subdomain validation #477
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your contribution. ❤️
Please note the required changes.
@@ -1,6 +1,6 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is for the v1 branch. It doesn't make sense to merge the current master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification.
public function testInvalidSubdomainThrows() | ||
{ | ||
new Client('...', ''); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid we would need a more specific test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to add one. What test would you like to see?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're introducing a regex, could you please provide a test that passes the validation and one that throws InvalidArgumentException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you ❤️
public function testInvalidSubdomainThrows() | ||
{ | ||
new Client('...', ''); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're introducing a regex, could you please provide a test that passes the validation and one that throws InvalidArgumentException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @sanmai.
We only maintain the latest version, but surely we welcome improving the value or older versions.
@@ -1,6 +1,6 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.