Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
---
id: LIQUID_UNSAFE_RENDER
version: 1
version: 2
severity: medium
precedence: union
---

# Liquid Unsafe Render

Inspect only theme-extension Liquid/HTML files the parser could not analyze. Liquid output is not automatically HTML-escaped. Check the destination: use `escape`/`escape_once` for HTML text and ordinary attributes, `json` when embedding a value as JavaScript data, and `metafield_tag` only for supported rich metafield rendering in HTML content. HTML escaping is not sufficient for event handlers, `srcdoc`, or a `<script src>` URL; avoid merchant-controlled output there and validate any dynamic URL. An ordinary non-script `src` attribute is not itself an executable sink. `{% raw %}` suppresses Liquid parsing, so apparent output inside it is literal text. Follow rendered snippets and inspect ambiguous filters or parser failures before reporting.

As of version 2, the deterministic rule exempts exact `block.settings.<id>` / `section.settings.<id>` output in HTML text and non-URL attribute contexts when the same file's `{% schema %}` defines the setting as `checkbox` or `range`, or as `select`/`radio` whose every option value is inert (no markup, quote, whitespace, or `=` characters). Missing, unparseable, or duplicated schema, unsafe option values, other types, property access, JavaScript/executable contexts, and URL attributes remain flagged as before.
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export const EMBEDDED_CHECK_SOURCES: ReadonlyArray<string> = [
"---\nid: EOL_API_VERSION\nversion: 2\nseverity: low\nprecedence: prefer-agent\n---\n\n# Eol Api Version\n\nInspect the selected `shopify.app*.toml` plus React Router `app/shopify.server.*` declarations. Shopify publishes quarterly versions in January, April, July, and October and supports each stable version for 12 months; the app security check allows a documented 30-day extension grace period before reporting it as end-of-life. Cite the exact declaration. For malformed config, computed `ApiVersion` values, or a Shopify-announced exceptional extension, inspect the source and current lifecycle policy rather than inferring from unrelated constants.\n",
"---\nid: EXPIRING_OFFLINE_TOKEN\nversion: 2\nseverity: medium\nprecedence: prefer-agent\n---\n\n# Expiring Offline Token\n\nFor supported React Router apps, verify `expiringOfflineAccessTokens` is enabled and the selected session storage persists `expires`, `refreshToken`, and `refreshTokenExpires` metadata needed for refresh and rotation. `isOnline: false` selects an offline session; it does not disable token expiry and is not a finding. Report an explicit `expiringOfflineAccessTokens: false`. Treat absent or computed flags, custom storage, and ambiguous Prisma schemas as unresolved investigation: inspect storage adapters, migrations, and serialization before returning a clean result. Config-only and unsupported frameworks are handled by the runtime applicability boundary.\n",
"---\nid: INSECURE_WEBHOOK_URL\nversion: 2\nseverity: high\nprecedence: prefer-agent\n---\n\n# Insecure Configured Callback Url\n\nInspect webhook destinations and OAuth redirect URLs in the selected Shopify app configuration. Relative Shopify paths and valid pubsub/eventbridge webhook destinations are allowed. Report HTTP, malformed, credential-bearing, wildcard-host, wildcard-path, or otherwise unsafe configured callback URLs.\n",
"---\nid: LIQUID_UNSAFE_RENDER\nversion: 1\nseverity: medium\nprecedence: union\n---\n\n# Liquid Unsafe Render\n\nInspect only theme-extension Liquid/HTML files the parser could not analyze. Liquid output is not automatically HTML-escaped. Check the destination: use `escape`/`escape_once` for HTML text and ordinary attributes, `json` when embedding a value as JavaScript data, and `metafield_tag` only for supported rich metafield rendering in HTML content. HTML escaping is not sufficient for event handlers, `srcdoc`, or a `<script src>` URL; avoid merchant-controlled output there and validate any dynamic URL. An ordinary non-script `src` attribute is not itself an executable sink. `{% raw %}` suppresses Liquid parsing, so apparent output inside it is literal text. Follow rendered snippets and inspect ambiguous filters or parser failures before reporting.\n",
"---\nid: LIQUID_UNSAFE_RENDER\nversion: 2\nseverity: medium\nprecedence: union\n---\n\n# Liquid Unsafe Render\n\nInspect only theme-extension Liquid/HTML files the parser could not analyze. Liquid output is not automatically HTML-escaped. Check the destination: use `escape`/`escape_once` for HTML text and ordinary attributes, `json` when embedding a value as JavaScript data, and `metafield_tag` only for supported rich metafield rendering in HTML content. HTML escaping is not sufficient for event handlers, `srcdoc`, or a `<script src>` URL; avoid merchant-controlled output there and validate any dynamic URL. An ordinary non-script `src` attribute is not itself an executable sink. `{% raw %}` suppresses Liquid parsing, so apparent output inside it is literal text. Follow rendered snippets and inspect ambiguous filters or parser failures before reporting.\n\nAs of version 2, the deterministic rule exempts exact `block.settings.<id>` / `section.settings.<id>` output in HTML text and non-URL attribute contexts when the same file's `{% schema %}` defines the setting as `checkbox` or `range`, or as `select`/`radio` whose every option value is inert (no markup, quote, whitespace, or `=` characters). Missing, unparseable, or duplicated schema, unsafe option values, other types, property access, JavaScript/executable contexts, and URL attributes remain flagged as before.\n",
"---\nid: METAFIELD_OFFLINE_TOKEN\nversion: 2\nseverity: high\n---\n\n# Metafield Offline Token\n\nTrace metafield and metaobject writes and determine their token provenance. Report a write that runs in an offline Admin API context that no verified request for the same shop authorizes: for example `unauthenticated.admin(shop)` or a stored offline session reached from an unauthenticated route, an unverified webhook or app proxy request, or a job whose shop or values come from request-controlled input.\n\nAn offline session returned by `authenticate.admin(request)` (the React Router SDK default when `useOnlineTokens` isn't set) is a verified, shop-bound context: Shopify verified the session token and the staff member's access to the app. Don't report a write only because its token is offline or because an online token would add user attribution or expiry. Report it when:\n\n- the app defines its own per-user permission, for example an app-level role or allowlist check, and this write bypasses it. Name that permission.\n- request-controlled values reach an app-reserved (`$app`) metafield or metaobject that the app treats as trusted state and merchants can't edit themselves, for example a definition with `access.admin = \"merchant_read\"` or no merchant access, or billing, entitlement or feature-flag data. Writing to the `$app` namespace isn't safe by itself.\n\nConfirm token provenance and compensating authorization before reporting.\n",
"---\nid: MISSING_AUTHORIZATION_CHECK\nversion: 3\nseverity: high\n---\n\nFind controller actions or route handlers that access resources without\nchecking whether the current user is authorized to access them, beyond\nauthentication. Authentication verifies WHO you are; authorization verifies\nWHAT you can do. An app can be authenticated but still access resources\nbelonging to another merchant if authorization checks are missing.\n\nThis is distinct from `MISSING_TENANT_ISOLATION` (database query\nscoping) — this check looks for missing policy/permission checks on\nactions, even when the data access is scoped. For example, an app might\nscope queries by shop but not check whether the merchant has the right to\ndelete a resource, or whether a staff member can access admin-only\nactions.\n\n## What to look for\n\n1. **Find authorization frameworks.** Check what the app uses:\n - Rails: Pundit (`authorize`, `policy`, `Pundit`), CanCanCan\n (`can?`, `ability`), action_access filters\n - Remix/Express: middleware that checks roles/permissions\n - Custom: `before_action :check_admin`, `if current_user.can?`\n\n2. **Find actions without authorization checks.** For each controller\n action or route handler, determine:\n - Is there a `before_action` that checks authorization (not just\n authentication)?\n - Is there a Pundit `authorize` call?\n - Is there a CanCanCan `authorize!` or `can?` check?\n - Is there a custom permission check?\n\n3. **Check for `skip_idor_protection` or equivalent opt-outs.** These\n disable IDOR/authorization checks. For each, determine:\n - Is the skip justified? (e.g., public endpoint, webhook, health check)\n - Does the skip expose a state-changing action to unauthorised users?\n - Is there a compensating control (HMAC, session token, etc.)?\n\n4. **Check for admin-only functionality reachable by merchants.** Look for:\n - Controllers under `admin/` namespace that don't check staff vs merchant\n - Actions that modify app configuration without checking the caller's role\n - Staff-only operations accessible through the merchant-facing UI\n\n5. **Check for missing object-level authorization.** Even if the query\n is scoped by shop, does the handler verify that the specific resource\n belongs to the current merchant?\n - `Order.find(params[:id])` scoped by shop — but does it check the\n merchant can access this specific order?\n - `Product.find(params[:id])` — is there a policy check, or just\n tenant scoping?\n\n6. **Compare authorization across state transitions.** Do not stop at one\n handler. Compare create/read/update/delete paths, replay after role or UI\n changes, direct URLs that survive permission downgrades, and backend actions\n that remain callable after a feature is hidden or disabled in the UI.\n\n## Shopify embedded apps (React Router)\n\n`authenticate.admin(request)` verifies the Shopify session token, so the caller is a staff member of that shop whom Shopify has granted access to this app. With offline tokens (the SDK default), Admin API calls use the app's granted scopes rather than the staff member's own permissions. That is Shopify's standard embedded app model, not a finding by itself. Report a handler only when it skips a concrete authorization boundary inside the app:\n\n- The app defines its own roles, owners, or allowlists (for example a check on `sessionToken.sub`, `onlineAccessInfo.associated_user`, `account_owner`, or an app database role), and a handler that performs the same or a more privileged operation doesn't apply it. Compare each route's loader and action, and sibling routes.\n- A request-supplied identifier selects an app-owned record that belongs to another user of the app.\n- The app uses online tokens to respect staff permissions on some paths, but a privileged path falls back to an offline session.\n\nIf none of these applies, record the check as executed with no finding.\n\n## What to report\n\nFor each action that exposes a concrete unauthorized operation beyond authentication:\n\n```json\n{\n \"file\": \"app/controllers/orders_controller.rb\",\n \"line\": 15,\n \"message\": \"Destroy action has no authorization check beyond authentication\",\n \"snippet\": \"def destroy\\n Order.find(params[:id]).destroy\\nend\",\n \"evidence\": [\n {\n \"file\": \"app/controllers/orders_controller.rb\",\n \"line\": 15,\n \"quote\": \"def destroy\"\n },\n {\n \"file\": \"app/controllers/orders_controller.rb\",\n \"line\": 5,\n \"quote\": \"before_action :authenticate_user (no authorize check)\"\n }\n ],\n \"confidence\": \"medium\",\n \"reasoning\": \"The destroy action authenticates the user but does not call authorize or check a policy. Any authenticated merchant can delete any order within their shop, even if they shouldn't have delete permissions.\"\n}\n```\n\nOnly report when you can show an unauthorized operation is actually reachable;\ndifferent helper names or inconsistent UI affordances are not enough by themselves.\n\nDo not report:\n\n- Actions with explicit `authorize` / `can?` / policy checks\n- Actions protected by a `before_action` that checks authorization\n- Public endpoints (health checks, static content)\n- Webhook handlers (HMAC is the authorization)\n- Actions that only read data the merchant owns (scoped by session.shop\n AND no object-level access control needed)\n- Internal/staff-only controllers (under `Internal::` namespace, behind\n employee SSO like `EmployeeIdentity`, `IdentityClient`, etc.)\n- Test files (under test/ or \\*\\_test.rb)\n- Test controllers\n",
"---\nid: MISSING_COMPLIANCE_WEBHOOKS\nversion: 1\nseverity: medium\nprecedence: prefer-agent\n---\n\n# Missing Compliance Webhooks\n\nInspect the selected `shopify.app*.toml` file. If it could not be parsed, keep the check unresolved. Verify all three compliance topics (`shop/redact`, `customers/data_request`, `customers/redact`) are declared in that configuration. Report only a confirmed missing topic.\n",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,81 @@ export function scanLiquidSecurity(files: SourceFile[]): LiquidScanResult {
parserFailures.push(file.path)
continue
}
issues.push(...visit<SourceCodeType.LiquidHtml, Issue>(ast, liquidUnsafeRenderVisitor(file)))
const schemaSettings = extractSchemaSettings(file.content)
issues.push(...visit<SourceCodeType.LiquidHtml, Issue>(ast, liquidUnsafeRenderVisitor(file, schemaSettings)))
issues.push(...visit<SourceCodeType.LiquidHtml, Issue>(ast, liquidExecutableContextVisitor(file)))
}
return {issues, parserFailures}
}

