diff --git a/.changeset/unsupported-filter-arguments.md b/.changeset/unsupported-filter-arguments.md new file mode 100644 index 000000000..0d40bce50 --- /dev/null +++ b/.changeset/unsupported-filter-arguments.md @@ -0,0 +1,15 @@ +--- +'@shopify/theme-check-common': minor +--- + +Add `UnsupportedFilterArguments` check to error when a filter is used in `render`, `include`, and `content_for` tags. + +Filters are not supported on values passed as arguments to `render`/`include`/`content_for` tags. In production, they are silently dropped, leading to unexpected outcomes when rendered. + +In the example below, the `bar` argument passes through unchanged when the snippet is rendered. + +```liquid +{% render 'foo', bar: 'hello' | append: ' world' %} +``` + +This check now reports an error pointing at the offending filter so the broken behavior is caught instead of failing silently. diff --git a/packages/theme-check-common/src/checks/index.ts b/packages/theme-check-common/src/checks/index.ts index be0370c41..6396762ed 100644 --- a/packages/theme-check-common/src/checks/index.ts +++ b/packages/theme-check-common/src/checks/index.ts @@ -49,6 +49,7 @@ import { UnrecognizedContentForArguments } from './unrecognized-content-for-argu import { UnrecognizedRenderSnippetArguments } from './unrecognized-render-snippet-arguments'; import { UnusedAssign } from './unused-assign'; import { UnsupportedDocTag } from './unsupported-doc-tag'; +import { UnsupportedFilterArguments } from './unsupported-filter-arguments'; import { UnusedDocParam } from './unused-doc-param'; import { ValidContentForArguments } from './valid-content-for-arguments'; import { ValidContentForArgumentTypes } from './valid-content-for-argument-types'; @@ -100,6 +101,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ MissingContentForArguments, MissingRenderSnippetArguments, MissingTemplate, + UnsupportedFilterArguments, AppBlockMissingSchema, OrphanedSnippet, PaginationSize, diff --git a/packages/theme-check-common/src/checks/unsupported-filter-arguments/index.spec.ts b/packages/theme-check-common/src/checks/unsupported-filter-arguments/index.spec.ts new file mode 100644 index 000000000..0ac72427d --- /dev/null +++ b/packages/theme-check-common/src/checks/unsupported-filter-arguments/index.spec.ts @@ -0,0 +1,121 @@ +import { expect, describe, it } from 'vitest'; +import { highlightedOffenses, runLiquidCheck } from '../../test'; +import { UnsupportedFilterArguments } from './index'; + +describe('Module: UnsupportedFilterArguments', () => { + it('reports an offense when a render argument uses a filter', async () => { + const sourceCode = `{% render 'foo', param1: 'bar', param2: 'hello' | append: ' world' %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain( + "Filters cannot be used on arguments passed to the 'render' tag", + ); + + const highlights = highlightedOffenses({ 'file.liquid': sourceCode }, offenses); + expect(highlights).toEqual([`| append: ' world'`]); + }); + + it('reports an offense when an include argument uses a filter', async () => { + const sourceCode = `{% include 'foo', x: bar | upcase %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain( + "Filters cannot be used on arguments passed to the 'include' tag", + ); + + const highlights = highlightedOffenses({ 'file.liquid': sourceCode }, offenses); + expect(highlights).toEqual([`| upcase`]); + }); + + it('does not report when no filter is used', async () => { + const sourceCode = `{% render 'foo', param1: 'bar', param2: 'hello' %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('reports an offense when a content_for argument uses a filter', async () => { + const sourceCode = `{% content_for 'block', type: 'foo', id: 'bar' | append: 'x' %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain( + "Filters cannot be used on arguments passed to the 'content_for' tag", + ); + + const highlights = highlightedOffenses({ 'file.liquid': sourceCode }, offenses); + expect(highlights).toEqual([`| append: 'x'`]); + }); + + it('does not report a false positive for a pipe inside a string literal', async () => { + const sourceCode = `{% render 'foo', sep: 'a|b' %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('does not report a false positive for a quoted pipe even when strict parse fails', async () => { + // `invalid_argument` forces the strict parse to fail, so the raw-markup + // scanner runs while the only pipe is still inside a quoted string literal. + const sourceCode = `{% render 'foo', sep: 'a|b', invalid_argument %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('does not report a false positive for a double-quoted pipe even when strict parse fails', async () => { + const sourceCode = `{% render 'foo', sep: "a|b", invalid_argument %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('highlights the real filter when a quoted pipe precedes it', async () => { + const sourceCode = `{% render 'foo', sep: 'a|b', x: bar | upcase %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(1); + + const highlights = highlightedOffenses({ 'file.liquid': sourceCode }, offenses); + expect(highlights).toEqual([`| upcase`]); + }); + + it('reports distinct highlights for two render tags on one line', async () => { + const sourceCode = `{% render 'foo', x: a | upcase %} {% render 'bar', y: b | downcase %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(2); + + const highlights = highlightedOffenses({ 'file.liquid': sourceCode }, offenses); + expect(highlights).toEqual([`| upcase`, `| downcase`]); + }); + + it('reports an offense for a render statement inside a {% liquid %} tag', async () => { + const sourceCode = `{% liquid\n render 'foo', x: bar | upcase\n%}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(1); + + const highlights = highlightedOffenses({ 'file.liquid': sourceCode }, offenses); + expect(highlights).toEqual([`| upcase`]); + }); + + it('does not report on a plain render with no arguments', async () => { + const sourceCode = `{% render 'foo' %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('reports offenses on nested render tags', async () => { + const sourceCode = `{% if true %}{% render 'foo', x: bar | upcase %}{% endif %}`; + const offenses = await runLiquidCheck(UnsupportedFilterArguments, sourceCode); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toContain( + "Filters cannot be used on arguments passed to the 'render' tag", + ); + }); +}); diff --git a/packages/theme-check-common/src/checks/unsupported-filter-arguments/index.ts b/packages/theme-check-common/src/checks/unsupported-filter-arguments/index.ts new file mode 100644 index 000000000..429c42790 --- /dev/null +++ b/packages/theme-check-common/src/checks/unsupported-filter-arguments/index.ts @@ -0,0 +1,89 @@ +import { NamedTags } from '@shopify/liquid-html-parser'; +import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; + +const CHECKED_TAGS: string[] = [NamedTags.render, NamedTags.include, NamedTags.content_for]; + +/** + * Finds the index of the first filter pipe (`|`) in a raw render/include/content_for + * markup string that is *not* inside a quoted string literal. Returns -1 if none is + * found. + * + * This is a heuristic on the raw markup because, when a `render`/`include`/`content_for` + * tag contains a filter, the strict grammar refuses it and the parser falls back to + * a base-case `LiquidTag` whose `markup` is a raw string (there is no structured + * `RenderMarkup` node to inspect). + */ +function indexOfFilterPipe(markup: string): number { + let quote: "'" | '"' | null = null; + + for (let i = 0; i < markup.length; i++) { + const char = markup[i]; + + if (quote) { + if (char === quote) { + quote = null; + } + continue; + } + + if (char === "'" || char === '"') { + quote = char; + continue; + } + + if (char === '|') { + return i; + } + } + + return -1; +} + +export const UnsupportedFilterArguments: LiquidCheckDefinition = { + meta: { + code: 'UnsupportedFilterArguments', + name: 'Unsupported Filter Arguments', + docs: { + description: + "This check warns against using filters on values passed as arguments to a 'render', 'include', or 'content_for' tag. " + + 'Filters are not applied in that position and the value is passed through unchanged, which silently ' + + 'produces incorrect output.', + recommended: true, + url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/unsupported-filter-arguments', + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.ERROR, + schema: {}, + targets: [], + }, + + create(context) { + return { + async LiquidTag(node) { + if (!CHECKED_TAGS.includes(node.name)) return; + + // When the tag parses successfully, `markup` is a structured + // `RenderMarkup` object and filters are impossible. A raw string means + // the strict parse failed (e.g. because of a filter pipe). + if (typeof node.markup !== 'string') return; + + const relativePipeIndex = indexOfFilterPipe(node.markup); + if (relativePipeIndex === -1) return; + + const markupStart = node.source.indexOf(node.markup, node.position.start); + if (markupStart === -1) return; + + const startIndex = markupStart + relativePipeIndex; + const endIndex = markupStart + node.markup.length; + + context.report({ + message: + `Filters cannot be used on arguments passed to the '${node.name}' tag. ` + + `Apply the filter beforehand (e.g. with {% assign %}) and pass the result instead.`, + startIndex, + endIndex, + }); + }, + }; + }, +}; diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index 426115894..c83e19ec4 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -160,6 +160,9 @@ UnrecognizedRenderSnippetArguments: UnsupportedDocTag: enabled: true severity: 0 +UnsupportedFilterArguments: + enabled: true + severity: 0 UnusedAssign: enabled: true severity: 1 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index adaa03fdf..79b8072e5 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -138,6 +138,9 @@ UnrecognizedRenderSnippetArguments: UnsupportedDocTag: enabled: true severity: 0 +UnsupportedFilterArguments: + enabled: true + severity: 0 UnusedAssign: enabled: true severity: 1