-
Notifications
You must be signed in to change notification settings - Fork 41
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
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
b293463
Initial security type checking.
rictic 72499eb
Add `securitySystem` config info to vscode plugin.
rictic 0007b87
Security: Handle directives, expand some types.
rictic 3d517c5
Ensure that security option is correct in makeConfig.
rictic 31e67e0
Update tests for recent changes.
rictic 57b8c27
Give more context for the security diagnostic.
rictic 366f956
Clarify a couple comments.
rictic 853eff1
Reuse isLitDirective
rictic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
109 changes: 109 additions & 0 deletions
109
packages/lit-analyzer/src/analyze/util/type/is-assignable-binding-under-security-system.ts
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
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(" | ")}'.`, | ||
rictic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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; | ||
} | ||
} |
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
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
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
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
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); | ||
}); |
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
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
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
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, butts-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 👍