Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

HMAC docs are misleading #981

Closed
jonathanstanley opened this issue Sep 6, 2023 · 2 comments
Closed

HMAC docs are misleading #981

jonathanstanley opened this issue Sep 6, 2023 · 2 comments

Comments

@jonathanstanley
Copy link

jonathanstanley commented Sep 6, 2023

Issue summary

1. test HTTPS?

Shopify docs for hooks state that app developers should verify hooks sent using HTTPS:
Screenshot 2023-09-06 at 12 03 42 PM

However, the linked docs to verify hooks actually do not verify HTTPS. Instead it asks devs to verify the HMAC. Checking HTTPS would have been much more straightforward.

2. provide a validator?

Although Shopify does have an HMAC validator as part of the standard js kit, it isn't usable and fails.

Shopify also has shopify.api.webhooks.validate() for javascript api but it isn't declared in the main docs?

3. what to use for HMAC?

for hmac data:

for hmac value:

  • webhooks keep the hmac in the header x-shopify-hmac-sha256.
  • app proxy stuffs the hmac into the query parameter signature
  • authenticated fetch hmac value is consistent with JWT specifications

for hmac digest encoding

and there are other situations in shopify that use tokens, oauth and cookies.

why not just use JWT in each of these cases?

Expected behavior

each of these contrived hmac schemes could be replaced with JWT. next best would be some consistency. or sample code:

/** tests an hmac value against the data as imposed by shopify  */
function verifyHmac(
  /** @type {string} */ data,
  /** @type {string} */ unverifiedHmac,
  /**
   * @note
   * * Shopify uses hex digest for app proxy https://shopify.dev/docs/apps/online-store/app-proxies#calculate-a-digital-signature
   * * Shopify uses base64 digest for webhooks https://shopify.dev/docs/apps/webhooks/configuration/https#step-5-verify-the-webhook
   * @type { import("crypto").BinaryToTextEncoding} */ encoding
) {
  const verifiedHmac = createHmac("sha256", shopify.api.config.apiSecretKey)
    .update(data) // accepts string or ArrayBufferView; we'll just use string
    .digest(encoding);
  return unverifiedHmac === verifiedHmac;
}

Actual behavior

🤯

@ashleyww93
Copy link

In my experience, the docs are misleading for almost everything I have tried to do so far...

@lizkenyon
Copy link
Contributor

Hi there 👋

Thank you for your detailed feedback!

For your concerns about the documentation would you be able to provide your feedback via the Was this section helpful? box on the offending pages. This will help out route the feedback to the correct teams in the most efficient way.

Regarding the HMAC validator for App proxies. This is something that we are aware of. Please watch for updates soon with the Remix App template that are working to resolve this.

Thank you for raising some of the inconsistencies on the platform. I agree this is something that we could definitely improve on. I can't promise any firm deadlines on when improvements would be made, but I really do appreciate you taking the time to provide this feedback.

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

No branches or pull requests

3 participants