diff --git a/.changeset/liquid-unsafe-render-schema-types.md b/.changeset/liquid-unsafe-render-schema-types.md new file mode 100644 index 00000000000..23243add12d --- /dev/null +++ b/.changeset/liquid-unsafe-render-schema-types.md @@ -0,0 +1,6 @@ +--- +'@shopify/app': patch +'@shopify/cli': patch +--- + +Stop `shopify app security check` from flagging inert schema-typed app block settings (checkbox, range, and select/radio with safe option values) under LIQUID_UNSAFE_RENDER when they render as element text or safe quoted attributes diff --git a/packages/app/src/cli/services/app-security-engine/checks/LIQUID_UNSAFE_RENDER.md b/packages/app/src/cli/services/app-security-engine/checks/LIQUID_UNSAFE_RENDER.md index a322754caea..19bb9add4d3 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/LIQUID_UNSAFE_RENDER.md +++ b/packages/app/src/cli/services/app-security-engine/checks/LIQUID_UNSAFE_RENDER.md @@ -1,6 +1,6 @@ --- id: LIQUID_UNSAFE_RENDER -version: 1 +version: 2 severity: medium precedence: union --- @@ -8,3 +8,5 @@ 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 `${select}`)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`
${select}`)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`x${select}`)).toContain('LIQUID_UNSAFE_RENDER') + // Missing or unparseable schema stays flagged. + expect(liquid('{{ block.settings.layout }}')).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid('{{ block.settings.layout }}{% schema %}not-json{% endschema %}')).toContain('LIQUID_UNSAFE_RENDER') + // Duplicate schema blocks, duplicate setting ids, and empty options stay flagged. + expect(liquid(`{{ block.settings.layout }}${select}${select}`)).toContain('LIQUID_UNSAFE_RENDER') + expect( + liquid(`{{ block.settings.layout }}${schema('{"type":"text","id":"layout"},{"type":"checkbox","id":"layout"}')}`), + ).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`{{ block.settings.layout }}${schema('{"type":"select","id":"layout","options":[]}')}`)).toContain( + 'LIQUID_UNSAFE_RENDER', + ) + // Inert radio is clean. + expect( + liquid( + `{{ block.settings.layout }}${schema('{"type":"radio","id":"layout","options":[{"value":"a"},{"value":"b"}]}')}`, + ), + ).not.toContain('LIQUID_UNSAFE_RENDER') + }) + + test('exempts only block settings in app block files, never the host section settings', () => { + const schema = (settings: string) => `{% schema %}{"settings":[${settings}]}{% endschema %}` + const range = schema('{"type":"range","id":"size"}') + const select = schema('{"type":"select","id":"layout","options":[{"value":"left"},{"value":"right"}]}') + const liquid = (content: string, path = 'extensions/theme/blocks/a.liquid') => + scanLiquidSecurity([source(content, path)]).issues.map((finding) => finding.id) + + expect(liquid(`{{ block.settings.layout }}${select}`)).not.toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`{{ block.settings.size }}${range}`)).not.toContain('LIQUID_UNSAFE_RENDER') + // `section.settings` in an app block is the host section's, which this file's schema doesn't describe. + expect(liquid(`{{ section.settings.size }}${range}`)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`
${select}`)).toContain('LIQUID_UNSAFE_RENDER') + // Outside app blocks, no setting is exempt. + expect(liquid(`{{ block.settings.layout }}${select}`, 'extensions/theme/snippets/a.liquid')).toContain( + 'LIQUID_UNSAFE_RENDER', + ) + }) + + test('exempts filtered inert settings only through inert filters with inert literal arguments', () => { + const schema = `{% schema %}{"settings":[{"type":"checkbox","id":"show"},{"type":"text","id":"title"}]}{% endschema %}` + const liquid = (output: string) => + scanLiquidSecurity([source(`${output}${schema}`, 'extensions/theme/blocks/a.liquid')]).issues.map( + (finding) => finding.id, + ) + + expect(liquid('{{ block.settings.show | downcase }}')).not.toContain('LIQUID_UNSAFE_RENDER') + expect(liquid('{{ block.settings.show | strip | upcase | handleize | capitalize }}')).not.toContain( + 'LIQUID_UNSAFE_RENDER', + ) + expect(liquid("{{ block.settings.show | default: 'x' }}")).not.toContain('LIQUID_UNSAFE_RENDER') + expect(liquid("{{ block.settings.show | default: 'x', allow_false: true }}")).not.toContain('LIQUID_UNSAFE_RENDER') + expect(liquid('{{ block.settings.show | default: 3 }}')).not.toContain('LIQUID_UNSAFE_RENDER') + // Filters that are not proven inert keep the finding, even with literal or no arguments. + for (const filter of [ + 'url_decode', + 'base64_decode', + 't', + 'script_tag', + 'stylesheet_tag', + 'img_tag', + "link_to: 'x'", + "replace: 'true', 'on'", + "append: 'x'", + 'prepend: nil', + "downcase: 'x'", + ]) { + expect(liquid(`{{ block.settings.show | ${filter} }}`)).toContain('LIQUID_UNSAFE_RENDER') + } + // A `default` fallback is rendered, so it must be an inert literal. + expect(liquid("{{ block.settings.show | default: '' }}")).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid("{{ block.settings.show | default: 'a b' }}")).toContain('LIQUID_UNSAFE_RENDER') + // A setting or variable in any filter argument can render a free-text value. + expect(liquid('{{ block.settings.show | default: block.settings.title }}')).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid('{{ block.settings.show | append: block.settings.title }}')).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid('{{ block.settings.show | prepend: section.settings.title }}')).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid("{{ block.settings.show | replace: 'true', block.settings.title }}")).toContain( + 'LIQUID_UNSAFE_RENDER', + ) + expect(liquid('{{ block.settings.show | default: product.metafields.app.title }}')).toContain( + 'LIQUID_UNSAFE_RENDER', + ) + expect(liquid('{{ block.settings.show | downcase | append: title }}')).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid("{{ block.settings.show | default: 'x', allow_false: title }}")).toContain('LIQUID_UNSAFE_RENDER') + }) + + test('keeps decoding and markup filters flagged for percent-encoded select values', () => { + const schema = `{% schema %}{"settings":[{"type":"select","id":"layout","options":[{"value":"%3Cimg%20src%3Dx%20onerror%3Dalert(1)%3E"},{"value":"left"}]}]}{% endschema %}` + const liquid = (output: string) => + scanLiquidSecurity([source(`${output}${schema}`, 'extensions/theme/blocks/a.liquid')]).issues.map( + (finding) => finding.id, + ) + + // Percent-encoded text is inert until something decodes it. + expect(liquid('{{ block.settings.layout }}')).not.toContain('LIQUID_UNSAFE_RENDER') + expect(liquid('{{ block.settings.layout | url_decode }}')).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid('{{ block.settings.layout | t }}')).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid('{{ block.settings.layout | script_tag }}')).toContain('LIQUID_UNSAFE_RENDER') + }) + + test('exempts inert settings only in quoted values of statically named inert attributes', () => { + const schema = + '{% schema %}{"settings":[{"type":"select","id":"layout","options":[{"value":"left"},{"value":"onclick"}]}]}{% endschema %}' + const liquid = (content: string) => + scanLiquidSecurity([source(`${content}${schema}`, 'extensions/theme/blocks/a.liquid')]).issues.map( + (finding) => finding.id, + ) + const output = '{{ block.settings.layout }}' + + for (const attribute of ['class', 'id', 'title', 'alt', 'role', 'width', 'height', 'lang', 'dir', 'aria-label']) { + expect(liquid(`
`)).not.toContain('LIQUID_UNSAFE_RENDER') + } + expect(liquid(`x ${output}`)).not.toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`

