Skip to content

fix(template): reject unescaped expressions in permission templates#7071

Merged
Vligai merged 2 commits into
mainfrom
fix/handlebars-template-escaping
Jun 29, 2026
Merged

fix(template): reject unescaped expressions in permission templates#7071
Vligai merged 2 commits into
mainfrom
fix/handlebars-template-escaping

Conversation

@Vligai

@Vligai Vligai commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Context

validateHandlebarTemplate / isValidHandleBarTemplate validate Handlebars templates by checking
that each mustache statement's path is an allowed expression. They did not check whether the
expression was escaped, so unescaped forms ({{{ ... }}} and the equivalent {{& ... }}) passed as
long as the path was allowed.

This adds an opt-in rejectUnescaped option to the validator: when set, only escaped expressions
({{ ... }}) are accepted. It is enabled everywhere permission rules are authored: role and
additional-privilege creation/update, and project-template creation/update (template roles are
materialized into project custom roles and rendered by the same permission templating, so they need
the same validation). The option defaults off, so the other callers, the dynamic-secret providers,
which render their statements with noEscape (e.g. LDAP renders raw values into LDIF), keep their
current behavior and are unaffected.

  • Before: the permission-template validator accepted unescaped expressions, and project templates
    did not run this validator at all.
  • After: with rejectUnescaped, role, additional-privilege, and project-template validators accept
    only escaped expressions; dynamic-secret statement validation is unchanged.

Changed files:

  • backend/src/lib/template/validate-handlebars.ts: add the opt-in rejectUnescaped option to
    validateHandlebarTemplate and isValidHandleBarTemplate.
  • backend/src/services/role/role-service.ts: pass rejectUnescaped: true on role create and update.
  • backend/src/services/additional-privilege/additional-privilege-service.ts: pass
    rejectUnescaped: true on additional-privilege create and update.
  • backend/src/ee/services/project-template/project-template-service.ts: validate template role
    permissions with rejectUnescaped: true on template create and update.
  • backend/src/lib/template/validate-handlebars.test.ts: unit tests for both modes (opt-in rejects
    unescaped forms; default accepts them, preserving non-permission caller behavior).

Screenshots

Steps to verify the change

  • Unit test (included): npm run test:unit -- src/lib/template/validate-handlebars.test.ts. It covers
    the opt-in mode (escaped accepted; {{{ }}} and {{& }} rejected) and the default mode (unescaped
    accepted), plus a disallowed expression.
  • Manual: creating/updating a role or additional privilege with normal {{ ... }} expressions works;
    one containing {{{ ... }}} is rejected. Dynamic-secret statements (including LDAP) that use
    {{{ ... }}} continue to validate and work as before.

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

  • Title follows the conventional commit format: type(scope): short description (scope is optional, e.g., fix: prevent crash on sync or fix(api): handle null response).
  • Tested locally
  • Updated docs (if needed)
  • Updated CLAUDE.md files (if needed)
  • Read the contributing guide

Add a rejectUnescaped option to validateHandlebarTemplate /
isValidHandleBarTemplate that accepts only escaped expressions ({{ }}) and
rejects unescaped ones ({{{ }}} / {{& }}). Enable it for role and
additional-privilege create/update. The option defaults off, so dynamic-secret
statement validation (rendered with noEscape, e.g. LDAP) is unchanged. Adds unit
tests for both modes.
@Vligai Vligai requested a review from akhilmhdh June 29, 2026 17:44
@infisical-review-police

Copy link
Copy Markdown

💬 Discussion in Slack: #pr-review-infisical-7071-fix-template-reject-unescaped-expressions-in-permission

Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 94552dfbdc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread backend/src/services/role/role-service.ts
@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR closes a gap in the permission-template validator: previously, {{{ ... }}} (triple-mustache) and {{& ... }} (ampersand) unescaped expressions were accepted as long as their path was on the allowlist, even though rendered output would bypass HTML escaping.

  • Adds an opt-in rejectUnescaped flag to validateHandlebarTemplate / isValidHandleBarTemplate; when set, the validator reads the Handlebars AST escaped property and rejects any MustacheStatement where it is not true.
  • The flag is enabled for role-create, role-update, additional-privilege-create, and additional-privilege-update; dynamic-secret providers (which legitimately render with noEscape: true) are unaffected because they don't pass the flag.
  • A new unit-test file exercises both strict and lenient modes, including both unescaped forms ({{{ }}} and {{& }}), and stubs the logger so BadRequestError surfaces cleanly in isolation.

Confidence Score: 4/5

Safe to merge; the change is a targeted input-validation tightening with no side effects on existing callers.

The logic is straightforward and relies directly on the escaped property that the Handlebars AST always populates on every MustacheStatement node, so there is no plausible edge case where a triple-mustache expression slips through when the flag is set. Dynamic-secret callers are explicitly left unaffected. Test coverage is solid. The only pre-existing cosmetic issue — the updateAdditionalPrivilege function passing "Additional Privilege Create" as the template name — is unchanged by this PR.

No files require special attention; all changes are small and self-contained.

Important Files Changed

Filename Overview
backend/src/lib/template/validate-handlebars.ts Adds opt-in rejectUnescaped flag to both validator functions; uses Handlebars AST's escaped property correctly to distinguish {{ }} from {{{ }}} and {{& }}; default remains false preserving all existing callers.
backend/src/lib/template/validate-handlebars.test.ts New unit-test file covering strict and lenient modes for both exported functions; logger is correctly stubbed so the BadRequestError surfaces cleanly in a unit context.
backend/src/services/role/role-service.ts Adds rejectUnescaped: true to both role-create and role-update validateHandlebarTemplate calls; change is minimal and correctly scoped.
backend/src/services/additional-privilege/additional-privilege-service.ts Adds rejectUnescaped: true to both privilege-create and privilege-update validateHandlebarTemplate calls; the pre-existing "Additional Privilege Create" template name used in the update path is unchanged by this PR.

Reviews (1): Last reviewed commit: "fix(template): reject unescaped expressi..." | Re-trigger Greptile

@Vligai Vligai merged commit b6f2071 into main Jun 29, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants