Skip to content

[Notifier] [OvhCloud] “Invalid signature” for message with slashes #39871

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 1 commit into from
Jan 21, 2021
Merged

[Notifier] [OvhCloud] “Invalid signature” for message with slashes #39871

merged 1 commit into from
Jan 21, 2021

Conversation

OneT0uch
Copy link
Contributor

Q A
Branch? 5.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #39836
License MIT

Test to show issue of invalid signature when message contains slash.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@OskarStark
Copy link
Contributor

CleanShot 2021-01-18 at 20 46 33

It looks like your Committer email is not associated with your Github account.

Could it be so easy to check if a / is present in the message and decide upon that how to create the signature? 🤔

@fabpot
Copy link
Member

fabpot commented Jan 19, 2021

@OneT0uch Are you working on a fix?

@OneT0uch
Copy link
Contributor Author

@OneT0uch Are you working on a fix?

@fabpot I opened the PR just to show the issue as asked but i don't know how to fix.

@fabpot
Copy link
Member

fabpot commented Jan 19, 2021

Then I think we are going to close as the core team is not able to fix provider issues. The only to get a fix is by users of the providers. You can also ping the various contributors (especially the one who submitted the provider in the first place) if they can help.

@OskarStark
Copy link
Contributor

I will have a detailed look 👍

@OneT0uch
Copy link
Contributor Author

@OskarStark Can't we just remove the unescaped slashes flag? Do you know why we need it?

@OskarStark
Copy link
Contributor

OskarStark commented Jan 19, 2021

I think not, based on their official PHP client they use it too:
https://github.com/ovh/php-ovh/blob/c473b3d67e5757c188b10e3048aba1ec43133653/src/Api.php#L267-L303

It would be maybe you can open an issue in their repo how they handle your case or if they have a recommendation 🤔

Can you ping me on the symfony slack ?

@OskarStark
Copy link
Contributor

Can you please try to apply the following diff in this PR and see if the tests pass?

diff --git a/src/Symfony/Component/Notifier/Bridge/OvhCloud/OvhCloudTransport.php b/src/Symfony/Component/Notifier/Bridge/OvhCloud/OvhCloudTransport.php
index 4d737019c3..a6f41bc6b3 100644
--- a/src/Symfony/Component/Notifier/Bridge/OvhCloud/OvhCloudTransport.php
+++ b/src/Symfony/Component/Notifier/Bridge/OvhCloud/OvhCloudTransport.php
@@ -76,7 +76,13 @@ final class OvhCloudTransport extends AbstractTransport
         $headers['X-Ovh-Application'] = $this->applicationKey;
         $headers['X-Ovh-Timestamp'] = $now;

-        $toSign = $this->applicationSecret.'+'.$this->consumerKey.'+POST+'.$endpoint.'+'.json_encode($content, \JSON_UNESCAPED_SLASHES).'+'.$now;
+        if (false !== strpos($message->getSubject(), '/')) {
+            $body = json_encode($content);
+        } else {
+            $body = json_encode($content, \JSON_UNESCAPED_SLASHES);
+        }
+
+        $toSign = $this->applicationSecret.'+'.$this->consumerKey.'+POST+'.$endpoint.'+'.$body.'+'.$now;
         $headers['X-Ovh-Consumer'] = $this->consumerKey;
         $headers['X-Ovh-Signature'] = '$1$'.sha1($toSign);

@OneT0uch
Copy link
Contributor Author

OneT0uch commented Jan 19, 2021

Can you please try to apply the following diff in this PR and see if the tests pass?

diff --git a/src/Symfony/Component/Notifier/Bridge/OvhCloud/OvhCloudTransport.php b/src/Symfony/Component/Notifier/Bridge/OvhCloud/OvhCloudTransport.php
index 4d737019c3..a6f41bc6b3 100644
--- a/src/Symfony/Component/Notifier/Bridge/OvhCloud/OvhCloudTransport.php
+++ b/src/Symfony/Component/Notifier/Bridge/OvhCloud/OvhCloudTransport.php
@@ -76,7 +76,13 @@ final class OvhCloudTransport extends AbstractTransport
         $headers['X-Ovh-Application'] = $this->applicationKey;
         $headers['X-Ovh-Timestamp'] = $now;

-        $toSign = $this->applicationSecret.'+'.$this->consumerKey.'+POST+'.$endpoint.'+'.json_encode($content, \JSON_UNESCAPED_SLASHES).'+'.$now;
+        if (false !== strpos($message->getSubject(), '/')) {
+            $body = json_encode($content);
+        } else {
+            $body = json_encode($content, \JSON_UNESCAPED_SLASHES);
+        }
+
+        $toSign = $this->applicationSecret.'+'.$this->consumerKey.'+POST+'.$endpoint.'+'.$body.'+'.$now;
         $headers['X-Ovh-Consumer'] = $this->consumerKey;
         $headers['X-Ovh-Signature'] = '$1$'.sha1($toSign);

Your solution is working but i think it will be better to follow official sdk :
They apply flag on the body too (https://github.com/ovh/php-ovh/blob/c473b3d67e5757c188b10e3048aba1ec43133653/src/Api.php#L273)

// Add Content type
$headers['Content-Type'] = 'application/json';

// Store json encode content 
$body = json_encode($content, \JSON_UNESCAPED_SLASHES);

$toSign = $this->applicationSecret.'+'.$this->consumerKey.'+POST+'.$endpoint.'+'.$body.'+'.$now;
$response = $this->client->request('POST', $endpoint, [
           'headers' => $headers,
           'body' => $body, // Use it here
       ]);

@OskarStark
Copy link
Contributor

That makes sense, can you please change the code accordingly?

@OskarStark OskarStark requested a review from fabpot January 19, 2021 10:30
@OskarStark
Copy link
Contributor

Thanks for fixing this bug @OneT0uch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants