From dc5fceb4685f02971ece962b33ea43362ab59a3a Mon Sep 17 00:00:00 2001 From: saimskywalker <15120878+saimskywalker@users.noreply.github.com> Date: Sat, 7 Nov 2026 01:08:30 +0800 Subject: [PATCH 1/3] feat: project checks for Info.plist and AndroidManifest A `checks:` section in storehound.yaml pins app-specific values in Info.plist (nested keys too) and in AndroidManifest attributes, with the operators exists, absent, equals, absent_or_equals, not_equals, contains, none_of and matches. Each check needs a reason and runs as the rule `check:`, so text, JSON, SARIF, ignore, severity and --explain work as for any rule. Mistakes in a check are configuration errors with a line. Part of #51. --- CHANGELOG.md | 12 + README.md | 19 + doc/configuration.md | 83 +- lib/src/cli.dart | 34 +- lib/src/config.dart | 232 ++++- lib/src/engine.dart | 5 +- lib/src/project.dart | 13 +- lib/src/rules/project_checks.dart | 452 +++++++++ lib/src/version.dart | 2 +- lib/src/xml_tree.dart | 48 + lib/storehound.dart | 1 + pubspec.yaml | 2 +- test/cli_test.dart | 53 ++ test/config_test.dart | 263 ++++++ test/fixtures/README.md | 3 + test/fixtures/production_app.golden.txt | 2 +- test/fixtures/production_app/storehound.yaml | 32 + test/production_app_test.dart | 63 +- test/rules/project_checks_test.dart | 909 +++++++++++++++++++ test/xml_tree_test.dart | 25 + 20 files changed, 2238 insertions(+), 15 deletions(-) create mode 100644 lib/src/rules/project_checks.dart create mode 100644 test/rules/project_checks_test.dart create mode 100644 test/xml_tree_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ad01a5..03498cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## 0.3.0 + +- Project checks: app-specific pins under `checks:` in `storehound.yaml`, for + `Info.plist` keys (with nested keys) and `AndroidManifest.xml` attributes, + with the operators `exists`, `absent`, `equals`, `absent_or_equals`, + `not_equals`, `contains`, `none_of` and `matches`. Each check needs a + `reason`, defaults to `error`, and is reported in text, JSON and SARIF as + `check:` with the file and line of the value (or of its parent when + it is missing). `ignore`, `rules` and `--explain check:` accept it. + A mistake in a check is a configuration error with its line (exit code 2). +- The production-shaped fixture pins five values with `checks:`. + ## 0.2.0 Renamed from imaji_store_preflight. Same rules and CLI; the command is now diff --git a/README.md b/README.md index 77ee8b6..b074729 100644 --- a/README.md +++ b/README.md @@ -141,6 +141,25 @@ including how `android-target-sdk` behaves on a Dart-only runner. `storehound --explain ` prints the full why, fix, options and documentation links for one rule. +### Your own checks + +Pins only your app cares about go under `checks:` in `storehound.yaml`, each +with a reason, and are reported like any rule as `check:`: + +```yaml +checks: + no-backup: + file: android_manifest + element: application + attribute: android:allowBackup + equals: "false" + reason: "Tokens live in shared preferences." +``` + +They work on `Info.plist` keys and `AndroidManifest.xml` attributes; see +[Project checks](https://github.com/saimskywalker/storehound/blob/main/doc/configuration.md#project-checks) +for the operators. + ## Is it noisy on a real app? It should not be. CI runs it with `--fail-on info` on diff --git a/doc/configuration.md b/doc/configuration.md index 1de7d3f..36c55cc 100644 --- a/doc/configuration.md +++ b/doc/configuration.md @@ -15,7 +15,7 @@ storehound [options] [project-dir] --brief Text output without the why/fix explanations --[no-]color Colour text output (default: when stdout is a terminal and NO_COLOR is unset) --list-rules List every rule - --explain= Explain one rule + --explain= Explain one rule, or a project check as check: --version Print the version -h, --help Show the usage ``` @@ -102,6 +102,87 @@ code 2) with the line number, so a typo cannot silently switch a check off. | `android-picker-queries` | `plugins` (extra file-picker packages) | | `android-target-sdk` | `target_sdk`, `deadlines` (`api`, `from`), `warn_days` | +## Project checks + +Pins that only your app cares about, the kind usually kept as hand-written +tests that parse `Info.plist` or `AndroidManifest.xml`, go under `checks:`. +Each one is reported like a built-in rule, with the rule id `check:`: + +```yaml +checks: + encryption-exempt: + file: info_plist + key: ITSAppUsesNonExemptEncryption + equals: false + reason: "Export compliance is answered in App Store Connect; keeps each upload from asking." + + background-modes-pinned: + file: info_plist + key: UIBackgroundModes + equals: [audio, remote-notification] + reason: "The modes App Review accepted. A new one needs review notes first." + + ats-no-arbitrary-loads: + file: info_plist + key: NSAppTransportSecurity.NSAllowsArbitraryLoads # a dot steps into a nested dictionary + absent_or_equals: false + reason: "All traffic is HTTPS." + + no-backup: + file: android_manifest + element: application + attribute: android:allowBackup + equals: "false" + severity: warning + reason: "Tokens live in shared preferences." + + no-media-permissions: + file: android_manifest + element: uses-permission + attribute: android:name + none_of: [android.permission.READ_MEDIA_IMAGES, android.permission.READ_MEDIA_VIDEO] + reason: "The app uses the system photo picker only." +``` + +| Key | Meaning | +|---|---| +| `file` | `info_plist` (`paths.info_plist`) or `android_manifest` (the main manifest, `paths.android_manifest`) | +| `key` | Info.plist only. A top-level key, or a dotted path into nested dictionaries. For a key that contains dots, list the parts: `[NSAppTransportSecurity, NSExceptionDomains, example.com]` | +| `element`, `attribute` | AndroidManifest only. Every element with that name, anywhere in the manifest, and the attribute's name as written, e.g. `android:allowBackup` | +| `reason` | Required: why the value is pinned. Shown as the "why" in every report and in `--explain` | +| `severity` | `error` (default), `warning` or `info` | + +Each check has exactly one operator: + +| Operator | Info.plist value | AndroidManifest attribute | +|---|---|---| +| `exists: true` | is present | at least one element sets it | +| `absent: true` | is not present | no element sets it | +| `equals: ` | is present and equal; a list equals an array with the same items in any order | every element sets it to the value | +| `absent_or_equals: ` | is missing or equal | every element that sets it uses the value | +| `not_equals: ` | is missing or different | no element sets it to the value | +| `contains: []` | is an array that holds every listed value | the elements' values together hold every listed value | +| `none_of: []` | is none of the values (for an array: no item is) | no element's value is listed | +| `matches: ` | is a string the regular expression matches | every element sets a value it matches | + +Values are compared with their type: `equals: false` matches ``, not +`NO`, and `equals: "42"` does not match `42`. +Manifest attributes are text, so quote values there (`equals: "false"`). +`matches` finds the pattern anywhere in the value; anchor it with `^` and `$` +to match the whole value. + +Findings point at the value's line; when the value is missing, at its +parent: the enclosing dictionary's key in Info.plist, the element in the +manifest, or `` when no element has that name. A check whose file +does not exist is a finding too, so a moved file cannot switch it off +silently. A check can be named in `ignore` (`rule: check:no-backup`) and under +`rules:` like any rule, and `storehound --explain check:no-backup` prints it. +Mistakes in a check (an unknown file or operator, a value of the wrong type, +a missing `reason`) are configuration errors with the line number, exit +code 2. + +The privacy manifest and entitlements are not supported yet. + ## Use it as a library ```dart diff --git a/lib/src/cli.dart b/lib/src/cli.dart index 8362f28..26cd1ca 100644 --- a/lib/src/cli.dart +++ b/lib/src/cli.dart @@ -9,6 +9,7 @@ import 'finding.dart'; import 'report/machine_reporters.dart'; import 'report/text_reporter.dart'; import 'rules/all_rules.dart'; +import 'rules/project_checks.dart'; import 'version.dart'; /// Exit codes of the command-line tool. @@ -81,7 +82,9 @@ ArgParser _parser() => ArgParser(usageLineLength: 80) ..addFlag('list-rules', negatable: false, help: 'List every rule and exit.') ..addOption( 'explain', - help: 'Explain one rule (why it exists, how to fix it) and exit.', + help: + 'Explain one rule (why it exists, how to fix it) and exit. ' + 'A project check is `check:`.', valueHelp: 'rule-id', ) ..addFlag('version', negatable: false, help: 'Print the version and exit.') @@ -127,15 +130,19 @@ int runCli( out.writeln(renderRuleList(builtInRules)); return ExitCodes.ok; } - if (args.option('explain') case final id?) { + final explain = args.option('explain'); + if (explain != null) { for (final rule in builtInRules) { - if (rule.id == id) { + if (rule.id == explain) { out.writeln(renderExplain(rule)); return ExitCodes.ok; } } - err.writeln('Unknown rule `$id`. Run --list-rules to see them all.'); - return ExitCodes.usage; + // A project check lives in the configuration, which is loaded below. + if (!explain.startsWith(checkRulePrefix)) { + err.writeln('Unknown rule `$explain`. Run --list-rules to see them all.'); + return ExitCodes.usage; + } } var projectArg = args.option('project')!; @@ -184,11 +191,28 @@ int runCli( err.writeln('Invalid configuration: $e'); return ExitCodes.usage; } + if (explain != null) { + for (final check in config.checkRules) { + if (check.id == explain) { + out.writeln(renderExplain(check)); + return ExitCodes.ok; + } + } + final name = explain.substring(checkRulePrefix.length); + err.writeln(switch (config.source) { + null => 'Unknown check `$explain`: no configuration file was found.', + final source => + 'Unknown check `$explain`: $source has no `$name` ' + 'under `checks:`.', + }); + return ExitCodes.usage; + } if (args.option('fail-on') case final failOn?) { config = PreflightConfig( rules: config.rules, ignores: config.ignores, paths: config.paths, + checkRules: config.checkRules, source: config.source, failOn: failOn == 'never' ? null : Severity.tryParse(failOn), ); diff --git a/lib/src/config.dart b/lib/src/config.dart index cd82191..54ec0fd 100644 --- a/lib/src/config.dart +++ b/lib/src/config.dart @@ -5,6 +5,7 @@ import 'package:yaml/yaml.dart'; import 'finding.dart'; import 'project.dart'; import 'rule.dart'; +import 'rules/project_checks.dart'; /// The configuration file name looked up in the project root. const configFileName = 'storehound.yaml'; @@ -113,6 +114,7 @@ class PreflightConfig { this.ignores = const [], this.failOn = Severity.error, this.paths = const ProjectPaths(), + this.checkRules = const [], this.source, }); @@ -128,6 +130,10 @@ class PreflightConfig { /// Path overrides. final ProjectPaths paths; + /// The project checks from the `checks:` section, in file order. + /// `runPreflight` runs them after the built-in rules. + final List checkRules; + /// The file this configuration was read from, if any. final String? source; @@ -215,7 +221,13 @@ class _Parser { } PreflightConfig parse(YamlMap doc) { - allowOnly(doc, const {'fail_on', 'paths', 'rules', 'ignore'}, 'the file'); + allowOnly(doc, const { + 'fail_on', + 'paths', + 'checks', + 'rules', + 'ignore', + }, 'the file'); Severity? failOn = Severity.error; final failOnValue = doc['fail_on']; @@ -233,9 +245,15 @@ class _Parser { } } + // Checks first: `rules` and `ignore` may name them as `check:`. + final checks = _checks(doc); + for (final check in checks) { + rules[check.id] = check; + } return PreflightConfig( failOn: failOn, paths: _paths(doc), + checkRules: checks, rules: _rules(doc), ignores: _ignores(doc), source: source, @@ -378,6 +396,218 @@ class _Parser { ); } + static final _checkName = RegExp(r'^[a-z0-9]+(-[a-z0-9]+)*$'); + + List _checks(YamlMap doc) { + final section = doc['checks']; + if (section == null) return const []; + if (section is! YamlMap) { + fail('`checks` must be a map of check names', nodeOf(doc, 'checks')); + } + return [ + for (final MapEntry(key: keyNode, value: valueNode) + in section.nodes.entries) + _check(keyNode, valueNode), + ]; + } + + ProjectCheckRule _check(Object? keyNode, YamlNode node) { + final name = keyNode is YamlScalar ? '${keyNode.value}' : '$keyNode'; + if (!_checkName.hasMatch(name)) { + fail( + 'check name `$name` must be kebab-case (lowercase letters, digits ' + 'and single hyphens), like `no-backup`', + keyNode, + ); + } + final where = '`checks.$name`'; + if (node is! YamlMap) fail('$where must be a map', node); + + final fileValue = optString(node, 'file'); + if (fileValue == null) { + fail( + '$where needs `file`: ' + '${CheckFile.values.map((f) => f.key).join(' or ')}', + node, + ); + } + final file = CheckFile.values.where((f) => f.key == fileValue).firstOrNull; + if (file == null) { + fail( + 'unknown file `$fileValue` in $where; expected one of ' + '${CheckFile.values.map((f) => f.key).join(', ')}', + nodeOf(node, 'file'), + ); + } + final operatorKeys = {for (final o in CheckOperator.values) o.key}; + allowOnly(node, { + 'file', + 'reason', + 'severity', + ...switch (file) { + CheckFile.infoPlist => const {'key'}, + CheckFile.androidManifest => const {'element', 'attribute'}, + }, + ...operatorKeys, + }, where); + + final reason = optString(node, 'reason'); + if (reason == null) { + fail( + '$where needs a non-empty `reason`, so the next person knows why ' + 'the value is pinned', + node, + ); + } + var severity = Severity.error; + if (node['severity'] != null) { + severity = + Severity.tryParse('${node['severity']}') ?? + fail( + '`severity` must be error, warning or info', + nodeOf(node, 'severity'), + ); + } + + final operators = [ + for (final o in CheckOperator.values) + if (node.containsKey(o.key)) o, + ]; + if (operators.length != 1) { + fail( + operators.isEmpty + ? '$where needs one of ${operatorKeys.join(', ')}' + : '$where has ${operators.map((o) => '`${o.key}`').join(' and ')}; ' + 'use one per check', + node, + ); + } + final operator = operators.single; + final expected = _operand( + operator, + file, + node.nodes[operator.key]!, + '$where `${operator.key}`', + ); + + switch (file) { + case CheckFile.infoPlist: + return ProjectCheckRule( + name: name, + file: file, + keyPath: _keyPath(node, where), + operator: operator, + expected: expected, + reason: reason, + severity: severity, + ); + case CheckFile.androidManifest: + final element = optString(node, 'element'); + final attribute = optString(node, 'attribute'); + if (element == null || attribute == null) { + fail( + '$where needs `element` and `attribute`, e.g. ' + '`element: application` and `attribute: android:allowBackup`', + node, + ); + } + return ProjectCheckRule( + name: name, + file: file, + element: element, + attribute: attribute, + operator: operator, + expected: expected, + reason: reason, + severity: severity, + ); + } + } + + /// `key: A.B.C` (a dot steps into a nested dictionary) or + /// `key: [A, B.with.dots]` (one item per dictionary, taken literally). + List _keyPath(YamlMap node, String where) { + final value = node['key']; + if (value == null) { + fail('$where needs `key`, e.g. `key: UIBackgroundModes`', node); + } + final keyNode = nodeOf(node, 'key'); + final segments = switch (value) { + final String s => s.split('.'), + final YamlList l when l.isNotEmpty && l.every((v) => v is String) => + l.cast().toList(), + _ => fail('`key` must be a string or a list of strings', keyNode), + }; + if (segments.any((s) => s.trim().isEmpty)) { + fail('`key` has an empty part: `$value`', keyNode); + } + return segments; + } + + /// Validates an operator's value and converts it to what + /// [ProjectCheckRule.expected] holds. + Object _operand( + CheckOperator operator, + CheckFile file, + YamlNode node, + String where, + ) { + final value = node.value; + final manifest = file == CheckFile.androidManifest; + final scalarHint = manifest + ? 'a string (attribute values are text: write `"false"`, not `false`)' + : 'a string, boolean or number'; + + Object scalar(YamlNode item) { + final v = item.value; + if (v is String) return v; + if (!manifest && (v is bool || v is num)) return v as Object; + fail('$where must be $scalarHint', item); + } + + switch (operator) { + case CheckOperator.exists || CheckOperator.absent: + if (value != true) { + fail( + '$where takes `true`; for the opposite, use ' + '`${operator == CheckOperator.exists ? 'absent' : 'exists'}: ' + 'true`', + node, + ); + } + return true; + case CheckOperator.equals || + CheckOperator.absentOrEquals || + CheckOperator.notEquals: + if (node is YamlList) { + if (manifest) { + fail( + '$where takes one value; use `contains` or `none_of` for ' + 'several', + node, + ); + } + return [for (final item in node.nodes) scalar(item)]; + } + return scalar(node); + case CheckOperator.contains || CheckOperator.noneOf: + if (node is YamlList) { + if (node.isEmpty) fail('$where must list at least one value', node); + return [for (final item in node.nodes) scalar(item)]; + } + return [scalar(node)]; + case CheckOperator.matches: + if (value is! String || value.isEmpty) { + fail('$where must be a regular expression, as a string', node); + } + try { + return RegExp(value); + } on FormatException catch (e) { + fail('$where is not a valid regular expression: ${e.message}', node); + } + } + } + /// Converts YAML collections into plain Dart ones so rules never depend on /// `package:yaml` types. static Object? _plain(Object? value) => switch (value) { diff --git a/lib/src/engine.dart b/lib/src/engine.dart index f7fc4df..6e37488 100644 --- a/lib/src/engine.dart +++ b/lib/src/engine.dart @@ -86,7 +86,8 @@ class PreflightReport { /// /// [today] fixes the date deadline rules measure against (default: now, in /// UTC). [rules] replaces the built-in rule set, which is mostly useful in -/// tests. [environment] is passed to [Project.load]. +/// tests; the project checks in [PreflightConfig.checkRules] run after them +/// either way. [environment] is passed to [Project.load]. PreflightReport runPreflight( String projectRoot, { PreflightConfig config = const PreflightConfig(), @@ -108,7 +109,7 @@ PreflightReport runPreflight( final suppressed = []; final usedIgnores = {}; - for (final rule in rules) { + for (final rule in [...rules, ...config.checkRules]) { final ruleConfig = config.forRule(rule.id); if (!ruleConfig.enabled) { disabled.add(DisabledRule(rule, ruleConfig.reason ?? '')); diff --git a/lib/src/project.dart b/lib/src/project.dart index 93b8461..c728ca7 100644 --- a/lib/src/project.dart +++ b/lib/src/project.dart @@ -173,6 +173,10 @@ class Project { /// `ios/Runner/Info.plist`. PlistFile? infoPlist; + /// Where [infoPlist] is read from (project-relative), whether or not it + /// exists. + late final String infoPlistPath; + /// The app's privacy manifest. PlistFile? privacyManifest; @@ -188,6 +192,10 @@ class Project { /// The main Android manifest. XmlFile? androidManifest; + /// Where [androidManifest] is read from (project-relative), whether or not + /// it exists. + late final String androidManifestPath; + /// The app module's Gradle script. TextFile? appGradle; @@ -267,7 +275,8 @@ class Project { pubspec = _yaml('pubspec.yaml'); _loadLock(); - infoPlist = _plist(paths.infoPlist ?? 'ios/Runner/Info.plist'); + infoPlistPath = paths.infoPlist ?? 'ios/Runner/Info.plist'; + infoPlist = _plist(infoPlistPath); privacyManifest = _plist( paths.privacyManifest ?? 'ios/Runner/PrivacyInfo.xcprivacy', ); @@ -281,7 +290,7 @@ class Project { } xcodeProject = _text('ios/Runner.xcodeproj/project.pbxproj'); - final manifestPath = + final manifestPath = androidManifestPath = paths.androidManifest ?? 'android/app/src/main/AndroidManifest.xml'; final manifestText = _read(manifestPath); if (manifestText != null) { diff --git a/lib/src/rules/project_checks.dart b/lib/src/rules/project_checks.dart new file mode 100644 index 0000000..7552d61 --- /dev/null +++ b/lib/src/rules/project_checks.dart @@ -0,0 +1,452 @@ +import '../finding.dart'; +import '../plist.dart'; +import '../rule.dart'; +import '../xml_tree.dart'; + +/// The prefix of every project check's rule id, e.g. `check:no-backup`. +/// +/// The colon keeps project checks from ever colliding with a built-in rule +/// id, which is kebab-case. +const checkRulePrefix = 'check:'; + +/// The file a project check reads. +enum CheckFile { + /// The app's `Info.plist` (`paths.info_plist`). + infoPlist('info_plist'), + + /// The app's main `AndroidManifest.xml` (`paths.android_manifest`). + androidManifest('android_manifest'); + + const CheckFile(this.key); + + /// How the file is named under `file:` in the configuration. + final String key; +} + +/// What a project check asserts about its value. +enum CheckOperator { + /// The value is present. + exists('exists'), + + /// The value is not present. + absent('absent'), + + /// The value is present and equal to the expected one. + equals('equals'), + + /// The value is absent, or present and equal to the expected one. + absentOrEquals('absent_or_equals'), + + /// The value is absent, or present and different from the expected one. + notEquals('not_equals'), + + /// The array (Info.plist) or the attribute values of every matching element + /// (AndroidManifest) contain each listed value. + contains('contains'), + + /// The value, an array item, or a matching element's attribute is none of + /// the listed values. + noneOf('none_of'), + + /// The value is a string the regular expression matches. + matches('matches'); + + const CheckOperator(this.key); + + /// How the operator is written in the configuration. + final String key; +} + +/// A project-specific invariant from the `checks:` section of the +/// configuration, run like any other rule under the id `check:`. +/// +/// The configuration parser validates every field, so the constructor +/// trusts its arguments: [expected] is `true` for [CheckOperator.exists] and +/// [CheckOperator.absent], a [RegExp] for [CheckOperator.matches], a +/// non-empty `List` of strings, booleans or numbers for +/// [CheckOperator.contains] and [CheckOperator.noneOf], and otherwise a +/// string, boolean, number or such a list (Info.plist) or a string +/// (AndroidManifest). +class ProjectCheckRule extends Rule { + /// Creates a check. + ProjectCheckRule({ + required this.name, + required this.file, + required this.operator, + required this.expected, + required this.reason, + this.keyPath = const [], + this.element = '', + this.attribute = '', + this.severity = Severity.error, + }); + + /// The check's name under `checks:`. + final String name; + + /// The file the check reads. + final CheckFile file; + + /// For [CheckFile.infoPlist]: the key, one segment per nested dictionary. + final List keyPath; + + /// For [CheckFile.androidManifest]: the element name, e.g. `application`. + final String element; + + /// For [CheckFile.androidManifest]: the attribute's qualified name, e.g. + /// `android:allowBackup`. + final String attribute; + + /// The assertion. + final CheckOperator operator; + + /// The operand; see the class documentation. + final Object expected; + + /// Why the value is pinned. Shown as the rule's [why]. + final String reason; + + /// The severity of every finding (default [Severity.error]). + final Severity severity; + + @override + String get id => '$checkRulePrefix$name'; + + @override + String get title => '${_subject()} ${_expectation()}'; + + @override + RulePlatform get platform => switch (file) { + CheckFile.infoPlist => RulePlatform.ios, + CheckFile.androidManifest => RulePlatform.android, + }; + + @override + Severity get defaultSeverity => severity; + + @override + String get why => reason; + + @override + String get fix => + 'Change the app so that ${_subject()} ${_expectation()}. If the pin ' + 'itself is out of date, update `checks.$name` in the configuration ' + 'file, and its reason with it.'; + + @override + List get docs => const []; + + String _subject() => switch (file) { + CheckFile.infoPlist => 'Info.plist `${keyPath.join('.')}`', + CheckFile.androidManifest => '<$element $attribute>', + }; + + String _expectation() => switch (operator) { + CheckOperator.exists => 'is present', + CheckOperator.absent => 'is absent', + CheckOperator.equals => 'equals ${_show(expected)}', + CheckOperator.absentOrEquals => 'is absent or equals ${_show(expected)}', + CheckOperator.notEquals => 'does not equal ${_show(expected)}', + CheckOperator.contains => 'contains ${_showEach(expected)}', + CheckOperator.noneOf => 'is none of ${_showEach(expected)}', + CheckOperator.matches => 'matches /${(expected as RegExp).pattern}/', + }; + + @override + Iterable check(RuleContext context) { + final project = context.project; + final (path, configKey) = switch (file) { + CheckFile.infoPlist => (project.infoPlistPath, 'paths.info_plist'), + CheckFile.androidManifest => ( + project.androidManifestPath, + 'paths.android_manifest', + ), + }; + final loaded = switch (file) { + CheckFile.infoPlist => project.infoPlist != null, + CheckFile.androidManifest => project.androidManifest != null, + }; + if (!loaded) { + // An unreadable file is reported once, by `unreadable-file`. + if (project.problems.any((problem) => problem.path == path)) return []; + return [ + finding( + '$path does not exist, so this check cannot run. Set `$configKey` ' + 'if the file is somewhere else.', + ), + ]; + } + return switch (file) { + CheckFile.infoPlist => _checkPlist(path, project.infoPlist!.root), + CheckFile.androidManifest => _checkManifest( + path, + project.androidManifest!.root, + ), + }; + } + + // Info.plist --------------------------------------------------------------- + + /// Looks up [keyPath]. Returns the value and the line of its key, or `null` + /// and the line of the deepest parent that exists. + (PlistValue?, int) _lookup(PlistDict root) { + var dict = root; + var parentLine = root.line; + for (var i = 0; i < keyPath.length; i++) { + final key = keyPath[i]; + final value = dict[key]; + if (value == null) return (null, parentLine); + final line = dict.lineOf(key); + if (i == keyPath.length - 1) return (value, line); + if (value is! PlistDict) return (null, line); + dict = value; + parentLine = line; + } + return (null, parentLine); + } + + Iterable _checkPlist(String path, PlistDict root) sync* { + final (value, line) = _lookup(root); + final key = '`${keyPath.join('.')}`'; + Finding at(String message, [int? where]) => + finding(message, path: path, line: where ?? line); + + if (value == null) { + switch (operator) { + case CheckOperator.exists: + yield at('$key is missing.'); + case CheckOperator.equals: + yield at('$key is missing; expected ${_show(expected)}.'); + case CheckOperator.contains: + yield at( + '$key is missing; expected an array containing ' + '${_showEach(expected)}.', + ); + case CheckOperator.matches: + yield at( + '$key is missing; expected a string matching ' + '/${(expected as RegExp).pattern}/.', + ); + case CheckOperator.absent || + CheckOperator.absentOrEquals || + CheckOperator.notEquals || + CheckOperator.noneOf: + break; + } + return; + } + + final actual = _showPlist(value); + switch (operator) { + case CheckOperator.exists: + break; + case CheckOperator.absent: + yield at('$key is present ($actual); expected it to be absent.'); + case CheckOperator.equals || CheckOperator.absentOrEquals: + if (!_plistEquals(expected, value)) { + yield at('$key is $actual; expected ${_show(expected)}.'); + } + case CheckOperator.notEquals: + if (_plistEquals(expected, value)) { + yield at('$key is $actual, which this check forbids.'); + } + case CheckOperator.contains: + if (value is! PlistArray) { + yield at('$key is $actual, not an array.'); + break; + } + for (final wanted in expected as List) { + if (!value.items.any((item) => _scalarEquals(wanted, item))) { + yield at('$key does not contain ${_show(wanted)}.'); + } + } + case CheckOperator.noneOf: + final forbidden = expected as List; + if (value is PlistArray) { + for (final item in value.items) { + if (forbidden.any((f) => _scalarEquals(f, item))) { + yield at( + '$key contains ${_showPlist(item)}, which this check ' + 'forbids.', + item.line, + ); + } + } + } else if (forbidden.any((f) => _scalarEquals(f, value))) { + yield at('$key is $actual, which this check forbids.'); + } + case CheckOperator.matches: + final pattern = expected as RegExp; + if (value is! PlistString) { + yield at('$key is $actual, not a string.'); + } else if (!pattern.hasMatch(value.value)) { + yield at( + '$key is $actual, which does not match /${pattern.pattern}/.', + ); + } + } + } + + // AndroidManifest ---------------------------------------------------------- + + Iterable _checkManifest(String path, XmlNode root) sync* { + final matching = <(XmlNode, XmlNode)>[]; + void walk(XmlNode node, XmlNode parent) { + if (node.name == element) matching.add((node, parent)); + for (final child in node.children) { + walk(child, node); + } + } + + walk(root, root); + Finding at(String message, int line) => + finding(message, path: path, line: line); + final tag = '<$element>'; + + if (matching.isEmpty) { + switch (operator) { + case CheckOperator.exists || + CheckOperator.equals || + CheckOperator.contains || + CheckOperator.matches: + yield at( + 'The manifest has no $tag element; expected one whose ' + '$attribute ${_expectation()}.', + root.line, + ); + case CheckOperator.absent || + CheckOperator.absentOrEquals || + CheckOperator.notEquals || + CheckOperator.noneOf: + break; + } + return; + } + + String has(String value) => '$tag has $attribute="$value"'; + switch (operator) { + case CheckOperator.exists: + if (!matching.any((m) => m.$1.attributes.containsKey(attribute))) { + yield at('No $tag sets $attribute.', matching.first.$1.line); + } + case CheckOperator.absent: + for (final (node, _) in matching) { + if (node.attributes[attribute] case final value?) { + yield at( + '${has(value)}; expected it to be absent.', + node.lineOf(attribute), + ); + } + } + case CheckOperator.equals || CheckOperator.absentOrEquals: + for (final (node, _) in matching) { + final value = node.attributes[attribute]; + if (value == null) { + if (operator == CheckOperator.equals) { + yield at( + '$tag has no $attribute; expected ${_show(expected)}.', + node.line, + ); + } + } else if (value != expected) { + yield at( + '${has(value)}; expected ${_show(expected)}.', + node.lineOf(attribute), + ); + } + } + case CheckOperator.notEquals: + for (final (node, _) in matching) { + if (node.attributes[attribute] case final value? + when value == expected) { + yield at( + '${has(value)}, which this check forbids.', + node.lineOf(attribute), + ); + } + } + case CheckOperator.contains: + final present = { + for (final (node, _) in matching) ?node.attributes[attribute], + }; + for (final wanted in expected as List) { + if (!present.contains(wanted)) { + yield at( + 'No $tag has $attribute="$wanted".', + matching.first.$2.line, + ); + } + } + case CheckOperator.noneOf: + final forbidden = expected as List; + for (final (node, _) in matching) { + if (node.attributes[attribute] case final value? + when forbidden.contains(value)) { + yield at( + '${has(value)}, which this check forbids.', + node.lineOf(attribute), + ); + } + } + case CheckOperator.matches: + final pattern = expected as RegExp; + for (final (node, _) in matching) { + final value = node.attributes[attribute]; + if (value == null) { + yield at( + '$tag has no $attribute; expected a value matching ' + '/${pattern.pattern}/.', + node.line, + ); + } else if (!pattern.hasMatch(value)) { + yield at( + '${has(value)}, which does not match /${pattern.pattern}/.', + node.lineOf(attribute), + ); + } + } + } + } +} + +bool _scalarEquals(Object expected, PlistValue actual) => + switch ((expected, actual)) { + (final bool e, PlistBool(:final value)) => e == value, + (final String e, PlistString(:final value)) => e == value, + (final num e, PlistNumber(:final value)) => e == value, + _ => false, + }; + +/// Whether [actual] equals [expected]. A list equals an array with the same +/// items in any order. +bool _plistEquals(Object expected, PlistValue actual) { + if (expected is! List) return _scalarEquals(expected, actual); + if (actual is! PlistArray || actual.items.length != expected.length) { + return false; + } + final remaining = [...actual.items]; + for (final wanted in expected) { + final i = remaining.indexWhere((item) => _scalarEquals(wanted, item)); + if (i < 0) return false; + remaining.removeAt(i); + } + return true; +} + +String _show(Object value) => switch (value) { + final String s => '"$s"', + final List l => '[${l.map(_show).join(', ')}]', + _ => '$value', +}; + +String _showEach(Object values) => + (values as List).map(_show).join(', '); + +String _showPlist(PlistValue value) => switch (value) { + PlistString(:final value) => '"$value"', + PlistBool(:final value) => '$value', + PlistNumber(:final value) => '$value', + PlistArray(:final items) => '[${items.map(_showPlist).join(', ')}]', + PlistDict() => 'a dictionary', + PlistOpaque(:final kind) => 'a <$kind>', +}; diff --git a/lib/src/version.dart b/lib/src/version.dart index 7bdcbb2..473f10a 100644 --- a/lib/src/version.dart +++ b/lib/src/version.dart @@ -1,5 +1,5 @@ /// The package version. Kept in sync with `pubspec.yaml` by a test. -const packageVersion = '0.2.0'; +const packageVersion = '0.3.0'; /// The package name, as published on pub.dev. const packageName = 'storehound'; diff --git a/lib/src/xml_tree.dart b/lib/src/xml_tree.dart index edf0dfe..4d76d90 100644 --- a/lib/src/xml_tree.dart +++ b/lib/src/xml_tree.dart @@ -19,6 +19,12 @@ class XmlNode { /// The 1-based line the element's start tag is on. final int line; + /// The 1-based line of each attribute, for start tags that span lines. + final Map attributeLines = {}; + + /// The line of attribute [attributeName], falling back to [line]. + int lineOf(String attributeName) => attributeLines[attributeName] ?? line; + /// Child elements, in document order. Comments are dropped. final List children = []; @@ -76,6 +82,9 @@ XmlNode parseXmlTree(String source) { final node = XmlNode(event.name, { for (final a in event.attributes) a.name: a.value, }, lineOf(event.start)); + if (event.attributes.isNotEmpty && event.start != null) { + _locateAttributes(source, event.start!, node, lineOf); + } if (stack.isEmpty) { root ??= node; } else { @@ -102,6 +111,45 @@ XmlNode parseXmlTree(String source) { return root; } +/// Records the line of each of [node]'s attributes by scanning its start +/// tag, which begins at [start] (the `<`), up to the first `>` outside a +/// quoted value. +void _locateAttributes( + String source, + int start, + XmlNode node, + int Function(int offset) lineOf, +) { + bool separates(String c) => c == '=' || c == '/' || c.trim().isEmpty; + var i = start + 1; + // Skip the element name. + while (i < source.length && !separates(source[i]) && source[i] != '>') { + i++; + } + String? quote; + var nameStart = -1; + for (; i < source.length; i++) { + final c = source[i]; + if (quote != null) { + if (c == quote) quote = null; + } else if (c == '"' || c == "'") { + quote = c; + } else if (c == '>') { + return; + } else if (separates(c)) { + if (nameStart >= 0) { + final name = source.substring(nameStart, i); + if (node.attributes.containsKey(name)) { + node.attributeLines[name] = lineOf(nameStart); + } + nameStart = -1; + } + } else if (nameStart < 0) { + nameStart = i; + } + } +} + List _lineStarts(String source) { final starts = [0]; for (var i = 0; i < source.length; i++) { diff --git a/lib/storehound.dart b/lib/storehound.dart index 1bde356..f8bf52f 100644 --- a/lib/storehound.dart +++ b/lib/storehound.dart @@ -19,4 +19,5 @@ export 'src/report/text_reporter.dart' show renderExplain, renderRuleList, renderText; export 'src/rule.dart'; export 'src/rules/all_rules.dart'; +export 'src/rules/project_checks.dart'; export 'src/version.dart'; diff --git a/pubspec.yaml b/pubspec.yaml index 0b032e5..216b8e1 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -3,7 +3,7 @@ description: >- Catch App Store and Google Play rejections in your Flutter project before review. A Dart CLI that lints Info.plist, privacy manifests, entitlements and AndroidManifest in CI. -version: 0.2.0 +version: 0.3.0 repository: https://github.com/saimskywalker/storehound issue_tracker: https://github.com/saimskywalker/storehound/issues topics: diff --git a/test/cli_test.dart b/test/cli_test.dart index 769a03e..671a38d 100644 --- a/test/cli_test.dart +++ b/test/cli_test.dart @@ -136,6 +136,59 @@ void main() { expect(run(['--explain', 'nope']).code, ExitCodes.usage); }); + group('project checks', () { + const checks = ''' +checks: + no-backup: + file: android_manifest + element: application + attribute: android:allowBackup + equals: "false" + reason: Tokens live in shared preferences. +'''; + String project() => writeProject({ + 'storehound.yaml': checks, + 'android/app/src/main/AndroidManifest.xml': + '\n' + ' \n' + '\n', + }); + + test('a failing check exits 1 and names its file and line', () { + final result = run([project(), '--no-color']); + expect(result.code, ExitCodes.findings, reason: result.out); + expect(result.out, contains('check:no-backup')); + expect(result.out, contains('AndroidManifest.xml:2')); + expect(result.out, contains('Tokens live in shared preferences.')); + }); + + test('--explain check: reads the configuration', () { + final root = project(); + final explain = run(['--explain', 'check:no-backup', root]); + expect(explain.code, ExitCodes.ok, reason: explain.err); + expect(explain.out, contains('check:no-backup (error, android)')); + expect(explain.out, contains('Tokens live in shared preferences.')); + + final unknown = run(['--explain', 'check:nope', root]); + expect(unknown.code, ExitCodes.usage); + expect(unknown.err, contains('has no `nope` under `checks:`')); + + final noConfig = run(['--explain', 'check:nope', writeProject({})]); + expect(noConfig.code, ExitCodes.usage); + expect(noConfig.err, contains('no configuration file')); + }); + + test('a mistake in a check exits 2 with its line', () { + final root = writeProject({ + 'storehound.yaml': checks.replaceFirst('"false"', 'false'), + }); + final result = run([root]); + expect(result.code, ExitCodes.usage); + expect(result.err, contains('storehound.yaml:6')); + expect(result.err, contains('attribute values are text')); + }); + }); + test('--version and --help', () { expect(run(['--version']).out.trim(), '$packageName $packageVersion'); final help = run(['--help']); diff --git a/test/config_test.dart b/test/config_test.dart index ab890a7..fd3cf68 100644 --- a/test/config_test.dart +++ b/test/config_test.dart @@ -155,6 +155,269 @@ ignore: ); }); + group('checks', () { + Matcher throwsConfigAt(Object message, int line) => throwsA( + isA() + .having((e) => e.message, 'message', message) + .having((e) => e.line, 'line', line), + ); + + test('reads Info.plist and AndroidManifest checks', () { + final config = parse(''' +checks: + ats-no-arbitrary-loads: + file: info_plist + key: NSAppTransportSecurity.NSAllowsArbitraryLoads + absent_or_equals: false + reason: All traffic is HTTPS. + no-media-permissions: + file: android_manifest + element: uses-permission + attribute: android:name + none_of: [android.permission.READ_MEDIA_IMAGES] + severity: warning + reason: The app uses the system photo picker only. +'''); + final [plist, manifest] = config.checkRules; + expect(plist.id, 'check:ats-no-arbitrary-loads'); + expect(plist.file, CheckFile.infoPlist); + expect(plist.keyPath, [ + 'NSAppTransportSecurity', + 'NSAllowsArbitraryLoads', + ]); + expect(plist.operator, CheckOperator.absentOrEquals); + expect(plist.expected, false); + expect(plist.defaultSeverity, Severity.error); + expect(plist.why, 'All traffic is HTTPS.'); + expect(manifest.file, CheckFile.androidManifest); + expect(manifest.element, 'uses-permission'); + expect(manifest.attribute, 'android:name'); + expect(manifest.expected, ['android.permission.READ_MEDIA_IMAGES']); + expect(manifest.defaultSeverity, Severity.warning); + }); + + test('a key can be a list of literal parts', () { + final check = parse(''' +checks: + subdomains: + file: info_plist + key: [NSAppTransportSecurity, NSExceptionDomains, example.com] + exists: true + reason: The API host is pinned. +''').checkRules.single; + expect(check.keyPath, [ + 'NSAppTransportSecurity', + 'NSExceptionDomains', + 'example.com', + ]); + }); + + test('rules and ignore can name a check', () { + final config = parse(''' +checks: + no-backup: + file: android_manifest + element: application + attribute: android:allowBackup + equals: "false" + reason: Tokens live in shared preferences. +rules: + check:no-backup: + severity: warning +ignore: + - rule: check:no-backup + reason: Being fixed in the next release. +'''); + expect(config.forRule('check:no-backup').severity, Severity.warning); + expect(config.ignores.single.rule, 'check:no-backup'); + expect( + () => parse('ignore:\n - rule: check:nope\n reason: x\n'), + throwsConfig(contains('unknown rule `check:nope`')), + ); + }); + + test('every check needs a reason', () { + expect( + () => parse(''' +checks: + pin: + file: info_plist + key: ITSAppUsesNonExemptEncryption + equals: false +'''), + throwsConfigAt(contains('non-empty `reason`'), 3), + ); + }); + + test('an unknown or missing file is an error', () { + expect( + () => parse(''' +checks: + pin: + file: build_gradle + key: minSdk + equals: 24 + reason: x +'''), + throwsConfigAt( + 'unknown file `build_gradle` in `checks.pin`; expected one of ' + 'info_plist, android_manifest', + 3, + ), + ); + expect( + () => parse('checks:\n pin:\n key: A\n exists: true\n'), + throwsConfigAt(contains('needs `file`'), 3), + ); + }); + + test('an unknown operator is an error that lists the operators', () { + expect( + () => parse(''' +checks: + pin: + file: info_plist + key: CFBundleVersion + greater_than: 3 + reason: x +'''), + throwsConfigAt( + allOf(contains('unknown key `greater_than`'), contains('none_of')), + 5, + ), + ); + }); + + test('exactly one operator per check', () { + expect( + () => parse(''' +checks: + pin: + file: info_plist + key: CFBundleVersion + reason: x +'''), + throwsConfigAt(contains('needs one of exists, absent, equals'), 3), + ); + expect( + () => parse(''' +checks: + pin: + file: info_plist + key: CFBundleVersion + exists: true + matches: x + reason: x +'''), + throwsConfigAt(contains('has `exists` and `matches`'), 3), + ); + }); + + test('keys of the other file kind are errors', () { + expect( + () => parse(''' +checks: + pin: + file: android_manifest + key: android:allowBackup + exists: true + reason: x +'''), + throwsConfigAt(contains('unknown key `key`'), 4), + ); + expect( + () => parse(''' +checks: + pin: + file: android_manifest + element: application + exists: true + reason: x +'''), + throwsConfigAt(contains('needs `element` and `attribute`'), 3), + ); + expect( + () => parse(''' +checks: + pin: + file: info_plist + exists: true + reason: x +'''), + throwsConfigAt(contains('needs `key`'), 3), + ); + }); + + test('wrong value types are errors with the value line', () { + String check(String file, String rest) => + 'checks:\n pin:\n file: $file\n reason: x\n$rest'; + String plistCheck(String op) => check('info_plist', ' key: K\n$op'); + String manifestCheck(String op) => check( + 'android_manifest', + ' element: application\n attribute: android:allowBackup\n$op', + ); + expect( + () => parse(manifestCheck(' equals: false\n')), + throwsConfigAt(contains('write `"false"`, not `false`'), 7), + ); + expect( + () => parse(manifestCheck(' equals: [a, b]\n')), + throwsConfigAt(contains('use `contains` or `none_of`'), 7), + ); + expect( + () => parse(plistCheck(' equals: {a: 1}\n')), + throwsConfigAt(contains('a string, boolean or number'), 6), + ); + expect( + () => parse(plistCheck(' equals:\n')), + throwsConfigAt(contains('a string, boolean or number'), 6), + ); + expect( + () => parse(plistCheck(' equals: [a, [b]]\n')), + throwsConfigAt(contains('a string, boolean or number'), 6), + ); + expect( + () => parse(plistCheck(' contains: []\n')), + throwsConfigAt(contains('at least one value'), 6), + ); + expect( + () => parse(plistCheck(' exists: false\n')), + throwsConfigAt(contains('use `absent: true`'), 6), + ); + expect( + () => parse(plistCheck(' matches: 3\n')), + throwsConfigAt(contains('a regular expression'), 6), + ); + expect( + () => parse(plistCheck(' matches: "("\n')), + throwsConfigAt(contains('not a valid regular expression'), 6), + ); + expect( + () => parse(check('info_plist', ' key: A..B\n exists: true\n')), + throwsConfigAt(contains('empty part'), 5), + ); + expect( + () => parse(check('info_plist', ' key: {A: B}\n exists: true\n')), + throwsConfigAt(contains('a string or a list of strings'), 5), + ); + }); + + test('check names are kebab-case and checks is a map', () { + expect( + () => parse('checks:\n No_Backup:\n file: info_plist\n'), + throwsConfigAt(contains('must be kebab-case'), 2), + ); + expect( + () => parse('checks:\n - pin\n'), + throwsConfigAt(contains('`checks` must be a map'), 2), + ); + expect( + () => parse('checks:\n pin: true\n'), + throwsConfigAt(contains('`checks.pin` must be a map'), 2), + ); + }); + }); + group('IgnoreEntry.matches', () { const finding = Finding( ruleId: 'alpha', diff --git a/test/fixtures/README.md b/test/fixtures/README.md index ae1c872..ceaeebd 100644 --- a/test/fixtures/README.md +++ b/test/fixtures/README.md @@ -61,6 +61,9 @@ What it covers: the `applinks:` associated domains. - **Two ExportOptions files**: App Store Connect (build number managed by CI) and ad hoc (`release-testing`). +- **Project checks** in `storehound.yaml` that pin the encryption answer, + the exact background modes, no arbitrary loads, `android:allowBackup` and + the absent media permissions. Not covered yet, because no rule reads these files: per-flavor and per-build-type Android manifests beyond `src/main`, per-flavor Info.plist or diff --git a/test/fixtures/production_app.golden.txt b/test/fixtures/production_app.golden.txt index 13be8e2..d0cc881 100644 --- a/test/fixtures/production_app.golden.txt +++ b/test/fixtures/production_app.golden.txt @@ -1 +1 @@ -✓ 15 rules checked: 0 errors, 0 warnings, 0 info. +✓ 20 rules checked: 0 errors, 0 warnings, 0 info. diff --git a/test/fixtures/production_app/storehound.yaml b/test/fixtures/production_app/storehound.yaml index 02f8533..a6ffb5f 100644 --- a/test/fixtures/production_app/storehound.yaml +++ b/test/fixtures/production_app/storehound.yaml @@ -17,3 +17,35 @@ rules: # CoreLocation (seen with nm -u on a release build); the app never # requests location. acme_payments: [NSLocationWhenInUseUsageDescription] + +# Pins this app keeps on top of the built-in rules. +checks: + encryption-exempt: + file: info_plist + key: ITSAppUsesNonExemptEncryption + equals: false + reason: Export compliance is answered once in App Store Connect. + background-modes-pinned: + file: info_plist + key: UIBackgroundModes + equals: [audio, remote-notification] + reason: The modes App Review accepted. A new one needs review notes first. + ats-no-arbitrary-loads: + file: info_plist + key: NSAppTransportSecurity.NSAllowsArbitraryLoads + absent_or_equals: false + reason: All traffic is HTTPS. + no-backup: + file: android_manifest + element: application + attribute: android:allowBackup + equals: "false" + reason: The session token lives in shared preferences. + no-media-permissions: + file: android_manifest + element: uses-permission + attribute: android:name + none_of: + - android.permission.READ_MEDIA_IMAGES + - android.permission.READ_MEDIA_VIDEO + reason: Receipts come through the system document picker, which needs no permission. diff --git a/test/production_app_test.dart b/test/production_app_test.dart index a1cb320..e8f440e 100644 --- a/test/production_app_test.dart +++ b/test/production_app_test.dart @@ -140,7 +140,17 @@ void main() { '\t\tfetch\n', ), }), - [isFinding(message: contains('`fetch`'))], + [ + isFinding(message: contains('`fetch`')), + // The pinned list in the fixture's `checks:` catches it too. + isA() + .having( + (f) => f.ruleId, + 'ruleId', + 'check:background-modes-pinned', + ) + .having((f) => f.line, 'line', 1081), + ], ); }); @@ -194,6 +204,57 @@ void main() { ); }); + test('a project check: backups are switched on', () { + expect( + runMutated({ + manifest: replace( + 'android:allowBackup="false"', + 'android:allowBackup="true"', + ), + }), + [ + isA() + .having((f) => f.ruleId, 'ruleId', 'check:no-backup') + .having((f) => f.path, 'path', manifest) + .having((f) => f.line, 'line', 23), + ], + ); + }); + + test('a project check: a media permission is added', () { + expect( + runMutated({ + manifest: replace( + ' \n', + ' \n' + ' \n', + ), + }), + [ + isA() + .having((f) => f.ruleId, 'ruleId', 'check:no-media-permissions') + .having((f) => f.line, 'line', 13), + ], + ); + }); + + test('a project check: the encryption answer flips', () { + expect( + runMutated({ + info: replace( + '\tITSAppUsesNonExemptEncryption\n\t', + '\tITSAppUsesNonExemptEncryption\n\t', + ), + }), + [ + isA() + .having((f) => f.ruleId, 'ruleId', 'check:encryption-exempt') + .having((f) => f.message, 'message', contains('is true')) + .having((f) => f.line, 'line', 61), + ], + ); + }); + test('targetSdk falls below the Play deadline', () { expect( runMutated({ diff --git a/test/rules/project_checks_test.dart b/test/rules/project_checks_test.dart new file mode 100644 index 0000000..56703cd --- /dev/null +++ b/test/rules/project_checks_test.dart @@ -0,0 +1,909 @@ +import 'dart:convert'; + +import 'package:storehound/storehound.dart'; +import 'package:test/test.dart'; + +import '../support/fixtures.dart'; + +const info = 'ios/Runner/Info.plist'; +const manifestPath = 'android/app/src/main/AndroidManifest.xml'; + +/// Runs only the project checks in [config] (a `storehound.yaml` body) +/// against a project built from [files]. +PreflightReport runChecks(String config, Map files) { + final root = writeProject(files); + return runPreflight( + root, + rules: const [], + config: PreflightConfig.parse(config, builtInRules), + today: fixedToday, + ); +} + +List findings(String config, Map files) => + runChecks(config, files).findings; + +/// One Info.plist check named `pin` with [body] as its key and operator. +String plistCheck(String body) => + ''' +checks: + pin: + file: info_plist + reason: Pinned on purpose. +$body +'''; + +/// One AndroidManifest check named `pin` on [element] / [attribute]. +String manifestCheck(String element, String attribute, String body) => + ''' +checks: + pin: + file: android_manifest + element: $element + attribute: $attribute + reason: Pinned on purpose. +$body +'''; + +/// The 1-based line of the first line of [text] containing [needle]. +int lineIn(String text, String needle) { + final lines = const LineSplitter().convert(text); + final i = lines.indexWhere((l) => l.contains(needle)); + if (i < 0) throw StateError('no line contains $needle'); + return i + 1; +} + +final encryption = infoPlist(''' + ITSAppUsesNonExemptEncryption + + UIBackgroundModes + + audio + remote-notification + + NSAppTransportSecurity + + NSAllowsArbitraryLoadsInWebContent + + NSExceptionDomains + + example.com + + NSIncludesSubdomains + + + + + CFBundleVersion + 42 + UIRequiredDeviceCapabilities + 7'''); + +/// The `` line of an [infoPlist] fixture. +final rootDictLine = lineIn(encryption, ''); + +final manifest = ''' + + + + + + + +'''; + +void main() { + group('info_plist', () { + Map files() => {info: encryption}; + + test('exists: passes when present, reports the parent line when ' + 'missing', () { + expect( + findings( + plistCheck(' key: CFBundleVersion\n exists: true'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck(' key: CFBundleName\n exists: true'), + files(), + ), + [ + isFinding( + message: '`CFBundleName` is missing.', + path: info, + line: rootDictLine, + severity: Severity.error, + ), + ], + ); + }); + + test('absent: reports the key line when present', () { + expect( + findings( + plistCheck(' key: NSAllowsArbitraryLoads\n absent: true'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck( + ' key: ITSAppUsesNonExemptEncryption\n absent: true', + ), + files(), + ), + [ + isFinding( + message: + '`ITSAppUsesNonExemptEncryption` is present (false); ' + 'expected it to be absent.', + line: lineIn(encryption, 'ITSAppUsesNonExemptEncryption'), + ), + ], + ); + }); + + test('equals: a boolean, a string and a number', () { + expect( + findings( + plistCheck( + ' key: ITSAppUsesNonExemptEncryption\n equals: false', + ), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck(' key: CFBundleVersion\n equals: "42"'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck(' key: UIRequiredDeviceCapabilities\n equals: 7'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck( + ' key: ITSAppUsesNonExemptEncryption\n equals: true', + ), + files(), + ), + [ + isFinding( + message: '`ITSAppUsesNonExemptEncryption` is false; expected true.', + line: lineIn(encryption, 'ITSAppUsesNonExemptEncryption'), + ), + ], + ); + }); + + test('equals: types must match, so the string "42" is not 42', () { + expect( + findings( + plistCheck(' key: CFBundleVersion\n equals: 42'), + files(), + ), + [isFinding(message: '`CFBundleVersion` is "42"; expected 42.')], + ); + }); + + test('equals: a missing key is reported at the parent', () { + expect( + findings( + plistCheck(' key: LSRequiresIPhoneOS\n equals: true'), + files(), + ), + [ + isFinding( + message: '`LSRequiresIPhoneOS` is missing; expected true.', + line: rootDictLine, + ), + ], + ); + }); + + test('equals: a list is the same items in any order', () { + expect( + findings( + plistCheck( + ' key: UIBackgroundModes\n' + ' equals: [remote-notification, audio]', + ), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck(' key: UIBackgroundModes\n equals: [audio]'), + files(), + ), + [ + isFinding( + message: + '`UIBackgroundModes` is ["audio", "remote-notification"]; ' + 'expected ["audio"].', + line: lineIn(encryption, 'UIBackgroundModes'), + ), + ], + ); + }); + + test('a dotted key reads nested dictionaries; a missing nested key is ' + 'reported at its parent key', () { + expect( + findings( + plistCheck( + ' key: NSAppTransportSecurity.NSAllowsArbitraryLoadsInWebContent\n' + ' equals: true', + ), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck( + ' key: NSAppTransportSecurity.NSAllowsArbitraryLoads\n' + ' equals: false', + ), + files(), + ), + [ + isFinding( + message: + '`NSAppTransportSecurity.NSAllowsArbitraryLoads` is ' + 'missing; expected false.', + line: lineIn(encryption, 'NSAppTransportSecurity'), + ), + ], + ); + }); + + test('a key given as a list takes each part literally, dots included', () { + expect( + findings( + plistCheck( + ' key: [NSAppTransportSecurity, NSExceptionDomains, example.com,' + ' NSIncludesSubdomains]\n' + ' equals: true', + ), + files(), + ), + isEmpty, + ); + }); + + test('a path through a value that is not a dictionary is missing', () { + expect( + findings( + plistCheck(' key: CFBundleVersion.Major\n exists: true'), + files(), + ), + [ + isFinding( + message: '`CFBundleVersion.Major` is missing.', + line: lineIn(encryption, 'CFBundleVersion'), + ), + ], + ); + }); + + test( + 'absent_or_equals: absent or equal passes, a different value fails', + () { + const key = 'NSAppTransportSecurity.NSAllowsArbitraryLoads'; + expect( + findings( + plistCheck(' key: $key\n absent_or_equals: false'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck( + ' key: ITSAppUsesNonExemptEncryption\n' + ' absent_or_equals: false', + ), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck( + ' key: ITSAppUsesNonExemptEncryption\n' + ' absent_or_equals: true', + ), + files(), + ), + [isFinding(message: contains('is false; expected true'))], + ); + }, + ); + + test( + 'not_equals: absent or different passes, the forbidden value fails', + () { + expect( + findings( + plistCheck(' key: CFBundleName\n not_equals: x'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck(' key: CFBundleVersion\n not_equals: "1"'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck(' key: CFBundleVersion\n not_equals: "42"'), + files(), + ), + [ + isFinding( + message: '`CFBundleVersion` is "42", which this check forbids.', + line: lineIn(encryption, 'CFBundleVersion'), + ), + ], + ); + }, + ); + + test('contains: one finding per value the array lacks', () { + expect( + findings( + plistCheck(' key: UIBackgroundModes\n contains: audio'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck( + ' key: UIBackgroundModes\n contains: [audio, fetch, processing]', + ), + files(), + ), + [ + isFinding(message: '`UIBackgroundModes` does not contain "fetch".'), + isFinding( + message: '`UIBackgroundModes` does not contain "processing".', + line: lineIn(encryption, 'UIBackgroundModes'), + ), + ], + ); + }); + + test('contains: a missing key or a value that is not an array fails', () { + expect( + findings( + plistCheck(' key: CFBundleLocalizations\n contains: [en]'), + files(), + ), + [ + isFinding( + message: contains('is missing; expected an array containing "en"'), + ), + ], + ); + expect( + findings( + plistCheck(' key: CFBundleVersion\n contains: [x]'), + files(), + ), + [isFinding(message: '`CFBundleVersion` is "42", not an array.')], + ); + }); + + test('none_of: reports each forbidden array item on its own line', () { + expect( + findings( + plistCheck( + ' key: UIBackgroundModes\n none_of: [fetch, location]', + ), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck(' key: CFBundleName\n none_of: [x]'), + files(), + ), + isEmpty, + ); + expect( + findings( + plistCheck( + ' key: UIBackgroundModes\n none_of: [remote-notification]', + ), + files(), + ), + [ + isFinding( + message: + '`UIBackgroundModes` contains "remote-notification", ' + 'which this check forbids.', + line: lineIn(encryption, 'remote-notification'), + ), + ], + ); + expect( + findings( + plistCheck(' key: CFBundleVersion\n none_of: ["42"]'), + files(), + ), + [ + isFinding( + message: '`CFBundleVersion` is "42", which this check forbids.', + ), + ], + ); + }); + + test('matches: a regular expression on a string value', () { + expect( + findings( + plistCheck( + ' key: CFBundleIdentifier\n' + r" matches: '^com\.example\.'", + ), + {info: infoPlist()}, + ), + isEmpty, + ); + expect( + findings( + plistCheck( + ' key: CFBundleVersion\n' + r" matches: '^\d+\.\d+$'", + ), + files(), + ), + [ + isFinding( + message: + r'`CFBundleVersion` is "42", which does not match /^\d+\.\d+$/.', + ), + ], + ); + expect( + findings( + plistCheck(' key: UIBackgroundModes\n matches: audio'), + files(), + ), + [ + isFinding( + message: + '`UIBackgroundModes` is ["audio", "remote-notification"], ' + 'not a string.', + ), + ], + ); + expect( + findings(plistCheck(' key: CFBundleName\n matches: x'), files()), + [ + isFinding( + message: + '`CFBundleName` is missing; expected a string matching /x/.', + ), + ], + ); + }); + + test('paths.info_plist moves the file the check reads', () { + expect( + findings( + 'paths:\n info_plist: app/Info.plist\n' + '${plistCheck(' key: ITSAppUsesNonExemptEncryption\n equals: false')}', + {'app/Info.plist': encryption}, + ), + isEmpty, + ); + }); + + test('a missing file is a finding, an unreadable one is left to ' + 'unreadable-file', () { + final check = plistCheck(' key: CFBundleVersion\n exists: true'); + expect(findings(check, {}), [ + isFinding(message: contains('ios/Runner/Info.plist does not exist')), + ]); + expect(findings(check, {info: ''}), isEmpty); + }); + }); + + group('android_manifest', () { + Map files() => {manifestPath: manifest}; + final backupLine = lineIn(manifest, 'android:allowBackup'); + final applicationLine = lineIn(manifest, ' has android:allowBackup="false"; ' + 'expected "true".', + path: manifestPath, + line: backupLine, + ), + ], + ); + }); + + test('equals: a missing attribute is reported at its element', () { + expect( + findings( + manifestCheck( + 'application', + 'android:fullBackupContent', + ' equals: "false"', + ), + files(), + ), + [ + isFinding( + message: + ' has no android:fullBackupContent; ' + 'expected "false".', + line: applicationLine, + ), + ], + ); + }); + + test('a missing element is reported at the root element', () { + expect( + findings( + manifestCheck('queries', 'android:name', ' exists: true'), + files(), + ), + [ + isFinding( + message: contains('The manifest has no element'), + line: lineIn(manifest, ' sets android:theme.')], + ); + expect( + findings( + manifestCheck( + 'application', + 'android:debuggable', + ' absent: true', + ), + files(), + ), + isEmpty, + ); + expect( + findings( + manifestCheck('application', 'android:label', ' absent: true'), + files(), + ), + [ + isFinding( + message: + ' has android:label="fixture"; ' + 'expected it to be absent.', + line: lineIn(manifest, 'android:label'), + ), + ], + ); + }); + + test('absent_or_equals and not_equals', () { + expect( + findings( + manifestCheck( + 'application', + 'android:usesCleartextTraffic', + ' absent_or_equals: "false"', + ), + files(), + ), + isEmpty, + ); + expect( + findings( + manifestCheck( + 'application', + 'android:allowBackup', + ' absent_or_equals: "true"', + ), + files(), + ), + [isFinding(message: contains('expected "true"'), line: backupLine)], + ); + expect( + findings( + manifestCheck( + 'activity', + 'android:exported', + ' not_equals: "false"', + ), + files(), + ), + isEmpty, + ); + expect( + findings( + manifestCheck( + 'activity', + 'android:exported', + ' not_equals: "true"', + ), + files(), + ), + [ + isFinding( + message: + ' has android:exported="true", which this check ' + 'forbids.', + line: lineIn(manifest, ' has ' + 'android:name="android.permission.POST_NOTIFICATIONS".', + line: lineIn(manifest, ' has android:name=' + '"android.permission.CAMERA", which this check forbids.', + line: lineIn(manifest, 'permission.CAMERA'), + ), + ]); + }); + + test('matches', () { + expect( + findings( + manifestCheck('activity', 'android:name', r" matches: '^\.'"), + files(), + ), + isEmpty, + ); + expect( + findings( + manifestCheck('application', 'android:label', ' matches: ^Acme'), + files(), + ), + [ + isFinding( + message: + ' has android:label="fixture", which does ' + 'not match /^Acme/.', + ), + ], + ); + expect( + findings( + manifestCheck('application', 'android:icon', ' matches: x'), + files(), + ), + [ + isFinding( + message: contains('has no android:icon'), + line: applicationLine, + ), + ], + ); + }); + + test('a missing manifest is a finding', () { + expect( + findings( + manifestCheck( + 'application', + 'android:allowBackup', + ' exists: true', + ), + {}, + ), + [isFinding(message: contains('paths.android_manifest'))], + ); + }); + }); + + group('like any rule', () { + final config = ''' +checks: + no-backup: + file: android_manifest + element: application + attribute: android:allowBackup + equals: "true" + reason: Restores keep the cart after a reinstall. + encryption-exempt: + file: info_plist + key: ITSAppUsesNonExemptEncryption + equals: true + severity: warning + reason: The app ships its own crypto and has an export compliance code. +'''; + Map files() => {info: encryption, manifestPath: manifest}; + + test('rule ids are check:; severity defaults to error', () { + expect(findings(config, files()).map((f) => (f.ruleId, f.severity)), [ + ('check:no-backup', Severity.error), + ('check:encryption-exempt', Severity.warning), + ]); + }); + + test('ignore suppresses a check and rules can change its severity', () { + final report = runChecks(''' +$config +rules: + check:encryption-exempt: + severity: info +ignore: + - rule: check:no-backup + reason: Backups are being switched back on in the next release. +''', files()); + expect(report.findings.single.ruleId, 'check:encryption-exempt'); + expect(report.findings.single.severity, Severity.info); + expect(report.suppressed.single.finding.ruleId, 'check:no-backup'); + }); + + test('text, JSON and SARIF report the check with its file and line', () { + final report = runChecks(config, files()); + final text = renderText(report); + expect( + text, + contains( + 'check:no-backup — equals "true"', + ), + ); + expect( + text, + contains('$manifestPath:${lineIn(manifest, 'allowBackup')}'), + ); + expect(text, contains('why: Restores keep the cart')); + + final json = jsonDecode(renderJson(report)) as Map; + expect((json['findings']! as List).first, { + 'rule': 'check:no-backup', + 'severity': 'error', + 'message': + ' has android:allowBackup="false"; ' + 'expected "true".', + 'path': manifestPath, + 'line': lineIn(manifest, 'allowBackup'), + }); + expect((json['rules']! as List).map((r) => (r as Map)['id']), [ + 'check:no-backup', + 'check:encryption-exempt', + ]); + + final sarif = jsonDecode(renderSarif(report)) as Map; + final run = (sarif['runs']! as List).single as Map; + final result = (run['results']! as List).last as Map; + expect(result['ruleId'], 'check:encryption-exempt'); + expect(result['level'], 'warning'); + expect( + (((result['locations']! as List).single as Map)['physicalLocation'] + as Map)['region'], + {'startLine': lineIn(encryption, 'ITSAppUsesNonExemptEncryption')}, + ); + final driver = (run['tool']! as Map)['driver']! as Map; + final rule = (driver['rules']! as List).last as Map; + expect(rule['id'], 'check:encryption-exempt'); + expect(rule['defaultConfiguration'], {'level': 'warning'}); + expect(rule.containsKey('helpUri'), isFalse); + }); + + test('explain shows the reason and the expected value', () { + final check = PreflightConfig.parse( + config, + builtInRules, + ).checkRules.first; + final text = renderExplain(check); + expect(text, startsWith('check:no-backup (error, android)')); + expect(text, contains('Restores keep the cart')); + expect(text, contains('android:allowBackup> equals "true"')); + }); + }); +} diff --git a/test/xml_tree_test.dart b/test/xml_tree_test.dart new file mode 100644 index 0000000..5603aa2 --- /dev/null +++ b/test/xml_tree_test.dart @@ -0,0 +1,25 @@ +import 'package:storehound/src/xml_tree.dart'; +import 'package:test/test.dart'; + +void main() { + test('attributes of a start tag that spans lines have their own lines', () { + final root = parseXmlTree(''' + + + + +'''); + final application = root.children.first; + expect(application.line, 2); + expect(application.lineOf('android:label'), 3); + expect(application.lineOf('android:allowBackup'), 4); + expect(application.lineOf('android:icon'), 5); + expect(application.lineOf('android:theme'), 2); + final activity = root.children.last; + expect(activity.lineOf('android:name'), 6); + expect(activity.lineOf('android:exported'), 6); + }); +} From 7859f4f069f6731ab811110f07700cd82800d441 Mon Sep 17 00:00:00 2001 From: saimskywalker <15120878+saimskywalker@users.noreply.github.com> Date: Sat, 7 Nov 2026 01:16:27 +0800 Subject: [PATCH 2/3] chore: keep 0.2.0, changelog under Unreleased; document disabling a check --- CHANGELOG.md | 2 +- doc/configuration.md | 10 ++++++++++ lib/src/version.dart | 2 +- pubspec.yaml | 2 +- test/rules/project_checks_test.dart | 13 +++++++++++++ 5 files changed, 26 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03498cd..71bf4f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## 0.3.0 +## Unreleased - Project checks: app-specific pins under `checks:` in `storehound.yaml`, for `Info.plist` keys (with nested keys) and `AndroidManifest.xml` attributes, diff --git a/doc/configuration.md b/doc/configuration.md index 36c55cc..617a53f 100644 --- a/doc/configuration.md +++ b/doc/configuration.md @@ -177,6 +177,16 @@ manifest, or `` when no element has that name. A check whose file does not exist is a finding too, so a moved file cannot switch it off silently. A check can be named in `ignore` (`rule: check:no-backup`) and under `rules:` like any rule, and `storehound --explain check:no-backup` prints it. +To switch a check off for a while without deleting it, disable it under +`rules:` with a reason, as for a built-in rule: + +```yaml +rules: + check:no-backup: + enabled: false + reason: "Backups are back on until the token moves to the keystore." +``` + Mistakes in a check (an unknown file or operator, a value of the wrong type, a missing `reason`) are configuration errors with the line number, exit code 2. diff --git a/lib/src/version.dart b/lib/src/version.dart index 473f10a..7bdcbb2 100644 --- a/lib/src/version.dart +++ b/lib/src/version.dart @@ -1,5 +1,5 @@ /// The package version. Kept in sync with `pubspec.yaml` by a test. -const packageVersion = '0.3.0'; +const packageVersion = '0.2.0'; /// The package name, as published on pub.dev. const packageName = 'storehound'; diff --git a/pubspec.yaml b/pubspec.yaml index 216b8e1..0b032e5 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -3,7 +3,7 @@ description: >- Catch App Store and Google Play rejections in your Flutter project before review. A Dart CLI that lints Info.plist, privacy manifests, entitlements and AndroidManifest in CI. -version: 0.3.0 +version: 0.2.0 repository: https://github.com/saimskywalker/storehound issue_tracker: https://github.com/saimskywalker/storehound/issues topics: diff --git a/test/rules/project_checks_test.dart b/test/rules/project_checks_test.dart index 56703cd..a4b08ef 100644 --- a/test/rules/project_checks_test.dart +++ b/test/rules/project_checks_test.dart @@ -848,6 +848,19 @@ ignore: expect(report.suppressed.single.finding.ruleId, 'check:no-backup'); }); + test('rules can disable a check with a reason', () { + final report = runChecks(''' +$config +rules: + check:no-backup: + enabled: false + reason: Backups are back on until the token moves to the keystore. +''', files()); + expect(report.findings.map((f) => f.ruleId), ['check:encryption-exempt']); + expect(report.disabled.single.rule.id, 'check:no-backup'); + expect(report.disabled.single.reason, contains('keystore')); + }); + test('text, JSON and SARIF report the check with its file and line', () { final report = runChecks(config, files()); final text = renderText(report); From 6b4eadc06edb40dd722690d6062c699f3512c820 Mon Sep 17 00:00:00 2001 From: saimskywalker <15120878+saimskywalker@users.noreply.github.com> Date: Sat, 7 Nov 2026 01:17:24 +0800 Subject: [PATCH 3/3] feat: list project checks in --list-rules Part of #51. --- CHANGELOG.md | 2 + doc/configuration.md | 2 +- lib/src/cli.dart | 75 ++++++++++++++++++++++--------- lib/src/report/text_reporter.dart | 25 ++++++++++- test/cli_test.dart | 28 ++++++++++++ 5 files changed, 108 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 71bf4f6..301d628 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ it is missing). `ignore`, `rules` and `--explain check:` accept it. A mistake in a check is a configuration error with its line (exit code 2). - The production-shaped fixture pins five values with `checks:`. +- `--list-rules` also lists the project checks from the configuration file, + with their severity and reason. ## 0.2.0 diff --git a/doc/configuration.md b/doc/configuration.md index 617a53f..b4b5c46 100644 --- a/doc/configuration.md +++ b/doc/configuration.md @@ -14,7 +14,7 @@ storehound [options] [project-dir] --today=YYYY-MM-DD Date deadlines are measured against (reproducible runs) --brief Text output without the why/fix explanations --[no-]color Colour text output (default: when stdout is a terminal and NO_COLOR is unset) - --list-rules List every rule + --list-rules List every rule, then the project checks from the configuration --explain= Explain one rule, or a project check as check: --version Print the version -h, --help Show the usage diff --git a/lib/src/cli.dart b/lib/src/cli.dart index 26cd1ca..a6868ae 100644 --- a/lib/src/cli.dart +++ b/lib/src/cli.dart @@ -79,7 +79,13 @@ ArgParser _parser() => ArgParser(usageLineLength: 80) 'Colour text output. Default: when stdout is a terminal and ' 'NO_COLOR is unset.', ) - ..addFlag('list-rules', negatable: false, help: 'List every rule and exit.') + ..addFlag( + 'list-rules', + negatable: false, + help: + 'List every rule, and the project checks in the configuration, ' + 'and exit.', + ) ..addOption( 'explain', help: @@ -126,10 +132,6 @@ int runCli( out.writeln('$packageName $packageVersion'); return ExitCodes.ok; } - if (args.flag('list-rules')) { - out.writeln(renderRuleList(builtInRules)); - return ExitCodes.ok; - } final explain = args.option('explain'); if (explain != null) { for (final rule in builtInRules) { @@ -153,6 +155,26 @@ int runCli( return ExitCodes.usage; } final root = p.normalize(p.join(cwd, projectArg)); + if (args.flag('list-rules')) { + // Works anywhere; project checks are listed when a configuration exists. + final PreflightConfig config; + try { + config = _loadConfig(args.option('config'), cwd, root, err); + } on ConfigException catch (e) { + err.writeln('Invalid configuration: $e'); + return ExitCodes.usage; + } + out.writeln( + renderRuleList( + builtInRules, + checks: config.checkRules, + checksSource: config.source == null + ? null + : p.relative(config.source!, from: cwd), + ), + ); + return ExitCodes.ok; + } if (!Directory(root).existsSync()) { err.writeln('Project directory not found: $root'); return ExitCodes.usage; @@ -171,22 +193,7 @@ int runCli( PreflightConfig config; try { - final explicit = args.option('config'); - var path = explicit != null - ? p.join(cwd, explicit) - : p.join(root, configFileName); - if (explicit == null && !File(path).existsSync()) { - final legacy = p.join(root, legacyConfigFileName); - if (File(legacy).existsSync()) { - err.writeln( - 'Reading $legacyConfigFileName; rename it to $configFileName.', - ); - path = legacy; - } - } - config = explicit != null || File(path).existsSync() - ? PreflightConfig.load(path, builtInRules) - : const PreflightConfig(); + config = _loadConfig(args.option('config'), cwd, root, err); } on ConfigException catch (e) { err.writeln('Invalid configuration: $e'); return ExitCodes.usage; @@ -277,6 +284,32 @@ int runCli( return report.failed ? ExitCodes.findings : ExitCodes.ok; } +/// Loads `--config` ([explicit], relative to [cwd]) or the project's +/// configuration file under [root], or returns the defaults when there is +/// none. Throws [ConfigException] for an invalid file. +PreflightConfig _loadConfig( + String? explicit, + String cwd, + String root, + StringSink err, +) { + var path = explicit != null + ? p.join(cwd, explicit) + : p.join(root, configFileName); + if (explicit == null && !File(path).existsSync()) { + final legacy = p.join(root, legacyConfigFileName); + if (File(legacy).existsSync()) { + err.writeln( + 'Reading $legacyConfigFileName; rename it to $configFileName.', + ); + path = legacy; + } + } + return explicit != null || File(path).existsSync() + ? PreflightConfig.load(path, builtInRules) + : const PreflightConfig(); +} + String _usage(ArgParser parser) => ''' Catch App Store and Google Play rejections in a Flutter project before review. diff --git a/lib/src/report/text_reporter.dart b/lib/src/report/text_reporter.dart index 0b1c30e..205eb75 100644 --- a/lib/src/report/text_reporter.dart +++ b/lib/src/report/text_reporter.dart @@ -127,8 +127,13 @@ String _indent(String text, String prefix, {int width = 80}) { return out.join('\n'); } -/// Renders `--list-rules` output. -String renderRuleList(List rules) { +/// Renders `--list-rules` output: [rules] by platform, then the project +/// [checks] from the configuration file [checksSource], with their reasons. +String renderRuleList( + List rules, { + List checks = const [], + String? checksSource, +}) { final out = StringBuffer(); for (final platform in RulePlatform.values) { final group = rules.where((r) => r.platform == platform).toList(); @@ -146,6 +151,22 @@ String renderRuleList(List rules) { } out.writeln(); } + if (checks.isNotEmpty) { + out.writeln( + checksSource == null + ? 'Project checks' + : 'Project checks (from $checksSource)', + ); + for (final c in checks) { + out + ..writeln( + ' ${c.id.padRight(30)} ${c.defaultSeverity.name.padRight(8)}' + ' ${c.title}', + ) + ..writeln(_indent(c.why, ' ')); + } + out.writeln(); + } out.write( 'Run `--explain ` for why a rule exists and how to fix ' 'its findings.', diff --git a/test/cli_test.dart b/test/cli_test.dart index 671a38d..6ea7c1b 100644 --- a/test/cli_test.dart +++ b/test/cli_test.dart @@ -178,6 +178,34 @@ checks: expect(noConfig.err, contains('no configuration file')); }); + test('--list-rules adds the project checks with severity and reason', () { + final root = project(); + final list = run(['--list-rules', '--project', root]); + expect(list.code, ExitCodes.ok, reason: list.err); + expect(list.out, contains('Project checks (from ')); + expect( + list.out, + matches( + RegExp(r'check:no-backup +error +'), + ), + ); + expect(list.out, contains(' Tokens live in shared preferences.')); + for (final rule in builtInRules) { + expect(list.out, contains(rule.id)); + } + + final none = run(['--list-rules', '--project', writeProject({})]); + expect(none.code, ExitCodes.ok); + expect(none.out, isNot(contains('Project checks'))); + + final bad = writeProject({ + 'storehound.yaml': checks.replaceFirst('"false"', 'false'), + }); + final invalid = run(['--list-rules', bad]); + expect(invalid.code, ExitCodes.usage); + expect(invalid.err, contains('storehound.yaml:6')); + }); + test('a mistake in a check exits 2 with its line', () { final root = writeProject({ 'storehound.yaml': checks.replaceFirst('"false"', 'false'),