feat: introduce secure API for template verification #693
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
diervo
changed the title
pmdartus
requested changes
Oct 1, 2018
| afterEach(() => (secure.enabled = false)); | ||
|
|
||
| it('forbidden access to template', () => { | ||
| function html($api) { |
There was a problem hiding this comment.
Can you add a comment to explain that you are not using the inline template compiler on purpose. I am in the process of removing all those handcrafting templates.
| verifyTemplate | ||
| }; | ||
|
|
||
| function registerTemplate(tmpl) { |
| } | ||
| } | ||
|
|
||
| function verifyTemplate(tmpl) { |
| SECURE_IMPORT_NAME, SECURE_REGISTER_TEMPLATE_METHOD_NAME, | ||
| LWC_MODULE_NAME | ||
| } from '../../shared/constants'; | ||
| import { FunctionDeclaration, ExportDefaultDeclaration } from 'babel-types'; |
There was a problem hiding this comment.
Use t.FunctionDeclaration imported line 1 instead of importing the types using named imports.
| @@ -1,3 +1,4 @@ | |||
| import { ResolvedConfig } from '../config'; | |||
There was a problem hiding this comment.
Move relative imports below module imports.
| const metadata = generateTemplateMetadata(state); | ||
| let templateBody: Array<FunctionDeclaration | ExportDefaultDeclaration> = [t.exportDefaultDeclaration(templateFn)]; |
There was a problem hiding this comment.
Performance wise, a if/else statement is better than overriding the default value with a single if block.
| @@ -17,6 +17,31 @@ function functionMatchCode(fn, code) { | |||
| ); | |||
| } | |||
|
|
|||
|
|
|||
| describe('option secure', () => { | |||
| it.only('validate secure transformation', () => { | |||
| describe('option secure', () => { | ||
| it.only('validate secure transformation', () => { | ||
| const { code } = compiler(`<template><x-test></x-test></template>`, { secure: true }); | ||
| functionMatchCode(code, ` |
| @@ -93,7 +95,7 @@ export function invokeComponentRenderMethod(vm: VM): VNodes { | |||
| try { | |||
| const html = callHook(component, render); | |||
| if (isFunction(html)) { | |||
| result = evaluateTemplate(vm, html); | |||
| result = evaluateTemplate(vm, secure.verifyTemplate(html)); | |||
There was a problem hiding this comment.
The validation should be moved inside the evaluateTemplate method. The invokeComponentRenderMethod is invoked for each render, we should only validate the template is the template is different than the previous render.
pmdartus
approved these changes
Oct 1, 2018
Benchmark resultsBase commit: |
Benchmark resultsBase commit: lwc-engine-benchmark
|
apapko
reviewed
Oct 1, 2018
| @@ -16,7 +16,8 @@ const DEFAULT_OUTPUT_CONFIG = { | |||
| NODE_ENV: "development" | |||
| }, | |||
| minify: false, | |||
| compat: false | |||
| compat: false, | |||
| secure: false | |||
There was a problem hiding this comment.
I propose making secure value true by default. In my opinion its better to be restrictive and allow open source consumers to opt-out if they like. Additionally, having secure true by default will minimize bugs in aura because it has multiple entry points into compiler invocation which can be more error prone - for example forgetting to pass in 'secure' or creating new compiler entry point without secure will result in inconsistent behavior. I feel like aura should have no control of this flag at all as secure should be enforced for internal dev at all time. Thoughts?
There was a problem hiding this comment.
we don't need an option, it should always be the case.
|
|
||
| it('forbidden access to template', () => { | ||
| // We can't use the inline template compiler here | ||
| // since precesily we are trying to test that handcrafted |
There was a problem hiding this comment.
typo: precisely instead of precesily
| return tmpl; | ||
| } | ||
|
|
||
| export const secure = SECURE_OBJECT; |
There was a problem hiding this comment.
should we expose a mechanism to de-register the template?
| @@ -97,6 +98,10 @@ export function evaluateTemplate(vm: VM, html: Template): Array<VNode|null> { | |||
| // template, because they could have similar IDs, and snabbdom just rely on the IDs. | |||
| resetShadowRoot(vm); | |||
| } | |||
|
|
|||
| // Check that the template is built by the compiler | |||
| secure.verifyTemplate(html); | |||
There was a problem hiding this comment.
what do you think of renaming verifyTemplate to something like assertTemplateRegistered? When i see verify i think of a boolean return instead a throw. Just a food for thought to improve readability
|
|
||
| import { | ||
| TEMPLATE_FUNCTION_NAME, | ||
| SECURE_IMPORT_NAME, SECURE_REGISTER_TEMPLATE_METHOD_NAME, |
There was a problem hiding this comment.
pls move SECURE_REGISTER_TEMPLATE_METHOD_NAME to a new line
| @@ -16,18 +23,47 @@ function moduleNameToImport(name: string): t.ImportDeclaration { | |||
| ); | |||
| } | |||
|
|
|||
| function generateSecureImport(): t.ImportDeclaration { | |||
| return t.importDeclaration( | |||
| [t.importSpecifier(t.identifier(SECURE_IMPORT_NAME), t.identifier(SECURE_IMPORT_NAME))], | |||
There was a problem hiding this comment.
are both importSpecifier parameters intentionally the same?
caridy
suggested changes
Oct 1, 2018
There was a problem hiding this comment.
I will like to restrict and simplify this a little bit more.
- compiler should use
import { template } from "lwc/secure"; - validate does not need to be exposed (the engine can validate itself, locker has no saying on this).
enabledis not needed, it should always be enabled.
The reason for 1, is that you don't have to restrict import *, or the typescript definition, or any other future option for modules. It should come from its own, restricted module specifier that can only be use by the transformation, and can't be, ever, use by userland code, not even high privilege code that import those things.
In the future, I want to move api.ts to be exposed via import { api } from "lwc/secure", so the same rules applies.
Reason for 2 is that we want to protect ourself, even on the open source, to prevent people from using manual creation of vnodes, since any future change my break them (look at VUE 3 as an example of this problem).
Reason for 3 is pretty much the same reason for 2.
|
@caridy You are missing some context:
Idealistically I would be ok making this non-optional, but the secure object intent was to be able to allow customization for all of this other security features that must be able to be configurable at runtime by the application owner. |
caridy
reviewed
Oct 1, 2018
| import { Template } from "../framework/template"; | ||
|
|
||
| const DEFAULT_SECURE_ENABLEMENT = false; | ||
| const VERIFIED_TEMPLATES = new Set(); |
There was a problem hiding this comment.
call this verifiedTemplateSet
caridy
reviewed
Oct 1, 2018
| verifyTemplate | ||
| }; | ||
|
|
||
| function registerTemplate(tmpl: Template) { |
There was a problem hiding this comment.
this logic should probably belong into template.ts
caridy
reviewed
Oct 1, 2018
| @@ -97,6 +98,10 @@ export function evaluateTemplate(vm: VM, html: Template): Array<VNode|null> { | |||
| // template, because they could have similar IDs, and snabbdom just rely on the IDs. | |||
| resetShadowRoot(vm); | |||
| } | |||
|
|
|||
| // Check that the template is built by the compiler | |||
| secure.verifyTemplate(html); | |||
There was a problem hiding this comment.
this should probably be a local function that does the validation against a local set.
Yes, we need to restrict access on a per module basis, not on a per export basis. cc @pmdartus |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
Oct 5, 2018
|
|
||
| export function verifyTemplate(tmpl: Template): Template { | ||
| if (!VERIFIED_TEMPLATE_SET.has(tmpl)) { | ||
| throw new TypeError('Unknown template'); |
There was a problem hiding this comment.
this should probably throw a ReferenceError instead. It should also provide more details about what component is at fault, otherwise you don't know much, this is because the render() method is called, and the result is the analyzed, so, throwing here will not have that render method in the error stack, which will make it very hard to know what's going on.
Probably in dev-mode we should have some nice logError with the proper component stack.
caridy
reviewed
Oct 5, 2018
|
|
||
| const VERIFIED_TEMPLATE_SET = new Set(); | ||
|
|
||
| export function verifyTemplate(tmpl: Template): Template { |
There was a problem hiding this comment.
the signature of this method is weird, it is a verification, but it returns the same template. Maybe calling this: isRegisteredTemplate(tmpl: Template): boolean
caridy
reviewed
Oct 5, 2018
| @@ -8,7 +8,8 @@ jest.mock('globalLib', () => { | |||
| }); | |||
|
|
|||
| describe('example-foo-inner', () => { | |||
| it('default snapshot', () => { | |||
| it.only('default snapshot', () => { | |||
| debugger; | |||
caridy
reviewed
Oct 5, 2018
| @@ -8,7 +8,8 @@ jest.mock('globalLib', () => { | |||
| }); | |||
|
|
|||
| describe('example-foo-inner', () => { | |||
| it('default snapshot', () => { | |||
| it.only('default snapshot', () => { | |||
caridy
reviewed
Oct 5, 2018
| @@ -0,0 +1,16 @@ | |||
| import { Template } from "./template"; | |||
There was a problem hiding this comment.
why does this need to be separate from template.ts? it looks to me that it belongs there, so we don't have to export the verification api, it is all internal to that template.ts
caridy
suggested changes
Oct 5, 2018
Benchmark resultsBase commit: lwc-engine-benchmark
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Adding a mechanism to prevent templates to be created in user land.
The compiler will produce this new code:
The engine will have the references saved so it can know when anything is not coming from the compiler. Note that the named import
registerTemplatemust not be available for consumption in user-land.Does this PR introduce a breaking change?