function liquidUnsafeRenderVisitor(file: SourceFile) {
interface SchemaSetting {
type?: unknown
options?: unknown
}

const URL_ATTRIBUTE_NAMES = new Set(['href', 'src', 'action', 'formaction', 'poster', 'cite', 'xlink:href'])
const INERT_OPTION_VALUE = /^[A-Za-z0-9\-._~:/?#@!$()*+,;%]*$/

// Extract id -> setting from the file's {% schema %} JSON. Returns undefined
// (fail-closed: no exemption) when the schema is missing, duplicated,
// unparseable, or has duplicate setting ids.
function extractSchemaSettings(content: string): Map<string, SchemaSetting> | undefined {
const blocks = [...content.matchAll(/\{%-?\s*schema\s*-?%\}([\s\S]*?)\{%-?\s*endschema\s*-?%\}/g)]
if (blocks.length !== 1) return undefined
let parsed: unknown
try {
parsed = JSON.parse(blocks[0]![1]!.trim())
// Unparseable schema fails closed: no exemption.
// eslint-disable-next-line no-catch-all/no-catch-all
} catch {
return undefined
}
if (!parsed || typeof parsed !== 'object') return undefined
const settings = (parsed as {settings?: unknown}).settings
if (!Array.isArray(settings)) return undefined
const byId = new Map<string, SchemaSetting>()
for (const setting of settings) {
if (!setting || typeof setting !== 'object') continue
const id = (setting as {id?: unknown}).id
if (typeof id !== 'string' || id === '') continue
if (byId.has(id)) return undefined
byId.set(id, setting as SchemaSetting)
}
return byId
}

function isSchemaExemptSetting(expression: string, schemaSettings: Map<string, SchemaSetting> | undefined): boolean {
if (!schemaSettings) return false
const baseExpression = expression.split('|')[0]!.trim()
const match = /^(?:block|section)\.settings\.([A-Za-z0-9_-]+)$/.exec(baseExpression)
if (!match) return false
const setting = schemaSettings.get(match[1]!)
if (!setting) return false
if (setting.type === 'checkbox' || setting.type === 'range') return true
if (setting.type === 'select' || setting.type === 'radio') {
if (!Array.isArray(setting.options) || setting.options.length === 0) return false
return setting.options.every((option) => {
if (!option || typeof option !== 'object') return false
const value = (option as {value?: unknown}).value
return typeof value === 'string' && INERT_OPTION_VALUE.test(value)
})
}
return false
}

function liquidUnsafeRenderVisitor(file: SourceFile, schemaSettings?: Map<string, SchemaSetting>) {
return {
LiquidVariableOutput(node: LiquidVariableOutput, ancestors: LiquidHtmlNode[]): Issue[] | undefined {
const expression = outputSource(node)
if (!/(?:\bmetafields?\b|\b(?:block|section)\.settings\b)/i.test(expression)) return undefined
const filters = filterNames(node)
const context = liquidOutputContext(ancestors)
if (isSafelyRendered(filters, context)) return undefined
if (
(context === 'html_text' || (context === 'html_attribute' && !isUrlAttributeContext(ancestors))) &&
isSchemaExemptSetting(expression, schemaSettings)
)
return undefined
return [
makeLiquidIssue(
'LIQUID_UNSAFE_RENDER',
Expand Down Expand Up @@ -116,6 +177,12 @@ function liquidOutputContext(ancestors: LiquidHtmlNode[]): LiquidOutputContext {
return 'html_attribute'
}

function isUrlAttributeContext(ancestors: LiquidHtmlNode[]): boolean {
const attribute = ancestors.find((ancestor): ancestor is AttributeNode => ancestor.type.startsWith('Attr'))
if (!attribute) return false
return URL_ATTRIBUTE_NAMES.has(attributeName(attribute).toLowerCase())
}

function attributeName(attribute: AttributeNode): string {
if (!('name' in attribute) || !Array.isArray(attribute.name)) return ''
return attribute.name.map((part) => ('value' in part && typeof part.value === 'string' ? part.value : '')).join('')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ const DETERMINISTIC_CHECK_DEFINITIONS: ReadonlyArray<DeterministicCheckDefinitio
jsCheck('CREDENTIAL_BROWSER_LEAKAGE', (context) => scanCredentialBrowserLeakage(context.sourceFiles), 'source', 2),
{
id: 'LIQUID_UNSAFE_RENDER',
version: 1,
version: 2,
lifecycle: 'active',
analysisMode: 'ast',
target: 'theme',
Expand Down
Loading
Loading