{% if true %}${output}{% endif %}

`)).not.toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`
`)).not.toContain('LIQUID_UNSAFE_RENDER') + + // URL, script, style, data-*, and unknown attributes keep the finding. + for (const markup of [ + ``, + ``, + ``, + ``, + `x`, + ``, + `
`, + ``, + `
`, + `x`, + `
`, + ``, + ``, + ``, + ``, + `
`, + `
`, + `
`, + `
`, + ]) { + expect(liquid(markup)).toContain('LIQUID_UNSAFE_RENDER') + } + + // Output that can supply an attribute or tag name, unquoted values, and raw-text elements keep the finding. + expect(liquid(``)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`
`)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`
`)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`
`)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`
`)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`<${output}>x`)).toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(``)).toContain('LIQUID_UNSAFE_RENDER') + }) + + test('disables the exemption in files that rebind block', () => { + const schema = '{% schema %}{"settings":[{"type":"range","id":"size"}]}{% endschema %}' + const liquid = (content: string) => + scanLiquidSecurity([source(`${content}${schema}`, 'extensions/theme/blocks/a.liquid')]).issues.map( + (finding) => finding.id, + ) + const output = '{{ block.settings.size }}' + + expect(liquid(`{% assign item = section %}${output}`)).not.toContain('LIQUID_UNSAFE_RENDER') + expect(liquid(`{% for item in section.blocks %}{% endfor %}${output}`)).not.toContain('LIQUID_UNSAFE_RENDER') + + for (const binding of [ + '{% assign block = section %}', + '{% for block in section.blocks %}{% endfor %}', + '{% tablerow block in section.blocks %}{% endtablerow %}', + '{% capture block %}x{% endcapture %}', + '{% increment block %}', + '{% decrement block %}', + '{% liquid\n assign block = section\n%}', + ]) { + expect(liquid(`${binding}${output}`)).toContain('LIQUID_UNSAFE_RENDER') + // The binding disables the exemption for the whole file, including output before it. + expect(liquid(`${output}${binding}`)).toContain('LIQUID_UNSAFE_RENDER') + } + expect(liquid(`{% for block in section.blocks %}${output}{% endfor %}`)).toContain('LIQUID_UNSAFE_RENDER') + }) })