Fix for issue #104: Verify webhook by civicrm - #105
Conversation
| $this->request = $request; | ||
| $this->files = $files; | ||
| $this->server = $server; | ||
| foreach(getallheaders() as $header => $headerValue) { |
There was a problem hiding this comment.
Why is the call of getallheaders() is necessary? Doesn't all the headers come in the $server argument?
There was a problem hiding this comment.
It is needed because depending on the webser (nginx vs apache) not all headers are in $_SERVER. For example on my local apache webserver. Te $_SERVER did not have the Authorization header. Which is one we needed for our specific use case
There was a problem hiding this comment.
Seems to be an Apache issue that could be solved by adding SetEnvIf Authorization "(.*)" HTTP_AUTHORIZATION=$1 to .htaccess or the Apache config.
Though if the code should work without that I'd suggest to put the call of getallheaders() into Request::createFromGlobals() with an explaining comment.
There was a problem hiding this comment.
I have added a comment to explain why we use getallheaders.
There was a problem hiding this comment.
👍 I still think that it shouldn't be part of the constructor so that it might be possible to create request objects without relying on that function, e.g. in unit tests. Now that there's a create() method I'd put it there.
| $this->request = $request; | ||
| $this->files = $files; | ||
| $this->server = $server; | ||
| foreach(getallheaders() as $header => $headerValue) { |
There was a problem hiding this comment.
👍 I still think that it shouldn't be part of the constructor so that it might be possible to create request objects without relying on that function, e.g. in unit tests. Now that there's a create() method I'd put it there.
|
@dontub can you do another review? I made some changes as you suggested them. |
| foreach ($server as $header => $headerValue) { | ||
| if (stripos($header, 'HTTP_') === 0) { | ||
| $this->headers[substr($header, 5)] = $headerValue; | ||
| $key = str_replace(' ', '-', ucwords(strtolower(str_replace('_', ' ', substr($header, 5))))); |
There was a problem hiding this comment.
Was this added for the headers from getallheaders()? If so, its not applied to those headers with the latest change anymore.
There was a problem hiding this comment.
Ah yes that change is not needed anymore.
There was a problem hiding this comment.
I think it's necessary for the headers from getallheaders(). In $_SERVER the key is for example HTTP_CONTENT_LENGTH while in getallheaders() it's Content-Length, isn't it?
There was a problem hiding this comment.
Yes it is. You are right
2 participants
This PR solves issue #104
It adds the following functionality:
The reason for doing the verification in CiviCRM is that for HMAC verification we need a secret key to do the verification and we do not want to have too many secrets on the proxy.
This PR also solves one bug with the Request class not containing all headers.