fix: Add Flagsmith signature header when testing webhook. #3666

shubham-padia · 2024-03-25T18:44:34Z

Thanks for submitting a PR! Please check the boxes below:

I have run pre-commit to check linting
I have added information to docs/ if required so people know about the feature!
No need to add info to the docs
I have filled in the "Changes" section below?
I have filled in the "How did you test this code" section below?
I have used a Conventional Commit title for this Pull Request

Changes

We are trying to create the same signature as the webhook in the python code. This commit assumes that the python code will use the same approach to create signature for long term.

We are relying on notes here to make sure that both frontend and backend are implementing the same signature function, which is not the perfect approach. An alternative would be a backend endpoint to test the webhook so that the implementation always remains the same, but not sure if its overkill at this point or not. Note to maintainers: Please let me know what you think about both the approaches and what makes sense in this case since you might know best how the users might use this feature and how often the sign function might change.

I'm also modifying the string on the frontend to be the exact same as the json.dumps output on the python side, so that the sign value is the same on both side.

How did you test this code?

Created a webhook on the project settings, set the secret to test123 and a dead example URL and noted down the header value: X-Flagsmith-Signature: 1907ef6a4c7a3e0010504757728d8dbbf3980247e3e2c5835757b80e9fd1f085

You can check that the replit here will give the same signature. I also generated it in test_unit_webhooks.py by modifying the code to use the same example as frontend just to make sure there is no different when using the replit, but didn't commit the code since the current test is sufficient enough.

I have not tested this on a real webhook, just a dead link, let me know if I need to test it on a real webhook, would be nice if there's an existing test endpoint for that.
I have not added any tests on the frontend side, since I could only find e2e tests, please let me know if tests needed to be added on frontend for this change.

NOTE: I found a lot of unrelated errors to my changes when running npm run lint:fix, I commited the changes related to my PR and ignored the rest, let me know what was the ideal approach here, same with pre-commit

vercel · 2024-03-25T18:44:39Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 10, 2024 3:16pm
flagsmith-frontend-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 10, 2024 3:16pm
flagsmith-frontend-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 10, 2024 3:16pm

vercel · 2024-03-25T18:44:40Z

@shubham-padia is attempting to deploy a commit to the Flagsmith Team on Vercel.

A member of the Team first needs to authorize it.

kyle-ssg · 2024-03-25T22:03:01Z

frontend/web/components/TestWebhook.tsx

  const [error, setError] = useState<string | null>(null)
  const [loading, setLoading] = useState(false)
  const [success, setSuccess] = useState(false)
+  const [sign, setSign] = useState('')
+  getSignature(json, secret, setSign)


I think rather than re-evaluating this each render we may as well just do this on submit

Agreed! Fixed at https://github.com/Flagsmith/flagsmith/compare/446e37228ee813f6f6503e5d2b950850f3b593a6..39cc696750aa1916fe6376fc6babbae6676cac0b along with a bit of refactoring

kyle-ssg · 2024-03-25T22:04:26Z

