From 717b70706e1bea81efdae4cee718d1c5340ef8d6 Mon Sep 17 00:00:00 2001 From: Kevin Gilpin Date: Mon, 6 Dec 2021 17:32:53 -0500 Subject: [PATCH 1/7] feat: Update @appland/models for upgraded SQL parsing --- package.json | 2 +- yarn.lock | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index 73e381e..a3475fb 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "typescript": "^4.4.2" }, "dependencies": { - "@appland/models": "^1.7.1", + "@appland/models": "^1.8.0", "@types/sinon": "^10.0.2", "ajv": "^8.8.2", "ansi-escapes": "^5.0.0", diff --git a/yarn.lock b/yarn.lock index e27744c..d04232d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -47,14 +47,14 @@ __metadata: languageName: node linkType: hard -"@appland/models@npm:^1.7.1": - version: 1.7.1 - resolution: "@appland/models@npm:1.7.1" +"@appland/models@npm:^1.8.0": + version: 1.8.0 + resolution: "@appland/models@npm:1.8.0" dependencies: cross-env: ^7.0.3 crypto-js: ^4.0.0 - sqlite-parser: ^1.0.1 - checksum: 0b4b222762c3c5ccec956447cce8159a268575f26602998b1d7f70b3b302742113ed694373b471e0df7f78fe52bea17bca2624938bd1a886af2c5ebb7edd1604 + sqlite-parser: applandinc/sqlite-parser + checksum: ca462f8c396345fcc7445429af5c53aef8231929836d4ffb3bb65d2b11307821a5a00f211425a041a4d155e9b33fa83250a0a916a25982e420d4371938585415 languageName: node linkType: hard @@ -62,7 +62,7 @@ __metadata: version: 0.0.0-use.local resolution: "@applandinc/scanner@workspace:." dependencies: - "@appland/models": ^1.7.1 + "@appland/models": ^1.8.0 "@semantic-release/changelog": ^6.0.1 "@semantic-release/git": ^10.0.1 "@types/glob": ^7.1.4 @@ -7693,12 +7693,12 @@ __metadata: languageName: node linkType: hard -"sqlite-parser@npm:^1.0.1": - version: 1.0.1 - resolution: "sqlite-parser@npm:1.0.1" +sqlite-parser@applandinc/sqlite-parser: + version: 1.1.0 + resolution: "sqlite-parser@https://github.com/applandinc/sqlite-parser.git#commit=ecd7db3afc8ca32ebf6c13612665bd334330b809" bin: sqlite-parser: bin/sqlite-parser - checksum: cc4131f3347a1963252619d6bd39d44e7d7e0f7ba10619412ae0bb9c7aa28dccc6baad5c97f2fad3f2295088eea176ea39bc32c20946265c5ccde4a0aa390df7 + checksum: ee07464d79841214a854bd3f957269fc5b230ad64ff6bf6284970987d90debdcc8b2cb999512f73af46229a2ceed344f91166ffa2ed4b3e38cffddcfe5449da5 languageName: node linkType: hard From e4f83114370421c2bbe45e6ab30cd799bc66495d Mon Sep 17 00:00:00 2001 From: Kevin Gilpin Date: Tue, 7 Dec 2021 12:46:31 -0500 Subject: [PATCH 2/7] wip: Change object naming to Rules and Checks --- bin/schema | 2 +- src/analyzer/recordSecrets.ts | 2 +- src/assertion.ts | 92 -------- src/check.ts | 47 ++++ src/checkInstance.ts | 56 +++++ src/command.ts | 23 +- src/configuration/configurationProvider.ts | 96 +++----- src/configuration/index.ts | 4 +- src/configuration/schema.json | 72 ------ src/configuration/schema/configuration.json | 13 +- src/configuration/types/assertionConfig.ts | 20 -- src/configuration/types/checkConfig.ts | 20 ++ src/configuration/types/configuration.ts | 4 +- src/formatter/formatter.ts | 11 +- src/formatter/prettyFormatter.ts | 9 +- src/formatter/progressFormatter.ts | 9 +- src/openapi/index.ts | 2 +- src/report/generator.ts | 4 +- src/{assertionChecker.ts => ruleChecker.ts} | 46 ++-- src/rules/authzBeforeAuthn.ts | 51 +++++ src/rules/circularDependency.ts | 232 ++++++++++++++++++++ src/rules/http500.ts | 18 ++ src/rules/illegalPackageDependency.ts | 46 ++++ src/rules/lib/hasParameterOrReceiver.ts | 9 + src/rules/lib/matchEvent.ts | 34 +++ src/rules/lib/matchPattern.ts | 26 +++ src/rules/types.d.ts | 87 ++++++++ src/rules/util.ts | 119 ++++++++++ src/sampleConfig/default.yml | 34 +-- src/types.d.ts | 85 ++++--- test/util.ts | 2 +- 31 files changed, 900 insertions(+), 375 deletions(-) delete mode 100644 src/assertion.ts create mode 100644 src/check.ts create mode 100644 src/checkInstance.ts delete mode 100644 src/configuration/schema.json delete mode 100644 src/configuration/types/assertionConfig.ts create mode 100644 src/configuration/types/checkConfig.ts rename src/{assertionChecker.ts => ruleChecker.ts} (72%) create mode 100644 src/rules/authzBeforeAuthn.ts create mode 100644 src/rules/circularDependency.ts create mode 100644 src/rules/http500.ts create mode 100644 src/rules/illegalPackageDependency.ts create mode 100644 src/rules/lib/hasParameterOrReceiver.ts create mode 100644 src/rules/lib/matchEvent.ts create mode 100644 src/rules/lib/matchPattern.ts create mode 100644 src/rules/types.d.ts create mode 100644 src/rules/util.ts diff --git a/bin/schema b/bin/schema index 3f8f345..3328d20 100755 --- a/bin/schema +++ b/bin/schema @@ -3,7 +3,7 @@ set -e set -x npx ts-json-schema-generator -i https://appland.com/schemas/scanner/configuration.json --path src/configuration/types/configuration.ts > src/configuration/schema/configuration.json -npx ts-json-schema-generator -i https://appland.com/schemas/scanner/options.json --path src/scanner/types.d.ts > src/configuration/schema/options.json +npx ts-json-schema-generator -i https://appland.com/schemas/scanner/options.json --path src/rules/types.d.ts > src/configuration/schema/options.json fix_match_pattern_config() { cd src/configuration/schema diff --git a/src/analyzer/recordSecrets.ts b/src/analyzer/recordSecrets.ts index abe02d8..e83d4b8 100644 --- a/src/analyzer/recordSecrets.ts +++ b/src/analyzer/recordSecrets.ts @@ -1,5 +1,5 @@ import { Event } from '@appland/models'; -import { emptyValue, verbose } from '../scanner/util'; +import { emptyValue, verbose } from '../rules/util'; export default function (secrets: Set, e: Event): void { if (!e.returnValue) { diff --git a/src/assertion.ts b/src/assertion.ts deleted file mode 100644 index 982f486..0000000 --- a/src/assertion.ts +++ /dev/null @@ -1,92 +0,0 @@ -import { AppMap, Event } from '@appland/models'; -import { verbose } from './scanner/util'; -import { EventFilter, Matcher } from './types'; - -export default class Assertion { - public where?: EventFilter; - public includeScope: EventFilter[]; - public excludeScope: EventFilter[]; - public includeEvent: EventFilter[]; - public excludeEvent: EventFilter[]; - public description?: string; - public options?: any; - - static assert( - id: string, - summaryTitle: string, - matcher: Matcher, - cb?: (assertion: Assertion) => void - ): Assertion { - const assertion = new Assertion(id, summaryTitle, matcher); - if (cb) { - cb(assertion); - } - return assertion; - } - - constructor(public id: string, public summaryTitle: string, public matcher: Matcher) { - this.includeScope = []; - this.excludeScope = []; - this.includeEvent = []; - this.excludeEvent = []; - } - - filterScope(event: Event, appMap?: AppMap): boolean { - if (this.includeScope.length > 0 && !this.includeScope.every((fn) => fn(event, appMap))) { - if (verbose()) { - console.warn(`\t'includeScope' clause is not satisifed.`); - } - return false; - } - if (this.excludeScope.some((fn) => fn(event, appMap))) { - if (verbose()) { - console.warn(`\t'excludeScope' clause is not satisifed.`); - } - return false; - } - return true; - } - - filterEvent(event: Event, appMap?: AppMap): boolean { - if (this.where && !this.where(event, appMap)) { - if (verbose()) { - console.warn(`\t'where' clause is not satisifed.`); - } - return false; - } - - if (this.includeEvent.length > 0 && !this.includeEvent.every((fn) => fn(event, appMap))) { - if (verbose()) { - console.warn(`\t'includeEvent' clause is not satisifed.`); - } - return false; - } - if (this.excludeEvent.some((fn) => fn(event, appMap))) { - if (verbose()) { - console.warn(`\t'excludeEvent' clause is not satisifed.`); - } - return false; - } - return true; - } - - toString(): string { - const tokens = [`[${this.id}]`]; - if (this.description) { - tokens.push(this.description); - } else { - tokens.push(this.matcher.toString()); - } - if (this.where) { - tokens.push(`(where ${this.where})`); - } - // eslint-disable-next-line @typescript-eslint/no-this-alias - const self: any = this; - ['includeScope', 'excludeScope', 'includeEvent', 'excludeEvent'].forEach((key) => { - if (self[key].length > 0) { - tokens.push(`(${key} ${self[key].join(' && ')})`); - } - }); - return tokens.join(' '); - } -} diff --git a/src/check.ts b/src/check.ts new file mode 100644 index 0000000..8d3657f --- /dev/null +++ b/src/check.ts @@ -0,0 +1,47 @@ +import { AppMap, Event } from '@appland/models'; +import { verbose } from './rules/util'; +import { EventFilter, RuleLogic, Rule, ScopeName } from './types'; + +export default class Check { + public scope: ScopeName; + public includeScope: EventFilter[]; + public excludeScope: EventFilter[]; + public includeEvent: EventFilter[]; + public excludeEvent: EventFilter[]; + + constructor(public rule: Rule, public options: Record) { + this.scope = rule.scope || 'root'; + this.includeScope = []; + this.excludeScope = []; + this.includeEvent = []; + this.excludeEvent = []; + } + + filterScope(event: Event, appMap?: AppMap): boolean { + if (this.includeScope.length > 0 && !this.includeScope.every((fn) => fn(event, appMap))) { + if (verbose()) { + console.warn(`\t'includeScope' clause is not satisifed.`); + } + return false; + } + if (this.excludeScope.some((fn) => fn(event, appMap))) { + if (verbose()) { + console.warn(`\t'excludeScope' clause is not satisifed.`); + } + return false; + } + return true; + } + + toString(): string { + const tokens = [`[${this.rule.id}]`]; + // eslint-disable-next-line @typescript-eslint/no-this-alias + const self: any = this; + ['includeScope', 'excludeScope', 'includeEvent', 'excludeEvent'].forEach((key) => { + if (self[key].length > 0) { + tokens.push(`(${key} ${self[key].join(' && ')})`); + } + }); + return tokens.join(' '); + } +} diff --git a/src/checkInstance.ts b/src/checkInstance.ts new file mode 100644 index 0000000..4513011 --- /dev/null +++ b/src/checkInstance.ts @@ -0,0 +1,56 @@ +import { AppMap, Event } from '@appland/models'; +import Check from './check'; +import { verbose } from './rules/util'; +import { RuleLogic, ScopeName } from './types'; + +export default class CheckInstance { + check: Check; + ruleLogic: RuleLogic; + + constructor(check: Check) { + this.check = check; + this.ruleLogic = check.rule.build(check.options); + } + + get id(): string { + return this.check.rule.id; + } + + get title(): string { + return this.check.rule.title; + } + + get scope(): ScopeName { + return this.check.scope; + } + + get enumerateScope(): boolean { + return this.check.rule.enumerateScope || true; + } + + filterEvent(event: Event, appMap?: AppMap): boolean { + if (this.ruleLogic.where && !this.ruleLogic.where(event, appMap)) { + if (verbose()) { + console.warn(`\t'where' clause is not satisifed.`); + } + return false; + } + + if ( + this.check.includeEvent.length > 0 && + !this.check.includeEvent.every((fn) => fn(event, appMap)) + ) { + if (verbose()) { + console.warn(`\t'includeEvent' clause is not satisifed.`); + } + return false; + } + if (this.check.excludeEvent.some((fn) => fn(event, appMap))) { + if (verbose()) { + console.warn(`\t'excludeEvent' clause is not satisifed.`); + } + return false; + } + return true; + } +} diff --git a/src/command.ts b/src/command.ts index 8409c0b..87c1ea7 100644 --- a/src/command.ts +++ b/src/command.ts @@ -2,19 +2,19 @@ import { buildAppMap } from '@appland/models'; import { glob as globCallback } from 'glob'; import { promisify } from 'util'; import { promises as fs, constants as fsConstants, PathLike } from 'fs'; -import AssertionChecker from './assertionChecker'; import ProgressFormatter from './formatter/progressFormatter'; import PrettyFormatter from './formatter/prettyFormatter'; import { ValidationError, AbortError } from './errors'; -import { AssertionPrototype, Finding } from './types'; +import { Finding } from './types'; import { Argv, Arguments } from 'yargs'; import chalk from 'chalk'; import { loadConfiguration } from './configuration'; -import { verbose } from './scanner/util'; +import { verbose } from './rules/util'; import { join } from 'path'; import postCommitStatus from './integration/github/commitStatus'; import postPullRequestComment from './integration/github/postPullRequestComment'; import Generator, { ReportFormat } from './report/generator'; +import RuleChecker from './ruleChecker'; enum ExitCode { ValidationError = 1, @@ -140,10 +140,10 @@ export default { files = [appmapFile]; } - const checker = new AssertionChecker(); + const checker = new RuleChecker(); const formatter = progressFormat === 'progress' ? new ProgressFormatter() : new PrettyFormatter(); - const assertionPrototypes = await loadConfiguration(config); + const checks = await loadConfiguration(config); let index = 0; const findings: Finding[] = []; @@ -162,14 +162,14 @@ export default { process.stderr.write(formatter.appMap(appMap)); await Promise.all( - assertionPrototypes.map(async (assertionPrototype: AssertionPrototype) => { + checks.map(async (check) => { index++; const matchCount = findings.length; - await checker.check(appMap, assertionPrototype, findings); + await checker.check(appMap, check, findings); const newMatches = findings.slice(matchCount, findings.length); newMatches.forEach((match) => (match.appMapFile = file)); - const message = formatter.result(assertionPrototype, newMatches, index); + const message = formatter.result(check, newMatches, index); if (message) { process.stderr.write(message); } @@ -179,7 +179,7 @@ export default { ); const reportGenerator = new Generator(formatter, reportFormat, reportFile, ide); - const summary = reportGenerator.generate(findings, files.length * assertionPrototypes.length); + const summary = reportGenerator.generate(findings, files.length * checks.length); if (pullRequestComment && findings.length > 0) { try { @@ -191,10 +191,7 @@ export default { if (commitStatus) { return findings.length === 0 - ? await postCommitStatus( - 'success', - `${files.length * assertionPrototypes.length} checks passed` - ) + ? await postCommitStatus('success', `${files.length * checks.length} checks passed`) : await postCommitStatus('failure', `${findings.length} findings`); } diff --git a/src/configuration/configurationProvider.ts b/src/configuration/configurationProvider.ts index e8777e5..f444c1d 100644 --- a/src/configuration/configurationProvider.ts +++ b/src/configuration/configurationProvider.ts @@ -2,52 +2,25 @@ import { ValidateFunction } from 'ajv'; import Ajv from 'ajv'; import yaml from 'js-yaml'; import { promises as fs } from 'fs'; -import Assertion from '../assertion'; -import { AssertionPrototype, AssertionSpec, EventFilter } from '../types'; +import { Rule, ScopeName } from '../types'; import options_schema from './schema/options.json'; import match_pattern_config_schema from './schema/match-pattern-config.json'; import configuration_schema from './schema/configuration.json'; -import { capitalize, verbose } from '../scanner/util'; -import { buildFilters as buildEventFilterArray } from '../scanner/lib/matchEvent'; +import { capitalize, verbose } from '../rules/util'; +import { buildFilters as buildEventFilterArray } from '../rules/lib/matchEvent'; import Configuration from './types/configuration'; -import AssertionConfig from './types/assertionConfig'; -import MatchEventConfig from './types/matchEventConfig'; +import CheckConfig from './types/checkConfig'; +import Check from '../check'; const ajv = new Ajv(); ajv.addSchema(match_pattern_config_schema); -function makeFilter(matchConfigs: MatchEventConfig[]): EventFilter[] { - if (!matchConfigs) { - return []; - } - - return buildEventFilterArray(matchConfigs!); -} - -function populateAssertion(assertion: Assertion, config: AssertionConfig): void { - assertion.includeScope = makeFilter( - (config.include || []).filter((item) => item.scope).map((item) => item.scope!) - ); - assertion.excludeScope = makeFilter( - (config.exclude || []).filter((item) => item.scope).map((item) => item.scope!) - ); - assertion.includeEvent = makeFilter( - (config.include || []).filter((item) => item.event).map((item) => item.event!) - ); - assertion.excludeEvent = makeFilter( - (config.exclude || []).filter((item) => item.event).map((item) => item.event!) - ); - if (config.description) { - assertion.description = config.description; - } -} - -async function buildBuiltinAssertion(config: AssertionConfig): Promise { - const assertionSpec: AssertionSpec = (await import(`../scanner/${config.id}`)).default; +async function buildBuiltinCheck(config: CheckConfig): Promise { + const rule: Rule = (await import(`../rules/${config.id}`)).default; let options: any; - if (assertionSpec.Options) { - options = new assertionSpec.Options(); + if (rule.Options) { + options = new rule.Options(); } else { options = {}; } @@ -58,16 +31,25 @@ async function buildBuiltinAssertion(config: AssertionConfig): Promise { - const result = assertionSpec.scanner(options); - populateAssertion(result, config); - return result; - }, - }; + const check = new Check(rule, options); + if (config.scope) { + check.scope = config.scope as ScopeName; + } + + check.includeScope = buildEventFilterArray( + (config.include || []).filter((item) => item.scope).map((item) => item.scope!) + ); + check.excludeScope = buildEventFilterArray( + (config.exclude || []).filter((item) => item.scope).map((item) => item.scope!) + ); + check.includeEvent = buildEventFilterArray( + (config.include || []).filter((item) => item.event).map((item) => item.event!) + ); + check.excludeEvent = buildEventFilterArray( + (config.exclude || []).filter((item) => item.event).map((item) => item.event!) + ); + + return check; } const validate = (validator: ValidateFunction, data: any, context: string): void => { @@ -90,7 +72,7 @@ const validate = (validator: ValidateFunction, data: any, context: string): void export default class ConfigurationProvider { constructor(private readonly config: string | Configuration) {} - async getConfig(): Promise { + async getConfig(): Promise { let rawConfig: Configuration | undefined; if (typeof this.config === 'string') { const yamlConfig = await fs.readFile(this.config, 'utf-8'); @@ -103,10 +85,10 @@ export default class ConfigurationProvider { validate(ajv.compile(configuration_schema), rawConfig, 'Scanner configuration'); - rawConfig.scanners - .filter((scanner) => scanner.properties) - .forEach((scanner) => { - const id = scanner.id; + rawConfig.checks + .filter((check) => check.properties) + .forEach((check) => { + const id = check.id; const schemaKey = [capitalize(id), 'Options'].join('.'); if (verbose()) { console.warn(schemaKey); @@ -117,17 +99,11 @@ export default class ConfigurationProvider { } if (verbose()) { console.warn(propertiesSchema); - console.warn(scanner.properties); + console.warn(check.properties); } - validate( - ajv.compile(propertiesSchema), - scanner.properties || {}, - `${scanner.id} properties` - ); + validate(ajv.compile(propertiesSchema), check.properties || {}, `${check.id} properties`); }); - return Promise.all( - rawConfig.scanners.map(async (c: AssertionConfig) => buildBuiltinAssertion(c)) - ); + return Promise.all(rawConfig.checks.map(async (c: CheckConfig) => buildBuiltinCheck(c))); } } diff --git a/src/configuration/index.ts b/src/configuration/index.ts index e10301c..fb24e13 100644 --- a/src/configuration/index.ts +++ b/src/configuration/index.ts @@ -1,7 +1,7 @@ import ConfigurationProvider from './configurationProvider'; -import { AssertionPrototype } from 'src/types'; +import Check from 'src/check'; -export async function loadConfiguration(path: string): Promise { +export async function loadConfiguration(path: string): Promise { const provider = new ConfigurationProvider(path); return await provider.getConfig(); } diff --git a/src/configuration/schema.json b/src/configuration/schema.json deleted file mode 100644 index 2cd80a6..0000000 --- a/src/configuration/schema.json +++ /dev/null @@ -1,72 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "$id": "https://app.land/scanner-config.json", - "title": "Scanners", - "type": "object", - "properties": { - "scanners": { - "type": "array", - "items": { - "type": "object", - "required": ["id"], - "additionalProperties": false, - "properties": { - "id": { - "type": "string" - }, - "include": { - "type": "array", - "items": { - "type": "object", - "additionalProperties": false, - "oneOf": [ - { - "required": ["regexp"] - }, - { - "required": ["function"] - } - ], - "properties": { - "regexp": { - "type": "string" - }, - "function": { - "type": "string" - } - } - } - }, - "exclude": { - "type": "array", - "items": { - "type": "object", - "additionalProperties": false, - "oneOf": [ - { - "required": ["regexp"] - }, - { - "required": ["function"] - } - ], - "properties": { - "regexp": { - "type": "string" - }, - "function": { - "type": "string" - } - } - } - }, - "properties": { - "type": "object" - } - } - } - } - }, - "additionalProperties": false, - "required": ["scanners"] -} diff --git a/src/configuration/schema/configuration.json b/src/configuration/schema/configuration.json index 118cb84..240dd3c 100644 --- a/src/configuration/schema/configuration.json +++ b/src/configuration/schema/configuration.json @@ -3,9 +3,9 @@ "$ref": "#/definitions/Configuration", "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { - "AssertionConfig": { + "CheckConfig": { "additionalProperties": false, - "description": "AssertionConfig represents the user's configuration of an Assertion, as read from the configuration file.", + "description": "CheckConfig represents the user's configuration of an Check, which is an instantiation of a Rule. Each CheckConfing is read from the scanners configuration file.", "properties": { "description": { "type": "string" @@ -45,6 +45,9 @@ }, "properties": { "type": "object" + }, + "scope": { + "type": "string" } }, "required": [ @@ -56,15 +59,15 @@ "additionalProperties": false, "description": "Configuration is the code representation of the scanner configuration file.", "properties": { - "scanners": { + "checks": { "items": { - "$ref": "#/definitions/AssertionConfig" + "$ref": "#/definitions/CheckConfig" }, "type": "array" } }, "required": [ - "scanners" + "checks" ], "type": "object" }, diff --git a/src/configuration/types/assertionConfig.ts b/src/configuration/types/assertionConfig.ts deleted file mode 100644 index b5a9ae8..0000000 --- a/src/configuration/types/assertionConfig.ts +++ /dev/null @@ -1,20 +0,0 @@ -import MatchEventConfig from './matchEventConfig'; - -interface MatchConfig { - scope?: MatchEventConfig; - event?: MatchEventConfig; -} - -/** - * AssertionConfig represents the user's configuration of an Assertion, as read from the - * configuration file. - */ -export default interface AssertionConfig { - // id is expected to match the file name of the scanner in src/scanner. - id: string; - include?: MatchConfig[]; - exclude?: MatchConfig[]; - description?: string; - // Properties are mapped to Assertion Options. - properties?: Record; -} diff --git a/src/configuration/types/checkConfig.ts b/src/configuration/types/checkConfig.ts new file mode 100644 index 0000000..2b3332b --- /dev/null +++ b/src/configuration/types/checkConfig.ts @@ -0,0 +1,20 @@ +import MatchEventConfig from './matchEventConfig'; + +interface MatchConfig { + scope?: MatchEventConfig; + event?: MatchEventConfig; +} + +/** + * CheckConfig represents the user's configuration of an Check, which is an + * instantiation of a Rule. Each CheckConfing is read from the scanners configuration file. + */ +export default interface CheckConfig { + // id is expected to match the file name of the rule in src/rules. + id: string; + scope?: string; + include?: MatchConfig[]; + exclude?: MatchConfig[]; + // Properties are mapped to rule Options. + properties?: Record; +} diff --git a/src/configuration/types/configuration.ts b/src/configuration/types/configuration.ts index 6149437..f391e65 100644 --- a/src/configuration/types/configuration.ts +++ b/src/configuration/types/configuration.ts @@ -1,8 +1,8 @@ -import AssertionConfig from './assertionConfig'; +import CheckConfig from './checkConfig'; /** * Configuration is the code representation of the scanner configuration file. */ export default interface Configuration { - scanners: AssertionConfig[]; + checks: CheckConfig[]; } diff --git a/src/formatter/formatter.ts b/src/formatter/formatter.ts index cc4ca69..21fafa3 100644 --- a/src/formatter/formatter.ts +++ b/src/formatter/formatter.ts @@ -1,17 +1,14 @@ import { AppMap } from '@appland/models'; import chalk from 'chalk'; -import { ScannerSummary } from 'src/report/scannerSummary'; -import { AssertionPrototype, Finding } from '../types'; +import Check from '../check'; +import { ScannerSummary } from '../report/scannerSummary'; +import { Finding } from '../types'; export default abstract class Formatter { private noColors = false; abstract appMap(appMap: AppMap): string; - abstract result( - assertionPrototype: AssertionPrototype, - findings: Finding[], - index: number - ): string | undefined; + abstract result(check: Check, findings: Finding[], index: number): string | undefined; summary(summary: ScannerSummary): string { const matchedStr = `${summary.findingTotal} finding${summary.findingTotal === 1 ? '' : 's'}`; diff --git a/src/formatter/prettyFormatter.ts b/src/formatter/prettyFormatter.ts index 7c8c11c..60b5d14 100644 --- a/src/formatter/prettyFormatter.ts +++ b/src/formatter/prettyFormatter.ts @@ -1,17 +1,16 @@ import Formatter from './formatter'; import chalk from 'chalk'; import { AppMap } from '@appland/models'; -import { AssertionPrototype, Finding } from '../types'; +import { Finding } from '../types'; +import Check from 'src/check'; export default class PrettyFormatter extends Formatter { appMap(appMap: AppMap): string { return '\n' + appMap.metadata.name + '\n'; } - result(assertionPrototype: AssertionPrototype, matches: Finding[]): string | undefined { - const readableAssertion = [assertionPrototype.config.id, assertionPrototype.config.description] - .filter((d) => d) - .join(' '); + result(check: Check, matches: Finding[]): string | undefined { + const readableAssertion = [check.rule.id, check.rule.title].filter((d) => d).join(' '); if (matches.length === 0) { return '\t' + chalk.stderr.green(readableAssertion) + '\n'; diff --git a/src/formatter/progressFormatter.ts b/src/formatter/progressFormatter.ts index d7d779f..fe09ce7 100644 --- a/src/formatter/progressFormatter.ts +++ b/src/formatter/progressFormatter.ts @@ -1,17 +1,14 @@ import Formatter from './formatter'; import chalk from 'chalk'; -import { AssertionPrototype, Finding } from '../types'; +import { Finding } from '../types'; +import Check from 'src/check'; export default class ProgressFormatter extends Formatter { appMap(): string { return ''; } - result( - _assertionPrototype: AssertionPrototype, - matches: Finding[], - index: number - ): string | undefined { + result(_check: Check, matches: Finding[], index: number): string | undefined { const ending = index % 80 === 0 ? '\n' : ''; if (matches.length === 0) { diff --git a/src/openapi/index.ts b/src/openapi/index.ts index 3ec99b5..051a52c 100644 --- a/src/openapi/index.ts +++ b/src/openapi/index.ts @@ -6,7 +6,7 @@ import { rpcRequestForEvent } from './rpcRequest'; import { Event } from '@appland/models'; import { OpenAPIV3 } from 'openapi-types'; import { writeFileSync } from 'fs'; -import { verbose } from '../scanner/util'; +import { verbose } from '../rules/util'; import { URL } from 'url'; interface OpenAPIV3Fragment { diff --git a/src/report/generator.ts b/src/report/generator.ts index 804879b..d8de60b 100644 --- a/src/report/generator.ts +++ b/src/report/generator.ts @@ -1,6 +1,6 @@ import * as fs from 'fs'; import chalk from 'chalk'; -import { ideLink } from '../scanner/util'; +import { ideLink } from '../rules/util'; import { Finding } from '../types'; import Formatter from '../formatter/formatter'; import { ScannerSummary } from './scannerSummary'; @@ -59,7 +59,7 @@ export default class Generator { } if (this.reportFormat === 'text') { - const message = match.message || match.condition; + const message = match.message; this.writeln(this.reportFile ? message : chalk.magenta(message)); this.writeln(`\tLink:\t${this.reportFile ? filePath : chalk.blue(filePath)}`); this.writeln(`\tRule:\t${match.scannerId}`); diff --git a/src/assertionChecker.ts b/src/ruleChecker.ts similarity index 72% rename from src/assertionChecker.ts rename to src/ruleChecker.ts index 740c764..18c94db 100644 --- a/src/assertionChecker.ts +++ b/src/ruleChecker.ts @@ -1,16 +1,17 @@ import { AppMap, Event } from '@appland/models'; -import Assertion from './assertion'; +import Check from './check'; import { AbortError } from './errors'; -import { AssertionPrototype, Finding } from './types'; -import { verbose } from './scanner/util'; +import { Finding } from './types'; +import { verbose } from './rules/util'; import ScopeIterator from './scope/scopeIterator'; import RootScope from './scope/rootScope'; import HTTPServerRequestScope from './scope/httpServerRequestScope'; import HTTPClientRequestScope from './scope/httpClientRequestScope'; import CommandScope from './scope/commandScope'; import SQLTransactionScope from './scope/sqlTransactionScope'; +import CheckInstance from './checkInstance'; -export default class AssertionChecker { +export default class RuleChecker { private scopes: Record = { root: new RootScope(), command: new CommandScope(), @@ -19,17 +20,13 @@ export default class AssertionChecker { transaction: new SQLTransactionScope(), }; - async check( - appMap: AppMap, - assertionPrototype: AssertionPrototype, - matches: Finding[] - ): Promise { + async check(appMap: AppMap, check: Check, matches: Finding[]): Promise { if (verbose()) { - console.warn(`Checking AppMap ${appMap.name} with scope ${assertionPrototype.scope}`); + console.warn(`Checking AppMap ${appMap.name} with scope ${check.scope}`); } - const scopeIterator = this.scopes[assertionPrototype.scope]; + const scopeIterator = this.scopes[check.scope]; if (!scopeIterator) { - throw new AbortError(`Invalid scope name "${assertionPrototype.scope}"`); + throw new AbortError(`Invalid scope name "${check.scope}"`); } const callEvents = function* (): Generator { @@ -42,16 +39,16 @@ export default class AssertionChecker { if (verbose()) { console.warn(`Scope ${scope.scope}`); } - const assertion = assertionPrototype.build(); - if (!assertion.filterScope(scope.scope, appMap)) { + const checkInstance = new CheckInstance(check); + if (!check.filterScope(scope.scope, appMap)) { continue; } - if (assertionPrototype.enumerateScope) { + if (checkInstance.enumerateScope) { for (const event of scope.events()) { - await this.checkEvent(event, scope.scope, appMap, assertion, matches); + await this.checkEvent(event, scope.scope, appMap, checkInstance, matches); } } else { - await this.checkEvent(scope.scope, scope.scope, appMap, assertion, matches); + await this.checkEvent(scope.scope, scope.scope, appMap, checkInstance, matches); } } } @@ -60,7 +57,7 @@ export default class AssertionChecker { event: Event, scope: Event, appMap: AppMap, - assertion: Assertion, + checkInstance: CheckInstance, findings: Finding[] ): Promise { if (!event.isCall()) { @@ -68,7 +65,7 @@ export default class AssertionChecker { } if (verbose()) { console.warn( - `Asserting ${assertion.id} on ${event.codeObject.fqid} event ${event.toString()}` + `Asserting ${checkInstance.id} on ${event.codeObject.fqid} event ${event.toString()}` ); } @@ -79,7 +76,7 @@ export default class AssertionChecker { return; } - if (!assertion.filterEvent(event, appMap)) { + if (!checkInstance.filterEvent(event, appMap)) { return; } @@ -93,8 +90,8 @@ export default class AssertionChecker { const findingEvent = matchEvent || event; return { appMapName: appMap.metadata.name, - scannerId: assertion.id, - scannerTitle: assertion.summaryTitle, + scannerId: checkInstance.id, + scannerTitle: checkInstance.title, event: findingEvent, hash: findingEvent.hash, scope, @@ -102,11 +99,10 @@ export default class AssertionChecker { groupMessage, occurranceCount, relatedEvents, - condition: assertion.description || assertion.matcher.toString(), }; }; - const matchResult = await assertion.matcher(event, appMap, assertion); + const matchResult = await checkInstance.ruleLogic.matcher(event, appMap, checkInstance.check); const numFindings = findings.length; if (matchResult === true) { findings.push(buildFinding(event)); @@ -129,7 +125,7 @@ export default class AssertionChecker { if (verbose()) { if (findings.length > numFindings) { findings.forEach((finding) => - console.log(`\tFinding: ${finding.scannerId} : ${finding.message || finding.condition}`) + console.log(`\tFinding: ${finding.scannerId} : ${finding.message}`) ); } } diff --git a/src/rules/authzBeforeAuthn.ts b/src/rules/authzBeforeAuthn.ts new file mode 100644 index 0000000..0286296 --- /dev/null +++ b/src/rules/authzBeforeAuthn.ts @@ -0,0 +1,51 @@ +import { Event, EventNavigator } from '@appland/models'; +import { isTruthy, providesAuthentication } from './util'; +import { MatcherResult, Rule, RuleLogic, ScopeName } from '../types.d'; + +function containsAuthentication(events: Generator) { + for (const iter of events) { + if (providesAuthentication(iter.event, SecurityAuthentication)) { + return true; + } + } + return false; +} + +function build(): RuleLogic { + function matcher(rootEvent: Event): MatcherResult { + for (const event of new EventNavigator(rootEvent).descendants()) { + if (providesAuthentication(event.event, SecurityAuthentication)) { + return; + } + if (event.event.labels.has(SecurityAuthorization) && isTruthy(event.event.returnValue)) { + // If the authorization event has a successful authentication descendant, allow this as well. + if (containsAuthentication(event.descendants())) { + return; + } else { + return [ + { + level: 'error', + event: rootEvent, + message: `${event.event} provides authorization, but the request is not authenticated`, + relatedEvents: [event.event], + }, + ]; + } + } + } + } + + return { matcher }; +} + +const SecurityAuthentication = 'security.authentication'; +const SecurityAuthorization = 'security.authorization'; + +export default { + id: 'authz-before-authn', + title: 'Authorization performed before authentication', + labels: [SecurityAuthorization, SecurityAuthentication], + scope: 'http_server_request' as ScopeName, + enumerateScope: false, + build, +} as Rule; diff --git a/src/rules/circularDependency.ts b/src/rules/circularDependency.ts new file mode 100644 index 0000000..40a1efc --- /dev/null +++ b/src/rules/circularDependency.ts @@ -0,0 +1,232 @@ +import { Event } from '@appland/models'; +import { MatchResult, Rule, RuleLogic, StringFilter } from '../types'; +import GraphEdge from '../algorithms/dataStructures/graph/GraphEdge'; +import GraphVertex from '../algorithms/dataStructures/graph/GraphVertex'; +import Graph from '../algorithms/dataStructures/graph/Graph'; +import detectDirectedCycle from '../algorithms/graph/detect-cycle'; +import { isAbsolute } from 'path'; +import * as types from './types'; +import { verbose } from './util'; +import MatchPatternConfig from '../configuration/types/matchPatternConfig'; +import { buildFilters } from './lib/matchPattern'; + +type PackageName = string; + +class Cycle { + constructor(public packages: PackageName[], public events: Map) {} +} + +function ignorePackage(event: Event, ignoredPackages: StringFilter[]): boolean { + const myPackage: string | null = event.codeObject.packageOf; + return ( + myPackage === '' || + ignoredPackages.some((filter) => filter(myPackage)) || + !event.codeObject.location || + isAbsolute(event.codeObject.location) + ); +} + +function detectCycles(root: Event, ignoredPackages: StringFilter[]): Cycle[] { + const graph = new Graph(true); + const vertices = new Map(); + const edges = new Set(); + const vertexEvents = new Map(); + + const makeVertex = (pkg: PackageName, event: Event): GraphVertex => { + let result = vertices.get(pkg); + if (!result) { + result = new GraphVertex(pkg); + vertices.set(pkg, result); + vertexEvents.set(pkg, [event]); + } else { + vertexEvents.get(pkg)!.push(event); + } + return result; + }; + + const collectEvent = ( + event: Event, + parentEvent: Event | null, + parentPackage: PackageName | null + ) => { + let myPackage: PackageName | null = event.codeObject.packageOf; + if (ignorePackage(event, ignoredPackages)) { + myPackage = null; + } + + if (myPackage) { + const vertex = makeVertex(myPackage, event); + if (parentPackage && parentPackage !== myPackage) { + const edge = new GraphEdge(vertices.get(parentPackage)!, vertex); + if (!edges.has(edge.getKey())) { + if (verbose()) { + console.warn(`New edge: ${parentPackage}/${parentEvent} -> ${myPackage}/${event}`); + } + edges.add(edge.getKey()); + graph.addEdge(edge); + } + } + parentPackage = myPackage; + } + event.children.forEach((child) => collectEvent(child, event, parentPackage)); + }; + + if (root.codeObject.packageOf !== '') { + makeVertex(root.codeObject.packageOf, root); + } + collectEvent(root, null, null); + + return detectDirectedCycle(graph).map((cycle) => { + return new Cycle( + cycle.map((vertex) => vertex.getKey()), + vertexEvents + ); + }); +} + +/** + * Given a list of package names which occur in a cycle, + * search the event tree to find a list of specific events whose sequence and package names match the cycle. + + * @returns Sequence of events whose package names match the cyclePath. + */ +const searchForCycle = (cycle: Cycle, ignoredPackages: StringFilter[]): Event[] | null => { + const traverseEvent = ( + event: Event, + recordEvent: boolean, + cyclePath: PackageName[], + cyclePathIndex = 0, + path: Event[] = [] + ): Event[] | null => { + if (recordEvent) { + if (verbose()) { + console.warn(`${Array(path.length).fill('').join(' ')}push: ${event}`); + } + path.push(event); + } else { + if (verbose()) { + console.warn(`${Array(path.length).fill('').join(' ')}traverse: ${event}`); + } + } + + if (cyclePathIndex === cyclePath.length - 1) { + if (verbose()) { + console.warn(`${Array(path.length).fill('').join(' ')}result: ${path}`); + } + return [...path]; + } + + const myPackage = event.codeObject.packageOf; + + if (verbose()) { + console.warn(event.children.map((child) => child.codeObject.fqid)); + } + + // Traverse children of ignored or same package + let result = event.children + .filter( + (child) => child.codeObject.packageOf === myPackage || ignorePackage(child, ignoredPackages) + ) + .map((child) => traverseEvent(child, false, cyclePath, cyclePathIndex, path)) + .filter(Boolean); + + // Traverse children of the next package in the graph + if (result.length === 0) { + result = event.children + .filter( + (child) => + child.codeObject.packageOf !== myPackage && + !ignorePackage(child, ignoredPackages) && + cyclePath[cyclePathIndex + 1] === child.codeObject.packageOf + ) + .map((child) => traverseEvent(child, true, cyclePath, cyclePathIndex + 1, path)) + .filter((path) => path); + } + + if (result.length > 0) { + return result[0]; + } else { + if (recordEvent) { + if (verbose()) { + console.warn( + `${Array(path.length - 1) + .fill('') + .join(' ')}pop` + ); + } + path.pop(); + } else { + if (verbose()) { + console.warn( + `${Array(path.length - 1) + .fill('') + .join(' ')}untraverse` + ); + } + } + return null; + } + }; + + // Look for a cycle starting at each package name. For each package name, consider the + // events that have that package. + for (let i = 0; i < cycle.packages.length; i++) { + const packageName = cycle.packages[i]; + const startEvents = cycle.events.get(packageName)!; + const cyclePath = []; + for (let k = 0; k < cycle.packages.length; k++) { + cyclePath[k] = cycle.packages[(i + k) % cycle.packages.length]; + } + cyclePath.push(packageName); + if (verbose()) { + console.warn(`Searching for event path for cycle ${cyclePath}`); + } + for (let j = 0; j < startEvents.length; j++) { + const startEvent = startEvents[j]; + const path = traverseEvent(startEvent, true, cyclePath); + if (path) { + return path; + } + } + } + return null; +}; + +class Options implements types.CircularDependency.Options { + public ignoredPackages: MatchPatternConfig[] = []; + public depth = 4; +} + +function build(options: Options): RuleLogic { + const ignoredPackages = buildFilters(options.ignoredPackages); + + function matcher(event: Event): MatchResult[] { + return detectCycles(event, ignoredPackages) + .filter((cycle) => cycle.packages.length + 1 >= options.depth) + .map((cycle) => searchForCycle(cycle, ignoredPackages)) + .filter((path) => path) + .map((path) => { + return { + event: path![0], + message: [ + 'Cycle in package dependency graph', + path!.map((event) => event.codeObject.packageOf).join(' -> '), + ].join(': '), + relatedEvents: path!, + } as MatchResult; + }); + } + + return { + matcher, + }; +} + +export default { + id: 'circular-dependency', + title: 'Circular package dependency', + scope: 'command', + Options, + enumerateScope: false, + build, +} as Rule; diff --git a/src/rules/http500.ts b/src/rules/http500.ts new file mode 100644 index 0000000..864de9e --- /dev/null +++ b/src/rules/http500.ts @@ -0,0 +1,18 @@ +import { Event } from '@appland/models'; +import { Rule, RuleLogic } from '../types'; + +function build(): RuleLogic { + return { + matcher: (e: Event) => + e.httpServerResponse!.status >= 500 && e.httpServerResponse!.status < 600, + where: (e: Event) => !!e.httpServerResponse, + }; +} + +export default { + id: 'http-5xx', + title: 'HTTP 5xx status code', + scope: 'http_server_request', + enumerateScope: false, + build, +} as Rule; diff --git a/src/rules/illegalPackageDependency.ts b/src/rules/illegalPackageDependency.ts new file mode 100644 index 0000000..ddcb302 --- /dev/null +++ b/src/rules/illegalPackageDependency.ts @@ -0,0 +1,46 @@ +import { Event } from '@appland/models'; +import types from './types'; +import { MatcherResult, Rule, RuleLogic, ScopeName } from '../types'; +import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; +import { buildFilter, buildFilters } from './lib/matchPattern'; + +class Options implements types.IllegalPackageDependency.Options { + public callerPackages: MatchPatternConfig[] = []; + public calleePackage: MatchPatternConfig = {} as MatchPatternConfig; +} + +function build(options: Options): RuleLogic { + const callerPatterns = buildFilters(options.callerPackages || []); + const calleePattern = buildFilter(options.calleePackage); + + function where(e: Event): boolean { + return !!e.parent && !!e.parent!.codeObject.packageOf && calleePattern(e.codeObject.packageOf); + } + + function matcher(e: Event): MatcherResult { + const packageNamesStr = options.callerPackages + .map((config) => config.equal || config.include || config.match) + .map(String) + .join(' or '); + + const parentPackage = e.parent!.codeObject.packageOf; + if ( + !( + e.codeObject.packageOf === parentPackage || + callerPatterns.some((pattern) => pattern(parentPackage)) + ) + ) { + return `Code object ${e.codeObject.id} was invoked from ${parentPackage}, not from ${packageNamesStr}`; + } + } + + return { where, matcher }; +} + +export default { + id: 'illegal-package-dependency', + title: 'Illegal use of code by a non-whitelisted package', + scope: 'command' as ScopeName, + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/lib/hasParameterOrReceiver.ts b/src/rules/lib/hasParameterOrReceiver.ts new file mode 100644 index 0000000..be3b4fd --- /dev/null +++ b/src/rules/lib/hasParameterOrReceiver.ts @@ -0,0 +1,9 @@ +import { Event } from '@appland/models'; + +// Builds a function that returns true if the provided event argument has the specified +// objectId as the receiver or as a parameter value. +export default (objectId: number) => { + return (event: Event): boolean => + (!!event.receiver && event.receiver!.object_id === objectId) || + (!!event.parameters && event.parameters!.some((param) => param.object_id === objectId)); +}; diff --git a/src/rules/lib/matchEvent.ts b/src/rules/lib/matchEvent.ts new file mode 100644 index 0000000..83e004d --- /dev/null +++ b/src/rules/lib/matchEvent.ts @@ -0,0 +1,34 @@ +import { Event } from '@appland/models'; +import { sqlNormalized } from '../../database'; +import MatchEventConfig from '../../configuration/types/matchEventConfig'; +import { EventFilter } from '../../types'; +import { buildFilter as buildMatchPattern } from './matchPattern'; + +export function buildFilter(pattern: MatchEventConfig): EventFilter { + const testFn = buildMatchPattern(pattern.test); + + const propertyFn = { + id: (e: Event) => e.codeObject.id, + type: (e: Event) => e.codeObject.type, + fqid: (e: Event) => e.codeObject.fqid, + query: (e: Event) => (e.sql ? sqlNormalized(e.sql) : null), + route: (e: Event) => e.route, + }; + + return (event: Event): boolean => { + const fn = propertyFn[pattern.property]; + if (!fn) { + throw new Error(`Unrecognized Event filter property: ${pattern.property}`); + } + const value = fn(event); + if (!value) { + return false; + } + + return testFn(value); + }; +} + +export function buildFilters(patterns: MatchEventConfig[]): EventFilter[] { + return patterns.map(buildFilter); +} diff --git a/src/rules/lib/matchPattern.ts b/src/rules/lib/matchPattern.ts new file mode 100644 index 0000000..77b83aa --- /dev/null +++ b/src/rules/lib/matchPattern.ts @@ -0,0 +1,26 @@ +import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; +import { StringFilter } from '../../types'; + +export function buildFilter(pattern: MatchPatternConfig): StringFilter { + function respectIgnoreCaseFlag(value: string): string { + return pattern.ignoreCase ? value.toLocaleLowerCase() : value; + } + + if (pattern.equal) { + const testStr = respectIgnoreCaseFlag(pattern.equal!); + return (value: string): boolean => respectIgnoreCaseFlag(value) === testStr; + } else if (pattern.include) { + const testStr = respectIgnoreCaseFlag(pattern.include!); + return (value: string): boolean => respectIgnoreCaseFlag(value).includes(testStr); + } else { + const regexp = + pattern.match instanceof RegExp + ? pattern.match + : new RegExp(pattern.match as unknown as string, pattern.ignoreCase ? 'i' : undefined); + return (value: string): boolean => regexp.test(value); + } +} + +export function buildFilters(patterns: MatchPatternConfig[]): StringFilter[] { + return patterns.map(buildFilter); +} diff --git a/src/rules/types.d.ts b/src/rules/types.d.ts new file mode 100644 index 0000000..5c658ee --- /dev/null +++ b/src/rules/types.d.ts @@ -0,0 +1,87 @@ +import MatchPatternConfig from '../configuration/types/matchPatternConfig'; + +interface TimeAllowed { + timeAllowed?: number; +} + +interface WarningLimit { + warningLimit?: number; +} + +export namespace IllegalPackageDependency { + export interface Options { + callerPackages: MatchPatternConfig[]; + calleePackage: MatchPatternConfig; + } +} + +export namespace CircularDependency { + export interface Options { + ignoredPackages?: MatchPatternConfig[]; + depth?: number; + } +} + +export namespace IncompatibleHttpClientRequest { + export interface Options { + schemata: Record; + } +} + +export namespace MissingAuthentication { + export interface Options { + includeContentTypes?: MatchPatternConfig[]; + excludeContentTypes?: MatchPatternConfig[]; + } +} + +export namespace NPlusOneQuery { + export interface Options extends WarningLimit { + errorLimit?: number; + } +} + +export namespace QueryFromInvalidPackage { + export interface Options { + allowedPackages: MatchPatternConfig[]; + allowedQueries?: MatchPatternConfig[]; + } +} + +export namespace QueryFromView { + export interface Options { + forbiddenLabel?: string; + } +} + +export namespace RPCWithoutCircuitBreaker { + export interface Options { + expectedLabel?: string; + } +} + +export namespace SlowFunctionCall { + export interface Options extends TimeAllowed { + functions?: MatchPatternConfig[]; + } +} + +export namespace SlowHTTPServerRequest { + // eslint-disable-next-line @typescript-eslint/no-empty-interface + export interface Options extends TimeAllowed {} +} + +export namespace SlowQuery { + // eslint-disable-next-line @typescript-eslint/no-empty-interface + export interface Options extends TimeAllowed {} +} + +export namespace TooManyJoins { + // eslint-disable-next-line @typescript-eslint/no-empty-interface + export interface Options extends WarningLimit {} +} + +export namespace TooManyUpdates { + // eslint-disable-next-line @typescript-eslint/no-empty-interface + export interface Options extends WarningLimit {} +} diff --git a/src/rules/util.ts b/src/rules/util.ts new file mode 100644 index 0000000..7e3561f --- /dev/null +++ b/src/rules/util.ts @@ -0,0 +1,119 @@ +import { Event } from '@appland/models'; +import { isAbsolute } from 'path'; + +let isVerbose = false; +function verbose(v: boolean | null = null): boolean { + if (v === true || v === false) { + isVerbose = v; + } + return isVerbose; +} + +function capitalize(str: string): string { + if (!str || str === '') { + return str; + } + return [str.charAt(0).toUpperCase(), str.slice(1)].join(''); +} + +function emptyValue(value: string): boolean { + return [null, undefined, ''].includes(value); +} + +function responseContentType(event: Event): string | undefined { + if (event.httpServerResponse?.headers) { + return event.httpServerResponse!.headers!['Content-Type']; + } else if (event.httpClientResponse?.headers) { + return event.httpClientResponse!.headers!['Content-Type']; + } +} + +function appMapDir(appMapFileName: string): string { + return appMapFileName.substring(0, appMapFileName.length - '.appmap.json'.length); +} + +// eslint-disable-next-line +function isFalsey(valueObj: any): boolean { + if (!valueObj) { + return true; + } + if (valueObj.class === 'FalseClass') { + return true; + } + if (valueObj.class === 'Array' && valueObj.value === '[]') { + return true; + } + if (valueObj.value === '') { + return true; + } + + return false; +} + +const isTruthy = (valueObj: any): boolean => !isFalsey(valueObj); + +function providesAuthentication(event: Event, label: string): boolean { + return event.returnValue && event.labels.has(label) && isTruthy(event.returnValue.value); +} + +function ideLink(filePath: string, ide: string, eventId: number): string { + const OSC = '\u001B]'; + const BEL = '\u0007'; + const SEP = ';'; + + // eslint-disable-next-line @typescript-eslint/no-var-requires + const supportsHyperlinks = require('supports-hyperlinks'); + + if (!supportsHyperlinks.stdout) { + return filePath; + } + + let path: string; + if (!isAbsolute(filePath)) { + path = `${__dirname}/../../../../../${filePath}`; + } else { + path = filePath; + } + const state = { currentView: 'viewFlow', selectedObject: `event:${eventId}` }; + const encodedState = encodeURIComponent(JSON.stringify(state)); + const link = + ide == 'vscode' + ? `vscode://appland.appmap/open?uri=${path}&state=${encodedState}` + : `${ide}://open?file=${path}`; + + return [OSC, '8', SEP, SEP, link, BEL, filePath, OSC, '8', SEP, SEP, BEL].join(''); +} + +const toRegExp = (value: string | RegExp): RegExp => { + return typeof value === 'string' ? new RegExp(value as string) : (value as RegExp); +}; + +const toRegExpArray = (value: string[] | RegExp[]): RegExp[] => { + return value.map(toRegExp); +}; + +const RootLabels = ['command', 'job']; + +const isRoot = (event: Event | undefined): boolean => { + if (!event) { + return true; + } + return ( + !!event.httpServerRequest || RootLabels.some((label) => event.codeObject.labels.has(label)) + ); +}; + +export { + appMapDir, + capitalize, + emptyValue, + isFalsey, + isTruthy, + ideLink, + isRoot, + providesAuthentication, + toRegExp, + responseContentType, + toRegExpArray, + verbose, +}; diff --git a/src/sampleConfig/default.yml b/src/sampleConfig/default.yml index 5d89969..8cc3c22 100644 --- a/src/sampleConfig/default.yml +++ b/src/sampleConfig/default.yml @@ -1,19 +1,19 @@ -scanners: +checks: - id: circularDependency - id: http500 - - id: missingContentType - - id: nPlusOneQuery - - id: slowHttpServerRequest - - id: slowQuery - - id: tooManyJoins - - id: tooManyUpdates - - id: updateInGetRequest - # Required labels: secret, log - # - id: secretInLog - # Required labels: security.authentication, security.authorization - # - id: authzBeforeAuthn - # Required labels: security.authentication - # Optional labels: public - # - id: missingAuthentication - # Required labels: dao.materialize - # - id: unbatchedMaterializedQuery +# - id: missingContentType +# - id: nPlusOneQuery +# - id: slowHttpServerRequest +# - id: slowQuery +# - id: tooManyJoins +# - id: tooManyUpdates +# - id: updateInGetRequest +# Required labels: secret, log +# - id: secretInLog +# Required labels: security.authentication, security.authorization +# - id: authzBeforeAuthn +# Required labels: security.authentication +# Optional labels: public +# - id: missingAuthentication +# Required labels: dao.materialize +# - id: unbatchedMaterializedQuery diff --git a/src/types.d.ts b/src/types.d.ts index caab715..6bd051f 100644 --- a/src/types.d.ts +++ b/src/types.d.ts @@ -1,24 +1,23 @@ import { AppMap, Event } from '@appland/models'; -import Assertion from './assertion'; -import AssertionConfig from './configuration/types/assertionConfig'; +import Check from './check'; /** - * Each Assertion in the scanner library wants to consider some set of events as it decides whether the code should be flagged or not. - * A Scope is a way of declaring how these "sets" are defined. A common scope is `http_server_request`. The assertion will look at each HTTP + * Each Rule in the scanner library wants to consider some set of events as it decides whether the code should be flagged or not. + * A Scope is a way of declaring how these "sets" are defined. A common scope is `http_server_request`. The rule will look at each HTTP * server request separately; what happens in one request is irrelevant to the next. For example, when looking for authz_before_authn, each HTTP * server request is considered separately. * * `http_server_request` is one example of a "command". Other types of commands are: CLI commands and background jobs. Each of these has a * defined beginning and end, and is logically completely separate from any other command. * - * Some assertions are relevant only to HTTP server requests - such as `http500`. Others are applicable to any kind of command - such as `nPlusOneQuery`. + * Some rules are relevant only to HTTP server requests - such as `http500`. Others are applicable to any kind of command - such as `nPlusOneQuery`. * - * Finally, other assertions simply want to look for a certain condition regardless of where it occurs. For example, Too many SQL joins will flag any - * query anywhere in the AppMap, even if it's not part of a command. This assertion uses the root scope, which yields a new scope for every root-level Event + * Finally, other rules simply want to look for a certain condition regardless of where it occurs. For example, Too many SQL joins will flag any + * query anywhere in the AppMap, even if it's not part of a command. This rule uses the root scope, which yields a new scope for every root-level Event * (root-level = "has no parent"). * * Ideally, AppMaps would not contain any events that are not part of a command - because without knowing the command, we don't really have any context - * of what the code is trying to do. But, anticipating that this may sometimes happen, "root" scope is a good choice for an assertion that may flag code + * of what the code is trying to do. But, anticipating that this may sometimes happen, "root" scope is a good choice for a rule that may flag code * anywhere in the AppMap. */ export type ScopeName = @@ -44,14 +43,14 @@ export type Level = 'warning' | 'error'; type StringFilter = (value: string) => boolean; /** - * EventFilter is used by Assertion to select Events that will be analyzed for findings. + * EventFilter is used by Rule to select Events that will be analyzed for findings. * The event filter is always applied to the Scope.scope event. If enumerateScope is true, * the filter is applied to all Scope.events as well. */ type EventFilter = (e: Event, appMap?: AppMap) => boolean; /** - * MatchResult is created by an Assertion when it matches an Event. + * MatchResult is created by a rule when it matches an Event. */ export interface MatchResult { level: Level; @@ -62,23 +61,26 @@ export interface MatchResult { relatedEvents?: Event[]; } +type MatcherResult = + | Promise + | boolean + | string + | MatchResult[] + | undefined; + /** - * Matcher function is part of an Assertion. It's applied to an Event to determine whether there is a finding + * Matcher function is part of a rule. It's applied to an Event to determine whether there is a finding * on this event. If the Matcher returns true, a string, or a MatchResult[], then finding(s) are created. */ -type Matcher = ( - e: Event, - appMap: AppMap, - assertion: Assertion -) => Promise | boolean | string | MatchResult[] | undefined; +type Matcher = (e: Event, appMap: AppMap, check: Check) => MatcherResult; /** - * Finding is the full data structure that is created when an Assertion matches an Event. + * Finding is the full data structure that is created when a Rule matches an Event. * - * The Assertion only needs to return a boolean, string, or MatchResult. The scanner framework + * The Rule only needs to return a boolean, string, or MatchResult. The scanner framework * adds the rest of the information to build the complete finding. */ -export interface Finding { +interface Finding { appMapName: string; appMapFile?: string; scannerId: string; @@ -86,38 +88,35 @@ export interface Finding { event: Event; hash: string; scope: Event; - condition: string; message?: string; groupMessage?: string; occurranceCount?: number; relatedEvents?: Event[]; } -/** - * AssertionSpec is provided by each Assertion to define itself. - */ -interface AssertionSpec { - // constructor function used to create an Assertion instance for each matching scope. - scanner: (options?: any) => Assertion; - // labels which the assertion depends on. +interface RuleLogic { + // Tests an event in the scope see if it matches the rule conditions. + matcher: Matcher; + // When specified by the rule, only events which pass the where filter + // will be passed to the matcher. + where?: EventFilter; +} + +interface Rule { + // Unique id of the rule. + id: string; + // Simple text description of the rule. + title: string; + // labels which the rule depends on. labels?: string[]; - // Scope used by the assertion. For each matching Scope in each AppMap, the scanner function is used - // to create a new Assertion instance. The Scope event is passed to the Assertion Matcher. + // Specifies which sub-tree events will be identified as "root events of concern". + // Events which are outside of any scope will not be processed by the rule. scope?: ScopeName; - // When enumerateScope is true, all the events in the scope are passed to the Assertion Matcher. + // Whether to pass all the events in the scope to the matcher. If false, only the scope event + // is passed to the matcher, and the rule should traverse the scope as needed. enumerateScope?: boolean; - // User-defined options for the assertion. + // User-defined options for the rule. Options?: any; -} - -/** - * AssertionPrototype is created by the Scanner framework by combining information in each AssertionConfig - * with the AssertionSpec provided by the assertion code. - */ -interface AssertionPrototype { - config: AssertionConfig; - scope: ScopeName; - enumerateScope: boolean; - // build is invoked by the framework to create a new Assertion for each Scope. - build(): Assertion; + // Function to instantiate the rule logic from configured options. + build: (options: Record) => RuleLogic; } diff --git a/test/util.ts b/test/util.ts index 4fbfbf0..3244c1d 100644 --- a/test/util.ts +++ b/test/util.ts @@ -3,7 +3,7 @@ import { readFile } from 'fs/promises'; import { join } from 'path'; import Assertion from '../src/assertion'; import AssertionChecker from '../src/assertionChecker'; -import AssertionConfig from '../src/configuration/types/assertionConfig'; +import AssertionConfig from '../src/configuration/types/checkConfig'; import MatchPatternConfig from '../src/configuration/types/matchPatternConfig'; import { verbose } from '../src/scanner/util'; import { AssertionPrototype, Finding, ScopeName } from '../src/types'; From 633ab1f8f7ef9fad31b009baedd776b86536e093 Mon Sep 17 00:00:00 2001 From: Kevin Gilpin Date: Tue, 7 Dec 2021 13:39:42 -0500 Subject: [PATCH 3/7] feat: Separate the rule name from check id --- src/check.ts | 2 ++ src/configuration/configurationProvider.ts | 13 +++++++++---- src/configuration/schema/configuration.json | 8 ++++---- src/configuration/types/checkConfig.ts | 6 ++++-- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/check.ts b/src/check.ts index 8d3657f..9c2e07a 100644 --- a/src/check.ts +++ b/src/check.ts @@ -3,6 +3,7 @@ import { verbose } from './rules/util'; import { EventFilter, RuleLogic, Rule, ScopeName } from './types'; export default class Check { + public id: string; public scope: ScopeName; public includeScope: EventFilter[]; public excludeScope: EventFilter[]; @@ -10,6 +11,7 @@ export default class Check { public excludeEvent: EventFilter[]; constructor(public rule: Rule, public options: Record) { + this.id = rule.id; this.scope = rule.scope || 'root'; this.includeScope = []; this.excludeScope = []; diff --git a/src/configuration/configurationProvider.ts b/src/configuration/configurationProvider.ts index f444c1d..76df9a8 100644 --- a/src/configuration/configurationProvider.ts +++ b/src/configuration/configurationProvider.ts @@ -16,7 +16,7 @@ const ajv = new Ajv(); ajv.addSchema(match_pattern_config_schema); async function buildBuiltinCheck(config: CheckConfig): Promise { - const rule: Rule = (await import(`../rules/${config.id}`)).default; + const rule: Rule = (await import(`../rules/${config.rule}`)).default; let options: any; if (rule.Options) { @@ -32,10 +32,15 @@ async function buildBuiltinCheck(config: CheckConfig): Promise { } const check = new Check(rule, options); + if (config.scope) { check.scope = config.scope as ScopeName; } + if (config.id) { + check.id = config.id; + } + check.includeScope = buildEventFilterArray( (config.include || []).filter((item) => item.scope).map((item) => item.scope!) ); @@ -88,8 +93,8 @@ export default class ConfigurationProvider { rawConfig.checks .filter((check) => check.properties) .forEach((check) => { - const id = check.id; - const schemaKey = [capitalize(id), 'Options'].join('.'); + const ruleId = check.rule; + const schemaKey = [capitalize(ruleId), 'Options'].join('.'); if (verbose()) { console.warn(schemaKey); } @@ -101,7 +106,7 @@ export default class ConfigurationProvider { console.warn(propertiesSchema); console.warn(check.properties); } - validate(ajv.compile(propertiesSchema), check.properties || {}, `${check.id} properties`); + validate(ajv.compile(propertiesSchema), check.properties || {}, `${ruleId} properties`); }); return Promise.all(rawConfig.checks.map(async (c: CheckConfig) => buildBuiltinCheck(c))); diff --git a/src/configuration/schema/configuration.json b/src/configuration/schema/configuration.json index 240dd3c..079138f 100644 --- a/src/configuration/schema/configuration.json +++ b/src/configuration/schema/configuration.json @@ -7,9 +7,6 @@ "additionalProperties": false, "description": "CheckConfig represents the user's configuration of an Check, which is an instantiation of a Rule. Each CheckConfing is read from the scanners configuration file.", "properties": { - "description": { - "type": "string" - }, "exclude": { "items": { "additionalProperties": false, @@ -46,12 +43,15 @@ "properties": { "type": "object" }, + "rule": { + "type": "string" + }, "scope": { "type": "string" } }, "required": [ - "id" + "rule" ], "type": "object" }, diff --git a/src/configuration/types/checkConfig.ts b/src/configuration/types/checkConfig.ts index 2b3332b..e9fa7ea 100644 --- a/src/configuration/types/checkConfig.ts +++ b/src/configuration/types/checkConfig.ts @@ -10,8 +10,10 @@ interface MatchConfig { * instantiation of a Rule. Each CheckConfing is read from the scanners configuration file. */ export default interface CheckConfig { - // id is expected to match the file name of the rule in src/rules. - id: string; + // rule is expected to match the file name of the rule in src/rules. + rule: string; + // default: id + id?: string; scope?: string; include?: MatchConfig[]; exclude?: MatchConfig[]; From 2d90d2d6c3b1b77e322346a6a283b1a36367532a Mon Sep 17 00:00:00 2001 From: Kevin Gilpin Date: Wed, 8 Dec 2021 08:39:10 -0500 Subject: [PATCH 4/7] feat: Continue adding rules --- src/check.ts | 2 +- src/checkInstance.ts | 10 ++- src/ruleChecker.ts | 17 +++-- src/rules/incompatibleHttpClientRequest.ts | 54 +++++++++++++++ src/rules/insecureCompare.ts | 72 ++++++++++++++++++++ src/rules/jobNotCancelled.ts | 44 +++++++++++++ src/rules/lib/rpcWithoutProtection.ts | 23 +++++++ src/rules/missingAuthentication.ts | 75 +++++++++++++++++++++ src/rules/missingContentType.ts | 31 +++++++++ src/rules/nPlusOneQuery.ts | 71 ++++++++++++++++++++ src/rules/queryFromInvalidPackage.ts | 44 +++++++++++++ src/rules/queryFromView.ts | 29 ++++++++ src/rules/rpcWithoutCircuitBreaker.ts | 30 +++++++++ src/rules/saveWithoutValidation.ts | 31 +++++++++ src/rules/secretInLog.ts | 77 ++++++++++++++++++++++ src/rules/slowFunctionCall.ts | 37 +++++++++++ src/rules/slowHttpServerRequest.ts | 22 +++++++ src/rules/slowQuery.ts | 21 ++++++ src/rules/tooManyJoins.ts | 65 ++++++++++++++++++ src/rules/tooManyUpdates.ts | 71 ++++++++++++++++++++ src/rules/unbatchedMaterializedQuery.ts | 71 ++++++++++++++++++++ src/rules/updateInGetRequest.ts | 71 ++++++++++++++++++++ src/sampleConfig/bike_index.yml | 20 +++--- src/sampleConfig/default.yml | 26 ++++---- src/sampleConfig/railsSampleApp6thEd.yml | 43 ++++++------ src/sampleConfig/solidus.yml | 41 +++++++----- src/types.d.ts | 9 +-- test/util.ts | 35 ++-------- 28 files changed, 1040 insertions(+), 102 deletions(-) create mode 100644 src/rules/incompatibleHttpClientRequest.ts create mode 100644 src/rules/insecureCompare.ts create mode 100644 src/rules/jobNotCancelled.ts create mode 100644 src/rules/lib/rpcWithoutProtection.ts create mode 100644 src/rules/missingAuthentication.ts create mode 100644 src/rules/missingContentType.ts create mode 100644 src/rules/nPlusOneQuery.ts create mode 100644 src/rules/queryFromInvalidPackage.ts create mode 100644 src/rules/queryFromView.ts create mode 100644 src/rules/rpcWithoutCircuitBreaker.ts create mode 100644 src/rules/saveWithoutValidation.ts create mode 100644 src/rules/secretInLog.ts create mode 100644 src/rules/slowFunctionCall.ts create mode 100644 src/rules/slowHttpServerRequest.ts create mode 100644 src/rules/slowQuery.ts create mode 100644 src/rules/tooManyJoins.ts create mode 100644 src/rules/tooManyUpdates.ts create mode 100644 src/rules/unbatchedMaterializedQuery.ts create mode 100644 src/rules/updateInGetRequest.ts diff --git a/src/check.ts b/src/check.ts index 9c2e07a..f702764 100644 --- a/src/check.ts +++ b/src/check.ts @@ -1,6 +1,6 @@ import { AppMap, Event } from '@appland/models'; import { verbose } from './rules/util'; -import { EventFilter, RuleLogic, Rule, ScopeName } from './types'; +import { EventFilter, Rule, ScopeName } from './types'; export default class Check { public id: string; diff --git a/src/checkInstance.ts b/src/checkInstance.ts index 4513011..8fe574e 100644 --- a/src/checkInstance.ts +++ b/src/checkInstance.ts @@ -9,10 +9,14 @@ export default class CheckInstance { constructor(check: Check) { this.check = check; - this.ruleLogic = check.rule.build(check.options); + this.ruleLogic = check.rule.build(check.options || {}); } - get id(): string { + get checkId(): string { + return this.check.id; + } + + get ruleId(): string { return this.check.rule.id; } @@ -25,7 +29,7 @@ export default class CheckInstance { } get enumerateScope(): boolean { - return this.check.rule.enumerateScope || true; + return this.check.rule.enumerateScope === undefined ? true : this.check.rule.enumerateScope; } filterEvent(event: Event, appMap?: AppMap): boolean { diff --git a/src/ruleChecker.ts b/src/ruleChecker.ts index 18c94db..75d783f 100644 --- a/src/ruleChecker.ts +++ b/src/ruleChecker.ts @@ -65,7 +65,7 @@ export default class RuleChecker { } if (verbose()) { console.warn( - `Asserting ${checkInstance.id} on ${event.codeObject.fqid} event ${event.toString()}` + `Asserting ${checkInstance.ruleId} on ${event.codeObject.fqid} event ${event.toString()}` ); } @@ -90,19 +90,24 @@ export default class RuleChecker { const findingEvent = matchEvent || event; return { appMapName: appMap.metadata.name, - scannerId: checkInstance.id, - scannerTitle: checkInstance.title, + checkId: checkInstance.checkId, + ruleId: checkInstance.ruleId, + ruleTitle: checkInstance.title, event: findingEvent, hash: findingEvent.hash, scope, - message, + message: message || checkInstance.title, groupMessage, occurranceCount, relatedEvents, }; }; - const matchResult = await checkInstance.ruleLogic.matcher(event, appMap, checkInstance.check); + const matchResult = await checkInstance.ruleLogic.matcher( + event, + appMap, + checkInstance.filterEvent.bind(checkInstance) + ); const numFindings = findings.length; if (matchResult === true) { findings.push(buildFinding(event)); @@ -125,7 +130,7 @@ export default class RuleChecker { if (verbose()) { if (findings.length > numFindings) { findings.forEach((finding) => - console.log(`\tFinding: ${finding.scannerId} : ${finding.message}`) + console.log(`\tFinding: ${finding.ruleId} : ${finding.message}`) ); } } diff --git a/src/rules/incompatibleHttpClientRequest.ts b/src/rules/incompatibleHttpClientRequest.ts new file mode 100644 index 0000000..b4d4e5c --- /dev/null +++ b/src/rules/incompatibleHttpClientRequest.ts @@ -0,0 +1,54 @@ +import { Event } from '@appland/models'; +import { forClientRequest, forURL, breakingChanges } from '../openapi'; +import { MatchResult, Rule, RuleLogic } from '../types'; +import * as types from './types'; +import OpenApiDiff from 'openapi-diff'; +import { OpenAPIV3 } from 'openapi-types'; + +class Options implements types.IncompatibleHttpClientRequest.Options { + public schemata: Record = {}; +} + +const changeMessage = (change: OpenApiDiff.DiffResult<'breaking'>): string => { + return `HTTP client request is incompatible with OpenAPI schema. Change details: ${ + change.action + } ${change.sourceSpecEntityDetails + .concat(change.destinationSpecEntityDetails) + .map((detail) => detail.location) + .join(', ')}`; +}; + +function build(options: Options): RuleLogic { + async function matcher(event: Event): Promise { + const clientFragment = forClientRequest(event); + const serverSchema = await forURL(event.httpClientRequest!.url!, options.schemata); + const clientSchema = { + openapi: '3.0.0', + info: { + title: 'Schema derived from client request', + version: serverSchema.info.version, // Indicate that it *should* be compatible. + }, + paths: clientFragment!.paths, + components: { securitySchemes: clientFragment!.securitySchemes }, + } as OpenAPIV3.Document; + const changes = await breakingChanges(clientSchema, serverSchema); + return changes.map((change: OpenApiDiff.DiffResult<'breaking'>) => ({ + level: 'error', + message: changeMessage(change), + })); + } + + return { + matcher, + where: (e: Event) => !!e.httpClientRequest && !!e.httpClientRequest!.url, + }; +} + +export default { + id: 'incompatible-http-client-request', + title: 'Incompatible HTTP client request', + scope: 'http_client_request', + enumerateScope: false, + Options, + build, +} as Rule; diff --git a/src/rules/insecureCompare.ts b/src/rules/insecureCompare.ts new file mode 100644 index 0000000..9ede3b0 --- /dev/null +++ b/src/rules/insecureCompare.ts @@ -0,0 +1,72 @@ +import { Event } from '@appland/models'; +import recordSecrets from '../analyzer/recordSecrets'; +import SecretsRegexes from '../analyzer/secretsRegexes'; +import { Rule, RuleLogic } from '../types.d'; + +const BCRYPT_REGEXP = /^[$]2[abxy]?[$](?:0[4-9]|[12][0-9]|3[01])[$][./0-9a-zA-Z]{53}$/; + +const secrets: Set = new Set(); + +function stringEquals(e: Event): string | boolean | import('../types').MatchResult[] | undefined { + if (!e.parameters || !e.receiver || e.parameters!.length !== 1) { + return; + } + + const args = [e.receiver!.value, e.parameters![0].value]; + + function isBcrypt(str: string): boolean { + return BCRYPT_REGEXP.test(str); + } + + function isSecret(str: string): boolean { + if (secrets.has(str)) { + return true; + } + return !!Object.keys(SecretsRegexes).find( + (key): boolean => !!SecretsRegexes[key].find((re: RegExp): boolean => re.test(str)) + ); + } + + // BCrypted strings are safe to compare using equals() + if (args.every(isBcrypt)) { + return; + } + if (!args.every(isSecret)) { + return; + } + + return true; +} + +function build(): RuleLogic { + function matcher(e: Event) { + if (e.codeObject.labels.has(Secret)) { + recordSecrets(secrets, e); + } + if (e.parameters && e.codeObject.labels.has(StringEquals)) { + return stringEquals(e); + } + } + + function where(e: Event): boolean { + return ( + e.isFunction && (e.codeObject.labels.has(StringEquals) || e.codeObject.labels.has(Secret)) + ); + } + + return { + matcher, + where, + }; +} + +const Secret = 'secret'; +const StringEquals = 'string.equals'; + +export default { + id: 'insecure-compare', + title: 'Insecure comparison of secrets', + labels: [Secret, StringEquals], + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/jobNotCancelled.ts b/src/rules/jobNotCancelled.ts new file mode 100644 index 0000000..d25c47a --- /dev/null +++ b/src/rules/jobNotCancelled.ts @@ -0,0 +1,44 @@ +import type { Event } from '@appland/models'; +import type { MatchResult, Rule, RuleLogic } from '../types'; +import Labels from '../wellKnownLabels'; +import { hasTransactionDetails } from '../scope/sqlTransactionScope'; + +function build(): RuleLogic { + function matcher(event: Event): MatchResult[] | undefined { + if (!hasTransactionDetails(event)) + throw new Error(`expected event ${event.id} to be a transaction`); + if (event.transaction.status === 'commit') return; + + const creationEvents = event.transaction.events.filter(({ labels }) => + labels.has(Labels.JobCreate) + ); + const cancellationEvents = event.transaction.events.filter(({ labels }) => + labels.has(Labels.JobCancel) + ); + const missing = creationEvents.length - cancellationEvents.length; + if (missing === 0) return; + + const result: MatchResult = { + level: 'error', + event: event, + message: `${missing} jobs created but not cancelled in this rolled back transaction`, + // if there's a mismatch and there are cancellations we can't tell + // for sure which creations they match, so return everything + relatedEvents: [...creationEvents, ...cancellationEvents], + }; + + return [result]; + } + + return { + matcher, + }; +} + +export default { + id: 'job-not-cancelled', + title: 'Job created in a rolled back transaction and not cancelled', + scope: 'transaction', + labels: [Labels.JobCreate, Labels.JobCancel], + build, +} as Rule; diff --git a/src/rules/lib/rpcWithoutProtection.ts b/src/rules/lib/rpcWithoutProtection.ts new file mode 100644 index 0000000..c342611 --- /dev/null +++ b/src/rules/lib/rpcWithoutProtection.ts @@ -0,0 +1,23 @@ +import { Event, Label } from '@appland/models'; +import { RuleLogic } from 'src/types'; + +export interface RPCWithoutProtectionOptions { + get expectedLabel(): Label; +} + +export function rpcWithoutProtection( + candidateGenerator: (httpClientRequest: Event) => Generator, + options: RPCWithoutProtectionOptions +): RuleLogic { + return { + matcher: (httpClientRequest: Event) => { + for (const candidate of candidateGenerator(httpClientRequest)) { + if (candidate.codeObject.labels.has(options.expectedLabel)) { + return false; + } + } + return true; + }, + where: (e: Event) => !!e.httpClientRequest, + }; +} diff --git a/src/rules/missingAuthentication.ts b/src/rules/missingAuthentication.ts new file mode 100644 index 0000000..65b89da --- /dev/null +++ b/src/rules/missingAuthentication.ts @@ -0,0 +1,75 @@ +import { Event, EventNavigator } from '@appland/models'; +import { rpcRequestForEvent } from '../openapi/rpcRequest'; +import * as types from './types'; +import { Rule, RuleLogic, StringFilter } from '../types'; +import { providesAuthentication } from './util'; +import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; +import { buildFilters } from './lib/matchPattern'; + +function isPublic(event: Event): boolean { + return event.labels.has(Public); +} + +const authenticatedBy = (iterator: Iterator): boolean => { + let i: IteratorResult = iterator.next(); + while (!i.done) { + if (isPublic(i.value.event) || providesAuthentication(i.value.event, SecurityAuthentication)) { + return true; + } + i = iterator.next(); + } + + return false; +}; + +class Options implements types.MissingAuthentication.Options { + public includeContentTypes: MatchPatternConfig[] = []; + public excludeContentTypes: MatchPatternConfig[] = []; +} + +function build(options: Options = new Options()): RuleLogic { + const includeContentTypes = buildFilters(options.includeContentTypes); + const excludeContentTypes = buildFilters(options.excludeContentTypes); + + function testContentType(contentType: string): boolean { + function test(filter: StringFilter): boolean { + return filter(contentType); + } + + return ( + (includeContentTypes.length === 0 || includeContentTypes.some(test)) && + !excludeContentTypes.some(test) + ); + } + + function matcher(event: Event): boolean { + return !authenticatedBy(new EventNavigator(event).descendants()); + } + + function where(e: Event) { + return ( + e.route !== undefined && + e.httpServerResponse !== undefined && + e.httpServerResponse.status < 300 && + !!rpcRequestForEvent(e) && + !!rpcRequestForEvent(e)!.contentType && + testContentType(rpcRequestForEvent(e)!.contentType) + ); + } + return { + where, + matcher, + }; +} +const Public = 'public'; +const SecurityAuthentication = 'security.authentication'; + +export default { + id: 'missing-authentication', + title: 'Unauthenticated HTTP server request', + scope: 'http_server_request', + labels: [Public, SecurityAuthentication], + enumerateScope: false, + Options, + build, +} as Rule; diff --git a/src/rules/missingContentType.ts b/src/rules/missingContentType.ts new file mode 100644 index 0000000..ec6d67d --- /dev/null +++ b/src/rules/missingContentType.ts @@ -0,0 +1,31 @@ +import { Event } from '@appland/models'; +import { MatcherResult, Rule, RuleLogic } from 'src/types'; +import { rpcRequestForEvent } from '../openapi/rpcRequest'; + +const isRedirect = (status: number) => [301, 302, 303, 307, 308].includes(status); +const hasContent = (status: number) => status !== 204; + +function build(): RuleLogic { + function matcher(e: Event) { + return rpcRequestForEvent(e)!.contentType === undefined; + } + function where(e: Event) { + return ( + !!e.httpServerResponse && + !isRedirect(e.httpServerResponse!.status) && + hasContent(e.httpServerResponse!.status) + ); + } + return { + matcher, + where, + }; +} + +export default { + id: 'missing-content-type', + title: 'HTTP server request without a Content-Type header', + scope: 'http_server_request', + enumerateScope: false, + build, +} as Rule; diff --git a/src/rules/nPlusOneQuery.ts b/src/rules/nPlusOneQuery.ts new file mode 100644 index 0000000..43ab098 --- /dev/null +++ b/src/rules/nPlusOneQuery.ts @@ -0,0 +1,71 @@ +import { AppMap, Event } from '@appland/models'; +import { EventFilter, Level, MatchResult, Rule, RuleLogic } from '../types'; +import * as types from './types'; +import { SQLCount, sqlStrings } from '../database'; +import Check from 'src/check'; +import CheckInstance from 'src/checkInstance'; + +class Options implements types.NPlusOneQuery.Options { + public warningLimit = 5; + public errorLimit = 10; +} + +// TODO: clean up according to https://github.com/applandinc/scanner/issues/43 +function build(options: Options): RuleLogic { + const sqlCount: Record = {}; + + function matcher( + command: Event, + _appMap: AppMap, + eventFilter: EventFilter + ): MatchResult[] | undefined { + for (const sqlEvent of sqlStrings(command, eventFilter)) { + let occurrence = sqlCount[sqlEvent.sql]; + if (!occurrence) { + occurrence = { + count: 1, + events: [sqlEvent.event], + }; + sqlCount[sqlEvent.sql] = occurrence; + } else { + occurrence.count += 1; + occurrence.events.push(sqlEvent.event); + } + } + + return Object.keys(sqlCount).reduce((matchResults, sql) => { + const occurrence = sqlCount[sql]; + + const buildMatchResult = (level: Level): MatchResult => { + return { + level: level, + event: occurrence.events[0], + message: `${occurrence.count} occurrences of SQL: ${sql}`, + groupMessage: sql, + occurranceCount: occurrence.count, + relatedEvents: occurrence.events, + }; + }; + + if (occurrence.count >= options.errorLimit) { + matchResults.push(buildMatchResult('error')); + } else if (occurrence.count >= options.warningLimit) { + matchResults.push(buildMatchResult('warning')); + } + return matchResults; + }, [] as MatchResult[]); + } + + return { + matcher, + }; +} + +export default { + id: 'n-plus-one-query', + title: 'N+1 SQL queries', + scope: 'command', + enumerateScope: false, + Options, + build, +} as Rule; diff --git a/src/rules/queryFromInvalidPackage.ts b/src/rules/queryFromInvalidPackage.ts new file mode 100644 index 0000000..15c0d71 --- /dev/null +++ b/src/rules/queryFromInvalidPackage.ts @@ -0,0 +1,44 @@ +import { Event } from '@appland/models'; +import { Rule, RuleLogic } from 'src/types'; +import * as types from './types'; +import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; +import { buildFilters } from './lib/matchPattern'; + +// TODO: Use the Query AST for this. +const WHITELIST = [/\bBEGIN\b/i, /\bCOMMIT\b/i, /\bROLLBACK\b/i, /\bRELEASE\b/i, /\bSAVEPOINT\b/i]; + +class Options implements types.QueryFromInvalidPackage.Options { + public allowedPackages: MatchPatternConfig[] = []; + public allowedQueries: MatchPatternConfig[] = WHITELIST.map( + (regexp) => ({ match: regexp } as MatchPatternConfig) + ); +} + +function build(options: Options): RuleLogic { + const allowedPackages = buildFilters(options.allowedPackages); + const allowedQueries = buildFilters(options.allowedQueries); + + function matcher(e: Event) { + if (!allowedPackages.some((filter) => filter(e.parent!.codeObject.packageOf))) { + return `${e.codeObject.id} is invoked from illegal package ${e.parent!.codeObject.packageOf}`; + } + return false; + } + + function where(e: Event) { + return !!e.sqlQuery && !!e.parent && !allowedQueries.some((pattern) => pattern(e.sqlQuery!)); + } + + return { + matcher, + where, + }; +} + +export default { + id: 'query-from-invalid-package', + title: 'Queries from invalid packages', + Options, + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/queryFromView.ts b/src/rules/queryFromView.ts new file mode 100644 index 0000000..de8bd5b --- /dev/null +++ b/src/rules/queryFromView.ts @@ -0,0 +1,29 @@ +import { Event, Label } from '@appland/models'; +import * as types from './types'; +import { Rule, RuleLogic } from 'src/types'; + +class Options implements types.QueryFromView.Options { + public forbiddenLabel: Label = 'mvc.template'; +} + +function build(options: Options = new Options()): RuleLogic { + function matcher(e: Event) { + return e.ancestors().some((e: Event) => e.codeObject.labels.has(options.forbiddenLabel)); + } + function where(e: Event) { + return !!e.sqlQuery; + } + + return { + matcher, + where, + }; +} + +export default { + id: 'query-from-view', + title: 'Queries from view', + Options, + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/rpcWithoutCircuitBreaker.ts b/src/rules/rpcWithoutCircuitBreaker.ts new file mode 100644 index 0000000..f45cc78 --- /dev/null +++ b/src/rules/rpcWithoutCircuitBreaker.ts @@ -0,0 +1,30 @@ +import { Event, EventNavigator } from '@appland/models'; +import * as types from './types'; +import { RPCWithoutProtectionOptions, rpcWithoutProtection } from './lib/rpcWithoutProtection'; +import { Rule, RuleLogic } from 'src/types'; + +class Options implements RPCWithoutProtectionOptions, types.RPCWithoutCircuitBreaker.Options { + public expectedLabel: string = RPCCircuitBreaker; +} + +// The circuit breaker will be found in a descendant of the httpClientRequest. +function* descendants(httpClientRequest: Event): Generator { + for (const candidate of new EventNavigator(httpClientRequest).descendants()) { + yield candidate.event; + } +} + +function build(options: Options = new Options()): RuleLogic { + return rpcWithoutProtection(descendants, options); +} + +const RPCCircuitBreaker = 'rpc.circuit_breaker'; + +export default { + id: 'rpc-without-circuit-breaker', + title: 'RPC without circuit breaker', + Options, + labels: [RPCCircuitBreaker], + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/saveWithoutValidation.ts b/src/rules/saveWithoutValidation.ts new file mode 100644 index 0000000..b446af6 --- /dev/null +++ b/src/rules/saveWithoutValidation.ts @@ -0,0 +1,31 @@ +import { Event, EventNavigator } from '@appland/models'; +import { Rule, RuleLogic } from '../types'; + +const validatedBy = (iterator: Iterator): boolean => { + let i: IteratorResult = iterator.next(); + while (!i.done) { + if ( + i.value.event.methodId !== undefined && + ['valid?', 'validate'].includes(i.value.event.methodId!) + ) { + return true; + } + i = iterator.next(); + } + + return false; +}; + +function build(): RuleLogic { + return { + matcher: (event: Event) => !validatedBy(new EventNavigator(event).descendants()), + where: (e: Event) => e.isFunction && ['save', 'save!'].includes(e.methodId!), + }; +} + +export default { + id: 'save-without-validation', + title: 'Save without validation', + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/secretInLog.ts b/src/rules/secretInLog.ts new file mode 100644 index 0000000..9af16e5 --- /dev/null +++ b/src/rules/secretInLog.ts @@ -0,0 +1,77 @@ +import { Event, ParameterObject } from '@appland/models'; +import { MatchResult, Rule, RuleLogic } from 'src/types'; +import SecretsRegexes from '../analyzer/secretsRegexes'; +import { emptyValue } from './util'; +import recordSecrets from '../analyzer/recordSecrets'; + +class Match { + constructor(public regexp: RegExp | string, public value: string) {} +} + +const secrets: Set = new Set(); + +const findMatchingValue = (regexps: RegExp[], parameters: readonly ParameterObject[]): Match[] => { + const matches: Match[] = []; + parameters + .filter((parameter) => !emptyValue(parameter.value)) + .forEach((parameter) => { + const value = parameter.value; + regexps + .filter((regexp) => regexp.test(value)) + .forEach((regexp) => { + matches.push(new Match(regexp, value)); + }); + }); + return matches; +}; + +const findInLog = (e: Event): MatchResult[] | undefined => { + const matches: Match[] = Object.keys(SecretsRegexes).reduce((memo, key) => { + const matches = findMatchingValue(SecretsRegexes[key], e.parameters!); + matches.forEach((match) => memo.push(match)); + return memo; + }, [] as Match[]); + + e.parameters!.filter((parameter) => !emptyValue(parameter.value)).forEach((parameter) => { + const value = parameter.value; + secrets.forEach((secret) => { + if (value.includes(secret)) { + matches.push(new Match(secret, value)); + } + }); + }); + + if (matches.length > 0) { + return matches.map((match) => ({ + level: 'error', + message: `${match.value} contains secret ${match.regexp}`, + })); + } +}; + +function build(): RuleLogic { + return { + matcher: (e) => { + if (e.codeObject.labels.has(Secret)) { + recordSecrets(secrets, e); + } + if (e.parameters && e.codeObject.labels.has(Log)) { + return findInLog(e); + } + }, + where: (e) => { + return e.codeObject.labels.has(Log) || e.codeObject.labels.has(Secret); + }, + }; +} + +const Secret = 'secret'; +const Log = 'log'; + +export default { + id: 'secret-in-log', + title: 'Secret in log', + labels: [Secret, Log], + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/slowFunctionCall.ts b/src/rules/slowFunctionCall.ts new file mode 100644 index 0000000..0d27ba1 --- /dev/null +++ b/src/rules/slowFunctionCall.ts @@ -0,0 +1,37 @@ +import { Rule, RuleLogic, ScopeName } from 'src/types'; +import * as types from './types'; +import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; +import { buildFilters } from './lib/matchPattern'; + +class Options implements types.SlowFunctionCall.Options { + public functions: MatchPatternConfig[] = []; + public timeAllowed = 0.1; +} + +function build(options: Options): RuleLogic { + const functionPatterns = buildFilters(options.functions || []); + + return { + matcher: (e) => { + if (e.returnEvent.elapsedTime! > options.timeAllowed) { + return `Slow ${e.codeObject.id} call (${e.returnEvent.elapsedTime}ms)`; + } + }, + where: (e) => + e.isFunction && + !!e.returnEvent && + !!e.returnEvent.elapsedTime && + !!e.codeObject.id && + (functionPatterns.length === 0 || + functionPatterns.some((pattern) => pattern(e.codeObject.id))), + }; +} + +export default { + id: 'slow-function-call', + title: 'Slow function call', + scope: 'root' as ScopeName, + enumerateScope: true, + Options, + build, +} as Rule; diff --git a/src/rules/slowHttpServerRequest.ts b/src/rules/slowHttpServerRequest.ts new file mode 100644 index 0000000..7d95aa8 --- /dev/null +++ b/src/rules/slowHttpServerRequest.ts @@ -0,0 +1,22 @@ +import { Rule, RuleLogic } from 'src/types'; +import * as types from './types'; + +class Options implements types.SlowHTTPServerRequest.Options { + public timeAllowed = 1; +} + +function build(options: Options): RuleLogic { + return { + matcher: (e) => e.elapsedTime! > options.timeAllowed, + where: (e) => !!e.httpServerRequest && e.elapsedTime !== undefined, + }; +} + +export default { + id: 'slow-http-server-request', + title: 'Slow HTTP server requests', + scope: 'http_server_request', + enumerateScope: false, + Options, + build, +} as Rule; diff --git a/src/rules/slowQuery.ts b/src/rules/slowQuery.ts new file mode 100644 index 0000000..b894edb --- /dev/null +++ b/src/rules/slowQuery.ts @@ -0,0 +1,21 @@ +import { Rule, RuleLogic } from 'src/types'; +import * as types from './types'; + +class Options implements types.SlowQuery.Options { + public timeAllowed = 1; +} + +function build(options: Options = new Options()): RuleLogic { + return { + matcher: (e) => e.elapsedTime! > options.timeAllowed, + where: (e) => !!e.sqlQuery && !!e.elapsedTime, + }; +} + +export default { + id: 'slow-query', + title: 'Slow SQL queries', + Options, + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/tooManyJoins.ts b/src/rules/tooManyJoins.ts new file mode 100644 index 0000000..607dafb --- /dev/null +++ b/src/rules/tooManyJoins.ts @@ -0,0 +1,65 @@ +import { AppMap, Event } from '@appland/models'; +import { EventFilter, MatchResult, Rule, RuleLogic } from '../types'; +import * as types from './types'; +import { countJoins, SQLCount, sqlStrings } from '../database'; + +export interface JoinCount extends SQLCount { + joins: number; +} + +class Options implements types.TooManyJoins.Options { + public warningLimit = 5; +} + +// TODO: clean up (https://github.com/applandinc/scanner/issues/43) +function build(options: Options = new Options()): RuleLogic { + const joinCount: Record = {}; + function matcher( + command: Event, + _appMap: AppMap, + eventFilter: EventFilter + ): MatchResult[] | undefined { + for (const sqlEvent of sqlStrings(command, eventFilter)) { + let occurrence = joinCount[sqlEvent.sql]; + + if (!occurrence) { + occurrence = { + count: 1, + joins: countJoins(sqlEvent.sql), + events: [sqlEvent.event], + }; + joinCount[sqlEvent.sql] = occurrence; + } else { + occurrence.count += 1; + occurrence.events.push(sqlEvent.event); + } + } + + return Object.keys(joinCount).reduce((matchResults, sql) => { + const occurrence = joinCount[sql]; + + if (occurrence.joins >= options.warningLimit) { + matchResults.push({ + level: 'warning', + event: occurrence.events[0], + message: `${occurrence.joins} join${occurrence.joins > 1 ? 's' : ''} in SQL "${sql}"`, + relatedEvents: occurrence.events, + }); + } + return matchResults; + }, [] as MatchResult[]); + } + + return { + matcher, + }; +} + +export default { + id: 'too-many-joins', + title: 'Too many joins', + scope: 'command', + enumerateScope: false, + Options, + build, +} as Rule; diff --git a/src/rules/tooManyUpdates.ts b/src/rules/tooManyUpdates.ts new file mode 100644 index 0000000..ef21b75 --- /dev/null +++ b/src/rules/tooManyUpdates.ts @@ -0,0 +1,71 @@ +import { Event, EventNavigator } from '@appland/models'; +import { MatchResult, Rule, RuleLogic } from 'src/types'; +import * as types from './types'; + +// TODO: Use the Query AST for this. +const QueryIncludes: RegExp[] = [/\bINSERT\b/i, /\bUPDATE\b/i]; +const UpdateMethods: string[] = ['put', 'post', 'patch']; + +class Options implements types.TooManyUpdates.Options { + public warningLimit = 20; +} + +function build(options: Options): RuleLogic { + const isUpdate = (event: Event): boolean => { + const isSQLUpdate = (): boolean => { + if (!event.sqlQuery) { + return false; + } + return QueryIncludes.some((pattern) => pattern.test(event.sqlQuery!)); + }; + + const isRPCUpdate = (): boolean => { + if (!event.httpClientRequest) { + return false; + } + return UpdateMethods.includes(event.httpClientRequest!.request_method.toLowerCase()); + }; + + return isSQLUpdate() || isRPCUpdate(); + }; + + const updateEvents = function* (event: Event): Generator { + for (const e of new EventNavigator(event).descendants()) { + if (!isUpdate(e.event)) { + continue; + } + yield e.event; + } + }; + + function matcher(command: Event): MatchResult[] | undefined { + const events: Event[] = []; + for (const updateEvent of updateEvents(command)) { + events.push(updateEvent); + } + + if (events.length > options.warningLimit) { + return [ + { + level: 'error', + message: `Command performs ${events.length} SQL and RPC updates`, + event: events[0], + relatedEvents: events, + }, + ]; + } + } + + return { + matcher, + }; +} + +export default { + id: 'too-many-updates', + title: 'Too many SQL and RPC updates performed in one command', + scope: 'command', + enumerateScope: false, + Options, + build, +} as Rule; diff --git a/src/rules/unbatchedMaterializedQuery.ts b/src/rules/unbatchedMaterializedQuery.ts new file mode 100644 index 0000000..1fb4ec1 --- /dev/null +++ b/src/rules/unbatchedMaterializedQuery.ts @@ -0,0 +1,71 @@ +import { buildQueryAST, Event } from '@appland/models'; +import { Rule, RuleLogic } from '../types'; +import { visit } from '../database/visit'; + +function isMaterialized(e: Event): boolean { + return e.ancestors().some(({ labels }) => labels.has(DAOMaterialize)); +} + +function isApplicable(e: Event): boolean { + try { + const ast = buildQueryAST(e.sqlQuery!); + let isSelect = false; + let isCount = false; + let hasLimitClause = false; + let isMetadataQuery = false; + + if (ast) { + const metadataTableNames = ['sqlite_master']; + + visit(ast, { + 'statement.select': (statement: any) => { + isSelect = true; + + if ( + statement.result && + Array.isArray(statement.result) && + statement.result.length === 1 && + statement.result[0].type === 'function' && + statement.result[0].name.name === 'count' + ) { + isCount = true; + } + }, + 'expression.limit': () => { + hasLimitClause = true; + }, + 'identifier.table': (identifier: any) => { + if (metadataTableNames.includes(identifier.name)) { + isMetadataQuery = true; + } + }, + }); + } + + const isBatched = hasLimitClause || isCount || isMetadataQuery; + + return isSelect && !isBatched && isMaterialized(e); + } catch (_) { + console.warn(`Unable to analyze query "${e.sqlQuery!}"`); + return false; + } +} + +function build(): RuleLogic { + return { + matcher: (e) => isApplicable(e), + where: (e) => !!e.sqlQuery, + }; +} + +// Example: ActiveRecord::Relation#records +const DAOMaterialize = 'dao.materialize'; + +export default { + id: 'unbatched-materialized-query', + title: 'Unbatched materialized SQL query', + labels: [DAOMaterialize], + scope: 'command', + enumerateScope: true, + build, +} as Rule; diff --git a/src/rules/updateInGetRequest.ts b/src/rules/updateInGetRequest.ts new file mode 100644 index 0000000..abf10c3 --- /dev/null +++ b/src/rules/updateInGetRequest.ts @@ -0,0 +1,71 @@ +import { Event } from '@appland/models'; +import { Rule, RuleLogic } from 'src/types'; +import { toRegExpArray } from './util'; + +class Options { + private _queryInclude: RegExp[]; + private _queryExclude: RegExp[]; + + constructor( + queryInclude: RegExp[] = [/\binsert\b/i, /\bupdate\b/i], + queryExclude: RegExp[] = [] + ) { + this._queryInclude = queryInclude; + this._queryExclude = queryExclude; + } + + get queryInclude(): RegExp[] { + return this._queryInclude; + } + + set queryInclude(value: string[] | RegExp[]) { + this._queryInclude = toRegExpArray(value); + } + + get queryExclude(): RegExp[] { + return this._queryExclude; + } + + set queryExclude(value: string[] | RegExp[]) { + this._queryExclude = toRegExpArray(value); + } +} + +function build(options: Options = new Options()): RuleLogic { + return { + matcher: (e) => { + let httpServerRequest: Event | undefined; + function hasHttpServerRequest() { + httpServerRequest = e + .ancestors() + .find( + (ancestor) => + ancestor.httpServerRequest && + ['head', 'get'].includes(ancestor.httpServerRequest.request_method.toLowerCase()) + ); + return httpServerRequest !== undefined; + } + + if ( + options.queryInclude.some((pattern) => e.sqlQuery!.match(pattern)) && + !options.queryExclude.some((pattern) => e.sqlQuery!.match(pattern)) && + !e.ancestors().some((ancestor) => ancestor.codeObject.labels.has(Audit)) && + hasHttpServerRequest() + ) { + return `Data update performed in ${httpServerRequest!.route}: ${e.sqlQuery}`; + } + }, + where: (e) => !!e.sqlQuery, + }; +} + +const Audit = 'audit'; + +export default { + id: 'update-in-get-request', + title: 'Data update performed in GET or HEAD request', + scope: 'http_server_request', + labels: [Audit], + Options, + build, +} as Rule; diff --git a/src/sampleConfig/bike_index.yml b/src/sampleConfig/bike_index.yml index aadda14..df00447 100644 --- a/src/sampleConfig/bike_index.yml +++ b/src/sampleConfig/bike_index.yml @@ -1,10 +1,10 @@ -scanners: - - id: http500 - - id: missingContentType - - id: missingAuthentication - - id: nPlusOneQuery - - id: saveWithoutValidation - - id: secretInLog - - id: slowHttpServerRequest - - id: slowQuery - - id: tooManyUpdates +checks: + - rule: http500 + - rule: missingContentType + - rule: missingAuthentication + - rule: nPlusOneQuery + - rule: saveWithoutValidation + - rule: secretInLog + - rule: slowHttpServerRequest + - rule: slowQuery + - rule: tooManyUpdates diff --git a/src/sampleConfig/default.yml b/src/sampleConfig/default.yml index 8cc3c22..f77073a 100644 --- a/src/sampleConfig/default.yml +++ b/src/sampleConfig/default.yml @@ -1,19 +1,19 @@ checks: - - id: circularDependency - - id: http500 -# - id: missingContentType -# - id: nPlusOneQuery -# - id: slowHttpServerRequest -# - id: slowQuery -# - id: tooManyJoins -# - id: tooManyUpdates -# - id: updateInGetRequest + - rule: circularDependency + - rule: http500 + - rule: missingContentType + - rule: nPlusOneQuery +# - rule: slowHttpServerRequest +# - rule: slowQuery +# - rule: tooManyJoins +# - rule: tooManyUpdates +# - rule: updateInGetRequest # Required labels: secret, log -# - id: secretInLog +# - rule: secretInLog # Required labels: security.authentication, security.authorization -# - id: authzBeforeAuthn +# - rule: authzBeforeAuthn # Required labels: security.authentication # Optional labels: public -# - id: missingAuthentication +# - rule: missingAuthentication # Required labels: dao.materialize -# - id: unbatchedMaterializedQuery +# - rule: unbatchedMaterializedQuery diff --git a/src/sampleConfig/railsSampleApp6thEd.yml b/src/sampleConfig/railsSampleApp6thEd.yml index d945e8e..3ea7c83 100644 --- a/src/sampleConfig/railsSampleApp6thEd.yml +++ b/src/sampleConfig/railsSampleApp6thEd.yml @@ -1,28 +1,29 @@ -scanners: - - id: authzBeforeAuthn - - id: http500 - - id: illegalPackageDependency - include: - - regexp: ^function:app\/controllers\/ +checks: + - rule: authzBeforeAuthn + - rule: http500 + - rule: illegalPackageDependency properties: - allowedPackages: [ 'actionpack' ] - - id: insecureCompare - - id: missingAuthentication - - id: missingContentType - - id: nPlusOneQuery - - id: secretInLog - - id: slowFunctionCall - include: - - regexp: ^function:app/controllers/\w+Controller#create$ + callerPackages: + - equal: actionpack + calleePackage: + equal: app/controllers + - rule: insecureCompare + - rule: missingAuthentication + - rule: missingContentType + - rule: nPlusOneQuery + - rule: secretInLog + - rule: slowFunctionCall properties: timeAllowed: 0.2 - - id: slowHttpServerRequest + functions: + - match: Controller# + - rule: slowHttpServerRequest properties: timeAllowed: 0.5 - - id: slowQuery + - rule: slowQuery properties: timeAllowed: 0.05 - - id: tooManyJoins - - id: tooManyUpdates - - id: unbatchedMaterializedQuery - - id: updateInGetRequest + - rule: tooManyJoins + - rule: tooManyUpdates + - rule: unbatchedMaterializedQuery + - rule: updateInGetRequest diff --git a/src/sampleConfig/solidus.yml b/src/sampleConfig/solidus.yml index dbb170d..ab173b6 100644 --- a/src/sampleConfig/solidus.yml +++ b/src/sampleConfig/solidus.yml @@ -1,22 +1,31 @@ -scanners: - - id: http500 - - id: illegalPackageDependency - include: - - pattern: function:app/controllers/* +checks: + - rule: http500 + - rule: illegalPackageDependency properties: - allowedPackages: [ 'actionpack', 'activesupport', 'solidus_core' ] - - id: missingContentType - - id: missingAuthentication - include: - - pattern: /\/api/\/ - - id: secretInLog - - id: slowHttpServerRequest + callerPackages: + - equal: actionpack + - equal: activesupport + - equal: solidus_core + calleePackage: + include: app/controllers + - rule: missingContentType + - rule: missingAuthentication + exclude: + - event: + property: route + test: + include: /api/ + - rule: secretInLog + - rule: slowHttpServerRequest properties: timeAllowed: 1.0 - - id: slowQuery + - rule: slowQuery properties: timeAllowed: 0.25 - - id: tooManyUpdates - - id: updateInGetRequest + - rule: tooManyUpdates + - rule: updateInGetRequest exclude: - - pattern: INSERT INTO "spree_order_mutexes" + - event: + property: query + test: + include: INSERT INTO "spree_order_mutexes" diff --git a/src/types.d.ts b/src/types.d.ts index 6bd051f..e27a13e 100644 --- a/src/types.d.ts +++ b/src/types.d.ts @@ -72,7 +72,7 @@ type MatcherResult = * Matcher function is part of a rule. It's applied to an Event to determine whether there is a finding * on this event. If the Matcher returns true, a string, or a MatchResult[], then finding(s) are created. */ -type Matcher = (e: Event, appMap: AppMap, check: Check) => MatcherResult; +type Matcher = (e: Event, appMap: AppMap, eventFilter: EventFilter) => MatcherResult; /** * Finding is the full data structure that is created when a Rule matches an Event. @@ -83,12 +83,13 @@ type Matcher = (e: Event, appMap: AppMap, check: Check) => MatcherResult; interface Finding { appMapName: string; appMapFile?: string; - scannerId: string; - scannerTitle: string; + checkId: string; + ruleId: string; + ruleTitle: string; event: Event; hash: string; scope: Event; - message?: string; + message: string; groupMessage?: string; occurranceCount?: number; relatedEvents?: Event[]; diff --git a/test/util.ts b/test/util.ts index 3244c1d..6690325 100644 --- a/test/util.ts +++ b/test/util.ts @@ -1,12 +1,10 @@ import { AppMap, buildAppMap } from '@appland/models'; import { readFile } from 'fs/promises'; import { join } from 'path'; -import Assertion from '../src/assertion'; -import AssertionChecker from '../src/assertionChecker'; -import AssertionConfig from '../src/configuration/types/checkConfig'; -import MatchPatternConfig from '../src/configuration/types/matchPatternConfig'; -import { verbose } from '../src/scanner/util'; -import { AssertionPrototype, Finding, ScopeName } from '../src/types'; +import RuleChecker from '../src/ruleChecker'; +import { verbose } from '../src/rules/util'; +import { Finding } from '../src/types'; +import Check from '../src/check'; if (process.env.VERBOSE_SCAN === 'true') { verbose(true); @@ -25,26 +23,7 @@ const fixtureAppMap = async (file: string): Promise => { return buildAppMap(appMapBytes).normalize().build(); }; -const makePrototype = ( - id: string, - buildFn: () => Assertion, - enumerateScope: boolean | undefined, - scope: ScopeName | undefined, -): AssertionPrototype => { - const config = { id } as AssertionConfig; - return { - config: config, - enumerateScope: enumerateScope === true ? true : false, - scope: scope || 'root', - build: buildFn, - } as AssertionPrototype; -}; - -const scan = async ( - assertionPrototype: AssertionPrototype, - appmapFile?: string, - appmap?: AppMap -): Promise => { +const scan = async (check: Check, appmapFile?: string, appmap?: AppMap): Promise => { let appMapData: AppMap; if (appmapFile) { appMapData = await fixtureAppMap(appmapFile); @@ -53,7 +32,7 @@ const scan = async ( } const findings: Finding[] = []; - await new AssertionChecker().check(appMapData, assertionPrototype, findings); + await new RuleChecker().check(appMapData, check, findings); if (process.env.VERBOSE_TEST === 'true') { console.log(JSON.stringify(findings, null, 2)); @@ -62,4 +41,4 @@ const scan = async ( return findings; }; -export { fixtureAppMap, fixtureAppMapFileName, makePrototype, scan }; +export { fixtureAppMap, fixtureAppMapFileName, scan }; From 07b074a91e47ab8f6ba0971c1bdac9eda5bc756f Mon Sep 17 00:00:00 2001 From: Kevin Gilpin Date: Wed, 8 Dec 2021 10:54:32 -0500 Subject: [PATCH 5/7] feat: Port tests to new architecture --- src/check.ts | 8 +- src/checkInstance.ts | 2 +- src/report/generator.ts | 8 +- src/ruleChecker.ts | 9 +- src/rules/illegalPackageDependency.ts | 1 + src/rules/jobNotCancelled.ts | 1 + src/rules/slowFunctionCall.ts | 2 +- src/rules/slowHttpServerRequest.ts | 3 +- src/types.d.ts | 4 +- test/config.spec.ts | 37 +- test/scanner.spec.ts | 350 ------------------ test/scanner/authzBeforeAuthn.spec.ts | 40 ++ test/scanner/circularDependency.spec.ts | 34 +- test/scanner/http500.spec.ts | 14 + test/scanner/illegalPackageDependency.spec.ts | 24 ++ .../incompatibleHTTPClientRequest.spec.ts | 23 ++ test/scanner/insecureCompare.spec.ts | 14 + test/scanner/jobNotCancelled.spec.ts | 15 +- test/scanner/missingAuthentication.spec.ts | 13 + test/scanner/nPlusOneQuery.spec.ts | 20 + test/scanner/rpcWithoutCircuitBreaker.spec.ts | 22 ++ test/scanner/secretInLog.spec.ts | 25 ++ test/scanner/slowFunctionCall.spec.ts | 20 + test/scanner/slowHttpServerRequest.spec.ts | 20 + test/scanner/tooManyJoins.spec.ts | 16 + test/scanner/tooManyUpdates.spec.ts | 23 ++ .../unbatchedMaterializedQuery.spec.ts | 15 + 27 files changed, 351 insertions(+), 412 deletions(-) delete mode 100644 test/scanner.spec.ts create mode 100644 test/scanner/authzBeforeAuthn.spec.ts create mode 100644 test/scanner/http500.spec.ts create mode 100644 test/scanner/illegalPackageDependency.spec.ts create mode 100644 test/scanner/incompatibleHTTPClientRequest.spec.ts create mode 100644 test/scanner/insecureCompare.spec.ts create mode 100644 test/scanner/missingAuthentication.spec.ts create mode 100644 test/scanner/nPlusOneQuery.spec.ts create mode 100644 test/scanner/rpcWithoutCircuitBreaker.spec.ts create mode 100644 test/scanner/secretInLog.spec.ts create mode 100644 test/scanner/slowFunctionCall.spec.ts create mode 100644 test/scanner/slowHttpServerRequest.spec.ts create mode 100644 test/scanner/tooManyJoins.spec.ts create mode 100644 test/scanner/tooManyUpdates.spec.ts create mode 100644 test/scanner/unbatchedMaterializedQuery.spec.ts diff --git a/src/check.ts b/src/check.ts index f702764..e5a5101 100644 --- a/src/check.ts +++ b/src/check.ts @@ -4,14 +4,20 @@ import { EventFilter, Rule, ScopeName } from './types'; export default class Check { public id: string; + public options: Record; public scope: ScopeName; public includeScope: EventFilter[]; public excludeScope: EventFilter[]; public includeEvent: EventFilter[]; public excludeEvent: EventFilter[]; - constructor(public rule: Rule, public options: Record) { + constructor(public rule: Rule, options?: Record) { + function makeOptions() { + return rule.Options ? new rule.Options() : {}; + } + this.id = rule.id; + this.options = options || makeOptions(); this.scope = rule.scope || 'root'; this.includeScope = []; this.excludeScope = []; diff --git a/src/checkInstance.ts b/src/checkInstance.ts index 8fe574e..4d040fb 100644 --- a/src/checkInstance.ts +++ b/src/checkInstance.ts @@ -29,7 +29,7 @@ export default class CheckInstance { } get enumerateScope(): boolean { - return this.check.rule.enumerateScope === undefined ? true : this.check.rule.enumerateScope; + return this.check.rule.enumerateScope; } filterEvent(event: Event, appMap?: AppMap): boolean { diff --git a/src/report/generator.ts b/src/report/generator.ts index d8de60b..65b01e7 100644 --- a/src/report/generator.ts +++ b/src/report/generator.ts @@ -35,14 +35,14 @@ export default class Generator { } findings.forEach((match) => { - let findingSummary = scannerSummary.findingSummary[match.scannerId]; + let findingSummary = scannerSummary.findingSummary[match.ruleId]; if (!findingSummary) { findingSummary = { - scannerTitle: match.scannerTitle, + scannerTitle: match.ruleTitle, findingTotal: 0, messages: new Set(), } as FindingSummary; - scannerSummary.findingSummary[match.scannerId] = findingSummary; + scannerSummary.findingSummary[match.ruleId] = findingSummary; } findingSummary.findingTotal += 1; if (match.message) { @@ -62,7 +62,7 @@ export default class Generator { const message = match.message; this.writeln(this.reportFile ? message : chalk.magenta(message)); this.writeln(`\tLink:\t${this.reportFile ? filePath : chalk.blue(filePath)}`); - this.writeln(`\tRule:\t${match.scannerId}`); + this.writeln(`\tRule:\t${match.ruleId}`); this.writeln(`\tAppMap name:\t${match.appMapName}`); this.writeln(eventMsg); this.writeln(`\tScope:\t${match.scope.id} - ${match.scope.toString()}`); diff --git a/src/ruleChecker.ts b/src/ruleChecker.ts index 75d783f..53e3254 100644 --- a/src/ruleChecker.ts +++ b/src/ruleChecker.ts @@ -110,7 +110,14 @@ export default class RuleChecker { ); const numFindings = findings.length; if (matchResult === true) { - findings.push(buildFinding(event)); + let finding; + if (checkInstance.ruleLogic.message) { + const message = checkInstance.ruleLogic.message(scope, event); + finding = buildFinding(event, message); + } else { + finding = buildFinding(event); + } + findings.push(finding); } else if (typeof matchResult === 'string') { const finding = buildFinding(event, matchResult as string); finding.message = matchResult as string; diff --git a/src/rules/illegalPackageDependency.ts b/src/rules/illegalPackageDependency.ts index ddcb302..446d4ff 100644 --- a/src/rules/illegalPackageDependency.ts +++ b/src/rules/illegalPackageDependency.ts @@ -42,5 +42,6 @@ export default { title: 'Illegal use of code by a non-whitelisted package', scope: 'command' as ScopeName, enumerateScope: true, + Options, build, } as Rule; diff --git a/src/rules/jobNotCancelled.ts b/src/rules/jobNotCancelled.ts index d25c47a..28be6f5 100644 --- a/src/rules/jobNotCancelled.ts +++ b/src/rules/jobNotCancelled.ts @@ -39,6 +39,7 @@ export default { id: 'job-not-cancelled', title: 'Job created in a rolled back transaction and not cancelled', scope: 'transaction', + enumerateScope: false, labels: [Labels.JobCreate, Labels.JobCancel], build, } as Rule; diff --git a/src/rules/slowFunctionCall.ts b/src/rules/slowFunctionCall.ts index 0d27ba1..e22c0cd 100644 --- a/src/rules/slowFunctionCall.ts +++ b/src/rules/slowFunctionCall.ts @@ -30,7 +30,7 @@ function build(options: Options): RuleLogic { export default { id: 'slow-function-call', title: 'Slow function call', - scope: 'root' as ScopeName, + scope: 'root', enumerateScope: true, Options, build, diff --git a/src/rules/slowHttpServerRequest.ts b/src/rules/slowHttpServerRequest.ts index 7d95aa8..db98dd1 100644 --- a/src/rules/slowHttpServerRequest.ts +++ b/src/rules/slowHttpServerRequest.ts @@ -8,13 +8,14 @@ class Options implements types.SlowHTTPServerRequest.Options { function build(options: Options): RuleLogic { return { matcher: (e) => e.elapsedTime! > options.timeAllowed, + message: () => `Slow HTTP server request (> ${options.timeAllowed * 1000}ms)`, where: (e) => !!e.httpServerRequest && e.elapsedTime !== undefined, }; } export default { id: 'slow-http-server-request', - title: 'Slow HTTP server requests', + title: 'Slow HTTP server request', scope: 'http_server_request', enumerateScope: false, Options, diff --git a/src/types.d.ts b/src/types.d.ts index e27a13e..56e144e 100644 --- a/src/types.d.ts +++ b/src/types.d.ts @@ -101,6 +101,8 @@ interface RuleLogic { // When specified by the rule, only events which pass the where filter // will be passed to the matcher. where?: EventFilter; + // When specified by the rule, provides a detailed message for a finding on a specific event. + message?: (scope: Event, event: Event) => string; } interface Rule { @@ -115,7 +117,7 @@ interface Rule { scope?: ScopeName; // Whether to pass all the events in the scope to the matcher. If false, only the scope event // is passed to the matcher, and the rule should traverse the scope as needed. - enumerateScope?: boolean; + enumerateScope: boolean; // User-defined options for the rule. Options?: any; // Function to instantiate the rule logic from configured options. diff --git a/test/config.spec.ts b/test/config.spec.ts index 8343c52..3cc64e0 100644 --- a/test/config.spec.ts +++ b/test/config.spec.ts @@ -1,28 +1,27 @@ import { load } from 'js-yaml'; +import Check from '../src/check'; import ConfigurationProvider from '../src/configuration/configurationProvider'; import Configuration from '../src/configuration/types/configuration'; -import { AssertionPrototype } from '../src/types'; describe('YAML config test', () => { it('propagates property settings', async () => { const yamlConfig = ` - scanners: - - id: slowHttpServerRequest + checks: + - rule: slowHttpServerRequest properties: timeAllowed: 0.251 `; const configObj = load(yamlConfig) as Configuration; const provider = new ConfigurationProvider(configObj); - const assertionPrototypes: readonly AssertionPrototype[] = await provider.getConfig(); - expect(assertionPrototypes).toHaveLength(1); - const assertion = assertionPrototypes[0].build(); - expect(assertion.description).toEqual(`Slow HTTP server request (> 251ms)`); + const checks: readonly Check[] = await provider.getConfig(); + expect(checks).toHaveLength(1); + expect(checks[0].rule.title).toEqual(`Slow HTTP server request`); }); it('loads event filter', async () => { const yamlConfig = ` - scanners: - - id: missingAuthentication + checks: + - rule: missingAuthentication include: - scope: property: route @@ -31,28 +30,24 @@ describe('YAML config test', () => { `; const configObj = load(yamlConfig) as Configuration; const provider = new ConfigurationProvider(configObj); - const assertionPrototypes: readonly AssertionPrototype[] = await provider.getConfig(); - expect(assertionPrototypes).toHaveLength(1); - expect(assertionPrototypes[0].config.include!).toHaveLength(1); - expect(assertionPrototypes[0].config.include![0].scope!.test.include!).toEqual('GET'); - const assertion = assertionPrototypes[0].build(); - expect(assertion.includeScope).toHaveLength(1); + const checks: readonly Check[] = await provider.getConfig(); + expect(checks).toHaveLength(1); + expect(checks[0].includeScope!).toHaveLength(1); }); it('propagates Record properties', async () => { const yamlConfig = ` - scanners: - - id: incompatibleHttpClientRequest + checks: + - rule: incompatibleHttpClientRequest properties: schemata: api.railsSampleApp.com: file:///railsSampleApp.openapiv3.yaml `; const configObj = load(yamlConfig) as Configuration; const provider = new ConfigurationProvider(configObj); - const assertionPrototypes: readonly AssertionPrototype[] = await provider.getConfig(); - expect(assertionPrototypes).toHaveLength(1); - const assertion = assertionPrototypes[0].build(); - expect(assertion.options.schemata).toEqual({ + const checks: readonly Check[] = await provider.getConfig(); + expect(checks).toHaveLength(1); + expect(checks[0].options.schemata).toEqual({ 'api.railsSampleApp.com': 'file:///railsSampleApp.openapiv3.yaml', }); }); diff --git a/test/scanner.spec.ts b/test/scanner.spec.ts deleted file mode 100644 index 3fd4dcf..0000000 --- a/test/scanner.spec.ts +++ /dev/null @@ -1,350 +0,0 @@ -import { buildAppMap, Event } from '@appland/models'; -import circularDependency from '../src/scanner/circularDependency'; -import http500 from '../src/scanner/http500'; -import insecureCompare from '../src/scanner/insecureCompare'; -import missingAuthentication from '../src/scanner/missingAuthentication'; -import slowHttpServerRequest from '../src/scanner/slowHttpServerRequest'; -import nPlusOneQuery from '../src/scanner/nPlusOneQuery'; -import secretInLog from '../src/scanner/secretInLog'; -import tooManyUpdates from '../src/scanner/tooManyUpdates'; -import illegalPackageAccess from '../src/scanner/illegalPackageDependency'; -import rpcWithoutCircuitBreaker from '../src/scanner/rpcWithoutCircuitBreaker'; -import incompatibleHTTPClientRequest from '../src/scanner/incompatibleHttpClientRequest'; -import slowFunctionCall from '../src/scanner/slowFunctionCall'; -import tooManyJoins from '../src/scanner/tooManyJoins'; -import authzBeforeAuthn from '../src/scanner/authzBeforeAuthn'; -import unbatchedMaterializedQuery from '../src/scanner/unbatchedMaterializedQuery'; -import { fixtureAppMapFileName, makePrototype, scan } from './util'; -import { ScopeName } from '../src/types'; -import { cwd } from 'process'; -import { join, normalize } from 'path'; -import { readFile } from 'fs/promises'; -import { verbose } from '../src/scanner/util'; -import MatchPatternConfig from '../src/configuration/types/matchPatternConfig'; -import Assertion from '../src/assertion'; -import { inspect } from 'util'; - -if (process.env.VERBOSE === 'true') { - verbose(true); -} - -describe('scanner', () => { - it('slow HTTP server request', async () => { - const { scope, enumerateScope, scanner, Options } = slowHttpServerRequest; - const options = new Options(); - options.timeAllowed = 0.5; - const findings = await scan( - makePrototype( - 'slow-http-server-request', - () => scanner(options), - enumerateScope, - scope as ScopeName - ), - 'Password_resets_password_resets.appmap.json' - ); - expect(findings).toHaveLength(1); - const finding = findings[0]; - expect(finding.appMapName).toEqual('Password_resets password resets'); - expect(finding.scannerId).toEqual('slow-http-server-request'); - expect(finding.event.id).toEqual(411); - expect(finding.message).toBeUndefined(); - expect(finding.condition).toEqual('Slow HTTP server request (> 500ms)'); - expect(finding.hash).toEqual( - 'a96c5258fc9f590493abcc9342c368e5b2262e95b7d38deded1dcbf97a2126d0' - ); - }); - - it('missing authentication', async () => { - const { scope, enumerateScope, scanner } = missingAuthentication; - const findings = await scan( - makePrototype('secret-in-log', () => scanner(), enumerateScope, scope as ScopeName), - 'Users_profile_profile_display_while_anonyomus.appmap.json' - ); - - expect(findings).toHaveLength(1); - const finding = findings[0]; - expect(finding.scannerId).toEqual('missing-authentication'); - expect(finding.event.id).toEqual(31); - expect(finding.message).toBeUndefined(); - }); - - it('secret in log file', async () => { - const { enumerateScope, scanner, scope } = secretInLog; - const findings = await scan( - makePrototype('secret-in-log', () => scanner(), enumerateScope, scope as ScopeName), - 'Users_signup_valid_signup_information_with_account_activation.appmap.json' - ); - expect(findings).toHaveLength(2); - { - const finding = findings[0]; - expect(finding.scannerId).toEqual('secret-in-log'); - expect(finding.event.id).toEqual(695); - expect(finding.message).toEqual( - `[2f025606-b6f0-4b64-8595-006f32f4d5d0] Started GET "/account_activations/-6SputWUtvALn3TLCfoYvA/edit contains secret -6SputWUtvALn3TLCfoYvA` - ); - } - { - const finding = findings[1]; - expect(finding.scannerId).toEqual('secret-in-log'); - expect(finding.event.id).toEqual(817); - } - }); - - it('n+1 query', async () => { - const { scope, enumerateScope, scanner, Options } = nPlusOneQuery; - const findings = await scan( - makePrototype( - 'n-plus-one-query', - () => scanner(new Options()), - enumerateScope, - scope as ScopeName - ), - 'Users_profile_profile_display_while_anonyomus.appmap.json' - ); - - expect(findings).toHaveLength(1); - // It's important that there is only one finding, since the query repeats 30 times. - // That's one finding; not 30 findings. - const finding1 = findings[0]; - expect(finding1.appMapName).toEqual('Users_profile profile display while anonyomus'); - expect(finding1.scannerId).toEqual('n-plus-one-query'); - expect(finding1.event.id).toEqual(133); - expect(finding1.relatedEvents!).toHaveLength(30); - expect(finding1.message).toEqual( - `30 occurrences of SQL: SELECT "active_storage_attachments".* FROM "active_storage_attachments" WHERE "active_storage_attachments"."record_id" = ? AND "active_storage_attachments"."record_type" = ? AND "active_storage_attachments"."name" = ? LIMIT ?` - ); - }); - - it('too many updates', async () => { - const { scope, enumerateScope, Options, scanner } = tooManyUpdates; - const options = new Options(); - options.warningLimit = 2; - const findings = await scan( - makePrototype('too-many-updates', () => scanner(options), enumerateScope, scope as ScopeName), - 'PaymentsController_create_no_user_email_on_file_makes_a_onetime_payment_with_no_user_but_associate_with_stripe.appmap.json' - ); - expect(findings).toHaveLength(1); - // It's important that there is only one finding, since the query repeats 30 times. - // That's one finding; not 30 findings. - const finding1 = findings[0]; - expect(finding1.appMapName).toEqual( - 'PaymentsController create no user email on file makes a onetime payment with no user, but associate with stripe' - ); - expect(finding1.scannerId).toEqual('too-many-updates'); - expect(finding1.event.id).toEqual(89); - expect(finding1.message).toEqual(`Command performs 3 SQL and RPC updates`); - expect(finding1.relatedEvents!).toHaveLength(3); - }); - - it('insecure comparison', async () => { - const { scanner, scope, enumerateScope } = insecureCompare; - const findings = await scan( - makePrototype('insecure-compare', () => scanner(), enumerateScope, scope as ScopeName), - 'Password_resets_password_resets_with_insecure_compare.appmap.json' - ); - expect(findings).toHaveLength(1); - const finding = findings[0]; - expect(finding.scannerId).toEqual('insecure-compare'); - expect(finding.event.id).toEqual(1094); - }); - - it('http 500', async () => { - const { scope, enumerateScope, scanner } = http500; - const findings = await scan( - makePrototype('http-500', () => scanner(), enumerateScope, scope as ScopeName), - 'Password_resets_password_resets_with_http500.appmap.json' - ); - expect(findings).toHaveLength(1); - const finding = findings[0]; - expect(finding.scannerId).toEqual('http-5xx'); - expect(finding.event.id).toEqual(1789); - }); - - it('illegal package dependency', async () => { - const { scanner, scope, enumerateScope, Options } = illegalPackageAccess; - const options = new Options(); - options.callerPackages = [ - { equal: 'lib/pkg_a' } as MatchPatternConfig, - { equal: 'lib/command' } as MatchPatternConfig, - ]; - options.calleePackages = [{ equal: 'lib/pkg_b' } as MatchPatternConfig]; - const findings = await scan( - makePrototype( - 'illegal-package-dependency', - () => scanner(options), - enumerateScope, - scope as ScopeName - ), - 'ruby/circular_dependency/tmp/appmap/minitest/Command_command.appmap.json' - ); - expect(findings).toHaveLength(1); - const finding = findings[0]; - expect(finding.scannerId).toEqual('illegal-package-dependency'); - expect(finding.event.id).toEqual(4); - expect(finding.message).toEqual( - `Code object lib/pkg_a/PkgA::A#cycle was invoked from lib/pkg_b, not from lib/pkg_a or lib/command` - ); - }); - - it('rpc without circuit breaker creates a finding', async () => { - const { scanner, enumerateScope, scope } = rpcWithoutCircuitBreaker; - const findings = await scan( - makePrototype( - 'rpc-without-circuit-breaker', - () => scanner(), - enumerateScope, - scope as ScopeName - ), - 'PaymentsController_create_no_user_email_on_file_makes_a_onetime_payment_with_no_user_but_associate_with_stripe.appmap.json' - ); - expect(findings).toHaveLength(4); - const finding = findings[0]; - expect(finding.scannerId).toEqual('rpc-without-circuit-breaker'); - expect(finding.event.id).toEqual(19); - }); - - it('all rpc have a circuit breaker ', async () => { - const { scanner, scope, enumerateScope } = rpcWithoutCircuitBreaker; - const findings = await scan( - makePrototype( - 'rpc-without-circuit-breaker', - () => scanner(), - enumerateScope, - scope as ScopeName - ), - 'Test_net_5xxs_trip_circuit_when_fatal_server_flag_enabled.appmap.json' - ); - expect(findings).toHaveLength(0); - }); - - it('slow function call', async () => { - const { scanner, scope, enumerateScope, Options } = slowFunctionCall; - const options = new Options(); - options.timeAllowed = 0.2; - const findings = await scan( - makePrototype( - 'slow-function-call', - () => { - const result: Assertion = scanner(options); - const pattern = new RegExp(/Controller#create$/); - result.includeEvent = [(event: Event) => pattern.test(event.codeObject.fqid)]; - return result; - }, - enumerateScope, - scope - ), - 'Microposts_interface_micropost_interface.appmap.json' - ); - expect(findings).toHaveLength(1); - const finding = findings[0]; - expect(finding.scannerId).toEqual('slow-function-call'); - expect(finding.event.id).toEqual(897); - expect(finding.message).toEqual( - 'Slow app/controllers/MicropostsController#create call (0.228481ms)' - ); - }); - - it('incompatible http client requests', async () => { - const railsSampleAppSchemaURL = `file://${normalize( - join(cwd(), 'test', 'fixtures', 'schemata', 'railsSampleApp6thEd.openapiv3.yaml') - )}`; - const { scanner, scope, enumerateScope, Options } = incompatibleHTTPClientRequest; - const options = new Options(); - options.schemata = { 'api.stripe.com': railsSampleAppSchemaURL }; - const findings = await scan( - makePrototype( - 'incompatible-http-client-request', - () => scanner(options), - enumerateScope, - scope as ScopeName - ), - 'PaymentsController_create_no_user_email_on_file_makes_a_onetime_payment_with_no_user_but_associate_with_stripe.appmap.json' - ); - expect(findings).toHaveLength(4); - const finding = findings[0]; - expect(finding.event.id).toEqual(19); - expect(finding.message).toEqual( - `HTTP client request is incompatible with OpenAPI schema. Change details: remove paths./v1/tokens` - ); - }); - - it('too many joins', async () => { - const { scope, enumerateScope, scanner, Options } = tooManyJoins; - const options = new Options(); - options.warningLimit = 1; - const findings = await scan( - makePrototype('too-many-joins', () => scanner(options), enumerateScope, scope as ScopeName), - 'Users_profile_profile_display_while_anonyomus.appmap.json' - ); - expect(findings).toHaveLength(2); - const finding = findings[0]; - expect(finding.scannerId).toEqual('too-many-joins'); - expect(finding.event.id).toEqual(97); - expect(finding.message).toEqual( - '1 join in SQL "SELECT COUNT(*) FROM "users" INNER JOIN "relationships" ON "users"."id" = "relationships"."followed_id" WHERE "relationships"."follower_id" = ?"' - ); - }); - - it('detects authorization before authentication', async () => { - const { scope, enumerateScope, scanner } = authzBeforeAuthn; - const findings = await scan( - makePrototype('authz-before-authn', () => scanner(), enumerateScope, scope as ScopeName), - 'Test_authz_before_authn.appmap.json' - ); - expect(findings).toHaveLength(0); - }); - - it('detects authorization before authentication', async () => { - const { scope, enumerateScope, scanner } = authzBeforeAuthn; - - // Remove security.authentication labels from the AppMap in order to - // trigger the finding. - const appMapBytes = await readFile( - fixtureAppMapFileName('Test_authz_before_authn.appmap.json'), - 'utf8' - ); - const baseAppMap = JSON.parse(appMapBytes); - const removeAuthenticationLabel = (codeObject: any) => { - if (codeObject.labels) { - codeObject.labels = codeObject.labels.filter( - (label: string) => label !== 'security.authentication' - ); - } - (codeObject.children || []).forEach(removeAuthenticationLabel); - }; - baseAppMap.classMap.forEach(removeAuthenticationLabel); - const appMapData = buildAppMap(JSON.stringify(baseAppMap)).normalize().build(); - - const findings = await scan( - makePrototype('authz-before-authn', () => scanner(), enumerateScope, scope as ScopeName), - undefined, - appMapData - ); - expect(findings).toHaveLength(1); - const finding = findings[0]; - expect(finding.scannerId).toEqual('authz-before-authn'); - expect(finding.event.id).toEqual(1); - expect(finding.message).toEqual( - `MicropostsController#correct_user provides authorization, but the request is not authenticated` - ); - expect(finding.relatedEvents!.map((e) => e.id)).toEqual([16]); - }); - - it('unbatched materialized query', async () => { - const { scope, enumerateScope, scanner } = unbatchedMaterializedQuery; - const findings = await scan( - makePrototype( - 'unbatched-materialized-query', - () => scanner(), - enumerateScope, - scope as ScopeName - ), - 'Users_index_index_as_admin_including_pagination_and_delete_links.appmap.json' - ); - expect(findings.map((finding) => finding.scope.id)).toEqual([1589]); - expect(findings.map((finding) => finding.event.id).sort()).toEqual([1689]); - expect(findings.map((finding) => finding.event.sqlQuery!)).toEqual([ - 'SELECT "microposts".* FROM "microposts" WHERE "microposts"."user_id" = ? ORDER BY "microposts"."created_at" DESC', - ]); - }); -}); diff --git a/test/scanner/authzBeforeAuthn.spec.ts b/test/scanner/authzBeforeAuthn.spec.ts new file mode 100644 index 0000000..f11c54e --- /dev/null +++ b/test/scanner/authzBeforeAuthn.spec.ts @@ -0,0 +1,40 @@ +import { buildAppMap } from '@appland/models'; +import { readFile } from 'fs/promises'; +import Check from '../../src/check'; +import rule from '../../src/rules/authzBeforeAuthn'; +import { fixtureAppMapFileName, scan } from '../util'; + +it('authentication precedes authentication', async () => { + const findings = await scan(new Check(rule), 'Test_authz_before_authn.appmap.json'); + expect(findings).toHaveLength(0); +}); + +it('authorization before (or without) authentication', async () => { + // Remove security.authentication labels from the AppMap in order to + // trigger the finding. + const appMapBytes = await readFile( + fixtureAppMapFileName('Test_authz_before_authn.appmap.json'), + 'utf8' + ); + const baseAppMap = JSON.parse(appMapBytes); + const removeAuthenticationLabel = (codeObject: any) => { + if (codeObject.labels) { + codeObject.labels = codeObject.labels.filter( + (label: string) => label !== 'security.authentication' + ); + } + (codeObject.children || []).forEach(removeAuthenticationLabel); + }; + baseAppMap.classMap.forEach(removeAuthenticationLabel); + const appMapData = buildAppMap(JSON.stringify(baseAppMap)).normalize().build(); + + const findings = await scan(new Check(rule), undefined, appMapData); + expect(findings).toHaveLength(1); + const finding = findings[0]; + expect(finding.ruleId).toEqual('authz-before-authn'); + expect(finding.event.id).toEqual(1); + expect(finding.message).toEqual( + `MicropostsController#correct_user provides authorization, but the request is not authenticated` + ); + expect(finding.relatedEvents!.map((e) => e.id)).toEqual([16]); +}); diff --git a/test/scanner/circularDependency.spec.ts b/test/scanner/circularDependency.spec.ts index 4087dc4..df164cd 100644 --- a/test/scanner/circularDependency.spec.ts +++ b/test/scanner/circularDependency.spec.ts @@ -1,30 +1,21 @@ -import circularDependency from '../../src/scanner/circularDependency'; -import type { ScopeName } from '../../src/types'; -import { makePrototype, scan } from '../util'; +import Check from '../../src/check'; +import rule from '../../src/rules/circularDependency'; +import { scan } from '../util'; -const { scope, enumerateScope, scanner, Options } = circularDependency; -const options = new Options(); const appMapFileName = 'ruby/circular_dependency/tmp/appmap/minitest/Command_command.appmap.json'; -const detectCycles = async () => { - return scan( - makePrototype( - 'circular-dependency', - () => scanner(options), - enumerateScope, - scope as ScopeName - ), - appMapFileName - ); +const detectCycles = async (check: Check) => { + return scan(check, appMapFileName); }; -describe('circular dependency scanner', () => { +describe('circular dependency', () => { it('finds a cycle', async () => { - options.depth = 3; - const findings = await detectCycles(); + const check = new Check(rule); + check.options.depth = 3; + const findings = await detectCycles(check); expect(findings).toHaveLength(1); const finding = findings[0]; - expect(finding.scannerId).toEqual('circular-dependency'); + expect(finding.ruleId).toEqual('circular-dependency'); expect(finding.event.id).toEqual(2); expect(finding.message).toEqual( `Cycle in package dependency graph: lib/pkg_a -> lib/pkg_b -> lib/pkg_a` @@ -33,8 +24,9 @@ describe('circular dependency scanner', () => { }); it('ignores cycles below the threshold length', async () => { - options.depth = 4; - const findings = await detectCycles(); + const check = new Check(rule); + check.options.depth = 4; + const findings = await detectCycles(check); expect(findings).toHaveLength(0); }); }); diff --git a/test/scanner/http500.spec.ts b/test/scanner/http500.spec.ts new file mode 100644 index 0000000..2d57da6 --- /dev/null +++ b/test/scanner/http500.spec.ts @@ -0,0 +1,14 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/http500'; +import { scan } from '../util'; + +it('http 500', async () => { + const findings = await scan( + new Check(rule), + 'Password_resets_password_resets_with_http500.appmap.json' + ); + expect(findings).toHaveLength(1); + const finding = findings[0]; + expect(finding.ruleId).toEqual('http-5xx'); + expect(finding.event.id).toEqual(1789); +}); diff --git a/test/scanner/illegalPackageDependency.spec.ts b/test/scanner/illegalPackageDependency.spec.ts new file mode 100644 index 0000000..d9cbe44 --- /dev/null +++ b/test/scanner/illegalPackageDependency.spec.ts @@ -0,0 +1,24 @@ +import Check from '../../src/check'; +import MatchPatternConfig from '../../src/configuration/types/matchPatternConfig'; +import rule from '../../src/rules/illegalPackageDependency'; +import { scan } from '../util'; + +it('illegal package dependency', async () => { + const options = new rule.Options(); + options.callerPackages = [ + { equal: 'lib/pkg_a' } as MatchPatternConfig, + { equal: 'lib/command' } as MatchPatternConfig, + ]; + options.calleePackages = [{ equal: 'lib/pkg_b' } as MatchPatternConfig]; + const findings = await scan( + new Check(rule, options), + 'ruby/circular_dependency/tmp/appmap/minitest/Command_command.appmap.json' + ); + expect(findings).toHaveLength(1); + const finding = findings[0]; + expect(finding.ruleId).toEqual('illegal-package-dependency'); + expect(finding.event.id).toEqual(4); + expect(finding.message).toEqual( + `Code object lib/pkg_a/PkgA::A#cycle was invoked from lib/pkg_b, not from lib/pkg_a or lib/command` + ); +}); diff --git a/test/scanner/incompatibleHTTPClientRequest.spec.ts b/test/scanner/incompatibleHTTPClientRequest.spec.ts new file mode 100644 index 0000000..13265f5 --- /dev/null +++ b/test/scanner/incompatibleHTTPClientRequest.spec.ts @@ -0,0 +1,23 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/incompatibleHttpClientRequest'; +import { scan } from '../util'; +import { join, normalize } from 'path'; +import { cwd } from 'process'; + +it('incompatible http client requests', async () => { + const railsSampleAppSchemaURL = `file://${normalize( + join(cwd(), 'test', 'fixtures', 'schemata', 'railsSampleApp6thEd.openapiv3.yaml') + )}`; + const check = new Check(rule); + check.options.schemata = { 'api.stripe.com': railsSampleAppSchemaURL }; + const findings = await scan( + check, + 'PaymentsController_create_no_user_email_on_file_makes_a_onetime_payment_with_no_user_but_associate_with_stripe.appmap.json' + ); + expect(findings).toHaveLength(4); + const finding = findings[0]; + expect(finding.event.id).toEqual(19); + expect(finding.message).toEqual( + `HTTP client request is incompatible with OpenAPI schema. Change details: remove paths./v1/tokens` + ); +}); diff --git a/test/scanner/insecureCompare.spec.ts b/test/scanner/insecureCompare.spec.ts new file mode 100644 index 0000000..ff927ac --- /dev/null +++ b/test/scanner/insecureCompare.spec.ts @@ -0,0 +1,14 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/insecureCompare'; +import { scan } from '../util'; + +it('insecure compare', async () => { + const findings = await scan( + new Check(rule), + 'Password_resets_password_resets_with_insecure_compare.appmap.json' + ); + expect(findings).toHaveLength(1); + const finding = findings[0]; + expect(finding.ruleId).toEqual('insecure-compare'); + expect(finding.event.id).toEqual(1094); +}); diff --git a/test/scanner/jobNotCancelled.spec.ts b/test/scanner/jobNotCancelled.spec.ts index 5cd63ff..300a369 100644 --- a/test/scanner/jobNotCancelled.spec.ts +++ b/test/scanner/jobNotCancelled.spec.ts @@ -1,16 +1,11 @@ -import JobNotCancelled from '../../src/scanner/jobNotCancelled'; -import type { AssertionPrototype } from '../../src/types'; +import Check from '../../src/check'; +import rule from '../../src/rules/jobNotCancelled'; import { scan } from '../util'; -test('job not cancelled scanner', async () => { - const prototype: AssertionPrototype = { - config: { id: JobNotCancelled.scanner().id }, - build: JobNotCancelled.scanner, - enumerateScope: false, - scope: 'transaction', - }; +test('job not cancelled', async () => { + const check = new Check(rule); const findings = await scan( - prototype, + check, 'Microposts_interface_micropost_interface_with_job.appmap.json' ); expect(findings).toHaveLength(1); diff --git a/test/scanner/missingAuthentication.spec.ts b/test/scanner/missingAuthentication.spec.ts new file mode 100644 index 0000000..3e4a276 --- /dev/null +++ b/test/scanner/missingAuthentication.spec.ts @@ -0,0 +1,13 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/missingAuthentication'; +import { scan } from '../util'; + +it('missing authentication', async () => { + const check = new Check(rule); + const findings = await scan(check, 'Users_profile_profile_display_while_anonyomus.appmap.json'); + + expect(findings).toHaveLength(1); + const finding = findings[0]; + expect(finding.ruleId).toEqual('missing-authentication'); + expect(finding.event.id).toEqual(31); +}); diff --git a/test/scanner/nPlusOneQuery.spec.ts b/test/scanner/nPlusOneQuery.spec.ts new file mode 100644 index 0000000..2598598 --- /dev/null +++ b/test/scanner/nPlusOneQuery.spec.ts @@ -0,0 +1,20 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/nPlusOneQuery'; +import { scan } from '../util'; + +it('n+1 query', async () => { + const check = new Check(rule); + const findings = await scan(check, 'Users_profile_profile_display_while_anonyomus.appmap.json'); + + expect(findings).toHaveLength(1); + // It's important that there is only one finding, since the query repeats 30 times. + // That's one finding; not 30 findings. + const finding1 = findings[0]; + expect(finding1.appMapName).toEqual('Users_profile profile display while anonyomus'); + expect(finding1.ruleId).toEqual('n-plus-one-query'); + expect(finding1.event.id).toEqual(133); + expect(finding1.relatedEvents!).toHaveLength(30); + expect(finding1.message).toEqual( + `30 occurrences of SQL: SELECT "active_storage_attachments".* FROM "active_storage_attachments" WHERE "active_storage_attachments"."record_id" = ? AND "active_storage_attachments"."record_type" = ? AND "active_storage_attachments"."name" = ? LIMIT ?` + ); +}); diff --git a/test/scanner/rpcWithoutCircuitBreaker.spec.ts b/test/scanner/rpcWithoutCircuitBreaker.spec.ts new file mode 100644 index 0000000..6859170 --- /dev/null +++ b/test/scanner/rpcWithoutCircuitBreaker.spec.ts @@ -0,0 +1,22 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/rpcWithoutCircuitBreaker'; +import { scan } from '../util'; + +it('rpc without circuit breaker', async () => { + const findings = await scan( + new Check(rule), + 'PaymentsController_create_no_user_email_on_file_makes_a_onetime_payment_with_no_user_but_associate_with_stripe.appmap.json' + ); + expect(findings).toHaveLength(4); + const finding = findings[0]; + expect(finding.ruleId).toEqual('rpc-without-circuit-breaker'); + expect(finding.event.id).toEqual(19); +}); + +it('all rpc have a circuit breaker ', async () => { + const findings = await scan( + new Check(rule), + 'Test_net_5xxs_trip_circuit_when_fatal_server_flag_enabled.appmap.json' + ); + expect(findings).toHaveLength(0); +}); diff --git a/test/scanner/secretInLog.spec.ts b/test/scanner/secretInLog.spec.ts new file mode 100644 index 0000000..a98a3a8 --- /dev/null +++ b/test/scanner/secretInLog.spec.ts @@ -0,0 +1,25 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/secretInLog'; +import { scan } from '../util'; + +it('secret in log file', async () => { + const check = new Check(rule); + const findings = await scan( + check, + 'Users_signup_valid_signup_information_with_account_activation.appmap.json' + ); + expect(findings).toHaveLength(2); + { + const finding = findings[0]; + expect(finding.ruleId).toEqual('secret-in-log'); + expect(finding.event.id).toEqual(695); + expect(finding.message).toEqual( + `[2f025606-b6f0-4b64-8595-006f32f4d5d0] Started GET "/account_activations/-6SputWUtvALn3TLCfoYvA/edit contains secret -6SputWUtvALn3TLCfoYvA` + ); + } + { + const finding = findings[1]; + expect(finding.ruleId).toEqual('secret-in-log'); + expect(finding.event.id).toEqual(817); + } +}); diff --git a/test/scanner/slowFunctionCall.spec.ts b/test/scanner/slowFunctionCall.spec.ts new file mode 100644 index 0000000..5842fce --- /dev/null +++ b/test/scanner/slowFunctionCall.spec.ts @@ -0,0 +1,20 @@ +import { Event } from '@appland/models'; +import Check from '../../src/check'; +import rule from '../../src/rules/slowFunctionCall'; +import { scan } from '../util'; + +it('slow function call', async () => { + const check = new Check(rule); + check.options.timeAllowed = 0.2; + const pattern = new RegExp(/Controller#create$/); + check.includeEvent = [(event: Event) => pattern.test(event.codeObject.fqid)]; + + const findings = await scan(check, 'Microposts_interface_micropost_interface.appmap.json'); + expect(findings).toHaveLength(1); + const finding = findings[0]; + expect(finding.ruleId).toEqual('slow-function-call'); + expect(finding.event.id).toEqual(897); + expect(finding.message).toEqual( + 'Slow app/controllers/MicropostsController#create call (0.228481ms)' + ); +}); diff --git a/test/scanner/slowHttpServerRequest.spec.ts b/test/scanner/slowHttpServerRequest.spec.ts new file mode 100644 index 0000000..ca27927 --- /dev/null +++ b/test/scanner/slowHttpServerRequest.spec.ts @@ -0,0 +1,20 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/slowHttpServerRequest'; +import { scan } from '../util'; + +it('slow HTTP server request', async () => { + const options = new rule.Options(); + options.timeAllowed = 0.5; + const findings = await scan( + new Check(rule, options), + 'Password_resets_password_resets.appmap.json' + ); + expect(findings).toHaveLength(1); + const finding = findings[0]; + expect(finding.appMapName).toEqual('Password_resets password resets'); + expect(finding.ruleId).toEqual('slow-http-server-request'); + expect(finding.event.id).toEqual(411); + expect(finding.ruleTitle).toEqual('Slow HTTP server request'); + expect(finding.message).toEqual('Slow HTTP server request (> 500ms)'); + expect(finding.hash).toEqual('a96c5258fc9f590493abcc9342c368e5b2262e95b7d38deded1dcbf97a2126d0'); +}); diff --git a/test/scanner/tooManyJoins.spec.ts b/test/scanner/tooManyJoins.spec.ts new file mode 100644 index 0000000..d41d9e7 --- /dev/null +++ b/test/scanner/tooManyJoins.spec.ts @@ -0,0 +1,16 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/tooManyJoins'; +import { scan } from '../util'; + +it('too many joins', async () => { + const check = new Check(rule); + check.options.warningLimit = 1; + const findings = await scan(check, 'Users_profile_profile_display_while_anonyomus.appmap.json'); + expect(findings).toHaveLength(2); + const finding = findings[0]; + expect(finding.ruleId).toEqual('too-many-joins'); + expect(finding.event.id).toEqual(97); + expect(finding.message).toEqual( + '1 join in SQL "SELECT COUNT(*) FROM "users" INNER JOIN "relationships" ON "users"."id" = "relationships"."followed_id" WHERE "relationships"."follower_id" = ?"' + ); +}); diff --git a/test/scanner/tooManyUpdates.spec.ts b/test/scanner/tooManyUpdates.spec.ts new file mode 100644 index 0000000..067aabd --- /dev/null +++ b/test/scanner/tooManyUpdates.spec.ts @@ -0,0 +1,23 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/tooManyUpdates'; +import { scan } from '../util'; + +it('too many updates', async () => { + const options = new rule.Options(); + options.warningLimit = 2; + const findings = await scan( + new Check(rule, options), + 'PaymentsController_create_no_user_email_on_file_makes_a_onetime_payment_with_no_user_but_associate_with_stripe.appmap.json' + ); + expect(findings).toHaveLength(1); + // It's important that there is only one finding, since the query repeats 30 times. + // That's one finding; not 30 findings. + const finding1 = findings[0]; + expect(finding1.appMapName).toEqual( + 'PaymentsController create no user email on file makes a onetime payment with no user, but associate with stripe' + ); + expect(finding1.ruleId).toEqual('too-many-updates'); + expect(finding1.event.id).toEqual(89); + expect(finding1.message).toEqual(`Command performs 3 SQL and RPC updates`); + expect(finding1.relatedEvents!).toHaveLength(3); +}); diff --git a/test/scanner/unbatchedMaterializedQuery.spec.ts b/test/scanner/unbatchedMaterializedQuery.spec.ts new file mode 100644 index 0000000..c82c2fb --- /dev/null +++ b/test/scanner/unbatchedMaterializedQuery.spec.ts @@ -0,0 +1,15 @@ +import Check from '../../src/check'; +import rule from '../../src/rules/unbatchedMaterializedQuery'; +import { scan } from '../util'; + +it('unbatched materialized query', async () => { + const findings = await scan( + new Check(rule), + 'Users_index_index_as_admin_including_pagination_and_delete_links.appmap.json' + ); + expect(findings.map((finding) => finding.scope.id)).toEqual([1589]); + expect(findings.map((finding) => finding.event.id).sort()).toEqual([1689]); + expect(findings.map((finding) => finding.event.sqlQuery!)).toEqual([ + 'SELECT "microposts".* FROM "microposts" WHERE "microposts"."user_id" = ? ORDER BY "microposts"."created_at" DESC', + ]); +}); From 95de185c77e0e9ece142ed2e0d373ff1e7fb0e9a Mon Sep 17 00:00:00 2001 From: Kevin Gilpin Date: Wed, 8 Dec 2021 10:55:16 -0500 Subject: [PATCH 6/7] refactor: Remove src/scanner in favor of src/rules --- src/scanner/authzBeforeAuthn.ts | 56 ----- src/scanner/circularDependency.ts | 229 ------------------- src/scanner/http500.ts | 17 -- src/scanner/illegalPackageDependency.ts | 47 ---- src/scanner/incompatibleHttpClientRequest.ts | 59 ----- src/scanner/insecureCompare.ts | 65 ------ src/scanner/jobNotCancelled.ts | 44 ---- src/scanner/lib/hasParameterOrReceiver.ts | 9 - src/scanner/lib/matchEvent.ts | 34 --- src/scanner/lib/matchPattern.ts | 26 --- src/scanner/lib/rpcWithoutProtection.ts | 30 --- src/scanner/missingAuthentication.ts | 76 ------ src/scanner/missingContentType.ts | 24 -- src/scanner/nPlusOneQuery.ts | 68 ------ src/scanner/queryFromInvalidPackage.ts | 42 ---- src/scanner/queryFromView.ts | 22 -- src/scanner/rpcWithoutCircuitBreaker.ts | 34 --- src/scanner/saveWithoutValidation.ts | 34 --- src/scanner/secretInLog.ts | 76 ------ src/scanner/slowFunctionCall.ts | 42 ---- src/scanner/slowHttpServerRequest.ts | 27 --- src/scanner/slowQuery.ts | 22 -- src/scanner/tooManyJoins.ts | 64 ------ src/scanner/tooManyUpdates.ts | 70 ------ src/scanner/types.d.ts | 87 ------- src/scanner/unbatchedMaterializedQuery.ts | 75 ------ src/scanner/updateInGetRequest.ts | 70 ------ src/scanner/util.ts | 119 ---------- 28 files changed, 1568 deletions(-) delete mode 100644 src/scanner/authzBeforeAuthn.ts delete mode 100644 src/scanner/circularDependency.ts delete mode 100644 src/scanner/http500.ts delete mode 100644 src/scanner/illegalPackageDependency.ts delete mode 100644 src/scanner/incompatibleHttpClientRequest.ts delete mode 100644 src/scanner/insecureCompare.ts delete mode 100644 src/scanner/jobNotCancelled.ts delete mode 100644 src/scanner/lib/hasParameterOrReceiver.ts delete mode 100644 src/scanner/lib/matchEvent.ts delete mode 100644 src/scanner/lib/matchPattern.ts delete mode 100644 src/scanner/lib/rpcWithoutProtection.ts delete mode 100644 src/scanner/missingAuthentication.ts delete mode 100644 src/scanner/missingContentType.ts delete mode 100644 src/scanner/nPlusOneQuery.ts delete mode 100644 src/scanner/queryFromInvalidPackage.ts delete mode 100644 src/scanner/queryFromView.ts delete mode 100644 src/scanner/rpcWithoutCircuitBreaker.ts delete mode 100644 src/scanner/saveWithoutValidation.ts delete mode 100644 src/scanner/secretInLog.ts delete mode 100644 src/scanner/slowFunctionCall.ts delete mode 100644 src/scanner/slowHttpServerRequest.ts delete mode 100644 src/scanner/slowQuery.ts delete mode 100644 src/scanner/tooManyJoins.ts delete mode 100644 src/scanner/tooManyUpdates.ts delete mode 100644 src/scanner/types.d.ts delete mode 100644 src/scanner/unbatchedMaterializedQuery.ts delete mode 100644 src/scanner/updateInGetRequest.ts delete mode 100644 src/scanner/util.ts diff --git a/src/scanner/authzBeforeAuthn.ts b/src/scanner/authzBeforeAuthn.ts deleted file mode 100644 index 945ad2b..0000000 --- a/src/scanner/authzBeforeAuthn.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { Event, EventNavigator } from '@appland/models'; -import { AssertionSpec, MatchResult } from '../types.d'; -import Assertion from '../assertion'; -import { isTruthy, providesAuthentication } from './util'; - -function containsAuthentication(events: Generator) { - for (const iter of events) { - if (providesAuthentication(iter.event, SecurityAuthentication)) { - return true; - } - } - return false; -} - -function scanner(): Assertion { - return Assertion.assert( - 'authz-before-authn', - 'Authorization before authentication', - (rootEvent: Event): MatchResult[] | undefined => { - for (const event of new EventNavigator(rootEvent).descendants()) { - if (providesAuthentication(event.event, SecurityAuthentication)) { - return; - } - if (event.event.labels.has(SecurityAuthorization) && isTruthy(event.event.returnValue)) { - // If the authorization event has a successful authentication descendant, allow this as well. - if (containsAuthentication(event.descendants())) { - return; - } else { - return [ - { - level: 'error', - event: rootEvent, - message: `${event.event} provides authorization, but the request is not authenticated`, - relatedEvents: [event.event], - }, - ]; - } - } - } - }, - (assertion: Assertion): void => { - assertion.where = (e) => Boolean(e.httpServerRequest); - assertion.description = 'Authorization performed before authentication'; - } - ); -} - -const SecurityAuthentication = 'security.authentication'; -const SecurityAuthorization = 'security.authorization'; - -export default { - labels: [SecurityAuthorization, SecurityAuthentication], - scope: 'http_server_request', - enumerateScope: false, - scanner, -} as AssertionSpec; diff --git a/src/scanner/circularDependency.ts b/src/scanner/circularDependency.ts deleted file mode 100644 index 5a916d2..0000000 --- a/src/scanner/circularDependency.ts +++ /dev/null @@ -1,229 +0,0 @@ -import { Event } from '@appland/models'; -import Assertion from '../assertion'; -import { AssertionSpec, MatchResult, StringFilter } from '../types'; -import GraphEdge from '../algorithms/dataStructures/graph/GraphEdge'; -import GraphVertex from '../algorithms/dataStructures/graph/GraphVertex'; -import Graph from '../algorithms/dataStructures/graph/Graph'; -import detectDirectedCycle from '../algorithms/graph/detect-cycle'; -import { isAbsolute } from 'path'; -import * as types from './types'; -import { verbose } from './util'; -import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; -import { buildFilters } from './lib/matchPattern'; - -type PackageName = string; - -class Cycle { - constructor(public packages: PackageName[], public events: Map) {} -} - -function ignorePackage(event: Event, ignoredPackages: StringFilter[]): boolean { - const myPackage: string | null = event.codeObject.packageOf; - return ( - myPackage === '' || - ignoredPackages.some((filter) => filter(myPackage)) || - !event.codeObject.location || - isAbsolute(event.codeObject.location) - ); -} - -function detectCycles(root: Event, ignoredPackages: StringFilter[]): Cycle[] { - const graph = new Graph(true); - const vertices = new Map(); - const edges = new Set(); - const vertexEvents = new Map(); - - const makeVertex = (pkg: PackageName, event: Event): GraphVertex => { - let result = vertices.get(pkg); - if (!result) { - result = new GraphVertex(pkg); - vertices.set(pkg, result); - vertexEvents.set(pkg, [event]); - } else { - vertexEvents.get(pkg)!.push(event); - } - return result; - }; - - const collectEvent = ( - event: Event, - parentEvent: Event | null, - parentPackage: PackageName | null - ) => { - let myPackage: PackageName | null = event.codeObject.packageOf; - if (ignorePackage(event, ignoredPackages)) { - myPackage = null; - } - - if (myPackage) { - const vertex = makeVertex(myPackage, event); - if (parentPackage && parentPackage !== myPackage) { - const edge = new GraphEdge(vertices.get(parentPackage)!, vertex); - if (!edges.has(edge.getKey())) { - if (verbose()) { - console.warn(`New edge: ${parentPackage}/${parentEvent} -> ${myPackage}/${event}`); - } - edges.add(edge.getKey()); - graph.addEdge(edge); - } - } - parentPackage = myPackage; - } - event.children.forEach((child) => collectEvent(child, event, parentPackage)); - }; - - if (root.codeObject.packageOf !== '') { - makeVertex(root.codeObject.packageOf, root); - } - collectEvent(root, null, null); - - return detectDirectedCycle(graph).map((cycle) => { - return new Cycle( - cycle.map((vertex) => vertex.getKey()), - vertexEvents - ); - }); -} - -/** - * Given a list of package names which occur in a cycle, - * search the event tree to find a list of specific events whose sequence and package names match the cycle. - - * @returns Sequence of events whose package names match the cyclePath. - */ -const searchForCycle = (cycle: Cycle, ignoredPackages: StringFilter[]): Event[] | null => { - const traverseEvent = ( - event: Event, - recordEvent: boolean, - cyclePath: PackageName[], - cyclePathIndex = 0, - path: Event[] = [] - ): Event[] | null => { - if (recordEvent) { - if (verbose()) { - console.warn(`${Array(path.length).fill('').join(' ')}push: ${event}`); - } - path.push(event); - } else { - if (verbose()) { - console.warn(`${Array(path.length).fill('').join(' ')}traverse: ${event}`); - } - } - - if (cyclePathIndex === cyclePath.length - 1) { - if (verbose()) { - console.warn(`${Array(path.length).fill('').join(' ')}result: ${path}`); - } - return [...path]; - } - - const myPackage = event.codeObject.packageOf; - - if (verbose()) { - console.warn(event.children.map((child) => child.codeObject.fqid)); - } - - // Traverse children of ignored or same package - let result = event.children - .filter( - (child) => child.codeObject.packageOf === myPackage || ignorePackage(child, ignoredPackages) - ) - .map((child) => traverseEvent(child, false, cyclePath, cyclePathIndex, path)) - .filter(Boolean); - - // Traverse children of the next package in the graph - if (result.length === 0) { - result = event.children - .filter( - (child) => - child.codeObject.packageOf !== myPackage && - !ignorePackage(child, ignoredPackages) && - cyclePath[cyclePathIndex + 1] === child.codeObject.packageOf - ) - .map((child) => traverseEvent(child, true, cyclePath, cyclePathIndex + 1, path)) - .filter((path) => path); - } - - if (result.length > 0) { - return result[0]; - } else { - if (recordEvent) { - if (verbose()) { - console.warn( - `${Array(path.length - 1) - .fill('') - .join(' ')}pop` - ); - } - path.pop(); - } else { - if (verbose()) { - console.warn( - `${Array(path.length - 1) - .fill('') - .join(' ')}untraverse` - ); - } - } - return null; - } - }; - - // Look for a cycle starting at each package name. For each package name, consider the - // events that have that package. - for (let i = 0; i < cycle.packages.length; i++) { - const packageName = cycle.packages[i]; - const startEvents = cycle.events.get(packageName)!; - const cyclePath = []; - for (let k = 0; k < cycle.packages.length; k++) { - cyclePath[k] = cycle.packages[(i + k) % cycle.packages.length]; - } - cyclePath.push(packageName); - if (verbose()) { - console.warn(`Searching for event path for cycle ${cyclePath}`); - } - for (let j = 0; j < startEvents.length; j++) { - const startEvent = startEvents[j]; - const path = traverseEvent(startEvent, true, cyclePath); - if (path) { - return path; - } - } - } - return null; -}; - -class Options implements types.CircularDependency.Options { - public ignoredPackages: MatchPatternConfig[] = []; - public depth = 4; -} - -function scanner(options: Options): Assertion { - const ignoredPackages = buildFilters(options.ignoredPackages); - - return Assertion.assert( - 'circular-dependency', - 'Circular package dependency', - (event: Event): MatchResult[] => { - return detectCycles(event, ignoredPackages) - .filter((cycle) => cycle.packages.length + 1 >= options.depth) - .map((cycle) => searchForCycle(cycle, ignoredPackages)) - .filter((path) => path) - .map((path) => { - return { - event: path![0], - message: [ - 'Cycle in package dependency graph', - path!.map((event) => event.codeObject.packageOf).join(' -> '), - ].join(': '), - relatedEvents: path!, - } as MatchResult; - }); - }, - (assertion: Assertion): void => { - assertion.description = `Code package dependency graph should not have cycles`; - } - ); -} - -export default { scanner, scope: 'command', enumerateScope: false, Options } as AssertionSpec; diff --git a/src/scanner/http500.ts b/src/scanner/http500.ts deleted file mode 100644 index 7aca83c..0000000 --- a/src/scanner/http500.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { Event } from '@appland/models'; -import { AssertionSpec } from 'src/types'; -import Assertion from '../assertion'; - -function scanner(): Assertion { - return Assertion.assert( - 'http-5xx', - 'HTTP 5xx status code', - (e: Event) => e.httpServerResponse!.status >= 500 && e.httpServerResponse!.status < 600, - (assertion: Assertion): void => { - assertion.where = (e: Event) => !!e.httpServerResponse; - assertion.description = `HTTP server request returns 5xx status`; - } - ); -} - -export default { scope: 'http_server_request', enumerateScope: false, scanner } as AssertionSpec; diff --git a/src/scanner/illegalPackageDependency.ts b/src/scanner/illegalPackageDependency.ts deleted file mode 100644 index a61dd36..0000000 --- a/src/scanner/illegalPackageDependency.ts +++ /dev/null @@ -1,47 +0,0 @@ -import { Event } from '@appland/models'; -import Assertion from '../assertion'; -import * as types from './types'; -import { AssertionSpec } from 'src/types'; -import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; -import { buildFilter, buildFilters } from './lib/matchPattern'; - -class Options implements types.IllegalPackageDependency.Options { - public callerPackages: MatchPatternConfig[] = []; - public calleePackage: MatchPatternConfig = {} as MatchPatternConfig; -} - -function scanner(options: Options): Assertion { - const packageNamesStr = options.callerPackages - .map((config) => config.equal || config.include || config.match) - .map(String) - .join(' or '); - - const callerPatterns = buildFilters(options.callerPackages || []); - const calleePattern = buildFilter(options.calleePackage); - - return Assertion.assert( - 'illegal-package-dependency', - 'Illegal use of code by a non-whitelisted package', - (e: Event) => { - const parentPackage = e.parent!.codeObject.packageOf; - if ( - !( - e.codeObject.packageOf === parentPackage || - callerPatterns.some((pattern) => pattern(parentPackage)) - ) - ) { - return `Code object ${e.codeObject.id} was invoked from ${parentPackage}, not from ${packageNamesStr}`; - } - }, - (assertion: Assertion): void => { - assertion.where = (e: Event) => { - return ( - !!e.parent && !!e.parent!.codeObject.packageOf && calleePattern(e.codeObject.packageOf) - ); - }; - assertion.description = `Code object must be invoked from package ${packageNamesStr}`; - } - ); -} - -export default { scanner, enumerateScope: true, Options } as AssertionSpec; diff --git a/src/scanner/incompatibleHttpClientRequest.ts b/src/scanner/incompatibleHttpClientRequest.ts deleted file mode 100644 index 3200957..0000000 --- a/src/scanner/incompatibleHttpClientRequest.ts +++ /dev/null @@ -1,59 +0,0 @@ -import { Event } from '@appland/models'; -import { forClientRequest, forURL, breakingChanges } from '../openapi'; -import { AssertionSpec, MatchResult } from 'src/types'; -import * as types from './types'; -import Assertion from '../assertion'; -import OpenApiDiff from 'openapi-diff'; -import { OpenAPIV3 } from 'openapi-types'; - -class Options implements types.IncompatibleHttpClientRequest.Options { - public schemata: Record = {}; -} - -const changeMessage = (change: OpenApiDiff.DiffResult<'breaking'>): string => { - return `HTTP client request is incompatible with OpenAPI schema. Change details: ${ - change.action - } ${change.sourceSpecEntityDetails - .concat(change.destinationSpecEntityDetails) - .map((detail) => detail.location) - .join(', ')}`; -}; - -function scanner(options: Options): Assertion { - const checkCompatibility = async (event: Event): Promise => { - const clientFragment = forClientRequest(event); - const serverSchema = await forURL(event.httpClientRequest!.url!, options.schemata); - const clientSchema = { - openapi: '3.0.0', - info: { - title: 'Schema derived from client request', - version: serverSchema.info.version, // Indicate that it *should* be compatible. - }, - paths: clientFragment!.paths, - components: { securitySchemes: clientFragment!.securitySchemes }, - } as OpenAPIV3.Document; - const changes = await breakingChanges(clientSchema, serverSchema); - return changes.map((change: OpenApiDiff.DiffResult<'breaking'>) => ({ - level: 'error', - message: changeMessage(change), - })); - }; - - return Assertion.assert( - 'incompatible-http-client-request', - 'Incompatible HTTP client request', - (event: Event) => checkCompatibility(event), - (assertion: Assertion): void => { - assertion.options = options; - assertion.where = (e: Event) => !!e.httpClientRequest && !!e.httpClientRequest!.url; - assertion.description = `HTTP client request is incompatible with the API specification`; - } - ); -} - -export default { - scope: 'http_client_request', - enumerateScope: false, - Options, - scanner, -} as AssertionSpec; diff --git a/src/scanner/insecureCompare.ts b/src/scanner/insecureCompare.ts deleted file mode 100644 index 04f9f43..0000000 --- a/src/scanner/insecureCompare.ts +++ /dev/null @@ -1,65 +0,0 @@ -import { Event } from '@appland/models'; -import recordSecrets from '../analyzer/recordSecrets'; -import SecretsRegexes from '../analyzer/secretsRegexes'; -import Assertion from '../assertion'; -import { AssertionSpec } from '../types.d'; - -const BCRYPT_REGEXP = /^[$]2[abxy]?[$](?:0[4-9]|[12][0-9]|3[01])[$][./0-9a-zA-Z]{53}$/; - -const secrets: Set = new Set(); - -function stringEquals(e: Event): string | boolean | import('../types').MatchResult[] | undefined { - if (!e.parameters || !e.receiver || e.parameters!.length !== 1) { - return; - } - - const args = [e.receiver!.value, e.parameters![0].value]; - - function isBcrypt(str: string): boolean { - return BCRYPT_REGEXP.test(str); - } - - function isSecret(str: string): boolean { - if (secrets.has(str)) { - return true; - } - return !!Object.keys(SecretsRegexes).find( - (key): boolean => !!SecretsRegexes[key].find((re: RegExp): boolean => re.test(str)) - ); - } - - // BCrypted strings are safe to compare using equals() - if (args.every(isBcrypt)) { - return; - } - if (!args.every(isSecret)) { - return; - } - - return true; -} - -const scanner = function (): Assertion { - return Assertion.assert( - 'insecure-compare', - 'Insecure comparison of secrets', - (e: Event) => { - if (e.codeObject.labels.has(Secret)) { - recordSecrets(secrets, e); - } - if (e.parameters && e.codeObject.labels.has(StringEquals)) { - return stringEquals(e); - } - }, - (assertion: Assertion): void => { - assertion.where = (e: Event) => - e.isFunction && (e.codeObject.labels.has(StringEquals) || e.codeObject.labels.has(Secret)); - assertion.description = `Insecure comparison of secrets`; - } - ); -}; - -const Secret = 'secret'; -const StringEquals = 'string.equals'; - -export default { labels: [Secret, StringEquals], enumerateScope: true, scanner } as AssertionSpec; diff --git a/src/scanner/jobNotCancelled.ts b/src/scanner/jobNotCancelled.ts deleted file mode 100644 index 2ebb92a..0000000 --- a/src/scanner/jobNotCancelled.ts +++ /dev/null @@ -1,44 +0,0 @@ -import type { Event } from '@appland/models'; -import type { AssertionSpec, MatchResult } from '../types'; - -import Labels from '../wellKnownLabels'; -import Assertion from '../assertion'; -import { hasTransactionDetails } from '../scope/sqlTransactionScope'; - -function matcher(event: Event): MatchResult[] | undefined { - if (!hasTransactionDetails(event)) - throw new Error(`expected event ${event.id} to be a transaction`); - if (event.transaction.status === 'commit') return; - - const creationEvents = event.transaction.events.filter(({ labels }) => - labels.has(Labels.JobCreate) - ); - const cancellationEvents = event.transaction.events.filter(({ labels }) => - labels.has(Labels.JobCancel) - ); - const missing = creationEvents.length - cancellationEvents.length; - if (missing === 0) return; - - const result: MatchResult = { - level: 'error', - event: event, - message: `${missing} jobs created but not cancelled in this rolled back transaction`, - // if there's a mismatch and there are cancellations we can't tell - // for sure which creations they match, so return everything - relatedEvents: [...creationEvents, ...cancellationEvents], - }; - - return [result]; -} - -const assertion = Assertion.assert( - 'job-not-cancelled', - 'Job created in a rolled back transaction and not cancelled', - matcher -); - -export default { - scope: 'transaction', - labels: [Labels.JobCreate, Labels.JobCancel], - scanner: (): Assertion => assertion, -} as AssertionSpec; diff --git a/src/scanner/lib/hasParameterOrReceiver.ts b/src/scanner/lib/hasParameterOrReceiver.ts deleted file mode 100644 index be3b4fd..0000000 --- a/src/scanner/lib/hasParameterOrReceiver.ts +++ /dev/null @@ -1,9 +0,0 @@ -import { Event } from '@appland/models'; - -// Builds a function that returns true if the provided event argument has the specified -// objectId as the receiver or as a parameter value. -export default (objectId: number) => { - return (event: Event): boolean => - (!!event.receiver && event.receiver!.object_id === objectId) || - (!!event.parameters && event.parameters!.some((param) => param.object_id === objectId)); -}; diff --git a/src/scanner/lib/matchEvent.ts b/src/scanner/lib/matchEvent.ts deleted file mode 100644 index 83e004d..0000000 --- a/src/scanner/lib/matchEvent.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { Event } from '@appland/models'; -import { sqlNormalized } from '../../database'; -import MatchEventConfig from '../../configuration/types/matchEventConfig'; -import { EventFilter } from '../../types'; -import { buildFilter as buildMatchPattern } from './matchPattern'; - -export function buildFilter(pattern: MatchEventConfig): EventFilter { - const testFn = buildMatchPattern(pattern.test); - - const propertyFn = { - id: (e: Event) => e.codeObject.id, - type: (e: Event) => e.codeObject.type, - fqid: (e: Event) => e.codeObject.fqid, - query: (e: Event) => (e.sql ? sqlNormalized(e.sql) : null), - route: (e: Event) => e.route, - }; - - return (event: Event): boolean => { - const fn = propertyFn[pattern.property]; - if (!fn) { - throw new Error(`Unrecognized Event filter property: ${pattern.property}`); - } - const value = fn(event); - if (!value) { - return false; - } - - return testFn(value); - }; -} - -export function buildFilters(patterns: MatchEventConfig[]): EventFilter[] { - return patterns.map(buildFilter); -} diff --git a/src/scanner/lib/matchPattern.ts b/src/scanner/lib/matchPattern.ts deleted file mode 100644 index 77b83aa..0000000 --- a/src/scanner/lib/matchPattern.ts +++ /dev/null @@ -1,26 +0,0 @@ -import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; -import { StringFilter } from '../../types'; - -export function buildFilter(pattern: MatchPatternConfig): StringFilter { - function respectIgnoreCaseFlag(value: string): string { - return pattern.ignoreCase ? value.toLocaleLowerCase() : value; - } - - if (pattern.equal) { - const testStr = respectIgnoreCaseFlag(pattern.equal!); - return (value: string): boolean => respectIgnoreCaseFlag(value) === testStr; - } else if (pattern.include) { - const testStr = respectIgnoreCaseFlag(pattern.include!); - return (value: string): boolean => respectIgnoreCaseFlag(value).includes(testStr); - } else { - const regexp = - pattern.match instanceof RegExp - ? pattern.match - : new RegExp(pattern.match as unknown as string, pattern.ignoreCase ? 'i' : undefined); - return (value: string): boolean => regexp.test(value); - } -} - -export function buildFilters(patterns: MatchPatternConfig[]): StringFilter[] { - return patterns.map(buildFilter); -} diff --git a/src/scanner/lib/rpcWithoutProtection.ts b/src/scanner/lib/rpcWithoutProtection.ts deleted file mode 100644 index 0e5b423..0000000 --- a/src/scanner/lib/rpcWithoutProtection.ts +++ /dev/null @@ -1,30 +0,0 @@ -import { Event, Label } from '@appland/models'; -import Assertion from '../../assertion'; - -export interface RPCWithoutProtectionOptions { - get expectedLabel(): Label; -} - -export function rpcWithoutProtection( - id: string, - summaryTitle: string, - candidateGenerator: (httpClientRequest: Event) => Generator, - options: RPCWithoutProtectionOptions -): Assertion { - return Assertion.assert( - id, - summaryTitle, - (httpClientRequest: Event) => { - for (const candidate of candidateGenerator(httpClientRequest)) { - if (candidate.codeObject.labels.has(options.expectedLabel)) { - return false; - } - } - return true; - }, - (assertion: Assertion): void => { - assertion.where = (e: Event) => !!e.httpClientRequest; - assertion.description = summaryTitle; - } - ); -} diff --git a/src/scanner/missingAuthentication.ts b/src/scanner/missingAuthentication.ts deleted file mode 100644 index 6342b4c..0000000 --- a/src/scanner/missingAuthentication.ts +++ /dev/null @@ -1,76 +0,0 @@ -import { Event, EventNavigator } from '@appland/models'; -import { rpcRequestForEvent } from '../openapi/rpcRequest'; -import * as types from './types'; -import { AssertionSpec, StringFilter } from '../types'; -import Assertion from '../assertion'; -import { providesAuthentication } from './util'; -import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; -import { buildFilters } from './lib/matchPattern'; - -function isPublic(event: Event): boolean { - return event.labels.has(Public); -} - -const authenticatedBy = (iterator: Iterator): boolean => { - let i: IteratorResult = iterator.next(); - while (!i.done) { - if (isPublic(i.value.event) || providesAuthentication(i.value.event, SecurityAuthentication)) { - return true; - } - i = iterator.next(); - } - - return false; -}; - -class Options implements types.MissingAuthentication.Options { - public includeContentTypes: MatchPatternConfig[] = []; - public excludeContentTypes: MatchPatternConfig[] = []; -} - -function scanner(options: Options = new Options()): Assertion { - const includeContentTypes = buildFilters(options.includeContentTypes); - const excludeContentTypes = buildFilters(options.excludeContentTypes); - - function testContentType(contentType: string): boolean { - function test(filter: StringFilter): boolean { - return filter(contentType); - } - - return ( - (includeContentTypes.length === 0 || includeContentTypes.some(test)) && - !excludeContentTypes.some(test) - ); - } - - return Assertion.assert( - 'missing-authentication', - 'Unauthenticated HTTP server requests', - (event: Event) => !authenticatedBy(new EventNavigator(event).descendants()), - (assertion: Assertion): void => { - assertion.options = options; - assertion.where = (e: Event) => { - return ( - e.route !== undefined && - e.httpServerResponse !== undefined && - e.httpServerResponse.status < 300 && - !!rpcRequestForEvent(e) && - !!rpcRequestForEvent(e)!.contentType && - testContentType(rpcRequestForEvent(e)!.contentType) - ); - }; - assertion.description = `HTTP server request must be authenticated`; - } - ); -} - -const Public = 'public'; -const SecurityAuthentication = 'security.authentication'; - -export default { - scope: 'http_server_request', - labels: [Public, SecurityAuthentication], - enumerateScope: false, - Options, - scanner, -} as AssertionSpec; diff --git a/src/scanner/missingContentType.ts b/src/scanner/missingContentType.ts deleted file mode 100644 index 2581a68..0000000 --- a/src/scanner/missingContentType.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { Event } from '@appland/models'; -import { rpcRequestForEvent } from '../openapi/rpcRequest'; -import { AssertionSpec } from '../types'; -import Assertion from '../assertion'; - -const isRedirect = (status: number) => [301, 302, 303, 307, 308].includes(status); -const hasContent = (status: number) => status !== 204; - -const scanner = (): Assertion => { - return Assertion.assert( - 'missing-content-type', - 'HTTP server requests without a Content-Type header', - (e: Event) => rpcRequestForEvent(e)!.contentType === undefined, - (assertion: Assertion): void => { - assertion.where = (e: Event) => - !!e.httpServerResponse && - !isRedirect(e.httpServerResponse!.status) && - hasContent(e.httpServerResponse!.status); - assertion.description = `HTTP server request must have a Content-Type header`; - } - ); -}; - -export default { scope: 'http_server_request', enumerateScope: false, scanner } as AssertionSpec; diff --git a/src/scanner/nPlusOneQuery.ts b/src/scanner/nPlusOneQuery.ts deleted file mode 100644 index 8c9b5bc..0000000 --- a/src/scanner/nPlusOneQuery.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { AppMap, Event } from '@appland/models'; -import { AssertionSpec, Level, MatchResult } from '../types'; -import * as types from './types'; -import Assertion from '../assertion'; -import { SQLCount, sqlStrings } from '../database'; - -class Options implements types.NPlusOneQuery.Options { - public warningLimit = 5; - public errorLimit = 10; -} - -// TODO: clean up according to https://github.com/applandinc/scanner/issues/43 -function scanner(options: Options): Assertion { - const sqlCount: Record = {}; - - function matcher( - command: Event, - _appMap: AppMap, - assertion: Assertion - ): MatchResult[] | undefined { - for (const sqlEvent of sqlStrings(command, assertion.filterEvent.bind(assertion!))) { - let occurrence = sqlCount[sqlEvent.sql]; - if (!occurrence) { - occurrence = { - count: 1, - events: [sqlEvent.event], - }; - sqlCount[sqlEvent.sql] = occurrence; - } else { - occurrence.count += 1; - occurrence.events.push(sqlEvent.event); - } - } - - return Object.keys(sqlCount).reduce((matchResults, sql) => { - const occurrence = sqlCount[sql]; - - const buildMatchResult = (level: Level): MatchResult => { - return { - level: level, - event: occurrence.events[0], - message: `${occurrence.count} occurrences of SQL: ${sql}`, - groupMessage: sql, - occurranceCount: occurrence.count, - relatedEvents: occurrence.events, - }; - }; - - if (occurrence.count >= options.errorLimit) { - matchResults.push(buildMatchResult('error')); - } else if (occurrence.count >= options.warningLimit) { - matchResults.push(buildMatchResult('warning')); - } - return matchResults; - }, [] as MatchResult[]); - } - - return Assertion.assert( - 'n-plus-one-query', - 'N+1 SQL queries', - matcher, - (assertion: Assertion): void => { - assertion.description = `SQL query should not be repeated within the same command`; - } - ); -} - -export default { scope: 'command', enumerateScope: false, Options, scanner } as AssertionSpec; diff --git a/src/scanner/queryFromInvalidPackage.ts b/src/scanner/queryFromInvalidPackage.ts deleted file mode 100644 index 7a545ee..0000000 --- a/src/scanner/queryFromInvalidPackage.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { Event } from '@appland/models'; -import { AssertionSpec } from 'src/types'; -import * as types from './types'; -import Assertion from '../assertion'; -import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; -import { buildFilters } from './lib/matchPattern'; - -// TODO: Use the Query AST for this. -const WHITELIST = [/\bBEGIN\b/i, /\bCOMMIT\b/i, /\bROLLBACK\b/i, /\bRELEASE\b/i, /\bSAVEPOINT\b/i]; - -class Options implements types.QueryFromInvalidPackage.Options { - public allowedPackages: MatchPatternConfig[] = []; - public allowedQueries: MatchPatternConfig[] = WHITELIST.map( - (regexp) => ({ match: regexp } as MatchPatternConfig) - ); -} - -function scanner(options: Options): Assertion { - const allowedPackages = buildFilters(options.allowedPackages); - const allowedQueries = buildFilters(options.allowedQueries); - - return Assertion.assert( - 'query-from-invalid-package', - 'Queries from invalid packages', - (e: Event) => { - if (!allowedPackages.some((filter) => filter(e.parent!.codeObject.packageOf))) { - return `${e.codeObject.id} is invoked from illegal package ${ - e.parent!.codeObject.packageOf - }`; - } - }, - (assertion: Assertion): void => { - assertion.where = (e: Event) => - !!e.sqlQuery && !!e.parent && !allowedQueries.some((pattern) => pattern(e.sqlQuery!)); - assertion.description = `Query must be invoked from one of (${options.allowedPackages.join( - ',' - )})`; - } - ); -} - -export default { Options, enumerateScope: true, scanner } as AssertionSpec; diff --git a/src/scanner/queryFromView.ts b/src/scanner/queryFromView.ts deleted file mode 100644 index a77a783..0000000 --- a/src/scanner/queryFromView.ts +++ /dev/null @@ -1,22 +0,0 @@ -import { Event, Label } from '@appland/models'; -import * as types from './types'; -import { AssertionSpec } from 'src/types'; -import Assertion from '../assertion'; - -class Options implements types.QueryFromView.Options { - public forbiddenLabel: Label = 'mvc.template'; -} - -function scanner(options: Options = new Options()): Assertion { - return Assertion.assert( - 'query-from-view', - 'Queries from view', - (e: Event) => e.ancestors().some((e: Event) => e.codeObject.labels.has(options.forbiddenLabel)), - (assertion: Assertion): void => { - assertion.where = (e: Event) => !!e.sqlQuery; - assertion.description = `SQL query from ${options.forbiddenLabel}`; - } - ); -} - -export default { Options, enumerateScope: true, scanner } as AssertionSpec; diff --git a/src/scanner/rpcWithoutCircuitBreaker.ts b/src/scanner/rpcWithoutCircuitBreaker.ts deleted file mode 100644 index fd11f52..0000000 --- a/src/scanner/rpcWithoutCircuitBreaker.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { Event, EventNavigator } from '@appland/models'; -import { AssertionSpec } from 'src/types'; -import * as types from './types'; -import Assertion from '../assertion'; -import { RPCWithoutProtectionOptions, rpcWithoutProtection } from './lib/rpcWithoutProtection'; - -class Options implements RPCWithoutProtectionOptions, types.RPCWithoutCircuitBreaker.Options { - public expectedLabel: string = RPCCircuitBreaker; -} - -// The circuit breaker will be found in a descendant of the httpClientRequest. -function* descendants(httpClientRequest: Event): Generator { - for (const candidate of new EventNavigator(httpClientRequest).descendants()) { - yield candidate.event; - } -} - -function scanner(options: Options = new Options()): Assertion { - return rpcWithoutProtection( - 'rpc-without-circuit-breaker', - 'RPC without circuit breaker', - descendants, - options - ); -} - -const RPCCircuitBreaker = 'rpc.circuit_breaker'; - -export default { - Options, - labels: [RPCCircuitBreaker], - enumerateScope: true, - scanner, -} as AssertionSpec; diff --git a/src/scanner/saveWithoutValidation.ts b/src/scanner/saveWithoutValidation.ts deleted file mode 100644 index 44f2c26..0000000 --- a/src/scanner/saveWithoutValidation.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { Event, EventNavigator } from '@appland/models'; -import { AssertionSpec } from 'src/types'; -import Assertion from '../assertion'; - -const validatedBy = (iterator: Iterator): boolean => { - let i: IteratorResult = iterator.next(); - while (!i.done) { - if ( - i.value.event.methodId !== undefined && - ['valid?', 'validate'].includes(i.value.event.methodId!) - ) { - return true; - } - i = iterator.next(); - } - - return false; -}; - -const scanner = (): Assertion => { - return Assertion.assert( - 'save-without-validation', - 'Save without validation', - // TODO: ensure that the object id on the 'validate' is the same as the object id on the 'save' - // TODO: if validate happens in a preceding event, this is also OK - (event: Event) => !validatedBy(new EventNavigator(event).descendants()), - (assertion: Assertion): void => { - assertion.where = (e: Event) => e.isFunction && ['save', 'save!'].includes(e.methodId!); - assertion.description = `Data must be valiadated before it is saved`; - } - ); -}; - -export default { scanner, enumerateScope: true } as AssertionSpec; diff --git a/src/scanner/secretInLog.ts b/src/scanner/secretInLog.ts deleted file mode 100644 index 621a4f4..0000000 --- a/src/scanner/secretInLog.ts +++ /dev/null @@ -1,76 +0,0 @@ -import { Event, ParameterObject } from '@appland/models'; -import { AssertionSpec, MatchResult } from 'src/types'; -import Assertion from '../assertion'; -import SecretsRegexes from '../analyzer/secretsRegexes'; -import { emptyValue } from './util'; -import recordSecrets from '../analyzer/recordSecrets'; - -class Match { - constructor(public regexp: RegExp | string, public value: string) {} -} - -const secrets: Set = new Set(); - -const findMatchingValue = (regexps: RegExp[], parameters: readonly ParameterObject[]): Match[] => { - const matches: Match[] = []; - parameters - .filter((parameter) => !emptyValue(parameter.value)) - .forEach((parameter) => { - const value = parameter.value; - regexps - .filter((regexp) => regexp.test(value)) - .forEach((regexp) => { - matches.push(new Match(regexp, value)); - }); - }); - return matches; -}; - -const findInLog = (e: Event): MatchResult[] | undefined => { - const matches: Match[] = Object.keys(SecretsRegexes).reduce((memo, key) => { - const matches = findMatchingValue(SecretsRegexes[key], e.parameters!); - matches.forEach((match) => memo.push(match)); - return memo; - }, [] as Match[]); - - e.parameters!.filter((parameter) => !emptyValue(parameter.value)).forEach((parameter) => { - const value = parameter.value; - secrets.forEach((secret) => { - if (value.includes(secret)) { - matches.push(new Match(secret, value)); - } - }); - }); - - if (matches.length > 0) { - return matches.map((match) => ({ - level: 'error', - message: `${match.value} contains secret ${match.regexp}`, - })); - } -}; - -const scanner = function (): Assertion { - return Assertion.assert( - 'secret-in-log', - 'Secret in log', - (e: Event) => { - if (e.codeObject.labels.has(Secret)) { - recordSecrets(secrets, e); - } - if (e.parameters && e.codeObject.labels.has(Log)) { - return findInLog(e); - } - }, - (assertion: Assertion): void => { - assertion.where = (e: Event) => - e.codeObject.labels.has(Log) || e.codeObject.labels.has(Secret); - assertion.description = `Log contains secret-like text`; - } - ); -}; - -const Secret = 'secret'; -const Log = 'log'; - -export default { labels: [Secret, Log], scanner, enumerateScope: true } as AssertionSpec; diff --git a/src/scanner/slowFunctionCall.ts b/src/scanner/slowFunctionCall.ts deleted file mode 100644 index a51c6eb..0000000 --- a/src/scanner/slowFunctionCall.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { Event } from '@appland/models'; -import { AssertionSpec, ScopeName } from 'src/types'; -import * as types from './types'; -import Assertion from '../assertion'; -import MatchPatternConfig from 'src/configuration/types/matchPatternConfig'; -import { buildFilters } from './lib/matchPattern'; - -class Options implements types.SlowFunctionCall.Options { - public functions: MatchPatternConfig[] = []; - public timeAllowed = 0.1; -} - -function scanner(options: Options): Assertion { - const functionPatterns = buildFilters(options.functions || []); - - return Assertion.assert( - 'slow-function-call', - 'Slow function calls', - (e: Event) => { - if (e.returnEvent.elapsedTime! > options.timeAllowed) { - return `Slow ${e.codeObject.id} call (${e.returnEvent.elapsedTime}ms)`; - } - }, - (assertion: Assertion): void => { - assertion.where = (e: Event) => - e.isFunction && - !!e.returnEvent && - !!e.returnEvent.elapsedTime && - !!e.codeObject.id && - (functionPatterns.length === 0 || - functionPatterns.some((pattern) => pattern(e.codeObject.id))); - assertion.description = `Slow function call (> ${options.timeAllowed * 1000}ms)`; - } - ); -} - -export default { - scope: 'root' as ScopeName, - enumerateScope: true, - Options, - scanner, -} as AssertionSpec; diff --git a/src/scanner/slowHttpServerRequest.ts b/src/scanner/slowHttpServerRequest.ts deleted file mode 100644 index 376373a..0000000 --- a/src/scanner/slowHttpServerRequest.ts +++ /dev/null @@ -1,27 +0,0 @@ -import { Event } from '@appland/models'; -import { AssertionSpec } from 'src/types'; -import * as types from './types'; -import Assertion from '../assertion'; - -class Options implements types.SlowHTTPServerRequest.Options { - public timeAllowed = 1; -} - -function scanner(options: Options): Assertion { - return Assertion.assert( - 'slow-http-server-request', - 'Slow HTTP server requests', - (e: Event) => e.elapsedTime! > options.timeAllowed, - (assertion: Assertion): void => { - assertion.where = (e: Event) => !!e.httpServerRequest && e.elapsedTime !== undefined; - assertion.description = `Slow HTTP server request (> ${options.timeAllowed * 1000}ms)`; - } - ); -} - -export default { - scope: 'http_server_request', - enumerateScope: false, - Options, - scanner, -} as AssertionSpec; diff --git a/src/scanner/slowQuery.ts b/src/scanner/slowQuery.ts deleted file mode 100644 index e15c15c..0000000 --- a/src/scanner/slowQuery.ts +++ /dev/null @@ -1,22 +0,0 @@ -import { Event } from '@appland/models'; -import { AssertionSpec } from 'src/types'; -import * as types from './types'; -import Assertion from '../assertion'; - -class Options implements types.SlowQuery.Options { - public timeAllowed = 1; -} - -function scanner(options: Options = new Options()): Assertion { - return Assertion.assert( - 'slow-query', - 'Slow SQL queries', - (e: Event) => e.elapsedTime! > options.timeAllowed, - (assertion: Assertion): void => { - assertion.where = (e: Event) => !!e.sqlQuery && !!e.elapsedTime; - assertion.description = `Slow SQL query (> ${options.timeAllowed * 1000}ms)`; - } - ); -} - -export default { Options, enumerateScope: true, scanner } as AssertionSpec; diff --git a/src/scanner/tooManyJoins.ts b/src/scanner/tooManyJoins.ts deleted file mode 100644 index 2f44739..0000000 --- a/src/scanner/tooManyJoins.ts +++ /dev/null @@ -1,64 +0,0 @@ -import { AppMap, Event } from '@appland/models'; -import { AssertionSpec, MatchResult } from '../types'; -import * as types from './types'; -import Assertion from '../assertion'; -import { countJoins, SQLCount, sqlStrings } from '../database'; - -export interface JoinCount extends SQLCount { - joins: number; -} - -class Options implements types.TooManyJoins.Options { - public warningLimit = 5; -} - -// TODO: clean up (https://github.com/applandinc/scanner/issues/43) -function scanner(options: Options = new Options()): Assertion { - const joinCount: Record = {}; - const matcher = ( - command: Event, - _appMap: AppMap, - assertion: Assertion - ): MatchResult[] | undefined => { - for (const sqlEvent of sqlStrings(command, assertion.filterEvent.bind(assertion))) { - let occurrence = joinCount[sqlEvent.sql]; - - if (!occurrence) { - occurrence = { - count: 1, - joins: countJoins(sqlEvent.sql), - events: [sqlEvent.event], - }; - joinCount[sqlEvent.sql] = occurrence; - } else { - occurrence.count += 1; - occurrence.events.push(sqlEvent.event); - } - } - - return Object.keys(joinCount).reduce((matchResults, sql) => { - const occurrence = joinCount[sql]; - - if (occurrence.joins >= options.warningLimit) { - matchResults.push({ - level: 'warning', - event: occurrence.events[0], - message: `${occurrence.joins} join${occurrence.joins > 1 ? 's' : ''} in SQL "${sql}"`, - relatedEvents: occurrence.events, - }); - } - return matchResults; - }, [] as MatchResult[]); - }; - - return Assertion.assert( - 'too-many-joins', - 'Too many joins', - matcher, - (assertion: Assertion): void => { - assertion.description = `SQL query has too many joins ${assertion.options}`; - } - ); -} - -export default { scope: 'command', enumerateScope: false, Options, scanner } as AssertionSpec; diff --git a/src/scanner/tooManyUpdates.ts b/src/scanner/tooManyUpdates.ts deleted file mode 100644 index a00d2ce..0000000 --- a/src/scanner/tooManyUpdates.ts +++ /dev/null @@ -1,70 +0,0 @@ -import { Event, EventNavigator } from '@appland/models'; -import { AssertionSpec, MatchResult } from 'src/types'; -import * as types from './types'; -import Assertion from '../assertion'; - -// TODO: Use the Query AST for this. -const QueryIncludes: RegExp[] = [/\bINSERT\b/i, /\bUPDATE\b/i]; -const UpdateMethods: string[] = ['put', 'post', 'patch']; - -class Options implements types.TooManyUpdates.Options { - public warningLimit = 20; -} - -function scanner(options: Options): Assertion { - const isUpdate = (event: Event): boolean => { - const isSQLUpdate = (): boolean => { - if (!event.sqlQuery) { - return false; - } - return QueryIncludes.some((pattern) => pattern.test(event.sqlQuery!)); - }; - - const isRPCUpdate = (): boolean => { - if (!event.httpClientRequest) { - return false; - } - return UpdateMethods.includes(event.httpClientRequest!.request_method.toLowerCase()); - }; - - return isSQLUpdate() || isRPCUpdate(); - }; - - const updateEvents = function* (event: Event): Generator { - for (const e of new EventNavigator(event).descendants()) { - if (!isUpdate(e.event)) { - continue; - } - yield e.event; - } - }; - - const matcher = (command: Event): MatchResult[] | undefined => { - const events: Event[] = []; - for (const updateEvent of updateEvents(command)) { - events.push(updateEvent); - } - - if (events.length > options.warningLimit) { - return [ - { - level: 'error', - message: `Command performs ${events.length} SQL and RPC updates`, - event: events[0], - relatedEvents: events, - }, - ]; - } - }; - - return Assertion.assert( - 'too-many-updates', - 'Too many SQL and RPC updates performed in one command', - matcher, - (assertion: Assertion): void => { - assertion.description = `Too many SQL and RPC updates performed in one transaction`; - } - ); -} - -export default { scope: 'command', enumerateScope: false, Options, scanner } as AssertionSpec; diff --git a/src/scanner/types.d.ts b/src/scanner/types.d.ts deleted file mode 100644 index 5c658ee..0000000 --- a/src/scanner/types.d.ts +++ /dev/null @@ -1,87 +0,0 @@ -import MatchPatternConfig from '../configuration/types/matchPatternConfig'; - -interface TimeAllowed { - timeAllowed?: number; -} - -interface WarningLimit { - warningLimit?: number; -} - -export namespace IllegalPackageDependency { - export interface Options { - callerPackages: MatchPatternConfig[]; - calleePackage: MatchPatternConfig; - } -} - -export namespace CircularDependency { - export interface Options { - ignoredPackages?: MatchPatternConfig[]; - depth?: number; - } -} - -export namespace IncompatibleHttpClientRequest { - export interface Options { - schemata: Record; - } -} - -export namespace MissingAuthentication { - export interface Options { - includeContentTypes?: MatchPatternConfig[]; - excludeContentTypes?: MatchPatternConfig[]; - } -} - -export namespace NPlusOneQuery { - export interface Options extends WarningLimit { - errorLimit?: number; - } -} - -export namespace QueryFromInvalidPackage { - export interface Options { - allowedPackages: MatchPatternConfig[]; - allowedQueries?: MatchPatternConfig[]; - } -} - -export namespace QueryFromView { - export interface Options { - forbiddenLabel?: string; - } -} - -export namespace RPCWithoutCircuitBreaker { - export interface Options { - expectedLabel?: string; - } -} - -export namespace SlowFunctionCall { - export interface Options extends TimeAllowed { - functions?: MatchPatternConfig[]; - } -} - -export namespace SlowHTTPServerRequest { - // eslint-disable-next-line @typescript-eslint/no-empty-interface - export interface Options extends TimeAllowed {} -} - -export namespace SlowQuery { - // eslint-disable-next-line @typescript-eslint/no-empty-interface - export interface Options extends TimeAllowed {} -} - -export namespace TooManyJoins { - // eslint-disable-next-line @typescript-eslint/no-empty-interface - export interface Options extends WarningLimit {} -} - -export namespace TooManyUpdates { - // eslint-disable-next-line @typescript-eslint/no-empty-interface - export interface Options extends WarningLimit {} -} diff --git a/src/scanner/unbatchedMaterializedQuery.ts b/src/scanner/unbatchedMaterializedQuery.ts deleted file mode 100644 index 2b2378f..0000000 --- a/src/scanner/unbatchedMaterializedQuery.ts +++ /dev/null @@ -1,75 +0,0 @@ -import { buildQueryAST, Event } from '@appland/models'; -import { AssertionSpec } from '../types'; -import Assertion from '../assertion'; -import { visit } from '../database/visit'; - -function isMaterialized(e: Event): boolean { - return e.ancestors().some(({ labels }) => labels.has(DAOMaterialize)); -} - -function isApplicable(e: Event): boolean { - try { - const ast = buildQueryAST(e.sqlQuery!); - let isSelect = false; - let isCount = false; - let hasLimitClause = false; - let isMetadataQuery = false; - - if (ast) { - const metadataTableNames = ['sqlite_master']; - - visit(ast, { - 'statement.select': (statement: any) => { - isSelect = true; - - if ( - statement.result && - Array.isArray(statement.result) && - statement.result.length === 1 && - statement.result[0].type === 'function' && - statement.result[0].name.name === 'count' - ) { - isCount = true; - } - }, - 'expression.limit': () => { - hasLimitClause = true; - }, - 'identifier.table': (identifier: any) => { - if (metadataTableNames.includes(identifier.name)) { - isMetadataQuery = true; - } - }, - }); - } - - const isBatched = hasLimitClause || isCount || isMetadataQuery; - - return isSelect && !isBatched && isMaterialized(e); - } catch (_) { - console.warn(`Unable to analyze query "${e.sqlQuery!}"`); - return false; - } -} - -function scanner(): Assertion { - return Assertion.assert( - 'unbatched-materialized-query', - 'Unbatched materialized SQL query', - (e: Event) => isApplicable(e), - (assertion: Assertion): void => { - assertion.where = (e: Event) => !!e.sqlQuery; - assertion.description = `Unbatched materialized SQL query`; - } - ); -} - -// Example: ActiveRecord::Relation#records -const DAOMaterialize = 'dao.materialize'; - -export default { - labels: [DAOMaterialize], - scope: 'command', - enumerateScope: true, - scanner, -} as AssertionSpec; diff --git a/src/scanner/updateInGetRequest.ts b/src/scanner/updateInGetRequest.ts deleted file mode 100644 index f022a34..0000000 --- a/src/scanner/updateInGetRequest.ts +++ /dev/null @@ -1,70 +0,0 @@ -import { Event } from '@appland/models'; -import { AssertionSpec } from 'src/types'; -import Assertion from '../assertion'; -import { toRegExpArray } from './util'; - -class Options { - private _queryInclude: RegExp[]; - private _queryExclude: RegExp[]; - - constructor( - queryInclude: RegExp[] = [/\binsert\b/i, /\bupdate\b/i], - queryExclude: RegExp[] = [] - ) { - this._queryInclude = queryInclude; - this._queryExclude = queryExclude; - } - - get queryInclude(): RegExp[] { - return this._queryInclude; - } - - set queryInclude(value: string[] | RegExp[]) { - this._queryInclude = toRegExpArray(value); - } - - get queryExclude(): RegExp[] { - return this._queryExclude; - } - - set queryExclude(value: string[] | RegExp[]) { - this._queryExclude = toRegExpArray(value); - } -} - -function scanner(options: Options = new Options()): Assertion { - return Assertion.assert( - 'update-in-get-request', - 'Data update performed in GET or HEAD request', - (e: Event) => { - let httpServerRequest: Event | undefined; - function hasHttpServerRequest() { - httpServerRequest = e - .ancestors() - .find( - (ancestor) => - ancestor.httpServerRequest && - ['head', 'get'].includes(ancestor.httpServerRequest.request_method.toLowerCase()) - ); - return httpServerRequest !== undefined; - } - - if ( - options.queryInclude.some((pattern) => e.sqlQuery!.match(pattern)) && - !options.queryExclude.some((pattern) => e.sqlQuery!.match(pattern)) && - !e.ancestors().some((ancestor) => ancestor.codeObject.labels.has(Audit)) && - hasHttpServerRequest() - ) { - return `Data update performed in ${httpServerRequest!.route}: ${e.sqlQuery}`; - } - }, - (assertion: Assertion): void => { - assertion.where = (e: Event) => !!e.sqlQuery; - assertion.description = `Data update performed in GET or HEAD request`; - } - ); -} - -const Audit = 'audit'; - -export default { scope: 'http_server_request', Options, scanner } as AssertionSpec; diff --git a/src/scanner/util.ts b/src/scanner/util.ts deleted file mode 100644 index 7e3561f..0000000 --- a/src/scanner/util.ts +++ /dev/null @@ -1,119 +0,0 @@ -import { Event } from '@appland/models'; -import { isAbsolute } from 'path'; - -let isVerbose = false; -function verbose(v: boolean | null = null): boolean { - if (v === true || v === false) { - isVerbose = v; - } - return isVerbose; -} - -function capitalize(str: string): string { - if (!str || str === '') { - return str; - } - return [str.charAt(0).toUpperCase(), str.slice(1)].join(''); -} - -function emptyValue(value: string): boolean { - return [null, undefined, ''].includes(value); -} - -function responseContentType(event: Event): string | undefined { - if (event.httpServerResponse?.headers) { - return event.httpServerResponse!.headers!['Content-Type']; - } else if (event.httpClientResponse?.headers) { - return event.httpClientResponse!.headers!['Content-Type']; - } -} - -function appMapDir(appMapFileName: string): string { - return appMapFileName.substring(0, appMapFileName.length - '.appmap.json'.length); -} - -// eslint-disable-next-line -function isFalsey(valueObj: any): boolean { - if (!valueObj) { - return true; - } - if (valueObj.class === 'FalseClass') { - return true; - } - if (valueObj.class === 'Array' && valueObj.value === '[]') { - return true; - } - if (valueObj.value === '') { - return true; - } - - return false; -} - -const isTruthy = (valueObj: any): boolean => !isFalsey(valueObj); - -function providesAuthentication(event: Event, label: string): boolean { - return event.returnValue && event.labels.has(label) && isTruthy(event.returnValue.value); -} - -function ideLink(filePath: string, ide: string, eventId: number): string { - const OSC = '\u001B]'; - const BEL = '\u0007'; - const SEP = ';'; - - // eslint-disable-next-line @typescript-eslint/no-var-requires - const supportsHyperlinks = require('supports-hyperlinks'); - - if (!supportsHyperlinks.stdout) { - return filePath; - } - - let path: string; - if (!isAbsolute(filePath)) { - path = `${__dirname}/../../../../../${filePath}`; - } else { - path = filePath; - } - const state = { currentView: 'viewFlow', selectedObject: `event:${eventId}` }; - const encodedState = encodeURIComponent(JSON.stringify(state)); - const link = - ide == 'vscode' - ? `vscode://appland.appmap/open?uri=${path}&state=${encodedState}` - : `${ide}://open?file=${path}`; - - return [OSC, '8', SEP, SEP, link, BEL, filePath, OSC, '8', SEP, SEP, BEL].join(''); -} - -const toRegExp = (value: string | RegExp): RegExp => { - return typeof value === 'string' ? new RegExp(value as string) : (value as RegExp); -}; - -const toRegExpArray = (value: string[] | RegExp[]): RegExp[] => { - return value.map(toRegExp); -}; - -const RootLabels = ['command', 'job']; - -const isRoot = (event: Event | undefined): boolean => { - if (!event) { - return true; - } - return ( - !!event.httpServerRequest || RootLabels.some((label) => event.codeObject.labels.has(label)) - ); -}; - -export { - appMapDir, - capitalize, - emptyValue, - isFalsey, - isTruthy, - ideLink, - isRoot, - providesAuthentication, - toRegExp, - responseContentType, - toRegExpArray, - verbose, -}; From 51cabe77a13596b1898a32aa6b06bd61129d9365 Mon Sep 17 00:00:00 2001 From: Kevin Gilpin Date: Wed, 8 Dec 2021 13:23:16 -0500 Subject: [PATCH 7/7] fix: Let the console handle the line breaks, because they are happening in the wrong place anyway --- src/command.ts | 4 +--- src/formatter/progressFormatter.ts | 8 +++----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/command.ts b/src/command.ts index 87c1ea7..c737776 100644 --- a/src/command.ts +++ b/src/command.ts @@ -145,7 +145,6 @@ export default { progressFormat === 'progress' ? new ProgressFormatter() : new PrettyFormatter(); const checks = await loadConfiguration(config); - let index = 0; const findings: Finding[] = []; await Promise.all( @@ -163,13 +162,12 @@ export default { await Promise.all( checks.map(async (check) => { - index++; const matchCount = findings.length; await checker.check(appMap, check, findings); const newMatches = findings.slice(matchCount, findings.length); newMatches.forEach((match) => (match.appMapFile = file)); - const message = formatter.result(check, newMatches, index); + const message = formatter.result(check, newMatches); if (message) { process.stderr.write(message); } diff --git a/src/formatter/progressFormatter.ts b/src/formatter/progressFormatter.ts index fe09ce7..9835071 100644 --- a/src/formatter/progressFormatter.ts +++ b/src/formatter/progressFormatter.ts @@ -8,13 +8,11 @@ export default class ProgressFormatter extends Formatter { return ''; } - result(_check: Check, matches: Finding[], index: number): string | undefined { - const ending = index % 80 === 0 ? '\n' : ''; - + result(_check: Check, matches: Finding[]): string | undefined { if (matches.length === 0) { - return chalk.stderr.green('.') + ending; + return chalk.stderr.green('.'); } else { - return chalk.stderr.magenta('!') + ending; + return chalk.stderr.magenta('!'); } } }