From a8b53cd7d6945b3673fa55abff64337a026b4467 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Tue, 1 Sep 2020 17:11:19 -0700 Subject: [PATCH] [BUGFIX] Prevent accidental dynamic component lookup Dynamic components invoked via `{{component}}` share a codepath with ones invoked via ``, and because of that currently if `` is a referring to a string, it will dynamically lookup the component. This is not expected behavior, and this PR prevents it by using a different, more restricted opcode for these lookups. --- .../lib/suites/components.ts | 15 +++++++ .../interfaces/lib/compile/encoder.d.ts | 1 + .../@glimmer/interfaces/lib/vm-opcodes.d.ts | 41 ++++++++++--------- .../lib/opcode-builder/helpers/components.ts | 7 +++- .../opcode-compiler/lib/syntax/builtins.ts | 2 + .../lib/syntax/push-compile.ts | 3 +- .../opcode-compiler/lib/syntax/statements.ts | 1 + .../lib/compiled/opcodes/-debug-strip.ts | 18 ++++++++ .../runtime/lib/compiled/opcodes/component.ts | 29 +++++++++++-- 9 files changed, 90 insertions(+), 27 deletions(-) diff --git a/packages/@glimmer/integration-tests/lib/suites/components.ts b/packages/@glimmer/integration-tests/lib/suites/components.ts index 511d4a6c34..baf7877140 100644 --- a/packages/@glimmer/integration-tests/lib/suites/components.ts +++ b/packages/@glimmer/integration-tests/lib/suites/components.ts @@ -556,6 +556,21 @@ export class BasicComponents extends RenderTest { this.assertStableRerender(); } + @test({ + kind: 'glimmer', + }) + 'invoking dynamic component (path) via angle brackets does not work for string'() { + class TestHarness extends EmberishGlimmerComponent { + public Foo: any; + } + this.registerComponent('Glimmer', 'TestHarness', '', TestHarness); + this.registerComponent('Glimmer', 'Foo', 'hello world!'); + + this.assert.throws(() => { + this.render(''); + }, /Expected a curried component definition, but received Foo. You may have accidentally done , where \"this.Foo\"/); + } + @test({ kind: 'glimmer', }) diff --git a/packages/@glimmer/interfaces/lib/compile/encoder.d.ts b/packages/@glimmer/interfaces/lib/compile/encoder.d.ts index 43b52f7647..d3d2aa93df 100644 --- a/packages/@glimmer/interfaces/lib/compile/encoder.d.ts +++ b/packages/@glimmer/interfaces/lib/compile/encoder.d.ts @@ -167,6 +167,7 @@ export interface DynamicComponentOp { args: WireFormat.Core.Hash; blocks: WireFormat.Core.Blocks | NamedBlocks; atNames: boolean; + curried: boolean; }; } diff --git a/packages/@glimmer/interfaces/lib/vm-opcodes.d.ts b/packages/@glimmer/interfaces/lib/vm-opcodes.d.ts index e9fa3609e4..1802f1db9d 100644 --- a/packages/@glimmer/interfaces/lib/vm-opcodes.d.ts +++ b/packages/@glimmer/interfaces/lib/vm-opcodes.d.ts @@ -77,26 +77,27 @@ export const enum Op { PushDynamicComponentInstance = 78, PushCurriedComponent = 79, ResolveDynamicComponent = 80, - PushArgs = 81, - PushEmptyArgs = 82, - PopArgs = 83, - PrepareArgs = 84, - CaptureArgs = 85, - CreateComponent = 86, - RegisterComponentDestructor = 87, - PutComponentOperations = 88, - GetComponentSelf = 89, - GetComponentTagName = 90, - GetComponentLayout = 91, - BindEvalScope = 92, - SetupForEval = 93, - PopulateLayout = 94, - InvokeComponentLayout = 95, - BeginComponentTransaction = 96, - CommitComponentTransaction = 97, - DidCreateElement = 98, - DidRenderLayout = 99, - InvokePartial = 100, + ResolveCurriedComponent = 81, + PushArgs = 82, + PushEmptyArgs = 83, + PopArgs = 84, + PrepareArgs = 85, + CaptureArgs = 86, + CreateComponent = 87, + RegisterComponentDestructor = 88, + PutComponentOperations = 89, + GetComponentSelf = 90, + GetComponentTagName = 91, + GetComponentLayout = 92, + BindEvalScope = 93, + SetupForEval = 94, + PopulateLayout = 95, + InvokeComponentLayout = 96, + BeginComponentTransaction = 97, + CommitComponentTransaction = 98, + DidCreateElement = 99, + DidRenderLayout = 100, + InvokePartial = 101, ResolveMaybeLocal = 102, Debugger = 103, Size = 104, diff --git a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/components.ts b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/components.ts index 8e91735c0a..c51b419751 100644 --- a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/components.ts +++ b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/components.ts @@ -52,6 +52,7 @@ interface AnyComponent { export interface DynamicComponent extends AnyComponent { definition: WireFormat.Expression; atNames: boolean; + curried: boolean; } // (component) @@ -250,7 +251,7 @@ export function InvokeStaticComponent({ export function InvokeDynamicComponent( meta: ContainingMetadata, - { definition, attrs, params, hash, atNames, blocks }: DynamicComponent + { definition, attrs, params, hash, atNames, blocks, curried }: DynamicComponent ): StatementCompileActions { return Replayable({ args: () => { @@ -263,7 +264,9 @@ export function InvokeDynamicComponent( body: () => { return [ op(Op.JumpUnless, label('ELSE')), - op(Op.ResolveDynamicComponent, templateMeta(meta.referrer)), + curried + ? op(Op.ResolveCurriedComponent) + : op(Op.ResolveDynamicComponent, templateMeta(meta.referrer)), op(Op.PushDynamicComponentInstance), InvokeComponent({ capabilities: true, diff --git a/packages/@glimmer/opcode-compiler/lib/syntax/builtins.ts b/packages/@glimmer/opcode-compiler/lib/syntax/builtins.ts index f8efb71cca..6b9bde293b 100644 --- a/packages/@glimmer/opcode-compiler/lib/syntax/builtins.ts +++ b/packages/@glimmer/opcode-compiler/lib/syntax/builtins.ts @@ -236,6 +236,7 @@ export function populateBuiltins( args: hash, atNames: false, blocks, + curried: false, }); }); @@ -259,6 +260,7 @@ export function populateBuiltins( hash, atNames: false, blocks: EMPTY_BLOCKS, + curried: false, }); }); diff --git a/packages/@glimmer/opcode-compiler/lib/syntax/push-compile.ts b/packages/@glimmer/opcode-compiler/lib/syntax/push-compile.ts index 48bf7ba81b..facdbd3378 100644 --- a/packages/@glimmer/opcode-compiler/lib/syntax/push-compile.ts +++ b/packages/@glimmer/opcode-compiler/lib/syntax/push-compile.ts @@ -66,7 +66,7 @@ function DynamicComponent( context: TemplateCompilationContext, action: DynamicComponentOp ): StatementCompileActions { - let { definition, attrs, params, args, blocks, atNames } = action.op1; + let { definition, attrs, params, args, blocks, atNames, curried } = action.op1; let attrsBlock = attrs && attrs.length > 0 ? compilableBlock(attrs, context.meta) : null; @@ -80,6 +80,7 @@ function DynamicComponent( hash: args, atNames, blocks: compiled, + curried, }); } diff --git a/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts b/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts index 295efd4627..b084bc8d47 100644 --- a/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts +++ b/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts @@ -142,6 +142,7 @@ STATEMENTS.add(SexpOpcodes.Component, ([, tag, attrs, args, blocks]) => { args, blocks, atNames: true, + curried: true, }); } }); diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/-debug-strip.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/-debug-strip.ts index 3afb84d3a0..ba2c1130ba 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/-debug-strip.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/-debug-strip.ts @@ -31,6 +31,10 @@ import { Tag, COMPUTE } from '@glimmer/validator'; import { PartialScopeImpl } from '../../scope'; import { VMArgumentsImpl } from '../../vm/arguments'; import { ComponentInstance, ComponentElementOperations } from './component'; +import { + CurriedComponentDefinition, + isCurriedComponentDefinition, +} from '../../component/curried-component'; export const CheckTag: Checker = CheckInterface({ [COMPUTE]: CheckFunction, @@ -106,6 +110,20 @@ export const CheckComponentDefinition: Checker = CheckInter manager: CheckComponentManager, }); +class CurriedComponentDefinitionChecker implements Checker { + type!: CurriedComponentDefinition; + + validate(value: unknown): value is CurriedComponentDefinition { + return isCurriedComponentDefinition(value); + } + + expected(): string { + return `CurriedComponentDefinition`; + } +} + +export const CheckCurriedComponentDefinition = new CurriedComponentDefinitionChecker(); + export const CheckInvocation: Checker = CheckInterface({ handle: CheckNumber, symbolTable: CheckProgramSymbolTable, diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/component.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/component.ts index 4aabc0e9c3..e576eb3639 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/component.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/component.ts @@ -5,6 +5,8 @@ import { CheckInstanceof, CheckInterface, CheckProgramSymbolTable, + CheckString, + CheckOr, } from '@glimmer/debug'; import { Bounds, @@ -74,6 +76,7 @@ import { CheckFinishedComponentInstance, CheckInvocation, CheckReference, + CheckCurriedComponentDefinition, } from './-debug-strip'; import { UpdateDynamicAttributeOpcode } from './dom'; import { DEBUG } from '@glimmer/env'; @@ -163,7 +166,10 @@ APPEND_OPCODES.add(Op.PushComponentDefinition, (vm, { op1: handle }) => { APPEND_OPCODES.add(Op.ResolveDynamicComponent, (vm, { op1: _meta }) => { let stack = vm.stack; - let component = valueForRef(check(stack.popJs(), CheckReference)) as Maybe; + let component = check( + valueForRef(check(stack.popJs(), CheckReference)), + CheckOr(CheckString, CheckCurriedComponentDefinition) + ); let meta = vm[CONSTANTS].getValue(_meta); vm.loadValue($t1, null); // Clear the temp register @@ -174,12 +180,27 @@ APPEND_OPCODES.add(Op.ResolveDynamicComponent, (vm, { op1: _meta }) => { let resolvedDefinition = resolveComponent(vm.runtime.resolver, component, meta); definition = expect(resolvedDefinition, `Could not find a component named "${component}"`); - } else if (isCurriedComponentDefinition(component)) { - definition = component; } else { - throw unreachable(); + definition = component; + } + + stack.pushJs(definition); +}); + +APPEND_OPCODES.add(Op.ResolveCurriedComponent, (vm) => { + let stack = vm.stack; + let ref = check(stack.popJs(), CheckReference); + let value = valueForRef(ref); + + if (DEBUG && !isCurriedComponentDefinition(value)) { + throw new Error( + `Expected a curried component definition, but received ${value}. You may have accidentally done <${ref.debugLabel}>, where "${ref.debugLabel}" was a string instead of a curried component definition. You must use the {{component}} helper to create a component definition when invoking dynamically.` + ); } + let definition = value as CurriedComponentDefinition; + + vm.loadValue($t1, null); // Clear the temp register stack.pushJs(definition); });