Skip to content

Signedrequest notices #88

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

Merged
merged 3 commits into from
Jun 11, 2019

Conversation

chrisminett
Copy link
Contributor

If expected header values are not set when using SignedRequest::createFromGlobals(), PHP Notices are issued, which may get in the way of normal operation (eg. returning other headers).

Update the method so that it checks for existence before access, and update the code example to show that ValidationExceptions can be thrown if these values are missing.

loadFromArray() is a public method, so the tests verify this behaviour.
PHP Notices were issued if the request doesn't contain the expected headers.
Now, null values are passed to loadFromArray(), where the values will fail validation.
If expected values are missing or invalid when calling createFromGlobals() an exception is thrown.
The example now demonstrates how an implementation can handle this and respond cleanly.
@chrisminett
Copy link
Contributor Author

@modprobe I've noticed that you've commented or merged other PRs since this one. Could I get some feedback please? This (and the next change I have ready for a PR to fix #87 ) are stopping us from using this security feature.

@marcelcorso
Copy link
Contributor

thanks @chrisminett! I think you picked the right person to review this. If Alexs doesn't do it then maybe @rjelierse.

Copy link
Contributor

@modprobe modprobe left a comment

Choose a reason for hiding this comment

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

Looks very good to me! Thank you very much for your contribution 🐦

@chrisminett
Copy link
Contributor Author

@modprobe are you able to merge if this is all good? Then I can open my other PR to fix the type issue on one of the same lines.

@marcelcorso marcelcorso merged commit 1fed1c2 into messagebird:master Jun 11, 2019
@chrisminett chrisminett deleted the signedrequest-notice branch June 11, 2019 12:04
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