fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries

.github/registry-corpus-state/orphaned-test-files.txt

@@ -0,0 +1,17 @@
+# Test files under a tests/ directory in the pinned aws-guard-rules-registry corpus that no .guard
+# file claims, so nothing runs them. Paths are relative to the rules directory passed to
+# `cfn-guard test -d`, with forward slashes so one list holds for the Windows job too.
+#
+# Listed here because an orphaned test file moves nothing: no verdict, no failure count, and no exit
+# code. A rules file renamed without its tests, or a tests/ file whose stem stopped matching, reads
+# as a clean run everywhere else. So this is the only thing that would notice a fourth one.
+#
+# A rules file is paired with a test file by longest matching filename stem, so these three are
+# orphaned for two different reasons. The first two have no .guard file with a matching stem at all.
+# The third begins with a literal space -- the file is named " cloudfront_accesslogs_enabled_tests.yml"
+# -- which is why its stem matches nothing; leave the space alone, it is the defect being recorded,
+# and renaming the file upstream is what removes this line.
+
+aws/amazon_rds/tests/rds_instance_logging_enabled_tests.yml
+aws/amazon_s3/tests/s3_bucket_default_lock_enable_tests.yml
+aws/cloudfront/tests/ cloudfront_accesslogs_enabled_tests.yml

.github/registry-corpus-state/unchecked-expectations.txt

@@ -0,0 +1,30 @@
+# Every expectation in the pinned aws-guard-rules-registry corpus that names a rule its .guard file
+# never declares, so the expectation is never consulted and gets no verdict. `<rule-name> <count>`,
+# where the count is how many test cases carry an expectation for that name.
+#
+# All 30 of them are in one file, rules/aws/aws_cloudformation/cfn_no_explicit_resource_names.guard.
+# That file used to declare per-resource-type rules with these names and now has a single aggregate
+# rule; the test file was never updated, so each of these asserts nothing.
+#
+# This list is what makes the integration jobs' green mean something. `cfn-guard test -d` over this
+# corpus exits 1, and 1 is also what a run that genuinely broke would give, so the exit code alone
+# cannot tell the two apart -- see .github/scripts/check-registry-corpus.sh. Read it as a lock on a
+# known-bad state: a name appearing here is a new dead expectation and turns the job red, and a name
+# disappearing means someone fixed one and this file has to say so.
+#
+# The count is here and not just the name so that a name losing some of its expectations, but not
+# all, is still a change. The fix that empties this file is
+# aws-cloudformation/aws-guard-rules-registry#288; when it merges, the pin in
+# .github/workflows/pr.yml moves forward and these lines go with it.
+
+no_static_names 2
+static_alarmname 2
+static_dbinstanceidentifier 2
+static_deploymentconfigname 2
+static_deploymentgroupname 2
+static_domainname 2
+static_groupname 4
+static_managedpolicyname 2
+static_name 7
+static_repositoryname 2
+static_rolename 3

.github/workflows/pr.yml

@@ -40,7 +40,7 @@ jobs:
     steps:
       - uses: actions/checkout@v2
       - name: Shellcheck
-        run: shellcheck install-guard.sh
+        run: shellcheck install-guard.sh .github/scripts/check-registry-corpus.sh
 
   formatting:
     name: Formatting check (cargo fmt)
@@ -93,6 +93,37 @@ jobs:
           components: clippy
       - run: cargo clippy -- -D warnings
 
+  ffiHeader:
+    name: Compile the FFI header and example (C and C++)
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions-rust-lang/setup-rust-toolchain@v1
+      - name: Build the FFI dylib
+        run: cargo build --release -p cfn-guard-ffi
+      # The header shipped with two defects that only a compiler finds, so nothing had ever compiled
+      # it: `int32_t` with no `#include <stdint.h>`, which broke the example it ships next to, and a
+      # parameter named `template`, which is a C++ keyword and broke the `#include` itself. Both are
+      # one-line fixes, and both come back the moment someone edits the header unless something
+      # compiles it.
+      - name: Header alone, as C and as C++
+        run: |
+          set -e
+          cd guard-ffi/example
+          printf '#include "cfn_guard.h"\nint main(void){return 0;}\n' > /tmp/header_only.c
+          printf '#include "cfn_guard.h"\nint main(){return 0;}\n' > /tmp/header_only.cpp
+          gcc -std=c11 -Wall -Wextra -Werror -c /tmp/header_only.c -I. -o /dev/null
+          g++ -std=c++17 -Wall -Wextra -Werror -c /tmp/header_only.cpp -I. -o /dev/null
+      # Links as well as compiles, which is what checks the `extern "C"` guard: without it a C++
+      # caller gets C++ name mangling and fails at the link step rather than the compile step.
+      - name: Example, linked against the dylib and run
+        run: |
+          set -e
+          cd guard-ffi/example
+          gcc -std=c11 -Wall -Wextra -Werror cfn_guard_test.c -I. -o /tmp/cfn_guard_test \
+            -L../../target/release -lcfn_guard_ffi -Wl,-rpath,"$PWD/../../target/release"
+          /tmp/cfn_guard_test
+
   typocheck:
     name: Spell Check with Typos
     runs-on: ubuntu-latest
@@ -146,15 +177,34 @@ jobs:
         with:
           repository: aws-cloudformation/aws-guard-rules-registry
           path: aws-guard-rules-registry
-          ref: main
+          # Pinned to a SHA rather than tracking `main`. A moving ref means a commit in another
+          # repository can turn this repository's CI red with no change here, and by the time anyone
+          # looks the corpus has moved on again, so the failure is not reproducible. This SHA is the
+          # `main` commit these integration tests were last measured against.
+          #
+          # Read it as a lock on a known state, not on a green one. `cfn-guard test -d rules` exits
+          # 1 at this commit: 30 expectations across 11 distinct rule names in
+          # rules/aws/aws_cloudformation name a rule their file never defines, so those expectations
+          # get no verdict. Pinning makes that failure deterministic and reproducible; it does not
+          # fix it. The fix is aws-cloudformation/aws-guard-rules-registry#288 -- once that merges,
+          # move this SHA forward to a commit that contains it, here and in the Windows job below.
+          ref: 7f7340c26ae5d5e8874651dbffeb12e0e9f505b6
+      # A green here means the pinned corpus is in the known-bad state the comment above describes,
+      # not that the corpus passes. This step used to assert exit 0, which cannot hold at this SHA --
+      # `cfn-guard test -d` exits 1 over these rules and is right to. Asserting nothing more than
+      # "exit 1" would be worse than asserting the wrong thing, though: `get_exit_code` in
+      # guard/src/commands/test.rs makes the error code sticky, so a run where a rule ran and failed
+      # its expectation for real exits 1 too, and tolerating that number would hide it. So the check
+      # holds the report against checked-in lists in .github/registry-corpus-state/ instead -- which
+      # expectations go unchecked, which test files nothing claims, and that nothing else went wrong.
+      # Fixing the corpus turns this red on purpose: when
+      # aws-cloudformation/aws-guard-rules-registry#288 merges, the pin and those lists move
+      # together, and the lists become empty.
       - name: Run integration tests using test command
         run: |
-          if cloudformation-guard/target/release/cfn-guard test -d aws-guard-rules-registry/rules; then
-              echo "The integration tests for test command have passed."
-          else
-              echo "The integration tests for test command have failed."
-              exit 1
-          fi
+          bash cloudformation-guard/.github/scripts/check-registry-corpus.sh \
+              cloudformation-guard/target/release/cfn-guard \
+              aws-guard-rules-registry/rules
 
       - name: Run integration tests using parse-tree command
         run: |
@@ -214,16 +264,44 @@ jobs:
         with:
           repository: aws-cloudformation/aws-guard-rules-registry
           path: aws-guard-rules-registry
-          ref: main
+          # The same pin as the linux/macos job above, and it has to stay in step with it: a SHA
+          # here and `main` there would have the two jobs testing different corpora, which is the
+          # one thing worse than both being wrong. See that job for why this commit is a known
+          # failing state, and for the PR that releases the pin
+          # (aws-cloudformation/aws-guard-rules-registry#288).
+          ref: 7f7340c26ae5d5e8874651dbffeb12e0e9f505b6
+      # The same script the linux/macos job runs, under `shell: bash` -- Git for Windows' bash, which
+      # this runner image ships -- rather than a pwsh transcription of it. See that job for what a
+      # green here means, and for why it is not "the corpus passes".
+      #
+      # One implementation and not two because the two had already drifted, on the one thing this
+      # check is about. This step was `cfn-guard test -d ...` followed by `if ($LASTEXITCODE -eq 0)`,
+      # and before that `if (<command>)` -- which does not test a command's exit code in PowerShell.
+      # The parenthesized expression captures the command's stdout and `if` coerces that to a
+      # boolean, where a collection of more than one element is always true. `cfn-guard test -d`
+      # prints thousands of lines of results against this ruleset, so the condition could never be
+      # false: the `else` was unreachable, and the step logged "have passed" for a run that had
+      # exited non-zero, while the linux/macos twin was testing exit status. The parse-tree step
+      # below is still pwsh and carries its own copy of that note, because its loop has nothing to
+      # share with a twin.
+      #
+      # Under `shell: bash` the step's status is the shell's, and the script is the whole body, so its
+      # exit status is the step's with nothing after it to overwrite the number. That follows from the
+      # invocation the workflow-syntax reference documents: `bash --noprofile --norc -eo pipefail {0}`,
+      # using the Git for Windows bash on this platform.
+      #
+      # How a pwsh step's exit code is derived is deliberately not asserted here. An earlier draft of
+      # this comment said Actions appends `exit $LASTEXITCODE` to every pwsh step; that is widely
+      # repeated and may well be true, but it could not be sourced -- the workflow-syntax reference
+      # truncates before any exit-code section and the add-scripts guide does not cover it. The
+      # observation above needs no such mechanism: the step logged "have passed" and the job was red,
+      # which is enough to establish that the condition was not testing what it appeared to.
       - name: Run integration tests using test command
+        shell: bash
         run: |
-          if (cloudformation-guard/target/release/cfn-guard test -d aws-guard-rules-registry/rules) {
-              echo "The integration tests for test command have passed."
-          }
-          else {
-              echo "The integration tests for test command have failed."
-              exit 1
-          }
+          bash cloudformation-guard/.github/scripts/check-registry-corpus.sh \
+              cloudformation-guard/target/release/cfn-guard \
+              aws-guard-rules-registry/rules
 
       - name: Run integration tests using parse-tree command
         run: |
@@ -234,21 +312,30 @@ jobs:
 
           $rules  = @(Get-ChildItem -Path .\ -Filter *.guard -Recurse -File)
 
+          # `elseif (<command>)` had the same defect as the test-command step above: it read the
+          # parse tree that `parse-tree` prints on stdout as the verdict, rather than the exit code.
+          # That happened to give the right answer for the two outcomes this command has today,
+          # because the success path always prints a non-empty tree and both failure paths print
+          # only to stderr -- but it silently passed any run that wrote partial output and then
+          # failed, so the step could print "All the rules have succeeded" and still exit non-zero.
+          # How the non-zero reached the step's own status is not asserted here; see the note on the
+          # test-command step above for why the pwsh exit-code mechanism is left unstated.
           Foreach ($rule in $rules) {
               $rule_files_without_comments = (Get-Content $rule.FullName) -replace '^[ \s]*#.*$', ''
-              if ([String]::IsNullOrWhiteSpace($rule_files_without_comments)){
-              $SKIPPED_RULES += "$rule"
-          }
-          elseif (../../cloudformation-guard/target/release/cfn-guard parse-tree --rules $rule.FullName) {
-              continue
-          } else {
-              $FAILED_RULES += "$rule"
-            }
+              if ([String]::IsNullOrWhiteSpace($rule_files_without_comments)) {
+                  $SKIPPED_RULES += "$rule"
+                  continue
+              }
+
+              ../../cloudformation-guard/target/release/cfn-guard parse-tree --rules $rule.FullName
+              if ($LASTEXITCODE -ne 0) {
+                  $FAILED_RULES += "$rule"
+              }
           }
 
           $SKIPPED_RULE_COUNT = $SKIPPED_RULES.Length
           if ($SKIPPED_RULE_COUNT -gt 0) {
-              echo "The following `$SKIPPED_RULE_COUNT.Length` rule(s) were skipped because they contained only comments:"
+              echo "The following $SKIPPED_RULE_COUNT rule(s) were skipped because they contained only comments:"
               echo $SKIPPED_RULES
           }
 

_typos.toml

@@ -12,5 +12,22 @@ ource = "ource"
 [default.extend-identifiers]
 # Ignore ID in evaluate_tests.rs
 fooCounterTaskDef49BA9021 = "fooCounterTaskDef49BA9021"
+# `typos` reads `ba` as `by`/`be`, so a short SHA is uncitable in prose when the PREFIX being
+# written contains `ba`. Truncation is not a general remedy and not a general failure either: it
+# removes a TRAILING `ba` and cannot remove an interior one. Measured, typos-cli 1.49.0, one SHA
+# per file:
+#
+#     34d965ba  rc=2        1ba4648d  rc=2
+#     34d965b   rc=0        1ba4648   rc=2
+#     34d96     rc=0        1ba       rc=2
+#
+# So check the prefix you are about to write, and allowlist only when it flags. An earlier version
+# of this comment said "every prefix length flags, because the token sits between digits" -- that
+# is false of `34d965ba`, whose seven-character prefix passes, and true only of `1ba4648d`, which
+# did not exist when it was written. Both entries below are needed because both SHAs are cited at
+# full length: the suite ledger at the head of `eval_tests.rs` names the commit each figure was
+# measured at, so the alternative to these lines is a row that cannot say what it measured.
+34d965ba = "34d965ba"
+1ba4648d = "1ba4648d"
 [files]
 extend-exclude = ["guard/ts-lib", "action"]

docs/FUNCTIONS.md

