Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
15 changes: 15 additions & 0 deletions .changeset/no-filters-in-render-arguments.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
'@shopify/theme-check-common': minor
---

Add `NoFiltersInRenderArguments` check to error when a filter is used in `render` and `include` tags.

Filters are not supported on values passed as arguments to `render`/`include` 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.
2 changes: 2 additions & 0 deletions packages/theme-check-common/src/checks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { MissingAsset } from './missing-asset';
import { MissingContentForArguments } from './missing-content-for-arguments';
import { MissingRenderSnippetArguments } from './missing-render-snippet-arguments';
import { MissingTemplate } from './missing-template';
import { NoFiltersInRenderArguments } from './no-filters-in-render-arguments';
import { OrphanedSnippet } from './orphaned-snippet';
import { PaginationSize } from './pagination-size';
import { ParserBlockingScript } from './parser-blocking-script';
Expand Down Expand Up @@ -100,6 +101,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [
MissingContentForArguments,
MissingRenderSnippetArguments,
MissingTemplate,
NoFiltersInRenderArguments,
AppBlockMissingSchema,
OrphanedSnippet,
PaginationSize,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { expect, describe, it } from 'vitest';
import { highlightedOffenses, runLiquidCheck } from '../../test';
import { NoFiltersInRenderArguments } from './index';

describe('Module: NoFiltersInRenderArguments', () => {
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(NoFiltersInRenderArguments, 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(NoFiltersInRenderArguments, 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(NoFiltersInRenderArguments, sourceCode);

expect(offenses).toHaveLength(0);
});

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(NoFiltersInRenderArguments, sourceCode);

expect(offenses).toHaveLength(0);
});

it('does not report on a plain render with no arguments', async () => {
const sourceCode = `{% render 'foo' %}`;
const offenses = await runLiquidCheck(NoFiltersInRenderArguments, 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(NoFiltersInRenderArguments, sourceCode);

expect(offenses).toHaveLength(1);
expect(offenses[0].message).toContain(
"Filters cannot be used on arguments passed to the 'render' tag",
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';

/**
* Finds the index of the first filter pipe (`|`) in a raw render/include 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` 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 NoFiltersInRenderArguments: LiquidCheckDefinition = {
meta: {
code: 'NoFiltersInRenderArguments',
name: 'No Filters in Render Arguments',
docs: {
description:
"This check warns against using filters on values passed as arguments to a 'render' or 'include' 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/no-filters-in-render-arguments',
},
type: SourceCodeType.LiquidHtml,
severity: Severity.ERROR,
schema: {},
targets: [],
},

create(context) {
return {
async LiquidTag(node) {
if (node.name !== 'render' && node.name !== 'include') return;
Comment thread
mbarak marked this conversation as resolved.
Outdated
Comment thread
mbarak marked this conversation as resolved.
Outdated

// 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,
});
},
};
},
};
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ MissingTemplate:
enabled: true
severity: 0
ignoreMissing: []
NoFiltersInRenderArguments:
enabled: true
severity: 0
OrphanedSnippet:
enabled: true
severity: 1
Expand Down
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/recommended.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ MissingTemplate:
enabled: true
severity: 0
ignoreMissing: []
NoFiltersInRenderArguments:
enabled: true
severity: 0
OrphanedSnippet:
enabled: true
severity: 1
Expand Down
Loading