feat(crm): shared template variable resolver for journey/API sends (EVO-1267)#137
Conversation
There was a problem hiding this comment.
Sorry @nickoliveira23, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
dpaes
left a comment
There was a problem hiding this comment.
Review EVO-1267 — Request changes 🔴
Great cross-repo work and the ACs are all functionally covered with real end-to-end specs. Holding the merge on one HIGH-severity security blocker in the shared resolver.
🔴 Blocker — TemplateVariableResolver has no read allowlist (secret disclosure)
app/services/template_variable_resolver.rb#resolve_path walks the AR object graph via arity-0 public_send, guarded only by a denylist (mutators) + arity + segment-format. That stops mutation/RCE but does nothing against reader traversal, and conversation → inbox → channel reaches live channel credentials:
{{conversation.inbox.channel.provider_config}}→ WhatsApp Cloudapi_key(jsonb){{conversation.inbox.channel.page_access_token}}/user_access_token→ Meta tokens (plain string columns){{conversation.inbox.channel.api_key}}→ Sendgrid virtual reader that Fernet-decrypts the at-rest ciphertext to plaintext{{conversation.inbox.channel.attributes}}/to_json/as_json/serializable_hash/inspect,{{contact.attributes}}→ bulk dumps
Resolution fires for journey/API callers (no variables_resolved:true), the value is .to_s'd and substituted into message.content with no escaping/redaction (the WhatsApp path via render_with_variables; the email path gsubs every processed_params key with no allowlist at all). It's reachable end-to-end from POST /api/v1/conversations/:id/messages by any authenticated principal, and via the Custom expression field (the frontend only validates balanced braces and ships the raw path). Net: an authenticated flow author can exfiltrate channel credentials into a message delivered to a recipient they control.
Fix direction: allowlist the terminal columns per root (the 9 curated SOURCE_FIELD_PATHS from the panel are the legitimate surface) and/or cap traversal depth + forbid crossing into inbox/channel. The denylist can't be made complete here — it's blind to readers. Worth adding a regression spec asserting {{conversation.inbox.channel.provider_config}} resolves to ''.
Non-blocking (this repo)
- [low] Fallbacks are wired only on the journey/API builder path —
AutomationRules::MessageActionHandlers#resolve_template_paramscallsresolve_paramswith nofallbacks, so automation sends get no fallback support. Intentional? Worth a comment if so. - [low] The
variables_resolvedflag request spec asserts only non-invocation + 201, never the resolved content; no spec covers the optional-variable case or pipeline multiplicity. - [info]
is_jsontwo-argstart_withrefactor + trailing-newline removal are behavior-equivalent — fine.
Note on CI
crm-community CI runs Sourcery/staleness/contract only (no RSpec), so the resolver (13) / contract (3) / automation (76) specs are self-reported — please confirm with a local bundle exec rspec after the fix.
Card moved back to Todo. PR stays open. 🙏
…VO-1267)
Extracts the Automation Rules {{root.path}} engine into
TemplateVariableResolver (contact/conversation roots + new pipeline
root, segment safety perimeter, per-variable fallbacks) and wires it
into Messages::MessageBuilder so evo-flow journey sends resolve
variables where the data lives. Automation executors pre-resolve and
flag the envelope (variables_resolved) so user-originated values are
never re-expanded; a required variable resolving blank no longer
fails the send (AC4) — the caller-rendered content is kept.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…267)
Self-review hardening: zero-arg-invocable writers/utilities
(update_columns, update_attribute, increment, reload, touch, freeze…)
join the denylist with a regression spec; root objects memoize per
resolver instance so several {{pipeline.x}} placeholders cost one
query; additional_attributes provenance documented as intentional.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…versal (EVO-1267)
The TemplateVariableResolver perimeter was denylist-only, which is blind to
readers: {{conversation.inbox.channel.provider_config}} / .api_key /
.page_access_token and mass-dump readers (attributes/to_json/inspect) walked
the ActiveRecord graph to channel credentials, reachable end-to-end via the
Custom expression field and POST /conversations/:id/messages.
Replace the denylist with a default-deny allowlist: the full dotted tail must
match a curated path per root (the panel's SOURCE_FIELD_PATHS + pipeline_stage_id).
Association traversal, bare roots and serializers now resolve to ''. The
arity reader guard stays as defence in depth. Resolution is a single upstream
chokepoint in Messages::MessageBuilder, so the WhatsApp, email, automation and
journey/API paths are all covered. Regression specs assert channel-credential
and mass-dump paths yield ''.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f30849b to
c36be32
Compare
dpaes
left a comment
There was a problem hiding this comment.
Code review — EVO-1267 [10.19] Send Template with Variables — ✅ Approved
Round 2 already cleared the substance (both HIGH blockers B1/B2 closed, 7/7 ACs). The only open item was the rebase of crm#137 over the EVO-1235/#138 SendResolver change. Rebase verified clean:
- B1 stays closed — in
Messages::MessageBuilder#process_template_content, the allowlist gateresolve_template_variables(template_info)runs before the channel branch (inbox_type == 'Email'→process_email_template/ else →render_template_content). Both email and WhatsApp paths consume allowlist-resolved params, so noinbox/channelcredential path can be traversed into rendered content. resolve_template_variablesdelegates to the default-denyTemplateVariableResolver; the inline vulnerable resolver is gone; automation handler setsvariables_resolved: trueso user-originated values are never re-expanded.- #138 reconciled and preserved:
SendResolver(id-first, global-aware) +persist_resolved_template_idintact. - Net diff vs develop = only the 5 EVO-1267 files; no #138 change lost.
- All 3 PRs (crm#137, evo-flow#55, frontend#155) MERGEABLE/CLEAN on
develop.
Approving and squash-merging in the order crm → evo-flow + frontend.
Note: crm/evo-flow/frontend CI is Sourcery-only (no RSpec/vitest/tsc) — the green specs are self-reported; reviewed manually + verified the rebase structurally.
Summary
{{root.path}}engine intoTemplateVariableResolverand wires it intoMessages::MessageBuilder, so evo-flow journey sends resolve template variables against the live conversation — the same engine for the modal flow, the canvas flow and the API path.pipelineroot (most recentPipelineItemof the conversation, memoized) alongsidecontact/conversation.variable_fallbacksfield on thetemplate_paramsenvelope: a blank resolution falls back instead of staying empty.variables_resolved), so user-originated values (a contact literally named{{contact.email}}) are never re-expanded downstream.Security
public_sendon user-supplied paths now runs behind a perimeter: segment format check, denylist covering destroyers AND zero-arg-invocable writers/utilities (update_columns,reload,touch,freeze, …), and arity ≤ 0 — with a regression spec invoking the denied segments.Test plan
bundle exec rspec spec/services/template_variable_resolver_spec.rb— 13 examples (roots, multi-placeholder expressions, undefined → '', fallbacks, denylist)bundle exec rspec spec/requests/api/v1/messages_template_variables_spec.rb— 3 examples (end-to-end resolve+fallback, AC4 no-crash, variables_resolved skip)bundle exec rspec spec/services/automation_rules/— 76 examples, zero regression on the delegating executorsbundle exec rubocopon touched files — better than the develop baseline (22 → 16), new files cleanChanged Files
app/services/template_variable_resolver.rb(new)app/services/automation_rules/message_action_handlers.rbapp/builders/messages/message_builder.rbspec/services/template_variable_resolver_spec.rb(new)spec/requests/api/v1/messages_template_variables_spec.rb(new)Related PRs
Linked Issue
🤖 Generated with Claude Code