frontend/web/components/TestWebhook.tsx

  const submit = () => {
    setError(null)
    setLoading(true)
    setSuccess(false)
    data
-      .post(webhook, JSON.parse(json), null)
+      .post(webhook, JSON.parse(json), headers)


RE above comment, this could be something more like

getSignature().then((sign)=>{ const headers = { 'X-Flagsmith-Signature': sign, } return data.post(...) }).then

Thanks for the review! Fixed at https://github.com/Flagsmith/flagsmith/compare/446e37228ee813f6f6503e5d2b950850f3b593a6..39cc696750aa1916fe6376fc6babbae6676cac0b along with a bit of refactoring

kyle-ssg

Great from a FE perspective, I'll handover to another member of the team from a backend perspective. Thank you very much for this!

github-actions · 2024-03-28T08:54:50Z

Uffizzi Preview deployment-49072 was deleted.

shubham-padia · 2024-03-29T09:21:36Z

Bump for review

api/core/signing.py

api/webhooks/webhooks.py

matthewelwell · 2024-04-03T09:33:18Z

@shubham-padia @khvn26 looks like we've got CI failures here now, can we fix?

codecov-commenter · 2024-04-03T10:49:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.90%. Comparing base (eca05ca) to head (15fd590).
Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3666      +/-   ##
==========================================
+ Coverage   95.89%   95.90%   +0.01%     
==========================================
  Files        1101     1102       +1     
  Lines       34568    34703     +135     
==========================================
+ Hits        33149    33283     +134     
- Misses       1419     1420       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Fixes Flagsmith#2786. We are trying to create the same signature as the webhook in the python code. This commit assumes that the python code will use the same approach to create signature for long term.

shubham-padia · 2024-04-03T12:03:36Z

@shubham-padia @khvn26 looks like we've got CI failures here now, can we fix?

@kyle-ssg fixed!

shubham-padia · 2024-04-05T11:34:43Z

Just checking if there are any blockers to getting this merged :) !

api/edge_api/identities/edge_request_forwarder.py

kyle-ssg · 2024-04-10T14:45:15Z

frontend/web/components/TestWebhook.tsx

