fix: Keep webhook event data as string-keyed map#108
Open
devin-ai-integration[bot] wants to merge 1 commit into
Open
fix: Keep webhook event data as string-keyed map#108devin-ai-integration[bot] wants to merge 1 commit into
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
WorkOS.Webhooks.Event.new/1 recursively converted every JSON object key in the webhook payload into an atom via String.to_atom/1. Payload content embeds third-party data (e.g. SCIM raw_attributes whose key names are chosen by whoever controls a connected directory), so a stream of genuine, correctly-signed directory-sync events carrying many unique attribute names could exhaust the BEAM atom table (default 1,048,576) and abort the entire node. Keep data as a string-keyed map, matching WorkOS.Events.Event.cast/1, so untrusted payload keys are never interned as atoms.
Contributor
Author
Original prompt from Linear User
|
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR keeps webhook event data as string-keyed JSON maps. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "Keep webhook event data as string-keyed ..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes SEC-1230. Automated first-pass security fix — requires human security review before merge.
WorkOS.Webhooks.Event.new/1recursively interned every JSON object key in a webhook payload as an atom viaString.to_atom/1(lib/workos/webhooks/event.ex:27). Atoms are never garbage-collected and the BEAM atom table is capped (default 1,048,576); exceeding it aborts the entire node, not just the request.Webhook payload content embeds third-party data — most notably SCIM
raw_attributes, whose key names are chosen by whoever controls a connected directory. A stream of genuine, correctly-signeddsync.user.*events carrying thousands of unique attribute names could therefore exhaust the atom table and crash any application using the SDK's documentedconstruct_event/3flow (a remote, cross-tenant denial of service). The HMAC check does not help — the events are authentic; only the key names are attacker-influenced.The fix keeps
dataas a plain string-keyed map, matching the siblingWorkOS.Events.Event.cast/1, so untrusted keys are never interned:Breaking change — needs reviewer decision
This changes the shape of
WorkOS.Webhooks.Event.data: nested keys are now strings instead of atoms (e.g.event.data["raw_attributes"]["userName"]instead ofevent.data.raw_attributes.userName). This is the same shape the Events API already returns, but it is a breaking change for existing webhook consumers and likely warrants a major version bump / changelog entry. An alternative (String.to_existing_atom/1with a string fallback) would preserve atom keys for already-interned names, but produces a non-deterministic mixed atom/string map, so this PR takes the consistent string-keyed approach the finding recommends. Reviewer should confirm the preferred compatibility path.Verification
mix formatclean; the existing webhooks specs (updated to assert the string-keyed shape) plus a new regression test all pass.does not intern payload keys as atomsprocesses 25 events × 1,000 unique keys and asserts:erlang.system_info(:atom_count)stays flat.+t 32768): after 3 crafted events the VM aborted withno more index entries in atom_tab (max=32768). With this fix the same sequence interns 0 new atoms and survives 1,000 events.Link to Devin session: https://app.devin.ai/sessions/fb86c5d749264c5fbfa97cbbeb5aa86f