@@ -420,7 +420,7 @@ rule check when %security_group !EMPTY {
 
 This function converts strings, and ints to their char equivalents
 
-NOTE: this function will cause an error if the int is not 0 <= n <= 9, it will also error out if a string has a length > 1
+NOTE: this function will cause an error if the int is not 0 <= n <= 9, it will also error out if a string is longer than one character. Length is counted in characters, not bytes, so a single character that is not ASCII converts like any other.
 
 #### Argument(s)
 

docs/KNOWN_ISSUES.md

@@ -34,6 +34,8 @@ let api_gws = Resources.*[ Type == 'AWS::ApiGateway::RestApi' ]
 2. When performing `!=` comparison, if the values are incompatible like comparing a `string` to `int`, an error is thrown internally but currently suppressed and converted to `false` to satisfy the requirements of Rust’s [PartialEq](https://doc.rust-lang.org/std/cmp/trait.PartialEq.html). We are tracking to release a fix for this issue soon.
 
    `integer` and `float` are no longer among the incompatible pairs: they compare against each other as numbers, so `Size > 10` holds for a `Size` of `50.5` and `Size == 50` holds for a `Size` of `50.0`. See [Guard: Clauses](CLAUSES.md) for the details, including why it mattered inside a `when` condition.
+
+   The non-finite spellings are strings, not numbers. `nan`, `inf` and `infinity` in a document are read as strings, which is what YAML resolves them to -- it spells the non-finite floats `.nan` and `.inf`, and those were always read as strings here. A clause comparing one of them against a number is therefore an incompatible pair as described above. A number in a document that is out of range for a 64 bit float, such as `1e999`, is read the same way and for the same reason -- it would otherwise become an infinity, which no comparison can decide against. A float literal in a *rule* that is out of range, or that rounds to zero, is a parse error rather than a silently different bound, because a rule is authored and its author can be told.
 3. `exists` and `empty` checks do not display the JSON pointer path inside the document in the error messages. Both these clauses often have retrieval errors which does not maintain this traversal information today. We are tracking to resolve this issue.
 4. <a name="function-limitation"></a> **No support for calling functions inline on the LHS of an operator**
 
@@ -109,3 +111,11 @@ let api_gws = Resources.*[ Type == 'AWS::ApiGateway::RestApi' ]
         %root.Properties."some-key" == true
     }
    ```
+
+   Key names that read as integers need the same quoting, for a different reason: unquoted, `.80` is an
+   array index, because that is how a list element is addressed without brackets. Quoted, `."80"` is a
+   key name. So an account id under `Mappings` is written
+
+   ```
+    Mappings.AccountToEnv."123456789012".Env == "prod"
+   ```

guard-ffi/example/cfn_guard.h

@@ -1,6 +1,22 @@
 #ifndef CFN_GUARD_H
 #define CFN_GUARD_H
 
+/* int32_t below. The header declared it without including anything, so it did not compile on its
+ * own: cfn_guard_test.c includes only stdio.h and stdlib.h, neither of which provides int32_t, and
+ * `gcc -std=c11 -c cfn_guard_test.c` failed with "unknown type name 'int32_t'". */
+#include <stdint.h>
+
+/* `bool` below, rather than the `_Bool` this used to declare. `_Bool` is a C keyword and is not a
+ * type in C++, so the header could not be included from C++ even after the parameter rename. In C99
+ * and later `bool` is a macro for `_Bool`, so the declared type and its width are unchanged. */
+#ifndef __cplusplus
+#include <stdbool.h>
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 typedef struct {
   int32_t code;
   char *message;
@@ -11,7 +27,18 @@ typedef struct {
   char *file_name;
 } validate_input_t;
 
-char* cfn_guard_run_checks(validate_input_t template, validate_input_t rules, _Bool verbose, extern_err_t * err);
+/* Every pointer in `data` and `rules` must be non-null and must hold valid UTF-8. A null one gives
+ * code 23 and a non-UTF-8 one gives code 24, both naming the field in `err.message`.
+ *
+ * The first parameter was named `template`, which is a reserved word in C++, so a C++ translation
+ * unit failed at the `#include` with "expected ',' or '...' before 'template'". It is now `data`,
+ * matching the Rust side. */
+char *cfn_guard_run_checks(validate_input_t data, validate_input_t rules, bool verbose,
+                           extern_err_t *err);
 void cfn_guard_free_string(char *);
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif

guard-ffi/example/cfn_guard_test.c

@@ -9,9 +9,11 @@ int main() {
   data.file_name = "data.json";
   rules.content = "rule check_foo { foo.bar == true }";
   rules.file_name = "check.rule";
-  char* result = cfn_guard_run_checks(data, rules, 0, &err);
+  char* result = cfn_guard_run_checks(data, rules, false, &err);
   if (err.code == 0) {
-    printf(result);
+    /* `printf(result)` reads the returned JSON as a format string, which -Wformat-security
+     * rejects and which a `%` in the report would act on. */
+    printf("%s", result);
     cfn_guard_free_string(result);
     cfn_guard_free_string(err.message);
   } else {