+}
+
+const signPayload = async (body: string, secret: string): Promise<string> => {
+  if(!secret) {


Just a note, added this as it was rejecting attempting to sign with an empty secret. Instead it just resolves with '' in this case.

github-actions bot added front-end Issue related to the React Front End Dashboard api Issue related to the REST API labels Mar 25, 2024

vercel bot deployed to Preview – docs March 25, 2024 18:45 View deployment

shubham-padia force-pushed the fix/webhook-secret-signature-frontend branch from a52cb1e to 446e372 Compare March 25, 2024 18:47

vercel bot deployed to Preview – docs March 25, 2024 18:48 View deployment

shubham-padia marked this pull request as ready for review March 25, 2024 18:50

shubham-padia changed the title ~~Draft: fix: Add Flagsmith signature header when testing webhook.~~ fix: Add Flagsmith signature header when testing webhook. Mar 25, 2024

shubham-padia mentioned this pull request Mar 25, 2024

Include x-flagsmith-signature header when webhook is triggered by test webhook widget #2786

Closed

kyle-ssg reviewed Mar 25, 2024

View reviewed changes

shubham-padia force-pushed the fix/webhook-secret-signature-frontend branch from 446e372 to 39cc696 Compare March 25, 2024 23:08

vercel bot deployed to Preview – docs March 25, 2024 23:09 View deployment

kyle-ssg approved these changes Mar 26, 2024

View reviewed changes

kyle-ssg requested review from a team and novakzaballa and removed request for a team March 26, 2024 08:01

dabeeeenster requested review from khvn26 and matthewelwell March 26, 2024 08:55

vercel bot deployed to Preview – flagsmith-frontend-preview March 28, 2024 08:52 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging March 28, 2024 08:54 View deployment

khvn26 reviewed Apr 2, 2024

View reviewed changes

api/core/signing.py Outdated Show resolved Hide resolved

shubham-padia force-pushed the fix/webhook-secret-signature-frontend branch 2 times, most recently from 5c6bef7 to 50c30ea Compare April 2, 2024 15:03

vercel bot deployed to Preview – docs April 2, 2024 15:03 View deployment

khvn26 approved these changes Apr 2, 2024

View reviewed changes

api/webhooks/webhooks.py Outdated Show resolved Hide resolved

api/webhooks/webhooks.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview – docs April 2, 2024 15:17 View deployment

shubham-padia force-pushed the fix/webhook-secret-signature-frontend branch from 2409a43 to f9d6237 Compare April 3, 2024 10:41

vercel bot deployed to Preview – docs April 3, 2024 10:42 View deployment

shubham-padia force-pushed the fix/webhook-secret-signature-frontend branch from f9d6237 to c731532 Compare April 3, 2024 10:42

vercel bot deployed to Preview – docs April 3, 2024 10:43 View deployment

fix: Add Flagsmith signature header when testing webhook.

c2c7c73

Fixes Flagsmith#2786. We are trying to create the same signature as the webhook in the python code. This commit assumes that the python code will use the same approach to create signature for long term.

shubham-padia force-pushed the fix/webhook-secret-signature-frontend branch from c731532 to c2c7c73 Compare April 3, 2024 11:13

vercel bot deployed to Preview – docs April 3, 2024 11:17 View deployment

novakzaballa removed their request for review April 4, 2024 13:37

matthewelwell removed their request for review April 10, 2024 12:41

gagantrivedi requested changes Apr 10, 2024

View reviewed changes

api/edge_api/identities/edge_request_forwarder.py Outdated Show resolved Hide resolved

return empty signing key if attempting to sign with an empty secret

ce35f94

kyle-ssg reviewed Apr 10, 2024

View reviewed changes

kyle-ssg added 2 commits April 10, 2024 15:58

Revert comments

7cea3d7

Revert comments

15fd590

kyle-ssg requested a review from gagantrivedi April 10, 2024 15:00

vercel bot deployed to Preview – flagsmith-frontend-preview April 10, 2024 15:13 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging April 10, 2024 15:15 View deployment

kyle-ssg enabled auto-merge April 10, 2024 15:16

vercel bot deployed to Preview – docs April 10, 2024 15:16 View deployment

gagantrivedi approved these changes Apr 11, 2024

View reviewed changes

kyle-ssg added this pull request to the merge queue Apr 11, 2024

Merged via the queue into Flagsmith:main with commit c950875 Apr 11, 2024
23 checks passed

dabeeeenster mentioned this pull request Apr 11, 2024

chore(main): release 2.107.4 #3754

Merged

dabeeeenster mentioned this pull request Apr 18, 2024

chore(main): release 2.109.0 #3798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add Flagsmith signature header when testing webhook. #3666

fix: Add Flagsmith signature header when testing webhook. #3666

shubham-padia commented Mar 25, 2024 •

edited

Loading

vercel bot commented Mar 25, 2024 •

edited

Loading

vercel bot commented Mar 25, 2024

kyle-ssg Mar 25, 2024

shubham-padia Mar 25, 2024

kyle-ssg Mar 25, 2024 •

edited

Loading

shubham-padia Mar 25, 2024

kyle-ssg left a comment

github-actions bot commented Mar 28, 2024 •

edited

Loading

shubham-padia commented Mar 29, 2024

matthewelwell commented Apr 3, 2024

codecov-commenter commented Apr 3, 2024 •

edited

Loading

shubham-padia commented Apr 3, 2024

shubham-padia commented Apr 5, 2024

kyle-ssg Apr 10, 2024

fix: Add Flagsmith signature header when testing webhook. #3666

fix: Add Flagsmith signature header when testing webhook. #3666

Conversation

shubham-padia commented Mar 25, 2024 • edited Loading

Changes

How did you test this code?

vercel bot commented Mar 25, 2024 • edited Loading

vercel bot commented Mar 25, 2024

kyle-ssg Mar 25, 2024

Choose a reason for hiding this comment

shubham-padia Mar 25, 2024

Choose a reason for hiding this comment

kyle-ssg Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

shubham-padia Mar 25, 2024

Choose a reason for hiding this comment

kyle-ssg left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2024 • edited Loading

shubham-padia commented Mar 29, 2024

matthewelwell commented Apr 3, 2024

codecov-commenter commented Apr 3, 2024 • edited Loading

Codecov Report

shubham-padia commented Apr 3, 2024

shubham-padia commented Apr 5, 2024

kyle-ssg Apr 10, 2024

Choose a reason for hiding this comment

shubham-padia commented Mar 25, 2024 •

edited

Loading

vercel bot commented Mar 25, 2024 •

edited

Loading

kyle-ssg Mar 25, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading

codecov-commenter commented Apr 3, 2024 •

edited

Loading