Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial security type checking. #62

Merged
merged 8 commits into from
Nov 19, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
4 changes: 4 additions & 0 deletions packages/lit-analyzer/src/analyze/lit-analyzer-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,12 @@ export function litDiagnosticRuleSeverity(config: LitAnalyzerConfig, ruleName: L

export type LitAnalyzerLogging = "off" | "error" | "warn" | "debug" | "verbose";

export type LitSecuritySystem = "off" | "ClosureSafeTypes";

export interface LitAnalyzerConfig {
strict: boolean;
rules: LitAnalyzerRules;
securitySystem: LitSecuritySystem;

disable: boolean;
logging: LitAnalyzerLogging;
Expand All @@ -165,6 +168,7 @@ export function makeConfig(userOptions: Partial<LitAnalyzerConfig> = {}): LitAna
return {
strict: userOptions.strict || false,
rules: makeRules(userOptions),
securitySystem: userOptions.securitySystem || "off",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this come straight from a user config file, or can we assume it's validated as a partial config already? (Otherwise should this validate and throw?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following convention here, I didn't throw. Added some validation, since I didn't see other validation. @runem what do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The configuration given to lit-analyzer is not validated as of now. First of all, I think the CLI should throw on invalid values, but ts-lit-plugin should only log warnings (no reason to crash when running in the IDE).

Therefore, I think that we should implement one of these solutions:
a. export a validateConfig function that validates the raw, partial config.
b. extend makeConfig with an option to assert values (perhaps a callback that is called on invalid config values).

I also think makeConfig should properly fall back to default values when encountering an invalid value (which it doesn't do for all attributes right now).

I created an issue for it here #65 so that it can be done in a future, separate PR 👍


disable: userOptions.disable || false,
logging: userOptions.logging || "off",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import { HtmlNodeAttr } from "../../types/html-node/html-node-attr-types";
import { SimpleType, toTypeString, SimpleTypeKind } from "ts-simple-type";
import { LitAnalyzerRequest } from "../../lit-analyzer-context";
import { LitHtmlDiagnostic, LitHtmlDiagnosticKind } from "../../types/lit-diagnostic";
import { LitAnalyzerRuleName } from "../../lit-analyzer-config";

/**
* If the user's security policy overrides normal type checking for this
* attribute binding, returns a (possibly empty) array of diagnostics.
*
* If the security policy does not apply to this binding, then
*/
export function isAssignableBindingUnderSecuritySystem(
htmlAttr: HtmlNodeAttr,
{ typeA, typeB }: { typeA: SimpleType; typeB: SimpleType },
request: LitAnalyzerRequest,
source: LitAnalyzerRuleName
): LitHtmlDiagnostic[] | undefined {
const securityPolicy = request.config.securitySystem;
switch (securityPolicy) {
case "off":
return undefined; // No security checks apply.
case "ClosureSafeTypes":
return checkClosureSecurityAssignability(typeB, htmlAttr, request, source);
default: {
const never: never = securityPolicy;
request.logger.error(`Unexpected security policy: ${never}`);
return undefined;
}
}
}

interface TagNameToSecurityOverrideMap {
[tagName: string]: SecurityOverrideMap | undefined;
}

// A map from attribute/property names to an array of type names.
// Assignments to the given attribute must match one of the given types.
interface SecurityOverrideMap {
[attributeName: string]: string[] | undefined;
}

const closureScopedOverrides: TagNameToSecurityOverrideMap = {
iframe: {
src: ["TrustedResourceUrl"]
},
a: {
href: ["TrustedResourceUrl", "SafeUrl"]
},
img: {
src: ["TrustedResourceUrl", "SafeUrl"]
},
script: {
src: ["TrustedResourceUrl"]
}
};
const closureGlobalOverrides: SecurityOverrideMap = {
style: ["SafeStyle"]
};

function checkClosureSecurityAssignability(
typeB: SimpleType,
htmlAttr: HtmlNodeAttr,
request: LitAnalyzerRequest,
source: LitAnalyzerRuleName
): LitHtmlDiagnostic[] | undefined {
const scopedOverride = closureScopedOverrides[htmlAttr.htmlNode.tagName];
const overriddenTypes = (scopedOverride && scopedOverride[htmlAttr.name]) || closureGlobalOverrides[htmlAttr.name];
if (overriddenTypes === undefined) {
return undefined;
}
if (!matchesAtLeastOneNominalType(overriddenTypes, typeB)) {
const nominalType: SimpleType = {
kind: SimpleTypeKind.INTERFACE,
members: [],
name: "A security type"
};
return [
{
kind: LitHtmlDiagnosticKind.INVALID_ATTRIBUTE_EXPRESSION_TYPE,
message: `Type '${toTypeString(typeB)}' is not assignable to '${overriddenTypes.join(" | ")}'.`,
severity: "error",
source,
location: { document: request.document, ...htmlAttr.location.name },
htmlAttr,
typeA: nominalType,
typeB
}
];
}
return [];
}

function matchesAtLeastOneNominalType(typeNames: string[], typeB: SimpleType): boolean {
if (typeB.name !== undefined && typeNames.includes(typeB.name)) {
return true;
}
switch (typeB.kind) {
case "UNION":
return typeB.types.every(t => matchesAtLeastOneNominalType(typeNames, t));
case "STRING_LITERAL":
case "STRING":
return typeNames.includes("string");
case "GENERIC_ARGUMENTS":
return matchesAtLeastOneNominalType(typeNames, typeB.target);
default:
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { HtmlNodeAttr } from "../../types/html-node/html-node-attr-types";
import { LitHtmlDiagnostic, LitHtmlDiagnosticKind } from "../../types/lit-diagnostic";
import { isAssignableToType } from "./is-assignable-to-type";
import { isLitDirective } from "../directive/is-lit-directive";
import { isAssignableBindingUnderSecuritySystem } from "./is-assignable-binding-under-security-system";

export function isAssignableInAttributeBinding(
htmlAttr: HtmlNodeAttr,
Expand All @@ -31,6 +32,21 @@ export function isAssignableInAttributeBinding(
];
}
} else {
if (assignment.kind !== HtmlNodeAttrAssignmentKind.STRING) {
// Purely static attributes are never security checked, they're handled
// in the lit-html internals as trusted by default, because they can
// not contain untrusted data, they were written by the developer.
//
// For everything else, we may need to apply a different type comparison
// for some security-sensitive built in attributes and properties (like
// <script src>).
const securityDiagnostics = isAssignableBindingUnderSecuritySystem(htmlAttr, { typeA, typeB }, request, "no-incompatible-type-binding");
if (securityDiagnostics !== undefined) {
// The security diagnostics are binding. Note that this may be an
// empty array.
return securityDiagnostics;
}
}
if (!isAssignableToType({ typeA, typeB }, request, { isAssignable: isAssignableToTypeWithStringCoercion })) {
return [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,20 @@ import { LitAnalyzerRequest } from "../../lit-analyzer-context";
import { HtmlNodeAttr } from "../../types/html-node/html-node-attr-types";
import { LitHtmlDiagnostic, LitHtmlDiagnosticKind } from "../../types/lit-diagnostic";
import { isAssignableToType } from "./is-assignable-to-type";
import { isAssignableBindingUnderSecuritySystem } from "./is-assignable-binding-under-security-system";

export function isAssignableInPropertyBinding(
htmlAttr: HtmlNodeAttr,
{ typeA, typeB }: { typeA: SimpleType; typeB: SimpleType },
request: LitAnalyzerRequest
): LitHtmlDiagnostic[] | undefined {
const securityDiagnostics = isAssignableBindingUnderSecuritySystem(htmlAttr, { typeA, typeB }, request, "no-incompatible-type-binding");
if (securityDiagnostics !== undefined) {
// The security diagnostics are binding. Note that this may be an
// empty array.
return securityDiagnostics;
}

if (!isAssignableToType({ typeA, typeB }, request)) {
return [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { LitHtmlDiagnosticKind } from "../analyze/types/lit-diagnostic";
import { RuleModule } from "../analyze/types/rule-module";
import { isLitDirective } from "../analyze/util/directive/is-lit-directive";
import { extractBindingTypes } from "../analyze/util/type/extract-binding-types";
import { isAssignableBindingUnderSecuritySystem } from "../analyze/util/type/is-assignable-binding-under-security-system";

/**
* This rule validates that complex types are not used within an expression in an attribute binding.
Expand All @@ -23,6 +24,12 @@ const rule: RuleModule = {

// Only primitive types should be allowed as "typeB"
if (!isAssignableToPrimitiveType(typeB)) {
if (isAssignableBindingUnderSecuritySystem(htmlAttr, { typeA, typeB }, request, this.name) !== undefined) {
// This is binding via a security sanitization system, let it do
// this check. Apparently complex values are OK to assign here.
return undefined;
}

return [
{
kind: LitHtmlDiagnosticKind.COMPLEX_NOT_BINDABLE_IN_ATTRIBUTE_BINDING,
Expand Down
123 changes: 123 additions & 0 deletions packages/lit-analyzer/test/rules/security-system.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import test from "ava";
import { getDiagnostics } from "../helpers/analyze";
import { hasDiagnostic, hasNoDiagnostics } from "../helpers/assert";

const preface = `
class TrustedResourceUrl {};
class SafeUrl {};
class SafeStyle {};

const trustedResourceUrl = new TrustedResourceUrl();
const safeUrl = new SafeUrl();
const safeStyle = new SafeStyle();
`;

test("May bind string to script src with default config", t => {
const { diagnostics } = getDiagnostics('html`<script .src=${"/foo.js"}></script>`', {});
hasNoDiagnostics(t, diagnostics);
});

test("May not bind string to script src with ClosureSafeTypes config", t => {
const { diagnostics } = getDiagnostics('html`<script src=${"/foo.js"}></script>`', { securitySystem: "ClosureSafeTypes" });
hasDiagnostic(t, diagnostics, "no-incompatible-type-binding");
});

test("May not bind string to script .src with ClosureSafeTypes config", t => {
const { diagnostics } = getDiagnostics('html`<script .src=${"/foo.js"}></script>`', { securitySystem: "ClosureSafeTypes" });
hasDiagnostic(t, diagnostics, "no-incompatible-type-binding");
});

test("May pass static string to script src with ClosureSafeTypes config", t => {
const { diagnostics } = getDiagnostics('html`<script src="/foo.js"></script>`', { securitySystem: "ClosureSafeTypes" });
hasNoDiagnostics(t, diagnostics);
});

let testName = "May not pass a TrustedResourceUrl to script src with default config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<script src=${trustedResourceUrl}></script>`");
hasDiagnostic(t, diagnostics, "no-complex-attribute-binding");
});

// Commented out because we're missing properties on built in elements like
// script.
testName = "May not pass a TrustedResourceUrl to script .src with default config";
test.skip(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<script .src=${trustedResourceUrl}></script>`");
hasDiagnostic(t, diagnostics, "no-complex-attribute-binding");
});

testName = "May pass a TrustedResourceUrl to script src with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<script src=${trustedResourceUrl}></script>`", { securitySystem: "ClosureSafeTypes" });
hasNoDiagnostics(t, diagnostics);
});

testName = "May pass a TrustedResourceUrl to script .src with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<script .src=${trustedResourceUrl}></script>`", { securitySystem: "ClosureSafeTypes" });
hasNoDiagnostics(t, diagnostics);
});

testName = "May not pass a SafeUrl to script src with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<script src=${safeUrl}></script>`", { securitySystem: "ClosureSafeTypes" });
hasDiagnostic(t, diagnostics, "no-incompatible-type-binding");
});

testName = "May not pass a SafeUrl to script .src with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<script .src=${safeUrl}></script>`", { securitySystem: "ClosureSafeTypes" });
hasDiagnostic(t, diagnostics, "no-incompatible-type-binding");
});

testName = "May pass either a SafeUrl or a TrustedResourceUrl to img src with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<img src=${safeUrl}></script>`", { securitySystem: "ClosureSafeTypes" });
hasNoDiagnostics(t, diagnostics);

const { diagnostics: moreDiagnostics } = getDiagnostics(preface + "html`<img src=${trustedResourceUrl}></script>`", {
securitySystem: "ClosureSafeTypes"
});
hasNoDiagnostics(t, moreDiagnostics);
});

testName = "May pass either a SafeUrl or a TrustedResourceUrl to img .src with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<img .src=${safeUrl}></script>`", { securitySystem: "ClosureSafeTypes" });
hasNoDiagnostics(t, diagnostics);

const { diagnostics: moreDiagnostics } = getDiagnostics(preface + "html`<img .src=${trustedResourceUrl}></script>`", {
securitySystem: "ClosureSafeTypes"
});
hasNoDiagnostics(t, moreDiagnostics);
});

testName = "May not pass a string to img src with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + 'html`<img src=${"/foo.webp"}></script>`', { securitySystem: "ClosureSafeTypes" });
hasDiagnostic(t, diagnostics, "no-incompatible-type-binding");
});

testName = "May not pass a string to style with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + 'html`<div style=${"color: red"}></div>`', { securitySystem: "ClosureSafeTypes" });
hasDiagnostic(t, diagnostics, "no-incompatible-type-binding");
});

testName = "May not pass a string to .style with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + 'html`<div .style=${"color: red"}></div>`', { securitySystem: "ClosureSafeTypes" });
hasDiagnostic(t, diagnostics, "no-incompatible-type-binding");
});

testName = "May pass a SafeStyle to style with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<div style=${safeStyle}></div>`", { securitySystem: "ClosureSafeTypes" });
hasNoDiagnostics(t, diagnostics);
});

testName = "May pass a SafeStyle to .style with ClosureSafeTypes config";
test(testName, t => {
const { diagnostics } = getDiagnostics(preface + "html`<div .style=${safeStyle}></div>`", { securitySystem: "ClosureSafeTypes" });
hasNoDiagnostics(t, diagnostics);
});
9 changes: 9 additions & 0 deletions packages/vscode-lit-plugin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,15 @@
"description": "Enable strict mode.",
"default": false
},
"lit-plugin.securitySystem": {
"type": "string",
"description": "The lit-html security sanitization strategy to assume.",
"default": "off",
"enum": [
"off",
"ClosureSafeTypes"
]
},
"lit-plugin.htmlTemplateTags": {
"type": "array",
"description": "List of template tags to enable html support in.",
Expand Down
6 changes: 6 additions & 0 deletions packages/vscode-lit-plugin/schemas/tsconfig.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@
"description": "Enable strict mode.",
"default": false
},
"securitySystem": {
"type": "string",
"description": "The lit-html security sanitization strategy to assume.",
"default": "off",
"enum": ["off", "ClosureSafeTypes"]
},
"htmlTemplateTags": {
"type": "array",
"description": "List of template tags to enable html support in.",
Expand Down
3 changes: 3 additions & 0 deletions packages/vscode-lit-plugin/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ function getConfig(): Partial<LitAnalyzerConfig> {
withConfigValue(config, "strict", value => {
outConfig.strict = value;
});
withConfigValue(config, "securitySystem", value => {
outConfig.securitySystem = value;
});

// Template tags
withConfigValue(config, "htmlTemplateTags", value => {
Expand Down