diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e39a669..3f66a64 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -207,3 +207,199 @@ jobs: path: artifacts/sarif if-no-files-found: warn + action: + name: First-party action + runs-on: ubuntu-latest + # Executes action.yml itself against a locally built image. Without this job the action is only + # ever exercised by consumers, so a change to its inputs or shell wiring regresses silently. + steps: + - uses: actions/checkout@v7 + - uses: docker/setup-buildx-action@v4 + + # A YAML parse cannot catch this: the file stays valid YAML, but GitHub parses expressions + # everywhere in a step - including inside a run script's shell comments - and an empty one + # fails the whole action to load. Checked before the image build so the failure is fast and + # names the cause. The pattern is escaped so this check is not itself read as an expression. + - name: Reject empty expressions in the action manifest + run: | + if grep -nE '\$\{\{[[:space:]]*\}\}' action.yml; then + echo "::error file=action.yml::Empty GitHub expression. The action will fail to load with 'An expression was expected'." + exit 1 + fi + + - name: Build CLI image (amd64, load) + uses: docker/build-push-action@v7 + with: + context: . + file: build/Dockerfile + load: true + tags: tmforge-cli:ci + cache-from: type=gha,scope=tmforge-cli + cache-to: type=gha,mode=max,scope=tmforge-cli + + # The example custom rule reports an error on the sample model, so the action must gate on it. + - name: Action reports the custom-rule finding + id: findings + uses: ./ + with: + models: examples/webshop.tm7 + image: tmforge-cli + version: ci + pull: "false" + rules: examples/corporate-policy.tmrules.json + upload-sarif: "false" + fail-on-findings: "false" + sarif-directory: .tmforge-findings + + # The suppression silences exactly that finding, so the same inputs must now clear the gate. + - name: Action passes once the finding is suppressed + id: suppressed + uses: ./ + with: + models: examples/webshop.tm7 + image: tmforge-cli + version: ci + pull: "false" + rules: examples/corporate-policy.tmrules.json + suppression-file: examples/corporate-policy.suppressions.json + upload-sarif: "false" + upload-report: "true" + report-artifact-name: action-findings-report + sarif-directory: .tmforge-suppressed + + - name: Action outcome matches the CLI + env: + FINDINGS_RESULT: ${{ steps.findings.outputs.result }} + FINDINGS_CODE: ${{ steps.findings.outputs.exit-code }} + SUPPRESSED_RESULT: ${{ steps.suppressed.outputs.result }} + SUPPRESSED_CODE: ${{ steps.suppressed.outputs.exit-code }} + run: | + set -uo pipefail + run_cli() { + docker run --rm -v "$PWD":/work -w /work tmforge-cli:ci analyze "$@" >/dev/null 2>&1 + echo $? + } + cli_findings=$(run_cli examples/webshop.tm7 \ + --rules examples/corporate-policy.tmrules.json --reportFolder .cli-findings) + cli_suppressed=$(run_cli examples/webshop.tm7 \ + --rules examples/corporate-policy.tmrules.json \ + --suppressionFile examples/corporate-policy.suppressions.json --reportFolder .cli-suppressed) + + echo "action: findings=$FINDINGS_RESULT/$FINDINGS_CODE suppressed=$SUPPRESSED_RESULT/$SUPPRESSED_CODE" + echo "cli: findings=$cli_findings suppressed=$cli_suppressed" + + rc=0 + if [ "$FINDINGS_CODE" != "2" ] || [ "$FINDINGS_RESULT" != "fail" ]; then + echo "::error::Expected the custom rule to gate (fail/2), got $FINDINGS_RESULT/$FINDINGS_CODE." + rc=1 + fi + if [ "$SUPPRESSED_CODE" != "0" ] || [ "$SUPPRESSED_RESULT" != "pass" ]; then + echo "::error::Expected the suppression to clear the gate (pass/0), got $SUPPRESSED_RESULT/$SUPPRESSED_CODE." + rc=1 + fi + if [ "$cli_findings" != "$FINDINGS_CODE" ]; then + echo "::error::CLI ($cli_findings) and action ($FINDINGS_CODE) disagree without the suppression." + rc=1 + fi + if [ "$cli_suppressed" != "$SUPPRESSED_CODE" ]; then + echo "::error::CLI ($cli_suppressed) and action ($SUPPRESSED_CODE) disagree with the suppression." + rc=1 + fi + exit $rc + + # A mistyped rule path must fail rather than analyze with the built-in rules and report clean. + - name: Action rejects a missing rules path + id: missing-rules + continue-on-error: true + uses: ./ + with: + models: examples/webshop.tm7 + image: tmforge-cli + version: ci + pull: "false" + rules: examples/no-such-pack.tmrules.json + upload-sarif: "false" + + - name: Assert the missing rules path failed + env: + OUTCOME: ${{ steps.missing-rules.outcome }} + run: | + if [ "$OUTCOME" != "failure" ]; then + echo "::error::A missing rules path must fail the action, not fall back to the built-in rules." + exit 1 + fi + echo "A missing rules path failed the action, as it must." + + # Drift is inert until the consumer names watched paths, and reports a reason either way. + - name: Drift reports skipped when switched off + id: drift-off + uses: ./ + with: + models: examples/webshop.tm7 + image: tmforge-cli + version: ci + pull: "false" + upload-sarif: "false" + drift: "off" + sarif-directory: .tmforge-drift-off + + # Exercises the real change list for this event against globs nothing can have touched. + - name: Drift reports no change against paths that did not change + id: drift-clean + uses: ./ + with: + models: examples/webshop.tm7 + image: tmforge-cli + version: ci + pull: "false" + upload-sarif: "false" + drift-watched-paths: no-such-directory/** + sarif-directory: .tmforge-drift-clean + + - name: Assert the drift contract + env: + OFF_DRIFT: ${{ steps.drift-off.outputs.drift }} + CLEAN_DRIFT: ${{ steps.drift-clean.outputs.drift }} + CLEAN_WATCHED: ${{ steps.drift-clean.outputs.drift-watched-count }} + CLEAN_REASON: ${{ steps.drift-clean.outputs.drift-reason }} + run: | + echo "drift(off)=$OFF_DRIFT drift(clean)=$CLEAN_DRIFT ($CLEAN_REASON)" + rc=0 + if [ "$OFF_DRIFT" != "skipped" ]; then + echo "::error::drift 'off' must report 'skipped', got '$OFF_DRIFT'." + rc=1 + fi + if [ "$CLEAN_DRIFT" != "false" ] || [ "$CLEAN_WATCHED" != "0" ]; then + echo "::error::Watched paths that did not change must report 'false' with a zero count, got '$CLEAN_DRIFT'/'$CLEAN_WATCHED'." + rc=1 + fi + exit $rc + + # Exercises the review path against model globs no pull request can have touched, so the + # assertion holds whatever this pull request happens to change. + - name: Review reports no model changes against paths that did not change + id: review + if: github.event_name == 'pull_request' + uses: ./ + with: + models: examples/webshop.tm7 + image: tmforge-cli + version: ci + pull: "false" + upload-sarif: "false" + review: "on" + drift-model-paths: no-such-directory/** + sarif-directory: .tmforge-review + + - name: Assert the review contract + if: github.event_name == 'pull_request' + env: + REVIEW: ${{ steps.review.outputs.review }} + CHANGED: ${{ steps.review.outputs.review-changed-models }} + run: | + echo "review=$REVIEW changed-models=$CHANGED" + if [ "$REVIEW" != "reviewed" ] || [ "$CHANGED" != "0" ]; then + echo "::error::Expected 'reviewed' with zero changed models, got '$REVIEW'/'$CHANGED'." + exit 1 + fi + diff --git a/action.yml b/action.yml index d76ec84..847a445 100644 --- a/action.yml +++ b/action.yml @@ -52,6 +52,29 @@ inputs: use the CLI default (error), so only errors gate unless you opt into warnings. required: false default: "" + rules: + description: >- + Path to a declarative custom rule pack (*.tmrules.json) or a directory of them, relative to the + repository root. Custom rules are added to the built-in rules, never a replacement for them. + required: false + default: "" + ruleset: + description: >- + Path to a .ruleset file that enables or disables built-in rules, relative to the repository root. + required: false + default: "" + suppression-file: + description: >- + Path to a suppression .json file, relative to the repository root. A suppressed finding is still + produced and recorded; it just stops gating the build. + required: false + default: "" + pull: + description: >- + Pull the image before running. Set to 'false' to run an image already loaded on the runner, which + is how this repository tests the action against a locally built image. + required: false + default: "true" fail-on-findings: description: Fail the action when analysis reports gating findings (exit code 2). Set to 'false' to report without failing. required: false @@ -68,6 +91,61 @@ inputs: description: Directory (relative to the workspace) the per-model SARIF files are written to. required: false default: .tmforge-sarif + upload-report: + description: >- + Upload the generated findings reports (SARIF, HTML, and JSON) as a workflow artifact. The SARIF + already reaches code scanning; this is for keeping the human-readable report with the run. + required: false + default: "false" + report-artifact-name: + description: Name of the workflow artifact created when 'upload-report' is enabled. + required: false + default: tmforge-reports + drift: + description: >- + What to do when architecture-relevant files changed but no threat model did: 'off', 'notice' + (report without failing), or 'fail'. Drift is inert until 'drift-watched-paths' is set, because + only the consumer knows which paths are architecture-relevant. + required: false + default: notice + drift-watched-paths: + description: >- + Globs, one per line, matching the files whose change should be accompanied by a threat-model + update (for example 'src/**' or 'infra/**'). Empty disables drift detection. + required: false + default: "" + drift-model-paths: + description: >- + Globs, one per line, matching the threat models that satisfy a drift check. Defaults to the + 'models' input. + required: false + default: "" + drift-comment: + description: >- + Keep a single pull-request comment up to date with the drift result. Requires + 'pull-requests: write'. A pull request from a fork receives a read-only token, so the comment is + skipped with a warning rather than failing the run. + required: false + default: "false" + review: + description: >- + Summarize how the threat models themselves changed in this pull request: 'off' or 'on'. The + review is informational and never gates the build. + required: false + default: "off" + review-comment: + description: >- + Keep a single pull-request comment up to date with the review summary. Requires + 'pull-requests: write'; a fork's token is read-only, so it warns rather than failing. + required: false + default: "false" + token: + description: >- + Token used to read the list of files changed in the event. Reading the change list from the API + rather than the work tree is what makes drift correct under a shallow clone and for deleted or + renamed files. + required: false + default: ${{ github.token }} outputs: result: @@ -79,10 +157,301 @@ outputs: sarif-directory: description: Directory the SARIF files were written to. value: ${{ inputs.sarif-directory }} + drift: + description: >- + 'true' (watched paths changed and no threat model did), 'false' (no drift), or 'skipped' (drift + was off, unconfigured, or the event carried no comparable change list). + value: ${{ steps.drift.outputs.drift }} + drift-reason: + description: Why the drift result is what it is, suitable for a log line or a comment body. + value: ${{ steps.drift.outputs.drift-reason }} + drift-watched-count: + description: Number of changed files that matched 'drift-watched-paths'. + value: ${{ steps.drift.outputs.drift-watched-count }} + drift-model-count: + description: Number of changed files that matched 'drift-model-paths'. + value: ${{ steps.drift.outputs.drift-model-count }} + review: + description: "'reviewed', or 'skipped' when review was off or the change list was unavailable." + value: ${{ steps.review.outputs.review }} + review-changed-models: + description: Number of threat models this pull request changed. + value: ${{ steps.review.outputs.review-changed-models }} runs: using: composite steps: + # Runs before analysis and never fails on its own: the gate step enforces the result, so a drift + # failure still lets the SARIF reach code scanning first. + - name: Detect threat-model drift + id: drift + shell: bash + env: + DRIFT_MODE: ${{ inputs.drift }} + DRIFT_WATCHED: ${{ inputs.drift-watched-paths }} + DRIFT_MODELS: ${{ inputs.drift-model-paths != '' && inputs.drift-model-paths || inputs.models }} + DRIFT_TOKEN: ${{ inputs.token }} + DRIFT_API: ${{ github.api_url }} + DRIFT_REPO: ${{ github.repository }} + DRIFT_EVENT: ${{ github.event_name }} + DRIFT_PR: ${{ github.event.pull_request.number }} + DRIFT_BEFORE: ${{ github.event.before }} + DRIFT_AFTER: ${{ github.sha }} + run: | + set -uo pipefail + + report() { + { + echo "drift=$1" + echo "drift-reason=$2" + echo "drift-watched-count=${3:-0}" + echo "drift-model-count=${4:-0}" + } >> "$GITHUB_OUTPUT" + } + + # Every skip below reports why. A drift check that quietly evaluates nothing is worse than one + # that is plainly off, because it reads as "no drift" on a run that never looked. + if [ "$DRIFT_MODE" = "off" ]; then + report skipped "drift detection is off" 0 0 + exit 0 + fi + if [ -z "${DRIFT_WATCHED//[[:space:]]/}" ]; then + report skipped "no drift-watched-paths are configured" 0 0 + exit 0 + fi + if ! command -v jq >/dev/null 2>&1; then + echo "::warning::Skipping drift detection: jq is not available on this runner." + report skipped "jq is not available on this runner" 0 0 + exit 0 + fi + + read_globs() { + local line + while IFS= read -r line; do + line=${line%$'\r'} + if [ -n "${line//[[:space:]]/}" ]; then + printf '%s\n' "$line" + fi + done <<< "$1" + } + + mapfile -t watched_globs < <(read_globs "$DRIFT_WATCHED") + mapfile -t model_globs < <(read_globs "$DRIFT_MODELS") + + api() { + curl -fsSL --retry 2 \ + -H "Authorization: Bearer $DRIFT_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + "$1" + } + + # The change list comes from the API rather than the work tree. That is what makes this correct + # under the default shallow clone, and what lets a deleted or renamed file still be seen. + changed="" + case "$DRIFT_EVENT" in + pull_request | pull_request_target) + if [ -z "$DRIFT_PR" ]; then + report skipped "the event carried no pull-request number" 0 0 + exit 0 + fi + page=1 + while :; do + if ! body=$(api "$DRIFT_API/repos/$DRIFT_REPO/pulls/$DRIFT_PR/files?per_page=100&page=$page"); then + echo "::warning::Skipping drift detection: could not read the pull request's file list." + report skipped "could not read the pull request's file list" 0 0 + exit 0 + fi + changed+=$(printf '%s' "$body" | jq -r '.[] | .filename, (.previous_filename // empty)')$'\n' + [ "$(printf '%s' "$body" | jq 'length')" -lt 100 ] && break + page=$((page + 1)) + done + ;; + push) + if [ -z "$DRIFT_BEFORE" ] || [ "$DRIFT_BEFORE" = "0000000000000000000000000000000000000000" ]; then + report skipped "the push had no previous commit to compare against" 0 0 + exit 0 + fi + if ! body=$(api "$DRIFT_API/repos/$DRIFT_REPO/compare/$DRIFT_BEFORE...$DRIFT_AFTER?per_page=100"); then + echo "::warning::Skipping drift detection: could not compare the pushed commits." + report skipped "could not compare the pushed commits" 0 0 + exit 0 + fi + changed=$(printf '%s' "$body" | jq -r '.files[]? | .filename, (.previous_filename // empty)') + ;; + *) + report skipped "drift is evaluated for pull_request and push events only" 0 0 + exit 0 + ;; + esac + + matches_any() { + local path="$1" + shift + local pattern + for pattern in "$@"; do + # Unquoted right-hand side: this is pattern matching against a string, not evaluation. + if [[ "$path" == $pattern ]]; then + return 0 + fi + # A leading '**/' should also match a file at the repository root. + if [[ "$pattern" == '**/'* ]] && [[ "$path" == ${pattern#'**/'} ]]; then + return 0 + fi + done + return 1 + } + + watched_count=0 + model_count=0 + while IFS= read -r path; do + [ -n "$path" ] || continue + # A path matching both is a model change: updating the model is what the check asks for. + if matches_any "$path" ${model_globs[@]+"${model_globs[@]}"}; then + model_count=$((model_count + 1)) + elif matches_any "$path" ${watched_globs[@]+"${watched_globs[@]}"}; then + watched_count=$((watched_count + 1)) + fi + done < <(printf '%s\n' "$changed" | sort -u) + + if [ "$watched_count" -gt 0 ] && [ "$model_count" -eq 0 ]; then + drift=true + reason="$watched_count watched file(s) changed and no threat model changed" + elif [ "$watched_count" -eq 0 ]; then + drift=false + reason="no watched files changed" + else + drift=false + reason="$watched_count watched file(s) changed alongside $model_count threat-model change(s)" + fi + + { + echo "### Threat model drift" + echo + if [ "$drift" = "true" ]; then + echo "**Drift detected** - $reason." + echo + echo "Review whether the threat model still describes the system, and update it if it does not." + else + echo "No drift - $reason." + fi + echo + echo "| Signal | Count |" + echo "| --- | --- |" + echo "| Changed files matching \`drift-watched-paths\` | $watched_count |" + echo "| Changed files matching \`drift-model-paths\` | $model_count |" + } >> "${GITHUB_STEP_SUMMARY:-/dev/null}" + + if [ "$drift" = "true" ]; then + if [ "$DRIFT_MODE" = "fail" ]; then + echo "::error::Threat-model drift: $reason." + else + echo "::notice::Threat-model drift: $reason." + fi + fi + + report "$drift" "$reason" "$watched_count" "$model_count" + echo "Threat model drift: $drift ($reason)" + + # Opt-in and best-effort: this step never fails the run, because a comment is a courtesy and the + # authoritative results are the outputs, the job summary, and the gate. + - name: Comment the drift result on the pull request + if: ${{ inputs.drift-comment == 'true' && github.event_name == 'pull_request' && steps.drift.outputs.drift != 'skipped' }} + shell: bash + env: + DRIFT: ${{ steps.drift.outputs.drift }} + DRIFT_REASON: ${{ steps.drift.outputs.drift-reason }} + DRIFT_WATCHED: ${{ steps.drift.outputs.drift-watched-count }} + DRIFT_MODEL: ${{ steps.drift.outputs.drift-model-count }} + DRIFT_TOKEN: ${{ inputs.token }} + DRIFT_API: ${{ github.api_url }} + DRIFT_REPO: ${{ github.repository }} + DRIFT_PR: ${{ github.event.pull_request.number }} + run: | + set -uo pipefail + + if ! command -v jq >/dev/null 2>&1; then + echo "::warning::Skipping the drift comment: jq is not available on this runner." + exit 0 + fi + + marker='' + + api() { + curl -fsSL --retry 2 \ + -H "Authorization: Bearer $DRIFT_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + "$1" + } + + # Find our own comment by its marker. Requiring the marker at the *start* of the body means a + # human quoting it cannot be mistaken for it, so this never edits somebody else's comment. + comment_id="" + page=1 + while :; do + if ! body=$(api "$DRIFT_API/repos/$DRIFT_REPO/issues/$DRIFT_PR/comments?per_page=100&page=$page"); then + echo "::warning::Could not read the pull request's comments; skipping the drift comment." + exit 0 + fi + comment_id=$(printf '%s' "$body" | + jq -r --arg m "$marker" '[.[] | select(.body | startswith($m))] | first | .id // empty') + [ -n "$comment_id" ] && break + [ "$(printf '%s' "$body" | jq 'length')" -lt 100 ] && break + page=$((page + 1)) + done + + # Say nothing on a clean pull request that never drifted. The comment exists to be resolved, + # not to appear on every run. + if [ "$DRIFT" != "true" ] && [ -z "$comment_id" ]; then + echo "No drift and no existing comment; nothing to post." + exit 0 + fi + + if [ "$DRIFT" = "true" ]; then + summary="**Drift detected** - $DRIFT_REASON." + detail="Review whether the threat model still describes the system, and update it if it does not." + else + summary="**Resolved** - $DRIFT_REASON." + detail="This comment is kept up to date by the Threat Model Forge action." + fi + + text=$(printf '%s\n### Threat model drift\n\n%s\n\n%s\n\n| Signal | Count |\n| --- | --- |\n| Changed files matching `drift-watched-paths` | %s |\n| Changed files matching `drift-model-paths` | %s |\n' \ + "$marker" "$summary" "$detail" "$DRIFT_WATCHED" "$DRIFT_MODEL") + + # jq builds the request body so the comment text is encoded rather than hand-escaped. + payload=$(jq -n --arg body "$text" '{body: $body}') + response=$(mktemp) + + if [ -n "$comment_id" ]; then + verb=PATCH + url="$DRIFT_API/repos/$DRIFT_REPO/issues/comments/$comment_id" + done_word=updated + else + verb=POST + url="$DRIFT_API/repos/$DRIFT_REPO/issues/$DRIFT_PR/comments" + done_word=created + fi + + code=$(curl -sS -o "$response" -w '%{http_code}' -X "$verb" \ + -H "Authorization: Bearer $DRIFT_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + -d "$payload" \ + "$url") + + case "$code" in + 2*) + echo "Drift comment $done_word." + ;; + 403 | 404) + echo "::warning::Not permitted to comment on this pull request (HTTP $code). A fork's token is read-only; for same-repository pull requests grant 'pull-requests: write'." + ;; + *) + echo "::warning::Could not post the drift comment (HTTP $code)." + ;; + esac + - name: Analyze threat models id: analyze shell: bash @@ -90,14 +459,52 @@ runs: TMF_IMAGE: ${{ inputs.image }}:${{ inputs.version }} TMF_MODELS: ${{ inputs.models }} TMF_MAX_SEVERITY: ${{ inputs.max-severity }} + TMF_RULES: ${{ inputs.rules }} + TMF_RULESET: ${{ inputs.ruleset }} + TMF_SUPPRESSIONS: ${{ inputs.suppression-file }} + TMF_PULL: ${{ inputs.pull }} TMF_SARIF_DIR: ${{ inputs.sarif-directory }} run: | set -uo pipefail mkdir -p "$TMF_SARIF_DIR" - # Expand the glob (** needs globstar) relative to the workspace. + # Every input arrives through the environment rather than being interpolated into this + # script, so no input is ever parsed as shell source. The globs below are expanded by the + # shell's pathname expansion only; nothing is passed to eval or to a command string. + # + # (Do not write a GitHub expression here, even inside a comment: the template engine parses + # the whole run block, so an empty one fails the action with "An expression was expected".) + # + # Read one glob per line. Splitting on newlines rather than whitespace is what lets a pattern + # (or a matched path) contain a space without silently becoming two patterns. + patterns=() + while IFS= read -r pattern; do + pattern=${pattern%$'\r'} + if [ -n "${pattern//[[:space:]]/}" ]; then + patterns+=( "$pattern" ) + fi + done <<< "$TMF_MODELS" + shopt -s globstar nullglob - models=( $TMF_MODELS ) + models=() + seen=$'\n' + for pattern in ${patterns[@]+"${patterns[@]}"}; do + # Clear IFS so the pattern is subject to pathname expansion but never to word splitting. + saved_ifs=$IFS + IFS= + matches=( $pattern ) + IFS=$saved_ifs + for match in ${matches[@]+"${matches[@]}"}; do + [ -f "$match" ] || continue + # A file matching two patterns is analyzed once. + case "$seen" in + *$'\n'"$match"$'\n'*) continue ;; + esac + models+=( "$match" ) + seen="${seen}${match}"$'\n' + done + done + if [ ${#models[@]} -eq 0 ]; then echo "::warning::No threat models matched '$TMF_MODELS'." { @@ -107,13 +514,44 @@ runs: exit 0 fi - sev_args=() + # A mistyped rule or suppression path must not analyze with the built-in rules and report a + # clean run: that is the failure mode where a model looks safe because the policy never loaded. + analyze_args=( --reportFolder "$TMF_SARIF_DIR" ) if [ -n "$TMF_MAX_SEVERITY" ]; then - sev_args=(--max-severity "$TMF_MAX_SEVERITY") + analyze_args+=( --max-severity "$TMF_MAX_SEVERITY" ) + fi + if [ -n "$TMF_RULES" ]; then + if [ ! -e "$TMF_RULES" ]; then + echo "::error::rules path '$TMF_RULES' does not exist." + exit 1 + fi + analyze_args+=( --rules "$TMF_RULES" ) + fi + if [ -n "$TMF_RULESET" ]; then + if [ ! -f "$TMF_RULESET" ]; then + echo "::error::ruleset path '$TMF_RULESET' does not exist." + exit 1 + fi + analyze_args+=( --ruleset "$TMF_RULESET" ) + fi + if [ -n "$TMF_SUPPRESSIONS" ]; then + if [ ! -f "$TMF_SUPPRESSIONS" ]; then + echo "::error::suppression-file path '$TMF_SUPPRESSIONS' does not exist." + exit 1 + fi + analyze_args+=( --suppressionFile "$TMF_SUPPRESSIONS" ) fi - echo "Pulling $TMF_IMAGE" - docker pull -q "$TMF_IMAGE" + if [ "$TMF_PULL" = "false" ]; then + if ! docker image inspect "$TMF_IMAGE" >/dev/null 2>&1; then + echo "::error::Image $TMF_IMAGE is not present on the runner and pull is disabled." + exit 1 + fi + echo "Using local image $TMF_IMAGE" + else + echo "Pulling $TMF_IMAGE" + docker pull -q "$TMF_IMAGE" + fi had_error=0 had_findings=0 @@ -122,7 +560,7 @@ runs: code=0 # Mount the workspace so analyze can read the model and write its SARIF report beside it. docker run --rm -v "$PWD":/work -w /work "$TMF_IMAGE" \ - analyze "$model" --reportFolder "$TMF_SARIF_DIR" "${sev_args[@]}" || code=$? + analyze "$model" "${analyze_args[@]}" || code=$? echo "::endgroup::" case "$code" in 0) echo "clean: $model" ;; @@ -145,6 +583,396 @@ runs: } >> "$GITHUB_OUTPUT" echo "Threat Model Forge result: $result (exit code $exit_code)" + # Runs after analysis so the image is already on the runner. Informational only: it never gates. + - name: Review model changes + id: review + if: ${{ inputs.review == 'on' && github.event_name == 'pull_request' }} + shell: bash + env: + REVIEW_IMAGE: ${{ inputs.image }}:${{ inputs.version }} + REVIEW_MODELS: ${{ inputs.drift-model-paths != '' && inputs.drift-model-paths || inputs.models }} + REVIEW_TOKEN: ${{ inputs.token }} + REVIEW_API: ${{ github.api_url }} + REVIEW_REPO: ${{ github.repository }} + REVIEW_PR: ${{ github.event.pull_request.number }} + REVIEW_BASE: ${{ github.event.pull_request.base.sha }} + REVIEW_DIR: ${{ inputs.sarif-directory }} + REVIEW_RULES: ${{ inputs.rules }} + REVIEW_RULESET: ${{ inputs.ruleset }} + REVIEW_SUPPRESSIONS: ${{ inputs.suppression-file }} + run: | + set -uo pipefail + + emit() { + { + echo "review=$1" + echo "review-changed-models=${2:-0}" + } >> "$GITHUB_OUTPUT" + } + + if ! command -v jq >/dev/null 2>&1; then + echo "::warning::Skipping the model review: jq is not available on this runner." + emit skipped 0 + exit 0 + fi + + read_globs() { + local line + while IFS= read -r line; do + line=${line%$'\r'} + if [ -n "${line//[[:space:]]/}" ]; then + printf '%s\n' "$line" + fi + done <<< "$1" + } + + mapfile -t model_globs < <(read_globs "$REVIEW_MODELS") + + matches_any() { + local path="$1" + shift + local pattern + for pattern in "$@"; do + # Unquoted right-hand side: pattern matching against a string, not evaluation. + if [[ "$path" == $pattern ]]; then + return 0 + fi + if [[ "$pattern" == '**/'* ]] && [[ "$path" == ${pattern#'**/'} ]]; then + return 0 + fi + done + return 1 + } + + api() { + curl -fsSL --retry 2 \ + -H "Authorization: Bearer $REVIEW_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + "$1" + } + + entries="" + page=1 + while :; do + if ! body=$(api "$REVIEW_API/repos/$REVIEW_REPO/pulls/$REVIEW_PR/files?per_page=100&page=$page"); then + echo "::warning::Skipping the model review: could not read the pull request's file list." + emit skipped 0 + exit 0 + fi + entries+=$(printf '%s' "$body" | + jq -r '.[] | [.status, .filename, (.previous_filename // "")] | @tsv')$'\n' + [ "$(printf '%s' "$body" | jq 'length')" -lt 100 ] && break + page=$((page + 1)) + done + + mkdir -p "$REVIEW_DIR" + # The base revisions are fetched into the workspace so the container can see them, then + # removed: they are scaffolding for the diff, not something to publish with the reports. + base_root=".tmforge-review-base" + rm -rf "$base_root" + mkdir -p "$base_root" + + # The base analysis has to be produced by the same rules and suppressions as the head + # analysis, or the delta reports the policy's differences as the model's. + analyze_args=() + [ -n "${REVIEW_RULES:-}" ] && [ -e "${REVIEW_RULES:-}" ] && analyze_args+=( --rules "$REVIEW_RULES" ) + [ -n "${REVIEW_RULESET:-}" ] && [ -f "${REVIEW_RULESET:-}" ] && analyze_args+=( --ruleset "$REVIEW_RULESET" ) + [ -n "${REVIEW_SUPPRESSIONS:-}" ] && [ -f "${REVIEW_SUPPRESSIONS:-}" ] && + analyze_args+=( --suppressionFile "$REVIEW_SUPPRESSIONS" ) + + # Analyzes the base revision of one model and compares it to the head analysis the analyze + # step already wrote. Prints the delta as JSON, or nothing when it cannot be produced. + # + # The base revision is analyzed AT THE MODEL'S OWN PATH, with the head content moved aside and + # restored afterwards. That looks roundabout, but a suppression is keyed by the file path it + # was declared for: analyzing the fetched copy under its own directory would match no + # suppression, and every suppressed finding would then read as one this pull request + # suppressed. Same path in, same policy applied. + findings_delta() { + local model="$1" base_copy="$2" + local stem head_doc base_doc base_reports code=0 + stem=$(basename "$model"); stem="${stem%.*}" + head_doc="$REVIEW_DIR/$stem.analysis.json" + [ -f "$head_doc" ] || return 1 + + base_reports="$base_root/.analysis" + mkdir -p "$base_reports" + + cp "$model" "$model.tmforge-head" || return 1 + # Restore the head content even if the runner interrupts the step. + trap 'mv -f "$model.tmforge-head" "$model" 2>/dev/null' RETURN + cp "$base_copy" "$model" || return 1 + + # analyze exits 2 when a model has findings, which is the ordinary case here. + docker run --rm -v "$PWD":/work -w /work "$REVIEW_IMAGE" \ + analyze "$model" "${analyze_args[@]}" --reportFolder "$base_reports" >/dev/null 2>&1 || code=$? + [ "$code" -le 2 ] || return 1 + + base_doc="$base_reports/$stem.analysis.json" + [ -f "$base_doc" ] || return 1 + + docker run --rm -v "$PWD":/work -w /work "$REVIEW_IMAGE" \ + analysis diff "$base_doc" "$head_doc" --json 2>/dev/null | jq -c '.data' || return 1 + } + + results="[]" + changed=0 + while IFS=$'\t' read -r status path previous; do + [ -n "${path:-}" ] || continue + matches_any "$path" ${model_globs[@]+"${model_globs[@]}"} || continue + changed=$((changed + 1)) + + if [ "$status" = "added" ] || [ "$status" = "removed" ]; then + results=$(jq --arg p "$path" --arg c "$status" '. + [{model: $p, change: $c}]' <<< "$results") + continue + fi + + # A rename is diffed against its previous path, so moving a model is not read as a rewrite. + base_path="${previous:-$path}" + dest="$base_root/$path" + mkdir -p "$(dirname "$dest")" + if ! curl -fsSL --retry 2 \ + -H "Authorization: Bearer $REVIEW_TOKEN" \ + -H "Accept: application/vnd.github.raw" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + -o "$dest" \ + "$REVIEW_API/repos/$REVIEW_REPO/contents/${base_path// /%20}?ref=$REVIEW_BASE"; then + echo "::warning::Could not fetch the base revision of $path; skipping its diff." + results=$(jq --arg p "$path" '. + [{model: $p, change: "unavailable"}]' <<< "$results") + continue + fi + + if ! diff_json=$(docker run --rm -v "$PWD":/work -w /work "$REVIEW_IMAGE" \ + diff "$dest" "$path" --json 2>/dev/null); then + echo "::warning::Could not diff $path against its base revision." + results=$(jq --arg p "$path" '. + [{model: $p, change: "error"}]' <<< "$results") + continue + fi + + results=$(jq --arg p "$path" --argjson d "$diff_json" \ + '. + [{model: $p, change: "modified", summary: $d.data.summary, detail: $d.data}]' <<< "$results") + + # `analysis diff` exits 2 when findings were introduced, so the delta is read from its + # output rather than its status. A model whose delta cannot be produced keeps its structural + # diff instead of losing the whole entry. + if delta_json=$(findings_delta "$path" "$dest") && [ -n "$delta_json" ]; then + results=$(jq --argjson d "$delta_json" \ + '(.[-1]) |= (. + {findings: $d.summary, findingDetail: $d})' <<< "$results") + else + echo "::warning::Could not compare findings for $path against its base revision." + fi + done <<< "$entries" + + rm -rf "$base_root" + + if [ "$changed" -eq 0 ]; then + echo "No threat models changed in this pull request." + emit reviewed 0 + { + echo "### Threat model review" + echo + echo "No threat models changed in this pull request." + } >> "${GITHUB_STEP_SUMMARY:-/dev/null}" + exit 0 + fi + + jq '.' <<< "$results" > "$REVIEW_DIR/review.json" + + # The comment and summary stay bounded; the artifact carries everything. + table=$(jq -r ' + .[] | + "| `" + .model + "` | " + + (if .change == "modified" + then "\(.summary.added) added, \(.summary.removed) removed, \(.summary.modified) modified" + + (if (.summary.crossingChanges // 0) > 0 + then ", \(.summary.crossingChanges) crossing change(s)" + else "" end) + + (if (.findings.introduced // 0) > 0 + then ", **\(.findings.introduced) new finding(s)**" + else "" end) + + (if (.findings.resolved // 0) > 0 + then ", \(.findings.resolved) resolved" + else "" end) + else .change end) + " |"' <<< "$results") + + # Findings introduced by the change lead the summary: they are the one thing a reviewer must + # not miss. They are matched by stable identity, so a renamed element does not manufacture a + # "new" finding, and a suppressed one is reported as reclassified rather than resolved. + max_findings=10 + findings=$(jq -r --argjson max "$max_findings" ' + [ .[] | select(.change == "modified") + | .findingDetail.introduced[]? + | { rule: .ruleId, severity: .severity, diagram: (.diagram // "model"), + message: (if (.message | length) > 180 + then (.message[0:180] + "\u2026") else .message end) } ] + | .[:$max][] + | "- **New " + .severity + "** `" + .rule + "` (" + .diagram + ") - " + .message' <<< "$results") + + total_findings=$(jq '[ .[] | .findings.introduced // 0 ] | add // 0' <<< "$results") + findings_overflow=$((total_findings - max_findings)) + reclassified=$(jq '[ .[] | .findings.reclassified // 0 ] | add // 0' <<< "$results") + + # Trust-boundary crossings get their own budget and are listed next. A crossing change can + # occur with no element changes at all - moving a store out of its boundary alters no stored + # property - so folding these into the element list would let a large rename sweep hide the + # one change that alters exposure. `// 0` and `[]?` keep this working against a CLI image + # that predates the crossing comparison rather than failing the review. + max_crossings=10 + crossings=$(jq -r --argjson max "$max_crossings" ' + [ .[] | select(.change == "modified") + | .detail.crossings[]? + | { flow: .flow, diagram: .diagram, kind: .kind, + added: [ .added[].name ], removed: [ .removed[].name ] } ] + | .[:$max][] + | "- **Crossing** `" + .flow + "` (" + .diagram + ")" + + (if .kind != "modified" then " _(" + .kind + " flow)_" else "" end) + + (if (.added | length) > 0 then " - now crosses: " + (.added | join(", ")) else "" end) + + (if (.removed | length) > 0 then " - no longer crosses: " + (.removed | join(", ")) else "" end)' <<< "$results") + + total_crossings=$(jq '[ .[] | select(.change == "modified") + | (.detail.crossings // []) | length ] | add // 0' <<< "$results") + crossing_overflow=$((total_crossings - max_crossings)) + + max_changes=20 + changes=$(jq -r --argjson max "$max_changes" ' + [ .[] | select(.change == "modified") | .model as $m + | (.detail.added + .detail.removed + .detail.modified)[] + | { m: $m, kind: .kind, element: .element, name: .name, diagram: .diagram, props: .properties } ] + | .[:$max][] + | "- **" + .kind + "** " + .element + " `" + .name + "` (" + .diagram + ")" + + (if (.props | length) > 0 + then " - " + ([ .props[] | .key + ": " + (.from // "unset") + " -> " + (.to // "unset") ] | join("; ")) + else "" end)' <<< "$results") + + total_changes=$(jq '[ .[] | select(.change == "modified") + | (.detail.added + .detail.removed + .detail.modified) | length ] | add // 0' <<< "$results") + overflow=$((total_changes - max_changes)) + + { + echo "### Threat model review" + echo + echo "| Model | Change |" + echo "| --- | --- |" + printf '%s\n' "$table" + if [ -n "$findings" ]; then + echo + printf '%s\n' "$findings" + if [ "$findings_overflow" -gt 0 ]; then + echo "_and $findings_overflow more new finding(s) - see \`review.json\` in the run's reports._" + fi + fi + if [ "$reclassified" -gt 0 ]; then + echo + echo "_$reclassified finding(s) changed disposition or severity without being introduced or resolved._" + fi + if [ -n "$crossings" ]; then + echo + printf '%s\n' "$crossings" + if [ "$crossing_overflow" -gt 0 ]; then + echo "_and $crossing_overflow more crossing change(s)._" + fi + fi + if [ -n "$changes" ]; then + echo + printf '%s\n' "$changes" + fi + if [ "$overflow" -gt 0 ]; then + echo + echo "_and $overflow more change(s) - see \`review.json\` in the run's reports._" + fi + } > "$REVIEW_DIR/review.md" + + cat "$REVIEW_DIR/review.md" >> "${GITHUB_STEP_SUMMARY:-/dev/null}" + emit reviewed "$changed" + echo "Reviewed $changed changed threat model(s)." + + # Opt-in and best-effort, exactly like the drift comment, and keyed by its own marker. + - name: Comment the review on the pull request + if: ${{ inputs.review == 'on' && inputs.review-comment == 'true' && github.event_name == 'pull_request' && steps.review.outputs.review == 'reviewed' }} + shell: bash + env: + REVIEW_DIR: ${{ inputs.sarif-directory }} + REVIEW_CHANGED: ${{ steps.review.outputs.review-changed-models }} + REVIEW_TOKEN: ${{ inputs.token }} + REVIEW_API: ${{ github.api_url }} + REVIEW_REPO: ${{ github.repository }} + REVIEW_PR: ${{ github.event.pull_request.number }} + run: | + set -uo pipefail + + if ! command -v jq >/dev/null 2>&1; then + echo "::warning::Skipping the review comment: jq is not available on this runner." + exit 0 + fi + + marker='' + + api() { + curl -fsSL --retry 2 \ + -H "Authorization: Bearer $REVIEW_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + "$1" + } + + comment_id="" + page=1 + while :; do + if ! body=$(api "$REVIEW_API/repos/$REVIEW_REPO/issues/$REVIEW_PR/comments?per_page=100&page=$page"); then + echo "::warning::Could not read the pull request's comments; skipping the review comment." + exit 0 + fi + comment_id=$(printf '%s' "$body" | + jq -r --arg m "$marker" '[.[] | select(.body | startswith($m))] | first | .id // empty') + [ -n "$comment_id" ] && break + [ "$(printf '%s' "$body" | jq 'length')" -lt 100 ] && break + page=$((page + 1)) + done + + if [ "$REVIEW_CHANGED" = "0" ] && [ -z "$comment_id" ]; then + echo "No threat models changed and no existing comment; nothing to post." + exit 0 + fi + + if [ -f "$REVIEW_DIR/review.md" ]; then + text="$marker"$'\n'"$(cat "$REVIEW_DIR/review.md")" + else + text="$marker"$'\n'"### Threat model review"$'\n\n'"No threat models changed in this pull request." + fi + + payload=$(jq -n --arg body "$text" '{body: $body}') + response=$(mktemp) + + if [ -n "$comment_id" ]; then + verb=PATCH + url="$REVIEW_API/repos/$REVIEW_REPO/issues/comments/$comment_id" + done_word=updated + else + verb=POST + url="$REVIEW_API/repos/$REVIEW_REPO/issues/$REVIEW_PR/comments" + done_word=created + fi + + code=$(curl -sS -o "$response" -w '%{http_code}' -X "$verb" \ + -H "Authorization: Bearer $REVIEW_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + -d "$payload" \ + "$url") + + case "$code" in + 2*) + echo "Review comment $done_word." + ;; + 403 | 404) + echo "::warning::Not permitted to comment on this pull request (HTTP $code). A fork's token is read-only; for same-repository pull requests grant 'pull-requests: write'." + ;; + *) + echo "::warning::Could not post the review comment (HTTP $code)." + ;; + esac + - name: Upload SARIF to code scanning # Runs even when analysis found issues, so results always reach the Security tab before the gate. if: ${{ always() && inputs.upload-sarif == 'true' }} @@ -153,25 +981,49 @@ runs: sarif_file: ${{ inputs.sarif-directory }} category: ${{ inputs.category }} + - name: Upload findings reports + # Also runs on findings: the human-readable report is most useful precisely when the gate fails. + if: ${{ always() && inputs.upload-report == 'true' }} + uses: actions/upload-artifact@v7 + with: + name: ${{ inputs.report-artifact-name }} + path: ${{ inputs.sarif-directory }} + if-no-files-found: warn + - name: Gate the build on the result shell: bash env: RESULT: ${{ steps.analyze.outputs.result }} FAIL_ON_FINDINGS: ${{ inputs.fail-on-findings }} + DRIFT: ${{ steps.drift.outputs.drift }} + DRIFT_MODE: ${{ inputs.drift }} + DRIFT_REASON: ${{ steps.drift.outputs.drift-reason }} run: | + # Both gates are evaluated before exiting, so a run that trips two of them says so once + # rather than hiding the second behind the first. + failed=0 + + if [ "$DRIFT" = "true" ] && [ "$DRIFT_MODE" = "fail" ]; then + echo "::error::Threat-model drift: $DRIFT_REASON." + failed=1 + fi + case "$RESULT" in error) echo "::error::One or more threat models could not be analyzed." - exit 1 + failed=1 ;; fail) if [ "$FAIL_ON_FINDINGS" = "true" ]; then echo "::error::Threat model analysis reported gating findings. See the code scanning results." - exit 1 + failed=1 + else + echo "::warning::Threat model analysis reported findings (not gating: fail-on-findings=false)." fi - echo "::warning::Threat model analysis reported findings (not gating: fail-on-findings=false)." ;; *) echo "Threat model analysis passed." ;; esac + + exit $failed diff --git a/docs/analysis-rules.md b/docs/analysis-rules.md index 4b06559..ffae4fd 100644 --- a/docs/analysis-rules.md +++ b/docs/analysis-rules.md @@ -632,23 +632,184 @@ when triage has to cross that kind of edit. ## CI integration -Gate a pipeline on threat-model findings. The example uses GitHub Actions; adapt the runner and paths -to your CI. +### The first-party GitHub Action + +The action runs the pinned CLI container, writes the reports, uploads SARIF to code scanning, and +gates the build: ```yaml name: threat-model on: [pull_request] + +permissions: + contents: read + security-events: write # required to upload SARIF to code scanning + jobs: analyze: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - name: Download tmforge - run: | - curl -fsSL -o tmforge.tar.gz \ - https://github.com/hacks4snacks/tmforge/releases/download/v0.1.0/tmforge-0.1.0-linux-x64.tar.gz - tar -xzf tmforge.tar.gz - echo "$PWD/tmforge-0.1.0-linux-x64" >> "$GITHUB_PATH" + - uses: hacks4snacks/tmforge@v0.3 + with: + version: "0.3" # pin the engine image, not just the action + models: "**/*.tm7" + rules: rules/corporate.tmrules.json + suppression-file: .tmforge/suppressions.json + max-severity: warning +``` + +| Input | Default | Purpose | +| --- | --- | --- | +| `models` | `**/*.tm7` | One glob per line. A file matched by two globs is analyzed once. | +| `rules` | *(none)* | Custom rule pack, or a directory of them. Added to the built-in rules. | +| `ruleset` | *(none)* | `.ruleset` file that enables or disables built-in rules. | +| `suppression-file` | *(none)* | Suppression `.json` file. | +| `max-severity` | `error` | Severity at or above which findings gate the build. | +| `fail-on-findings` | `true` | Set `false` to report findings without failing. | +| `upload-sarif` | `true` | Upload the SARIF to code scanning. | +| `upload-report` | `false` | Also keep the reports as a workflow artifact. | +| `image` / `version` | `ghcr.io/hacks4snacks/tmforge-cli` / `latest` | Pin `version` for a reproducible gate. | +| `pull` | `true` | Set `false` to run an image already loaded on the runner. | + +Outputs are `result` (`pass`, `fail`, or `error`), `exit-code`, and `sarif-directory`. + +A `rules`, `ruleset`, or `suppression-file` path that does not exist **fails the action** rather than +analyzing with the built-in rules alone. A model must never look clean because the policy it was +supposed to be judged against silently failed to load. + +`examples/corporate-policy.tmrules.json` and `examples/corporate-policy.suppressions.json` are a +working pair: the rule reports an error on `examples/webshop.tm7`, and the suppression clears exactly +that finding. This repository's CI runs both through the action and through the CLI and requires the +two to agree. + +### Drift detection + +Analysis can only judge the model you have. It cannot tell you the model stopped describing the +system. Drift detection covers that gap: it reports a change that touched architecture-relevant code +without touching a threat model. + +```yaml + - uses: hacks4snacks/tmforge@v0.3 + with: + drift: notice # 'off', 'notice' (default), or 'fail' + drift-watched-paths: | + src/** + infra/** +``` + +| Input | Default | Purpose | +| --- | --- | --- | +| `drift` | `notice` | `off`, `notice` (report without failing), or `fail`. | +| `drift-watched-paths` | *(none)* | Globs, one per line, whose change should come with a model update. | +| `drift-model-paths` | the `models` input | Globs that satisfy the check. | +| `drift-comment` | `false` | Keep one pull-request comment up to date with the result. | +| `token` | `github.token` | Used to read the event's file list. | + +Outputs are `drift` (`true`, `false`, or `skipped`), `drift-reason`, `drift-watched-count`, and +`drift-model-count`. Every run writes a job-summary table, so the result is visible without granting +any additional permission. + +Points worth knowing: + +- **Drift is inert until `drift-watched-paths` is set.** Only you know which paths are + architecture-relevant, so the default configuration reports `skipped` rather than guessing. +- **A file matching both glob sets counts as a model change**, because updating the model is exactly + what the check asks for. +- **The change list comes from the API, not the work tree.** That is what makes it correct under + `actions/checkout`'s default shallow clone, and what lets a deleted or renamed file still be seen. + A rename counts under both its old and new path. +- **A run that cannot compute a change list reports `skipped`, never `false`.** That covers a first + push, a `schedule` or `workflow_dispatch` run, and an unreadable file list. "We looked and found + nothing" and "we never looked" are different answers, and only one of them is reassuring. +- `fail` mode is enforced in the same gate as findings, so the SARIF still reaches code scanning + first and a run that trips both gates reports both. + +#### Commenting on the pull request + +`drift-comment: true` keeps **one** comment up to date instead of adding a new one per run: + +```yaml +permissions: + contents: read + security-events: write + pull-requests: write # only needed for drift-comment + +# ... + with: + drift-comment: "true" + drift-watched-paths: src/** +``` + +The comment is found by a hidden `` marker and updated in place, so a pull +request that drifts and is then fixed ends with a single comment saying it is resolved rather than a +stale warning. The marker must be at the *start* of a comment body, so quoting it in a review +conversation cannot cause the action to edit someone else's comment. + +Commenting is best-effort and never fails the run: a pull request from a fork receives a read-only +token, and that case reports a warning. Nothing else about drift needs `pull-requests: write` — the +outputs, the job summary, and the gate all work without it. + +### Reviewing model changes + +Drift asks whether the model was updated. Review shows **how** it changed, so a reviewer does not +have to read a diff of serialized XML: + +```yaml + - uses: hacks4snacks/tmforge@v0.3 + with: + review: "on" + review-comment: "true" # optional; needs pull-requests: write + upload-report: "true" # optional; keeps the full detail with the run +``` + +For every threat model the pull request touches, the action fetches the base revision and runs +`tmforge diff` against it, then renders one summary: a table of models with added, removed, and +modified counts, the findings the change introduced, the trust boundary crossings that changed, and +the individual element and property changes. + +| Input | Default | Purpose | +| --- | --- | --- | +| `review` | `off` | `off` or `on`. Review is informational and never gates the build. | +| `review-comment` | `false` | Keep one pull-request comment up to date with the summary. | + +Outputs are `review` (`reviewed` or `skipped`) and `review-changed-models`. + +Points worth knowing: + +- **Findings introduced by the change lead the summary.** The base revision is analyzed with the + same rules and suppressions as the head, and the two analysis documents are compared by + [finding identity](cli-reference.md#comparing-two-analyses) rather than by count, so a renamed + element does not manufacture a new finding and a suppressed one is reported as reclassified rather + than resolved. Up to 10 appear in the summary. +- **Trust boundary crossings are listed next and budgeted separately.** Which boundaries a flow + crosses is derived from geometry, so moving an element across one changes no stored property and + shows up in none of the element counts. A model can therefore report `0 added, 0 removed, 0 + modified` and still have changed what is exposed. Crossings get their own cap of 10 lines so a + large rename sweep cannot crowd out the one change that alters exposure. +- **The summary is bounded and the detail is not.** At most 20 element changes appear in the summary + and the comment; the complete diff for every model is written to `review.json` in the report + directory, which `upload-report: true` attaches to the run alongside the HTML findings reports. A + pull request that rewrites a model should not produce a comment nobody can read. +- **A renamed model is diffed against its previous path**, so moving a file reads as a move rather + than a wholesale rewrite. +- **Comparison is by element id, not file position.** Re-layout and re-serialization produce no diff, + which is what makes the summary worth reading. Boundaries are matched by id too, so renaming one is + not reported as a crossing change. +- If a base revision or a diff cannot be produced for one model, that model is reported as + `unavailable` and the rest of the review still runs. A findings delta that cannot be produced + leaves the structural review in place rather than dropping the model, and a `tmforge` image that + predates either comparison simply reports neither instead of failing the review. +- Like the drift comment, the review comment is opt-in, idempotent through its own + `` marker, and best-effort: a fork's read-only token produces a warning + rather than a failure. + +### Without the action + +Any runner that can execute the CLI works the same way — `analyze` returns `2` when a model has +findings, which fails the step, and `1` for a tool error: + +```yaml - name: Analyze threat models run: | set -e @@ -662,7 +823,6 @@ jobs: sarif_file: reports ``` -`tmforge analyze` returns `2` when a model has findings, which fails the step; `1` signals a tool error. See the [deployment guide](deployment.md#cicd) for container-based pipelines. ## See also diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 5b08dfd..333cb33 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -45,7 +45,7 @@ tmforge [options] | [`layout`](#layout) | Author | Auto-lay-out the diagram (layered; no hand-placed coordinates). | | [`rules`](#rules) | Analyze | Compile an MTMT `.tb7` template into a versioned rule pack. | | [`analyze`](#analyze) | Analyze | Evaluate the analysis rules against a model. | -| [`analysis`](#analysis) | Analyze | Validate a stored analysis document (and check whether it is stale). | +| [`analysis`](#analysis) | Analyze | Validate a stored analysis document (and check whether it is stale), or compare two of them. | | [`threats`](#threats) | Analyze | Report or author threats — the persisted, triaged view of the findings (`--write` to persist; `--add`/`--edit`/`--remove` to author). | | [`accept`](#accept) | Analyze | Accept a generated threat's risk (records a justification). | | [`report`](#report) | Report | Generate a self-contained HTML report. | @@ -229,6 +229,30 @@ tmforge diff payments.v1.tm7 payments.v2.tm7 --json Identity is preserved in `.tm7`; other formats do not round-trip element ids, so `diff` is most useful on `.tm7`. +#### Trust boundary crossings + +Ignoring geometry is what keeps the structural diff quiet when a model is merely re-laid-out, but +**which trust boundaries a flow crosses is derived from geometry**. Dragging a data store out of its +boundary changes no stored property at all, so on the structural comparison alone that edit is +indistinguishable from no edit — while being the single change most likely to matter in review. + +`diff` therefore reports crossings as their own section: + +```text +Boundary crossings: + ~ flow "Browse & checkout" [Diagram 1] 6bf80ac0-f21b-46b4-ae4f-bafb8beef13a + - no longer crosses "Public Internet" + + now crosses "Partner Network" + +0 added, 0 removed, 0 modified, 1 with changed boundary crossings. +``` + +Flows and boundaries are matched by id, so renaming a boundary is not a crossing change. A flow that +was added or deleted is labelled as such, and one that never crossed anything is left out — the +element sections already report it, and repeating it here would bury the crossings that carry the +signal. Under `--json` the same information appears as `data.crossings`, counted in +`data.summary.crossingChanges`. + #### Readable `.tm7` diffs in git `--textconv` prints a canonical, deterministic outline of a **single** model. Wired as a git @@ -569,10 +593,11 @@ tmforge analyze payments.tm7 --suppressionFile suppressions.json --json ### `analysis` Validate a stored [analysis document](analysis-rules.md#the-analysis-document) — the -`.analysis.json` written by `analyze --reportFolder`. +`.analysis.json` written by `analyze --reportFolder` — or compare two of them. ```text tmforge analysis validate [--model ] [--expect-version ] [--json] +tmforge analysis diff [--json] ``` | Option | Meaning | @@ -609,6 +634,45 @@ The command also refuses a document written by a **newer** build, rather than re assumptions and silently misinterpreting fields whose meaning has changed. Version 1 is currently the only schema version, so there is nothing to migrate from yet. +#### Comparing two analyses + +`analysis diff` answers the question a count cannot: not how many findings there are now, but **which +ones arrived**. + +```bash +tmforge analysis diff before/payments.analysis.json after/payments.analysis.json +``` + +```text +Introduced: + warning TM1014:48761fb5…:f59d24bf…:0 (generated-threat) + Data store [Session Secrets …] stores credentials but is not encrypted at rest… + +Resolved: + warning TM1021:48761fb5…:6b3c361c…:0 (generated-threat) + Data store [Audit Log …] holds log or audit data and its Signed property is not evidenced… + +4 introduced, 1 resolved, 0 reclassified, 1 unchanged. +``` + +Findings are matched on the stable id every document carries, +`{ruleId}:{diagram}:{target}:{occurrence}` — never on position, which renumbers the moment a rule is +enabled or disabled. Three consequences are worth knowing: + +- **Renaming an element is not a finding change.** The message carries the display name, so it is + rewritten; the identity is not. Editorial churn stays out of the delta. +- **A suppressed finding is reclassified, not resolved.** Suppressing something records it with a new + disposition rather than dropping it, and saying "resolved" would claim the underlying condition + went away — which a suppression explicitly does not claim. A rule whose severity was reconfigured + lands in the same bucket. +- **A changed rule selection is reported, not folded in.** If the analyzer fingerprints differ, the + command still compares but warns, so a finding that appeared only because a new rule was switched + on is not read as one the change introduced. + +**Exit codes:** `0` when nothing was introduced, `1` on tool error, `2` when findings were +introduced — so a gate can depend on "no new findings" without comparing counts itself. Resolving +findings never fails the command. + ### `threats` Report the model's **threats** — the persisted, triaged view of the threat-bearing analysis findings. diff --git a/docs/deployment.md b/docs/deployment.md index 4a5625e..06b762d 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -201,8 +201,28 @@ The image mounts your files at `/work`, so paths in your commands are relative t ## CI/CD -You can run validation two ways in CI: with a downloaded self-contained binary, or with the CLI -container image. +On GitHub, use the first-party action. Elsewhere, run the self-contained binary or the CLI container. + +### With the first-party GitHub Action (recommended) + +```yaml +permissions: + contents: read + security-events: write + +steps: + - uses: actions/checkout@v4 + - uses: hacks4snacks/tmforge@v0.3 + with: + version: "0.3" # pin the engine image, not just the action ref + models: "**/*.tm7" + max-severity: warning +``` + +It analyzes every matching model, uploads SARIF to code scanning, and gates the build. It also +accepts `rules`, `ruleset`, and `suppression-file`, so a pipeline gets the same custom-rule and +suppression behavior as the CLI. See [Analysis rules & CI](analysis-rules.md#ci-integration) for the +full input list. ### With the prebuilt binary (fastest cold start) diff --git a/examples/README.md b/examples/README.md index 20507cb..6757b35 100644 --- a/examples/README.md +++ b/examples/README.md @@ -32,3 +32,29 @@ tmforge analyze examples/webshop.tm7 # Machine-readable SARIF + HTML report into a folder. tmforge analyze examples/webshop.tm7 --reportFolder out/reports ``` + +## Custom rules and suppressions + +A matched pair showing how an organization layers its own policy on top of the built-in rules, and +how a reviewed exception is recorded. + +| File | What it is | +|------|------------| +| [`corporate-policy.tmrules.json`](corporate-policy.tmrules.json) | One declarative rule: a store holding audit data must state a `RetentionDays` retention period. | +| [`corporate-policy.suppressions.json`](corporate-policy.suppressions.json) | Records a reviewed exception for the audit log in `webshop.tm7`. | + +The rule reports an **error** against `webshop.tm7`, so it changes the gate outcome, and the +suppression clears exactly that finding: + +```bash +tmforge analyze examples/webshop.tm7 \ + --rules examples/corporate-policy.tmrules.json # exit 2 + +tmforge analyze examples/webshop.tm7 \ + --rules examples/corporate-policy.tmrules.json \ + --suppressionFile examples/corporate-policy.suppressions.json # exit 0 +``` + +CI runs this pair through both the CLI and the first-party Action and requires the same outcome from +each, which is what keeps the Action's rule and suppression wiring honest. A suppressed finding is +still produced and recorded — it simply stops gating the build. diff --git a/examples/corporate-policy.suppressions.json b/examples/corporate-policy.suppressions.json new file mode 100644 index 0000000..8c46605 --- /dev/null +++ b/examples/corporate-policy.suppressions.json @@ -0,0 +1,15 @@ +{ + "files": [ + { + "file": "webshop.tm7", + "suppressions": [ + { + "rule": "example-corporate/CORP-1", + "model": "Diagram 1", + "target": "Audit Log (Generic Data Store) ID=6b3c361c-b7be-5f01-be4b-0cd6e29c6686", + "justification": "Example only. Retention for this store is set by the platform log policy rather than the model, and the exception is reviewed each quarter." + } + ] + } + ] +} diff --git a/examples/corporate-policy.tmrules.json b/examples/corporate-policy.tmrules.json new file mode 100644 index 0000000..468bee7 --- /dev/null +++ b/examples/corporate-policy.tmrules.json @@ -0,0 +1,31 @@ +{ + "schema": "tmforge-rules", + "version": 2, + "dialect": "urn:tmforge:rules:flat-v1", + "pack": { + "id": "example-corporate", + "name": "Example corporate policy", + "version": "1.0.0" + }, + "elementTypes": [ + { "id": "GE.DS", "name": "Data store", "parentId": "ROOT" } + ], + "properties": [ + { + "name": "RetentionDays", + "elementTypeIds": ["GE.DS"] + } + ], + "rules": [ + { + "id": "CORP-1", + "severity": "error", + "appliesTo": "datastore", + "message": "{name} holds audit data but declares no RetentionDays, so how long the evidence survives is unstated.", + "fullDescription": "An internal policy example: a store flagged as holding log or audit data must state its retention period, because evidence that is silently aged out cannot support an investigation.", + "helpText": "Set a RetentionDays property on the store to the number of days records are kept.", + "when": { "property": "StoresLogData", "equals": "Yes" }, + "assert": { "property": "RetentionDays", "present": true } + } + ] +} diff --git a/src/ThreatModelForge.Analysis/BoundaryCrossingDiff.cs b/src/ThreatModelForge.Analysis/BoundaryCrossingDiff.cs new file mode 100644 index 0000000..68ce40e --- /dev/null +++ b/src/ThreatModelForge.Analysis/BoundaryCrossingDiff.cs @@ -0,0 +1,186 @@ +namespace ThreatModelForge.Analysis +{ + using System; + using System.Collections.Generic; + using System.Linq; + using ThreatModelForge.Editing; + using ThreatModelForge.Model; + using ThreatModelForge.Model.Abstracts; + + /// + /// Compares which trust boundaries each flow crosses between two revisions of a model. + /// + /// + /// + /// This exists because deliberately ignores geometry, and trust-boundary + /// containment is derived entirely from geometry. Dragging a data store out of its boundary + /// changes no stored property, so the structural diff correctly reports nothing — while the thing + /// a security reviewer most needs to know has just changed. Comparing the derived crossing state + /// closes that gap without weakening the structural diff's useful property of staying silent when + /// a model is merely re-laid-out. + /// + /// + /// Flows and boundaries are matched by their stable ids, so renaming a boundary is not a crossing + /// change and moving a flow between diagrams does not read as one flow deleted and another added. + /// + /// + public static class BoundaryCrossingDiff + { + /// + /// Captures which trust boundaries every flow in the model crosses. + /// + /// The model to capture. + /// One entry per flow, in the order the diagrams and flows are stored. + public static IReadOnlyList Capture(ThreatModel model) + { + if (model == null) + { + throw new ArgumentNullException(nameof(model)); + } + + List captured = new List(); + foreach (DrawingSurfaceModel surface in model.DrawingSurfaceList) + { + string diagramName = string.IsNullOrEmpty(surface.Header) ? "Diagram" : surface.Header!; + foreach (Connector connector in surface.Lines.Values.OfType()) + { + List crossed = surface + .TrustBoundaryCrossings(connector) + .Select(boundary => new CrossedBoundary + { + Id = boundary.Guid, + Name = DiagramElementHelper.GetName(boundary), + }) + .OrderBy(boundary => boundary.Id) + .ToList(); + + captured.Add(new FlowCrossings + { + FlowId = connector.Guid, + FlowName = DiagramElementHelper.GetName(connector), + DiagramName = diagramName, + DiagramId = surface.Guid, + Boundaries = crossed, + }); + } + } + + return captured; + } + + /// + /// Compares the trust-boundary crossings of two models. + /// + /// The base (left-hand) model. + /// The revised (right-hand) model. + /// The flows whose crossings differ; empty when nothing crosses differently. + public static CrossingDifference Compare(ThreatModel baseModel, ThreatModel revisedModel) + { + if (baseModel == null) + { + throw new ArgumentNullException(nameof(baseModel)); + } + + if (revisedModel == null) + { + throw new ArgumentNullException(nameof(revisedModel)); + } + + Dictionary before = Index(Capture(baseModel)); + Dictionary after = Index(Capture(revisedModel)); + + List changes = new List(); + + foreach (KeyValuePair entry in before) + { + if (after.TryGetValue(entry.Key, out FlowCrossings? revised)) + { + Add(changes, entry.Value, revised, ChangeKind.Modified); + } + else + { + Add(changes, entry.Value, null, ChangeKind.Removed); + } + } + + foreach (KeyValuePair entry in after.Where(entry => !before.ContainsKey(entry.Key))) + { + Add(changes, null, entry.Value, ChangeKind.Added); + } + + changes.Sort(CompareChanges); + return new CrossingDifference { Changes = changes }; + } + + private static Dictionary Index(IReadOnlyList crossings) + { + Dictionary map = new Dictionary(); + foreach (FlowCrossings crossing in crossings) + { + map[crossing.FlowId] = crossing; + } + + return map; + } + + /// + /// Records the crossing delta for one flow, unless nothing crosses differently. A flow that was + /// added or removed without ever crossing a boundary is left out: the structural diff already + /// reports it, and repeating it here would bury the crossings that matter. + /// + /// The list being built. + /// The flow's crossings in the base model, or when it is new. + /// The flow's crossings in the revised model, or when it was deleted. + /// What happened to the flow itself. + private static void Add( + ICollection changes, + FlowCrossings? before, + FlowCrossings? after, + ChangeKind kind) + { + IReadOnlyList previous = before?.Boundaries ?? Array.Empty(); + IReadOnlyList current = after?.Boundaries ?? Array.Empty(); + + List added = Except(current, previous); + List removed = Except(previous, current); + if (added.Count == 0 && removed.Count == 0) + { + return; + } + + FlowCrossings identity = after ?? before!; + changes.Add(new CrossingChange + { + FlowId = identity.FlowId, + FlowName = identity.FlowName, + DiagramName = identity.DiagramName, + Kind = kind, + Added = added, + Removed = removed, + }); + } + + private static List Except( + IReadOnlyList source, + IReadOnlyList exclude) + { + HashSet excluded = new HashSet(exclude.Select(boundary => boundary.Id)); + return source + .Where(boundary => !excluded.Contains(boundary.Id)) + .OrderBy(boundary => boundary.Id) + .ToList(); + } + + private static int CompareChanges(CrossingChange left, CrossingChange right) + { + int byDiagram = string.CompareOrdinal(left.DiagramName, right.DiagramName); + if (byDiagram != 0) + { + return byDiagram; + } + + int byName = string.CompareOrdinal(left.FlowName, right.FlowName); + return byName != 0 ? byName : left.FlowId.CompareTo(right.FlowId); + } + } +} diff --git a/src/ThreatModelForge.Analysis/CrossedBoundary.cs b/src/ThreatModelForge.Analysis/CrossedBoundary.cs new file mode 100644 index 0000000..602a9d0 --- /dev/null +++ b/src/ThreatModelForge.Analysis/CrossedBoundary.cs @@ -0,0 +1,17 @@ +namespace ThreatModelForge.Analysis +{ + using System; + + /// + /// A trust boundary that a flow crosses, identified by the stable id the boundary carries so a + /// renamed boundary is still recognised as the same one. + /// + public sealed class CrossedBoundary + { + /// Gets the boundary's stable id. + public Guid Id { get; init; } + + /// Gets the boundary's display name at the time it was captured. + public string Name { get; init; } = string.Empty; + } +} diff --git a/src/ThreatModelForge.Analysis/CrossingChange.cs b/src/ThreatModelForge.Analysis/CrossingChange.cs new file mode 100644 index 0000000..7e5c110 --- /dev/null +++ b/src/ThreatModelForge.Analysis/CrossingChange.cs @@ -0,0 +1,35 @@ +namespace ThreatModelForge.Analysis +{ + using System; + using System.Collections.Generic; + using ThreatModelForge.Editing; + + /// + /// How one flow's trust-boundary crossings changed between two revisions of a model. + /// + public sealed class CrossingChange + { + /// Gets the flow's stable id. + public Guid FlowId { get; init; } + + /// Gets the flow's display name, taken from the revision it exists in. + public string FlowName { get; init; } = string.Empty; + + /// Gets the name of the diagram the flow is drawn on. + public string DiagramName { get; init; } = string.Empty; + + /// + /// Gets what happened to the flow itself: and + /// mean the flow appeared or disappeared, so its crossings are + /// reported wholesale; means the flow exists in both + /// revisions and only what it crosses changed. + /// + public ChangeKind Kind { get; init; } = ChangeKind.Modified; + + /// Gets the boundaries the flow crosses now but did not cross before, ordered by id. + public IReadOnlyList Added { get; init; } = Array.Empty(); + + /// Gets the boundaries the flow crossed before but no longer crosses, ordered by id. + public IReadOnlyList Removed { get; init; } = Array.Empty(); + } +} diff --git a/src/ThreatModelForge.Analysis/CrossingDifference.cs b/src/ThreatModelForge.Analysis/CrossingDifference.cs new file mode 100644 index 0000000..5c54506 --- /dev/null +++ b/src/ThreatModelForge.Analysis/CrossingDifference.cs @@ -0,0 +1,17 @@ +namespace ThreatModelForge.Analysis +{ + using System; + using System.Collections.Generic; + + /// + /// The trust-boundary crossings that changed between two revisions of a model. + /// + public sealed class CrossingDifference + { + /// Gets the flows whose crossings changed, ordered by diagram and then by flow name. + public IReadOnlyList Changes { get; init; } = Array.Empty(); + + /// Gets a value indicating whether nothing crosses differently. + public bool IsEmpty => this.Changes.Count == 0; + } +} diff --git a/src/ThreatModelForge.Analysis/FlowCrossings.cs b/src/ThreatModelForge.Analysis/FlowCrossings.cs new file mode 100644 index 0000000..65f9be2 --- /dev/null +++ b/src/ThreatModelForge.Analysis/FlowCrossings.cs @@ -0,0 +1,32 @@ +namespace ThreatModelForge.Analysis +{ + using System; + using System.Collections.Generic; + + /// + /// The set of trust boundaries one flow crosses on one diagram. + /// + /// + /// This is derived state, not stored state. Which boundaries a flow crosses follows from where the + /// flow's endpoints sit relative to each boundary's geometry, so it changes when someone drags an + /// element even though no stored property changed. That is exactly why it is captured separately: + /// the structural diff ignores geometry on purpose, and would otherwise report nothing at all. + /// + public sealed class FlowCrossings + { + /// Gets the flow's stable id. + public Guid FlowId { get; init; } + + /// Gets the flow's display name. + public string FlowName { get; init; } = string.Empty; + + /// Gets the name of the diagram the flow is drawn on. + public string DiagramName { get; init; } = string.Empty; + + /// Gets the id of the diagram the flow is drawn on. + public Guid DiagramId { get; init; } + + /// Gets the boundaries the flow crosses, ordered by id. + public IReadOnlyList Boundaries { get; init; } = Array.Empty(); + } +} diff --git a/src/ThreatModelForge.Cli/AnalysisCommand.cs b/src/ThreatModelForge.Cli/AnalysisCommand.cs index 956946c..b6f6fc6 100644 --- a/src/ThreatModelForge.Cli/AnalysisCommand.cs +++ b/src/ThreatModelForge.Cli/AnalysisCommand.cs @@ -8,15 +8,17 @@ namespace ThreatModelForge.Cli using ThreatModelForge.Model; /// - /// Implements tmforge analysis validate: checks a stored tmforge-analysis document, - /// and optionally checks it against the model it claims to describe. + /// Implements tmforge analysis: checks a stored tmforge-analysis document, and + /// compares two of them. /// /// /// A stored analysis is evidence, and evidence gets read long after it was written — by a gate, an /// auditor, or a person deciding whether a review still holds. Two things can be wrong with it and /// both are silent: the document can contradict itself, and it can be perfectly coherent but stale, - /// describing a model that has since changed. This checks for both, because a clean report about - /// last month's architecture is the more dangerous of the two. + /// describing a model that has since changed. validate checks for both, because a clean + /// report about last month's architecture is the more dangerous of the two. diff answers the + /// other question the artifact exists for: not how many findings there are now, but which ones + /// arrived. /// internal static class AnalysisCommand { @@ -46,6 +48,11 @@ public static int Run(string[] args) return SuccessExitCode; } + if (string.Equals(subcommand, "diff", StringComparison.Ordinal)) + { + return Diff(args[1..]); + } + if (!string.Equals(subcommand, "validate", StringComparison.Ordinal)) { Console.Error.WriteLine("Unknown subcommand: " + subcommand); @@ -133,6 +140,183 @@ private static int Validate(string[] args) return Report(parsed.Json, input!, problems, stale, document); } + /// + /// Compares two stored analyses by finding identity, so the answer is which findings arrived + /// and which went away rather than how many there are. + /// + /// The arguments after the subcommand. + /// Zero when nothing was introduced; 1 on tool error; 2 when findings were introduced. + private static int Diff(string[] args) + { + CliArgs parsed = CliArgs.Parse(args, Array.Empty(), Array.Empty()); + if (parsed.Help) + { + PrintUsage(); + return SuccessExitCode; + } + + if (parsed.UnknownFlags.Count > 0) + { + Console.Error.WriteLine("Unknown option: " + parsed.UnknownFlags[0]); + PrintUsage(); + return ErrorExitCode; + } + + if (parsed.Positionals.Count < 2) + { + PrintUsage(); + return ErrorExitCode; + } + + if (!TryLoad(parsed.Positionals[0], out AnalysisDocumentDto? baseDocument) || + !TryLoad(parsed.Positionals[1], out AnalysisDocumentDto? headDocument)) + { + return ErrorExitCode; + } + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(baseDocument!, headDocument!); + + if (parsed.Json) + { + CliJson.WriteEnvelope("analysis", new + { + operation = "diff", + baseDocument = parsed.Positionals[0], + headDocument = parsed.Positionals[1], + summary = new + { + introduced = difference.Introduced.Count, + resolved = difference.Resolved.Count, + reclassified = difference.Reclassified.Count, + unchanged = difference.Unchanged, + }, + introduced = difference.Introduced.Select(Describe).ToArray(), + resolved = difference.Resolved.Select(Describe).ToArray(), + reclassified = difference.Reclassified + .Select(change => new + { + id = change.After.Id, + ruleId = change.After.RuleId, + diagram = change.After.Diagram, + message = change.After.Message, + from = new { disposition = change.Before.Disposition, severity = change.Before.Severity }, + to = new { disposition = change.After.Disposition, severity = change.After.Severity }, + }) + .ToArray(), + warnings = difference.Warnings, + }); + } + else + { + WriteDifference(difference); + } + + // The warnings go to stderr in both modes: they qualify the answer rather than form part + // of it, and a caller piping --json into a tool should still see them. + foreach (string warning in difference.Warnings) + { + Console.Error.WriteLine("warning: " + warning); + } + + return difference.Introduced.Count > 0 ? ProblemsExitCode : SuccessExitCode; + } + + private static bool TryLoad(string path, out AnalysisDocumentDto? document) + { + document = null; + if (!File.Exists(path)) + { + Console.Error.WriteLine("File not found: " + path); + return false; + } + + if (AnalysisDocumentReader.TryRead( + File.ReadAllText(path), + null, + out document, + out IReadOnlyList problems)) + { + return true; + } + + foreach (string problem in problems) + { + Console.Error.WriteLine(path + ": " + problem); + } + + return false; + } + + private static void WriteDifference(AnalysisDifference difference) + { + if (difference.IsEmpty) + { + Console.WriteLine( + "No change in findings (" + + difference.Unchanged.ToString(CultureInfo.InvariantCulture) + " unchanged)."); + return; + } + + WriteFindings("Introduced", difference.Introduced); + WriteFindings("Resolved", difference.Resolved); + + if (difference.Reclassified.Count > 0) + { + Console.WriteLine("Reclassified:"); + foreach (AnalysisFindingChange change in difference.Reclassified) + { + Console.WriteLine( + " " + change.After.Id + " " + + Classification(change.Before) + " -> " + Classification(change.After)); + Console.WriteLine(" " + change.After.Message); + } + + Console.WriteLine(); + } + + Console.WriteLine( + difference.Introduced.Count.ToString(CultureInfo.InvariantCulture) + " introduced, " + + difference.Resolved.Count.ToString(CultureInfo.InvariantCulture) + " resolved, " + + difference.Reclassified.Count.ToString(CultureInfo.InvariantCulture) + " reclassified, " + + difference.Unchanged.ToString(CultureInfo.InvariantCulture) + " unchanged."); + } + + private static void WriteFindings(string title, IReadOnlyList findings) + { + if (findings.Count == 0) + { + return; + } + + Console.WriteLine(title + ":"); + foreach (AnalysisFindingDto finding in findings) + { + Console.WriteLine(" " + finding.Severity + " " + finding.Id + " (" + finding.Disposition + ")"); + Console.WriteLine(" " + finding.Message); + } + + Console.WriteLine(); + } + + private static string Classification(AnalysisFindingDto finding) + { + return finding.Severity + "/" + finding.Disposition; + } + + private static object Describe(AnalysisFindingDto finding) + { + return new + { + id = finding.Id, + ruleId = finding.RuleId, + severity = finding.Severity, + disposition = finding.Disposition, + diagram = finding.Diagram, + message = finding.Message, + threatId = finding.ThreatId, + }; + } + private static int Report( bool json, string path, @@ -173,9 +357,10 @@ private static int Report( private static void PrintUsage() { - Console.Error.WriteLine("Validate a stored tmforge-analysis document."); + Console.Error.WriteLine("Validate a stored tmforge-analysis document, or compare two of them."); Console.Error.WriteLine("Usage:"); Console.Error.WriteLine(" tmforge analysis validate [--model ] [--expect-version ] [--json] "); + Console.Error.WriteLine(" tmforge analysis diff [--json] "); Console.Error.WriteLine(); Console.Error.WriteLine("Options:"); Console.Error.WriteLine(" --model Also check the document against the model it describes, so a"); @@ -184,7 +369,12 @@ private static void PrintUsage() Console.Error.WriteLine(" other version is refused rather than read on wrong assumptions."); Console.Error.WriteLine(" --json Emit the result as JSON."); Console.Error.WriteLine(); - Console.Error.WriteLine("Exit codes: 0 = sound; 1 = tool error; 2 = problems found."); + Console.Error.WriteLine("diff matches findings on their stable id, so it reports which findings were introduced"); + Console.Error.WriteLine("and resolved rather than how the count moved, and renaming an element is not a change."); + Console.Error.WriteLine("A changed rule selection is reported as a warning, not silently folded in."); + Console.Error.WriteLine(); + Console.Error.WriteLine("Exit codes: 0 = sound (diff: nothing introduced); 1 = tool error;"); + Console.Error.WriteLine(" 2 = problems found (diff: findings introduced)."); } } } diff --git a/src/ThreatModelForge.Cli/DiffCommand.cs b/src/ThreatModelForge.Cli/DiffCommand.cs index 5cd9b69..f97b038 100644 --- a/src/ThreatModelForge.Cli/DiffCommand.cs +++ b/src/ThreatModelForge.Cli/DiffCommand.cs @@ -5,6 +5,7 @@ namespace ThreatModelForge.Cli using System.IO; using System.Linq; using System.Text; + using ThreatModelForge.Analysis; using ThreatModelForge.Editing; using ThreatModelForge.Model; @@ -14,6 +15,12 @@ namespace ThreatModelForge.Cli /// produces no diff; only added, removed, and modified elements (with per-property changes) are /// reported. /// + /// + /// The structural diff ignores geometry, which is what keeps it quiet when a model is merely + /// re-laid-out. Trust-boundary containment, however, is derived from geometry, so the diff is + /// paired with a comparison of what each flow crosses — otherwise dragging a data store out of its + /// boundary would report nothing at all. + /// internal static class DiffCommand { /// @@ -61,14 +68,15 @@ public static int Run(string[] args) (ThreatModel revisedModel, _) = CliModelLoader.Load(revisedPath); ModelDifference difference = ModelDiff.Compare(baseModel, revisedModel); + CrossingDifference crossings = BoundaryCrossingDiff.Compare(baseModel, revisedModel); if (parsed.Json) { - CliJson.WriteEnvelope("diff", BuildPayload(difference)); + CliJson.WriteEnvelope("diff", BuildPayload(difference, crossings)); return 0; } - WriteText(difference); + WriteText(difference, crossings); return 0; } @@ -136,7 +144,7 @@ private static string RenderCanonical(ThreatModel model) return builder.ToString(); } - private static object BuildPayload(ModelDifference difference) + private static object BuildPayload(ModelDifference difference, CrossingDifference crossings) { return new { @@ -145,13 +153,33 @@ private static object BuildPayload(ModelDifference difference) added = difference.Added.Count, removed = difference.Removed.Count, modified = difference.Modified.Count, + crossingChanges = crossings.Changes.Count, }, added = difference.Added.Select(ToPayload).ToArray(), removed = difference.Removed.Select(ToPayload).ToArray(), modified = difference.Modified.Select(ToPayload).ToArray(), + crossings = crossings.Changes.Select(ToPayload).ToArray(), }; } + private static object ToPayload(CrossingChange change) + { + return new + { + flowId = change.FlowId, + flow = change.FlowName, + diagram = change.DiagramName, + kind = change.Kind.ToString().ToLowerInvariant(), + added = change.Added.Select(ToPayload).ToArray(), + removed = change.Removed.Select(ToPayload).ToArray(), + }; + } + + private static object ToPayload(CrossedBoundary boundary) + { + return new { id = boundary.Id, name = boundary.Name }; + } + private static object ToPayload(ElementChange change) { return new @@ -167,9 +195,9 @@ private static object ToPayload(ElementChange change) }; } - private static void WriteText(ModelDifference difference) + private static void WriteText(ModelDifference difference, CrossingDifference crossings) { - if (difference.IsEmpty) + if (difference.IsEmpty && crossings.IsEmpty) { Console.WriteLine("No differences."); return; @@ -178,11 +206,48 @@ private static void WriteText(ModelDifference difference) WriteSection("Added", "+", difference.Added, includeProperties: false); WriteSection("Removed", "-", difference.Removed, includeProperties: false); WriteSection("Modified", "~", difference.Modified, includeProperties: true); + WriteCrossings(crossings); Console.WriteLine( difference.Added.Count + " added, " + difference.Removed.Count + " removed, " - + difference.Modified.Count + " modified."); + + difference.Modified.Count + " modified, " + + crossings.Changes.Count + " with changed boundary crossings."); + } + + /// + /// Writes the flows whose trust-boundary crossings changed. This is reported separately from + /// the element sections because it is derived from geometry rather than from any stored + /// property: a flow can appear in no other section and still have started crossing a boundary. + /// + /// The crossing difference to render. + private static void WriteCrossings(CrossingDifference crossings) + { + if (crossings.IsEmpty) + { + return; + } + + Console.WriteLine("Boundary crossings:"); + foreach (CrossingChange change in crossings.Changes) + { + string page = string.IsNullOrEmpty(change.DiagramName) ? string.Empty : " [" + change.DiagramName + "]"; + string state = change.Kind == ChangeKind.Modified + ? string.Empty + : " (" + change.Kind.ToString().ToLowerInvariant() + " flow)"; + Console.WriteLine(" ~ flow \"" + change.FlowName + "\"" + page + state + " " + change.FlowId); + foreach (CrossedBoundary boundary in change.Added) + { + Console.WriteLine(" + now crosses \"" + boundary.Name + "\""); + } + + foreach (CrossedBoundary boundary in change.Removed) + { + Console.WriteLine(" - no longer crosses \"" + boundary.Name + "\""); + } + } + + Console.WriteLine(); } private static void WriteSection(string title, string marker, IReadOnlyList changes, bool includeProperties) @@ -226,6 +291,10 @@ private static void PrintUsage() Console.Error.WriteLine("Elements are compared by their stable id, so re-layout or re-serialization produces"); Console.Error.WriteLine("no diff. Reports added, removed, and modified elements with per-property changes."); Console.Error.WriteLine(); + Console.Error.WriteLine("Trust-boundary crossings are reported separately, because which boundaries a flow"); + Console.Error.WriteLine("crosses is derived from geometry: moving an element across a boundary changes no"); + Console.Error.WriteLine("stored property, so it would otherwise not show up as a difference at all."); + Console.Error.WriteLine(); Console.Error.WriteLine("--textconv prints a canonical, deterministic outline of a single model, for use as a"); Console.Error.WriteLine("git textconv so 'git diff' renders readable .tm7 changes. Wire it up with:"); Console.Error.WriteLine(" .gitattributes: *.tm7 diff=tmforge"); diff --git a/src/ThreatModelForge.Editing/DiagramElementHelper.cs b/src/ThreatModelForge.Editing/DiagramElementHelper.cs index 50bd04e..3f390fd 100644 --- a/src/ThreatModelForge.Editing/DiagramElementHelper.cs +++ b/src/ThreatModelForge.Editing/DiagramElementHelper.cs @@ -77,6 +77,16 @@ public static void SetCustomProperty(Entity element, string key, string value) throw new ArgumentException("Value cannot be null or empty.", nameof(key)); } + // A property the tool has already typed is stored as a list selection, not as a custom + // attribute. Update that selection in place: adding a custom attribute beside it would + // leave the element holding two answers to the same question, and on the next read + // whichever one happens to come first wins. + if (TrySelectListValue(element, key, value)) + { + RemoveCustomProperty(element, key); + return; + } + string encoded = key + ":" + value; string prefix = key + ":"; foreach (CustomStringDisplayAttribute property in element.Properties.OfType()) @@ -148,5 +158,56 @@ private static bool IsNameProperty(StringDisplayAttribute property) return string.Equals(property.Name, NamePropertyName, StringComparison.OrdinalIgnoreCase) || string.Equals(property.DisplayName, NamePropertyName, StringComparison.OrdinalIgnoreCase); } + + /// + /// Points an existing typed list property at . + /// + /// + /// Returns when the element has no such list property, or when the + /// value is not one of the options the tool offers for it. In that second case the caller + /// falls back to a custom attribute, which preserves the value rather than dropping it and + /// which readers give precedence over the typed selection. + /// + /// The element. + /// The property key, matched against the list's display name. + /// The value to select. + /// when a typed selection was updated. + private static bool TrySelectListValue(Entity element, string key, string value) + { + foreach (ListDisplayAttribute list in element.Properties.OfType()) + { + if (!string.Equals(list.DisplayName ?? string.Empty, key, StringComparison.OrdinalIgnoreCase) || + !(list.Value is string[] options)) + { + continue; + } + + int index = Array.FindIndex( + options, + option => string.Equals(option, value, StringComparison.OrdinalIgnoreCase)); + if (index < 0) + { + return false; + } + + list.SelectedIndex = index; + return true; + } + + return false; + } + + private static void RemoveCustomProperty(Entity element, string key) + { + string prefix = key + ":"; + foreach (CustomStringDisplayAttribute stale in element.Properties + .OfType() + .Where(property => (property.Value as string ?? string.Empty) + .StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) + .ToList()) + { + element.Properties.Remove(stale); + } + } } } diff --git a/src/ThreatModelForge.Editing/SchemaBackedProperties.cs b/src/ThreatModelForge.Editing/SchemaBackedProperties.cs index 8d341cb..aecdebe 100644 --- a/src/ThreatModelForge.Editing/SchemaBackedProperties.cs +++ b/src/ThreatModelForge.Editing/SchemaBackedProperties.cs @@ -142,6 +142,12 @@ private static void TypeElement(Entity? element) } element.Properties.Remove(custom); + + // Replace any typed property already recorded for this key rather than adding a + // second one. Readers take the first list attribute they find, so leaving the old one + // in place would silently keep the old value and quietly grow the element by one + // attribute per edit. + RemoveListProperties(element, descriptor.Name); element.Properties.Add(new ListDisplayAttribute { Name = descriptor.Name, @@ -152,6 +158,17 @@ private static void TypeElement(Entity? element) } } + private static void RemoveListProperties(Entity element, string name) + { + foreach (ListDisplayAttribute stale in element.Properties + .OfType() + .Where(list => string.Equals(list.DisplayName ?? string.Empty, name, StringComparison.OrdinalIgnoreCase)) + .ToList()) + { + element.Properties.Remove(stale); + } + } + private static bool IsListProperty(PropertyDescriptor descriptor) { return string.Equals(descriptor.Kind, "enum", StringComparison.OrdinalIgnoreCase) diff --git a/src/ThreatModelForge.Engine/AnalysisDifference.cs b/src/ThreatModelForge.Engine/AnalysisDifference.cs new file mode 100644 index 0000000..dd9c017 --- /dev/null +++ b/src/ThreatModelForge.Engine/AnalysisDifference.cs @@ -0,0 +1,37 @@ +namespace ThreatModelForge.Engine +{ + using System; + using System.Collections.Generic; + + /// + /// What changed between two tmforge-analysis documents. + /// + public sealed class AnalysisDifference + { + /// Gets the findings the head analysis reports and the base analysis did not. + public IReadOnlyList Introduced { get; init; } = Array.Empty(); + + /// Gets the findings the base analysis reported and the head analysis does not. + public IReadOnlyList Resolved { get; init; } = Array.Empty(); + + /// Gets the findings present in both whose disposition or severity moved. + public IReadOnlyList Reclassified { get; init; } = Array.Empty(); + + /// Gets the number of findings present in both and classified identically. + public int Unchanged { get; init; } + + /// + /// Gets the conditions that make the comparison less meaningful than it looks — a changed rule + /// selection, two different models, or a difference no recorded input explains. + /// + /// + /// These are reported rather than acted on. Refusing to compare would be unhelpful, and + /// comparing silently would let a rule change read as a model change. + /// + public IReadOnlyList Warnings { get; init; } = Array.Empty(); + + /// Gets a value indicating whether the two analyses reached the same conclusions. + public bool IsEmpty => + this.Introduced.Count == 0 && this.Resolved.Count == 0 && this.Reclassified.Count == 0; + } +} diff --git a/src/ThreatModelForge.Engine/AnalysisDocumentDiff.cs b/src/ThreatModelForge.Engine/AnalysisDocumentDiff.cs new file mode 100644 index 0000000..0389da6 --- /dev/null +++ b/src/ThreatModelForge.Engine/AnalysisDocumentDiff.cs @@ -0,0 +1,162 @@ +namespace ThreatModelForge.Engine +{ + using System; + using System.Collections.Generic; + using System.Linq; + + /// + /// Compares two tmforge-analysis documents by finding identity. + /// + /// + /// + /// Findings are matched on the stable id every document carries, + /// {ruleId}:{diagram}:{target}:{occurrence}. That is the point of the whole exercise: a + /// count tells you the number moved, and a positional comparison renumbers the moment a rule is + /// enabled or disabled, but an identity tells you which finding arrived and which one + /// went away. Renaming an element changes a finding's message and not its identity, so editorial + /// churn does not read as security churn. + /// + /// + /// The comparison never refuses. Two documents can be less comparable than they look — a different + /// rule selection, or two different models entirely — and in that case the difference is reported + /// alongside a warning saying so, because a reviewer who can see the caveat is better served than + /// one who gets an error or, worse, a confident answer. + /// + /// + public static class AnalysisDocumentDiff + { + /// + /// Compares a base analysis against a head analysis. + /// + /// The earlier analysis. + /// The later analysis. + /// The findings introduced, resolved, and reclassified between them. + public static AnalysisDifference Compare( + AnalysisDocumentDto baseDocument, + AnalysisDocumentDto headDocument) + { + _ = baseDocument ?? throw new ArgumentNullException(nameof(baseDocument)); + _ = headDocument ?? throw new ArgumentNullException(nameof(headDocument)); + + Dictionary before = Index(baseDocument.Findings); + Dictionary after = Index(headDocument.Findings); + + List introduced = new List(); + List resolved = new List(); + List reclassified = new List(); + int unchanged = 0; + + foreach (KeyValuePair entry in before) + { + if (!after.TryGetValue(entry.Key, out AnalysisFindingDto? head)) + { + resolved.Add(entry.Value); + continue; + } + + if (IsReclassified(entry.Value, head)) + { + reclassified.Add(new AnalysisFindingChange { Before = entry.Value, After = head }); + } + else + { + unchanged++; + } + } + + introduced.AddRange(after.Where(entry => !before.ContainsKey(entry.Key)).Select(entry => entry.Value)); + + introduced.Sort(CompareFindings); + resolved.Sort(CompareFindings); + reclassified.Sort((left, right) => CompareFindings(left.After, right.After)); + + return new AnalysisDifference + { + Introduced = introduced, + Resolved = resolved, + Reclassified = reclassified, + Unchanged = unchanged, + Warnings = Warnings(baseDocument, headDocument, introduced, resolved, reclassified), + }; + } + + private static Dictionary Index(IReadOnlyList findings) + { + Dictionary map = + new Dictionary(StringComparer.Ordinal); + foreach (AnalysisFindingDto finding in findings) + { + // Indexer assignment rather than Add: a duplicate id is a document defect that + // 'analysis validate' reports, and refusing to compare here would only hide it. + map[finding.Id] = finding; + } + + return map; + } + + /// + /// Decides whether a surviving finding was classified differently. Only the disposition and the + /// severity count: the message carries the element's display name, so treating it as a change + /// would report every rename as a finding change. + /// + /// The finding as the base document recorded it. + /// The finding as the head document records it. + /// when the classification moved. + private static bool IsReclassified(AnalysisFindingDto before, AnalysisFindingDto after) + { + return !string.Equals(before.Disposition, after.Disposition, StringComparison.Ordinal) + || !string.Equals(before.Severity, after.Severity, StringComparison.Ordinal); + } + + private static IReadOnlyList Warnings( + AnalysisDocumentDto baseDocument, + AnalysisDocumentDto headDocument, + IReadOnlyList introduced, + IReadOnlyList resolved, + IReadOnlyList reclassified) + { + List warnings = new List(); + + if (!string.Equals(baseDocument.Model.Name, headDocument.Model.Name, StringComparison.Ordinal)) + { + warnings.Add( + $"These documents describe different models ('{baseDocument.Model.Name}' and " + + $"'{headDocument.Model.Name}'). Findings are being compared across models."); + } + + bool sameAnalyzer = string.Equals( + baseDocument.Analyzer.Fingerprint, + headDocument.Analyzer.Fingerprint, + StringComparison.Ordinal); + + if (!sameAnalyzer) + { + warnings.Add( + "The analyzer or its rule selection changed between these runs, so some of these " + + "differences may come from the rules rather than from the model."); + } + + bool sameModel = string.Equals( + baseDocument.Model.Fingerprint, + headDocument.Model.Fingerprint, + StringComparison.Ordinal); + + bool differs = introduced.Count > 0 || resolved.Count > 0 || reclassified.Count > 0; + if (sameModel && sameAnalyzer && differs) + { + // Both fingerprints cover every input except the suppression list, and the documents + // carry no timestamp, so this combination has exactly one ordinary explanation. + warnings.Add( + "The model and analyzer fingerprints are identical yet the findings differ. " + + "Suppressions are the only other input to an analysis; check whether they changed."); + } + + return warnings; + } + + private static int CompareFindings(AnalysisFindingDto left, AnalysisFindingDto right) + { + return string.CompareOrdinal(left.Id, right.Id); + } + } +} diff --git a/src/ThreatModelForge.Engine/AnalysisFindingChange.cs b/src/ThreatModelForge.Engine/AnalysisFindingChange.cs new file mode 100644 index 0000000..9e9ff36 --- /dev/null +++ b/src/ThreatModelForge.Engine/AnalysisFindingChange.cs @@ -0,0 +1,20 @@ +namespace ThreatModelForge.Engine +{ + /// + /// One finding that survived between two analyses but whose classification moved. + /// + /// + /// The finding is the same one — same rule, same target, same occurrence — so this is not a change + /// to what was detected but to what was concluded about it. Accepting a risk and a rule's severity + /// being reconfigured both land here, and both are worth a reviewer's attention for different + /// reasons than an introduced finding is. + /// + public sealed class AnalysisFindingChange + { + /// Gets the finding as the base document recorded it. + public AnalysisFindingDto Before { get; init; } = new AnalysisFindingDto(); + + /// Gets the finding as the head document records it. + public AnalysisFindingDto After { get; init; } = new AnalysisFindingDto(); + } +} diff --git a/test/ThreatModelForge.Analysis.Tests/BoundaryCrossingDiffTests.cs b/test/ThreatModelForge.Analysis.Tests/BoundaryCrossingDiffTests.cs new file mode 100644 index 0000000..adb2107 --- /dev/null +++ b/test/ThreatModelForge.Analysis.Tests/BoundaryCrossingDiffTests.cs @@ -0,0 +1,249 @@ +namespace ThreatModelForge.Analysis.Tests +{ + using System; + using System.Collections.Generic; + using System.Linq; + using Microsoft.VisualStudio.TestTools.UnitTesting; + using ThreatModelForge.Editing; + using ThreatModelForge.Model; + + /// + /// Unit tests for the class. + /// + [TestClass] + public class BoundaryCrossingDiffTests + { + /// + /// Gets or sets the test context. + /// + public TestContext? TestContext { get; set; } + + /// + /// The reason this comparison exists at all: moving an element across a trust boundary changes + /// no stored property, so the structural diff correctly sees nothing. If that ever stops being + /// true this test should be revisited rather than deleted — but until then, the crossing + /// comparison is the only thing standing between a reviewer and a silent change of exposure. + /// + [TestMethod] + public void MovingAFlowOutOfABoundaryIsInvisibleToTheStructuralDiffButReportedAsACrossingChange() + { + ThreatModel baseModel = Build(out Guid flowId, out Guid boundaryId); + ThreatModel revised = Build(out _, out _, sourceX: 10, sourceY: 10); + + Assert.IsTrue( + ModelDiff.Compare(baseModel, revised).IsEmpty, + "the structural diff is expected to see nothing here; that is the gap being covered"); + + CrossingDifference crossings = BoundaryCrossingDiff.Compare(baseModel, revised); + + CrossingChange change = crossings.Changes.Single(); + Assert.AreEqual(flowId, change.FlowId); + Assert.AreEqual(ChangeKind.Modified, change.Kind); + Assert.AreEqual(0, change.Added.Count); + Assert.AreEqual(boundaryId, change.Removed.Single().Id); + Assert.AreEqual("Public Internet", change.Removed.Single().Name); + } + + /// + /// Verifies that a flow which starts crossing a boundary is reported as an addition, which is + /// the direction a reviewer cares about most. + /// + [TestMethod] + public void AFlowThatStartsCrossingIsReportedAsAdded() + { + ThreatModel baseModel = Build(out _, out Guid boundaryId, sourceX: 10, sourceY: 10); + ThreatModel revised = Build(out _, out _); + + CrossingChange change = BoundaryCrossingDiff.Compare(baseModel, revised).Changes.Single(); + + Assert.AreEqual(ChangeKind.Modified, change.Kind); + Assert.AreEqual(boundaryId, change.Added.Single().Id); + Assert.AreEqual(0, change.Removed.Count); + } + + /// + /// Verifies that renaming a boundary is not mistaken for the flow crossing a different one. + /// Boundaries are matched by id precisely so that editorial changes stay quiet. + /// + [TestMethod] + public void RenamingABoundaryIsNotACrossingChange() + { + ThreatModel baseModel = Build(out _, out _); + ThreatModel revised = Build(out _, out _, boundaryName: "Renamed Boundary"); + + Assert.IsTrue(BoundaryCrossingDiff.Compare(baseModel, revised).IsEmpty); + } + + /// + /// Verifies that a model compared against itself reports nothing. + /// + [TestMethod] + public void AnUnchangedModelHasNoCrossingChanges() + { + ThreatModel baseModel = Build(out _, out _); + ThreatModel revised = Build(out _, out _); + + Assert.IsTrue(BoundaryCrossingDiff.Compare(baseModel, revised).IsEmpty); + } + + /// + /// Verifies that a brand new flow which crosses a boundary is reported, and labelled as a new + /// flow so the summary does not imply an existing flow changed. + /// + [TestMethod] + public void ANewFlowThatCrossesABoundaryIsReportedAsAnAddedFlow() + { + ThreatModel baseModel = Build(out _, out Guid boundaryId); + ThreatModel revised = Build(out _, out _); + AddFlow(revised, Guid.NewGuid(), "Second flow", 60, 60, 500, 500); + + CrossingChange change = BoundaryCrossingDiff.Compare(baseModel, revised).Changes.Single(); + + Assert.AreEqual(ChangeKind.Added, change.Kind); + Assert.AreEqual("Second flow", change.FlowName); + Assert.AreEqual(boundaryId, change.Added.Single().Id); + } + + /// + /// Verifies that deleting a flow which crossed a boundary is reported, so that removing + /// exposure is as visible as adding it. + /// + [TestMethod] + public void ADeletedFlowThatCrossedABoundaryIsReportedAsARemovedFlow() + { + ThreatModel baseModel = Build(out Guid flowId, out Guid boundaryId); + ThreatModel revised = Build(out _, out _); + revised.DrawingSurfaceList[0].Lines.Remove(flowId); + + CrossingChange change = BoundaryCrossingDiff.Compare(baseModel, revised).Changes.Single(); + + Assert.AreEqual(ChangeKind.Removed, change.Kind); + Assert.AreEqual(boundaryId, change.Removed.Single().Id); + } + + /// + /// Verifies that adding or removing a flow which never crossed anything produces no crossing + /// change. The structural diff already reports it, and repeating it here would drown the + /// crossings that carry the security signal. + /// + [TestMethod] + public void AFlowThatCrossesNothingIsNotReported() + { + ThreatModel baseModel = Build(out _, out _); + ThreatModel revised = Build(out _, out _); + AddFlow(revised, Guid.NewGuid(), "Internal call", 500, 500, 520, 520); + + Assert.IsTrue(BoundaryCrossingDiff.Compare(baseModel, revised).IsEmpty); + } + + /// + /// Verifies that a flow which swaps one boundary for another reports both halves, rather than + /// netting them out to nothing. + /// + [TestMethod] + public void SwappingOneBoundaryForAnotherReportsBothHalves() + { + Guid second = Guid.NewGuid(); + ThreatModel baseModel = Build(out _, out Guid first); + ThreatModel revised = Build(out _, out _); + + // Put a second boundary around the far endpoint in the revised model and move the near + // endpoint out of the first, so exactly one crossing is traded for another. + AddBoundary(revised, second, "Partner Network", 400, 400, 200, 200); + Connector flow = revised.DrawingSurfaceList[0].Lines.Values.OfType().Single(); + flow.SourceX = 450; + flow.SourceY = 450; + flow.TargetX = 800; + flow.TargetY = 800; + + CrossingChange change = BoundaryCrossingDiff.Compare(baseModel, revised).Changes.Single(); + + Assert.AreEqual(second, change.Added.Single().Id); + Assert.AreEqual(first, change.Removed.Single().Id); + } + + /// + /// Verifies that records the diagram a + /// flow belongs to, so a multi-diagram model reports crossings against the right page. + /// + [TestMethod] + public void CaptureRecordsTheDiagramTheFlowIsDrawnOn() + { + ThreatModel model = Build(out Guid flowId, out _); + + FlowCrossings captured = BoundaryCrossingDiff.Capture(model).Single(); + + Assert.AreEqual(flowId, captured.FlowId); + Assert.AreEqual("DFD-0", captured.DiagramName); + Assert.AreEqual("Public Internet", captured.Boundaries.Single().Name); + } + + /// + /// Builds a one-diagram model with a single boundary and a single flow that crosses it: the + /// flow starts inside the boundary and ends outside it. The ids are deterministic so two calls + /// produce comparable models. Passing a source outside the boundary produces the same model + /// with no crossing. + /// + /// Receives the flow's id. + /// Receives the boundary's id. + /// The flow's source x coordinate. + /// The flow's source y coordinate. + /// The boundary's display name. + /// The model. + private static ThreatModel Build( + out Guid flowId, + out Guid boundaryId, + int sourceX = 60, + int sourceY = 60, + string boundaryName = "Public Internet") + { + flowId = new Guid("11111111-1111-1111-1111-111111111111"); + boundaryId = new Guid("22222222-2222-2222-2222-222222222222"); + + ThreatModel model = new ThreatModel(); + model.DrawingSurfaceList.Add(new DrawingSurfaceModel + { + Guid = new Guid("33333333-3333-3333-3333-333333333333"), + Header = "DFD-0", + }); + + AddBoundary(model, boundaryId, boundaryName, 50, 50, 200, 200); + AddFlow(model, flowId, "Browse", sourceX, sourceY, 500, 500); + return model; + } + + private static void AddBoundary(ThreatModel model, Guid id, string name, int left, int top, int width, int height) + { + BorderBoundary boundary = new BorderBoundary + { + Guid = id, + GenericTypeId = "GE.TB.B", + TypeId = "GE.TB.B", + Left = left, + Top = top, + Width = width, + Height = height, + }; + + DiagramElementHelper.SetName(boundary, name); + model.DrawingSurfaceList[0].Borders.Add(id, boundary); + } + + private static void AddFlow(ThreatModel model, Guid id, string name, int sourceX, int sourceY, int targetX, int targetY) + { + Connector flow = new Connector + { + Guid = id, + GenericTypeId = "GE.DF", + TypeId = "GE.DF", + SourceX = sourceX, + SourceY = sourceY, + TargetX = targetX, + TargetY = targetY, + }; + + DiagramElementHelper.SetName(flow, name); + model.DrawingSurfaceList[0].Lines.Add(id, flow); + } + } +} diff --git a/test/ThreatModelForge.Api.Tests/EngineAnalysisDiffTest.cs b/test/ThreatModelForge.Api.Tests/EngineAnalysisDiffTest.cs new file mode 100644 index 0000000..2ec08c8 --- /dev/null +++ b/test/ThreatModelForge.Api.Tests/EngineAnalysisDiffTest.cs @@ -0,0 +1,202 @@ +namespace ThreatModelForge.Api.Tests +{ + using System.Collections.Generic; + using System.Linq; + using Microsoft.VisualStudio.TestTools.UnitTesting; + using ThreatModelForge.Engine; + + /// + /// Tests for , the findings delta between two stored analyses. + /// + [TestClass] + public class EngineAnalysisDiffTest + { + /// + /// The property the whole comparison exists for: findings are matched on identity, so one that + /// went away and one that arrived are named rather than netted out into a count that happens to + /// be unchanged. + /// + [TestMethod] + public void FindingsAreMatchedByIdentityRatherThanCounted() + { + AnalysisDocumentDto before = Document(Finding("TM1021:d:a:0"), Finding("TM1029:d:b:0")); + AnalysisDocumentDto after = Document(Finding("TM1029:d:b:0"), Finding("TM1014:d:c:0")); + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(before, after); + + Assert.AreEqual(before.Findings.Count, after.Findings.Count, "the counts are equal on purpose"); + Assert.AreEqual("TM1014:d:c:0", difference.Introduced.Single().Id); + Assert.AreEqual("TM1021:d:a:0", difference.Resolved.Single().Id); + Assert.AreEqual(1, difference.Unchanged); + } + + /// + /// A finding's message carries the element's display name, so renaming an element rewrites it. + /// That must not read as a finding change, or every cosmetic edit would look like security + /// churn and the delta would stop being worth reading. + /// + [TestMethod] + public void RenamingTheElementAFindingIsAboutIsNotAChange() + { + AnalysisDocumentDto before = Document(Finding("TM1029:d:b:0", message: "The Web App ... is not audited")); + AnalysisDocumentDto after = Document(Finding("TM1029:d:b:0", message: "The Storefront ... is not audited")); + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(before, after); + + Assert.IsTrue(difference.IsEmpty); + Assert.AreEqual(1, difference.Unchanged); + } + + /// + /// Suppressing a finding records it with a new disposition rather than dropping it, so the + /// delta must report it as reclassified. Reporting it as resolved would say the underlying + /// condition went away, which is exactly what a suppression does not claim. + /// + [TestMethod] + public void ASuppressedFindingIsReclassifiedRatherThanResolved() + { + AnalysisDocumentDto before = Document(Finding("TM1029:d:b:0", disposition: FindingDispositions.GeneratedThreat)); + AnalysisDocumentDto after = Document(Finding("TM1029:d:b:0", disposition: FindingDispositions.Suppressed)); + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(before, after); + + Assert.AreEqual(0, difference.Resolved.Count); + Assert.AreEqual(0, difference.Introduced.Count); + AnalysisFindingChange change = difference.Reclassified.Single(); + Assert.AreEqual(FindingDispositions.GeneratedThreat, change.Before.Disposition); + Assert.AreEqual(FindingDispositions.Suppressed, change.After.Disposition); + } + + /// A rule whose severity was reconfigured is reported as reclassified. + [TestMethod] + public void AChangedSeverityIsReclassified() + { + AnalysisDocumentDto before = Document(Finding("TM1029:d:b:0", severity: "warning")); + AnalysisDocumentDto after = Document(Finding("TM1029:d:b:0", severity: "error")); + + AnalysisFindingChange change = AnalysisDocumentDiff.Compare(before, after).Reclassified.Single(); + + Assert.AreEqual("warning", change.Before.Severity); + Assert.AreEqual("error", change.After.Severity); + } + + /// Two analyses that reached the same conclusions report nothing. + [TestMethod] + public void IdenticalAnalysesReportNoDifference() + { + AnalysisDocumentDto document = Document(Finding("TM1021:d:a:0"), Finding("TM1029:d:b:0")); + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(document, document); + + Assert.IsTrue(difference.IsEmpty); + Assert.AreEqual(2, difference.Unchanged); + Assert.AreEqual(0, difference.Warnings.Count); + } + + /// + /// A changed rule selection is reported, because otherwise a finding that arrived only because + /// a new rule was switched on would read as a finding the change introduced. + /// + [TestMethod] + public void AChangedRuleSelectionIsWarnedAboutRatherThanFoldedIn() + { + AnalysisDocumentDto before = Document(Finding("TM1021:d:a:0")); + AnalysisDocumentDto after = Document(Finding("TM1021:d:a:0"), Finding("CORP-1:d:a:0")); + after = new AnalysisDocumentDto + { + Model = after.Model, + Analyzer = new AnalysisIdentityDto { Name = "tmforge", Version = "1", Fingerprint = "sha256:other" }, + Findings = after.Findings, + }; + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(before, after); + + Assert.AreEqual("CORP-1:d:a:0", difference.Introduced.Single().Id); + Assert.IsTrue( + difference.Warnings.Any(warning => warning.Contains("rule selection")), + string.Join(" | ", difference.Warnings)); + } + + /// Comparing analyses of two different models is reported as the mistake it usually is. + [TestMethod] + public void ComparingTwoDifferentModelsIsWarnedAbout() + { + AnalysisDocumentDto before = Document(Finding("TM1021:d:a:0")); + AnalysisDocumentDto after = new AnalysisDocumentDto + { + Model = new AnalysisIdentityDto { Name = "payments", Version = "1", Fingerprint = "sha256:model-b" }, + Analyzer = before.Analyzer, + Findings = before.Findings, + }; + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(before, after); + + Assert.IsTrue( + difference.Warnings.Any(warning => warning.Contains("different models")), + string.Join(" | ", difference.Warnings)); + } + + /// + /// The fingerprints cover every input to an analysis except the suppression list, and the + /// documents carry no timestamp, so identical fingerprints with differing findings has exactly + /// one ordinary explanation and the tool should name it rather than leave a reader guessing. + /// + [TestMethod] + public void IdenticalFingerprintsWithDifferingFindingsNamesTheRemainingInput() + { + AnalysisDocumentDto before = Document(Finding("TM1029:d:b:0", disposition: FindingDispositions.GeneratedThreat)); + AnalysisDocumentDto after = Document(Finding("TM1029:d:b:0", disposition: FindingDispositions.Suppressed)); + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(before, after); + + Assert.IsTrue( + difference.Warnings.Any(warning => warning.Contains("Suppressions")), + string.Join(" | ", difference.Warnings)); + } + + /// The result is ordered by identity so two runs of the same comparison agree. + [TestMethod] + public void IntroducedAndResolvedAreOrderedByIdentity() + { + AnalysisDocumentDto before = Document(Finding("TM1029:d:b:0"), Finding("TM1021:d:a:0")); + AnalysisDocumentDto after = Document(Finding("TM1099:d:z:0"), Finding("TM1014:d:c:0")); + + AnalysisDifference difference = AnalysisDocumentDiff.Compare(before, after); + + CollectionAssert.AreEqual( + new[] { "TM1014:d:c:0", "TM1099:d:z:0" }, + difference.Introduced.Select(finding => finding.Id).ToArray()); + CollectionAssert.AreEqual( + new[] { "TM1021:d:a:0", "TM1029:d:b:0" }, + difference.Resolved.Select(finding => finding.Id).ToArray()); + } + + private static AnalysisDocumentDto Document(params AnalysisFindingDto[] findings) + { + return new AnalysisDocumentDto + { + Model = new AnalysisIdentityDto { Name = "webshop", Version = "1", Fingerprint = "sha256:model-a" }, + Analyzer = new AnalysisIdentityDto { Name = "tmforge", Version = "1", Fingerprint = "sha256:rules-a" }, + Findings = findings, + }; + } + + private static AnalysisFindingDto Finding( + string id, + string severity = "warning", + string disposition = FindingDispositions.GeneratedThreat, + string message = "finding") + { + return new AnalysisFindingDto + { + Id = id, + RuleId = id.Split(':')[0], + Severity = severity, + Disposition = disposition, + Message = message, + Diagram = "Diagram 1", + ElementIds = new List(), + }; + } + } +} diff --git a/test/ThreatModelForge.Cli.Tests/AnalysisCommandTest.cs b/test/ThreatModelForge.Cli.Tests/AnalysisCommandTest.cs index c443678..4c58da9 100644 --- a/test/ThreatModelForge.Cli.Tests/AnalysisCommandTest.cs +++ b/test/ThreatModelForge.Cli.Tests/AnalysisCommandTest.cs @@ -31,6 +31,20 @@ public class AnalysisCommandTest "{\"id\":\"p1\",\"kind\":\"process\",\"name\":\"Checkout\",\"x\":200,\"y\":0}]," + "\"flows\":[{\"id\":\"f2\",\"source\":\"p1\",\"target\":\"s1\",\"name\":\"write\"}]}"; + // The sample model with the log store's tamper-evidence recorded. This retires a finding + // without changing the model's shape, so the comparison against SampleJson resolves and + // introduces nothing. + private const string SignedJson = + "{\"schema\":\"tmforge-json\",\"version\":\"0.1\"," + + "\"elements\":[" + + "{\"id\":\"s1\",\"kind\":\"datastore\",\"name\":\"Ledger\",\"x\":0,\"y\":0," + + "\"properties\":{\"StoresLogData\":\"Yes\",\"Signed\":\"Yes\"}}," + + "{\"id\":\"p1\",\"kind\":\"process\",\"name\":\"Checkout\",\"x\":200,\"y\":0}," + + "{\"id\":\"e1\",\"kind\":\"external\",\"name\":\"Customer\",\"x\":400,\"y\":0}]," + + "\"flows\":[" + + "{\"id\":\"f1\",\"source\":\"e1\",\"target\":\"p1\",\"name\":\"order\"}," + + "{\"id\":\"f2\",\"source\":\"p1\",\"target\":\"s1\",\"name\":\"write\"}]}"; + /// Gets or sets the working directory for one test. private string WorkingDirectory { get; set; } = string.Empty; @@ -181,6 +195,87 @@ public void UnreadableTaxonomyIsAnError() Assert.AreEqual(1, exit); } + /// + /// Findings introduced between two analyses fail the command, so a gate can depend on "no new + /// findings" without having to compare counts itself. + /// + [TestMethod] + public void DiffReportsIntroducedFindingsAndFails() + { + string basePath = this.WriteAnalysisAs("base", EditedJson); + string headPath = this.WriteAnalysisAs("head", SampleJson); + + (int exit, string stdout) = Run(new[] { "diff", basePath, headPath }); + + Assert.AreEqual(2, exit); + StringAssert.Contains(stdout, "Introduced:"); + } + + /// + /// Recording the control a finding asked for resolves it and introduces nothing, so the + /// comparison succeeds. Resolving findings is not something a gate should fail on. + /// + [TestMethod] + public void DiffReportsResolvedFindingsAndSucceeds() + { + string basePath = this.WriteAnalysisAs("base", SampleJson); + string headPath = this.WriteAnalysisAs("head", SignedJson); + + (int exit, string stdout) = Run(new[] { "diff", basePath, headPath }); + + Assert.AreEqual(0, exit, stdout); + StringAssert.Contains(stdout, "Resolved:"); + } + + /// A document compared against itself reports no change. + [TestMethod] + public void DiffOfADocumentAgainstItselfIsEmpty() + { + string document = this.WriteAnalysisAs("base", SampleJson); + + (int exit, string stdout) = Run(new[] { "diff", document, document }); + + Assert.AreEqual(0, exit); + StringAssert.Contains(stdout, "No change in findings"); + } + + /// + /// The JSON envelope carries the identities, not just the counts, so a caller can report which + /// findings arrived rather than re-deriving them. + /// + [TestMethod] + public void DiffJsonCarriesTheIntroducedIdentities() + { + string basePath = this.WriteAnalysisAs("base", EditedJson); + string headPath = this.WriteAnalysisAs("head", SampleJson); + + (int exit, string stdout) = Run(new[] { "diff", basePath, headPath, "--json" }); + + Assert.AreEqual(2, exit); + JsonElement data = JsonDocument.Parse(stdout).RootElement.GetProperty("data"); + Assert.AreEqual("diff", data.GetProperty("operation").GetString()); + + JsonElement introduced = data.GetProperty("introduced"); + Assert.IsTrue(introduced.GetArrayLength() > 0); + Assert.AreEqual( + introduced.GetArrayLength(), + data.GetProperty("summary").GetProperty("introduced").GetInt32()); + Assert.IsFalse( + string.IsNullOrEmpty(Identity(introduced.EnumerateArray().First())), + "every reported finding carries the identity a caller reconciles on"); + } + + /// A document that cannot be read is a tool error, not an empty comparison. + [TestMethod] + public void DiffAgainstAMissingDocumentIsAToolError() + { + string document = this.WriteAnalysisAs("base", SampleJson); + + (int exit, _) = Run(new[] { "diff", document, Path.Join(this.WorkingDirectory, "absent.json") }); + + Assert.AreEqual(1, exit); + } + private static string Identity(JsonElement finding) => finding.GetProperty("id").GetString() ?? string.Empty; private static JsonElement ReadDocument(string path) @@ -226,5 +321,25 @@ private string WriteAnalysis(string json) RunAnalyze(new[] { model, "--reportFolder", reports }); return Path.Join(reports, "model.analysis.json"); } + + /// + /// Writes one analysis into its own subtree, so two of them can be compared. The model file + /// keeps the same name in both so the documents describe the same logical model and the + /// comparison is not warned about as a cross-model one. + /// + /// A name for the revision, used as the subfolder. + /// The canonical model to analyze. + /// The path of the written analysis document. + private string WriteAnalysisAs(string revision, string json) + { + string folder = Path.Join(this.WorkingDirectory, revision); + Directory.CreateDirectory(folder); + string model = Path.Join(folder, "model.json"); + File.WriteAllText(model, json); + + string reports = Path.Join(folder, "reports"); + RunAnalyze(new[] { model, "--reportFolder", reports }); + return Path.Join(reports, "model.analysis.json"); + } } } diff --git a/test/ThreatModelForge.Cli.Tests/DiffCommandTest.cs b/test/ThreatModelForge.Cli.Tests/DiffCommandTest.cs index 4427c54..3107f38 100644 --- a/test/ThreatModelForge.Cli.Tests/DiffCommandTest.cs +++ b/test/ThreatModelForge.Cli.Tests/DiffCommandTest.cs @@ -145,6 +145,48 @@ public void TextconvIsStableAcrossReserialization() Assert.AreEqual(original, reserialized); } + /// + /// Verifies that moving a flow out of a trust boundary is reported, even though it changes no + /// stored property and so produces no structural difference at all. Without this the command + /// would print "No differences." for a change of exposure. + /// + [TestMethod] + public void MovingAFlowOutOfABoundaryIsReportedDespiteNoStructuralChange() + { + string basePath = this.Write("base.tm7", BuildWithBoundary(crossing: true)); + string revisedPath = this.Write("revised.tm7", BuildWithBoundary(crossing: false)); + + (int exit, string output) = Capture(new[] { basePath, revisedPath }); + + Assert.AreEqual(0, exit); + StringAssert.Contains(output, "Boundary crossings:"); + StringAssert.Contains(output, "no longer crosses \"Perimeter\""); + Assert.IsFalse(output.Contains("No differences."), output); + } + + /// + /// Verifies that the JSON envelope carries the crossing changes and counts them in the summary. + /// + [TestMethod] + public void CrossingChangesAppearInTheJsonEnvelope() + { + string basePath = this.Write("base.tm7", BuildWithBoundary(crossing: false)); + string revisedPath = this.Write("revised.tm7", BuildWithBoundary(crossing: true)); + + (int exit, string output) = Capture(new[] { basePath, revisedPath, "--json" }); + + Assert.AreEqual(0, exit); + using JsonDocument document = JsonDocument.Parse(output); + JsonElement data = document.RootElement.GetProperty("data"); + Assert.AreEqual(1, data.GetProperty("summary").GetProperty("crossingChanges").GetInt32()); + + JsonElement crossing = data.GetProperty("crossings").EnumerateArray().Single(); + Assert.AreEqual("modified", crossing.GetProperty("kind").GetString()); + Assert.AreEqual( + "Perimeter", + crossing.GetProperty("added").EnumerateArray().Single().GetProperty("name").GetString()); + } + private static (int Exit, string Output) Capture(string[] args) { using StringWriter writer = new StringWriter(); @@ -192,6 +234,54 @@ private static ThreatModel BuildBase(out Guid process) return model; } + /// + /// Builds a model with a trust boundary and one flow, where only the flow's source coordinate + /// differs between the two variants. Every stored property is identical either way, so the two + /// models differ solely in what the flow crosses. + /// + /// Whether the flow should start inside the boundary and so cross it. + /// The model. + private static ThreatModel BuildWithBoundary(bool crossing) + { + Guid boundary = new Guid("44444444-4444-4444-4444-444444444444"); + Guid flow = new Guid("55555555-5555-5555-5555-555555555555"); + + BorderBoundary boundaryElement = new BorderBoundary + { + Guid = boundary, + GenericTypeId = "GE.TB.B", + TypeId = "GE.TB.B", + Left = 50, + Top = 50, + Width = 200, + Height = 200, + }; + DiagramElementHelper.SetName(boundaryElement, "Perimeter"); + + Connector flowElement = new Connector + { + Guid = flow, + TypeId = "GE.DF", + SourceX = crossing ? 60 : 10, + SourceY = crossing ? 60 : 10, + TargetX = 500, + TargetY = 500, + }; + DiagramElementHelper.SetName(flowElement, "query"); + + DrawingSurfaceModel surface = new DrawingSurfaceModel + { + Guid = new Guid("66666666-6666-6666-6666-666666666666"), + Header = "Main", + }; + surface.Borders[boundary] = boundaryElement; + surface.Lines[flow] = flowElement; + + ThreatModel model = new ThreatModel(); + model.DrawingSurfaceList.Add(surface); + return model; + } + private string Write(string name, ThreatModel model) { string path = Path.Join(this.WorkingDirectory, name); diff --git a/test/ThreatModelForge.Cli.Tests/SetCommandTest.cs b/test/ThreatModelForge.Cli.Tests/SetCommandTest.cs index b933dff..d6e68d0 100644 --- a/test/ThreatModelForge.Cli.Tests/SetCommandTest.cs +++ b/test/ThreatModelForge.Cli.Tests/SetCommandTest.cs @@ -3,10 +3,12 @@ namespace ThreatModelForge.Cli.Tests using System; using System.Collections.Generic; using System.IO; + using System.Linq; using System.Text.Json; using Microsoft.VisualStudio.TestTools.UnitTesting; using ThreatModelForge.Editing; using ThreatModelForge.Engine; + using ThreatModelForge.KnowledgeBase; using ThreatModelForge.Model; using ThreatModelForge.Model.Abstracts; @@ -130,6 +132,54 @@ public void SetNothingToSetReturnsError() Assert.AreEqual(1, exit); } + /// + /// The regression this guards: saving a .tm7 converts a schema-backed property into the + /// tool's typed representation, and a later set has to move that typed value. Writing a + /// custom attribute beside it instead reports success, changes nothing a reader will see, and + /// leaves the element carrying one more property for every edit. + /// + [TestMethod] + public void SetOverwritesAPropertyTheFileHasAlreadyTyped() + { + string path = this.NewModel(); + string id = this.AddElement("store", "Ledger"); + Guid element = Guid.Parse(id); + + Capture(() => SetCommand.Run(new[] { path, "--id", id, "--property", "Encrypted=At-rest" })); + Assert.AreEqual("At-rest", this.LoadProperties(element)["Encrypted"]); + + (int exit, _) = Capture(() => SetCommand.Run(new[] { path, "--id", id, "--property", "Encrypted=No" })); + + Assert.AreEqual(0, exit); + Assert.AreEqual("No", this.LoadProperties(element)["Encrypted"]); + Assert.AreEqual( + 1, + this.TypedPropertyCount(element, "Encrypted"), + "the element should carry one Encrypted property, not one per edit"); + } + + /// + /// Verifies that repeated edits keep converging on a single property. A duplicate is not only + /// a stale read: the tool refuses a model that carries two properties with the same identity. + /// + [TestMethod] + public void RepeatedSetsDoNotAccumulateProperties() + { + string path = this.NewModel(); + string id = this.AddElement("store", "Ledger"); + Guid element = Guid.Parse(id); + + foreach (string value in new[] { "At-rest", "No", "TDE", "Platform" }) + { + Assert.AreEqual( + 0, + Capture(() => SetCommand.Run(new[] { path, "--id", id, "--property", "Encrypted=" + value })).Exit); + } + + Assert.AreEqual("Platform", this.LoadProperties(element)["Encrypted"]); + Assert.AreEqual(1, this.TypedPropertyCount(element, "Encrypted")); + } + private static (int Exit, string Stdout) Capture(Func run) { using StringWriter outWriter = new StringWriter(); @@ -179,5 +229,22 @@ private IReadOnlyDictionary LoadProperties(Guid id) Assert.IsNotNull(element); return DiagramElementHelper.GetCustomProperties(element); } + + /// + /// Counts how many typed (list) properties the saved model records under one display name. + /// + /// The element id. + /// The property's display name. + /// The number of typed properties carrying that name. + private int TypedPropertyCount(Guid id, string name) + { + (ThreatModel model, _) = CliModelLoader.Load(this.ModelPath); + DrawingSurfaceModel? diagram = AuthoringSupport.FirstDiagram(model); + Assert.IsNotNull(diagram); + Entity? element = DiagramEditor.FindElement(diagram, id); + Assert.IsNotNull(element); + return element!.Properties.OfType() + .Count(list => string.Equals(list.DisplayName, name, StringComparison.OrdinalIgnoreCase)); + } } } diff --git a/test/ThreatModelForge.Core.Tests/Fixtures/MtmtOpenability/README.md b/test/ThreatModelForge.Core.Tests/Fixtures/MtmtOpenability/README.md new file mode 100644 index 0000000..766bd5c --- /dev/null +++ b/test/ThreatModelForge.Core.Tests/Fixtures/MtmtOpenability/README.md @@ -0,0 +1,79 @@ +# MTMT openability probe + +`Tm7MtmtCompatibilityTests` locks the on-disk shape a `.tm7` must have for the Microsoft Threat +Modeling Tool to open it. That is the best proxy CI can run, and it is only a proxy: the assertions +encode what we currently *believe* the tool requires. This script asks the tool. + +No Microsoft binaries are stored in this directory. + +## What it answers + +For every `.tm7` in a directory, it constructs MTMT's own `ObjectModel(StorageFile, bool)` and reads +`ModelLoadHasIssues` / `ModelLoadIssues` — the properties `DashboardViewModel` inspects before it +raises the "coordinates are corrupted" dialog. A model whose construction throws is a hard refusal: +the tool will not open it at all. + +Given `-ElementId` and `-PropertyName` it also reads one property back through MTMT's `PropertyMap`, +which answers the stronger question: does the tool see the value tmforge wrote? + +Exit code is `0` when every model opens and `1` when any does not, so it can gate a release check. + +## Getting a payload + +The probe does not download anything. Reuse the verified download in the sibling +[`MtmtDifferential`](../../../ThreatModelForge.Analysis.Tests/Fixtures/MtmtDifferential/README.md) +capture script, which fetches MTMT from Microsoft's versioned ClickOnce distribution and checks the +application manifest SHA-256 and every payload digest against it: + +```powershell +powershell.exe -NoProfile -ExecutionPolicy Bypass -File ` + test/ThreatModelForge.Analysis.Tests/Fixtures/MtmtDifferential/capture-mtmt-root-scope.ps1 ` + -Preflight -WorkingDirectory C:\mtmt +``` + +That leaves a verified payload at `C:\mtmt\TMT7_7_3_51110_1`. `-Preflight` stops after verifying, so +it needs neither WPF nor the 32-bit host. + +## Running it + +```powershell +powershell.exe -NoProfile -ExecutionPolicy Bypass -File ` + test/ThreatModelForge.Core.Tests/Fixtures/MtmtOpenability/probe-mtmt-openability.ps1 ` + -PayloadDirectory C:\mtmt\TMT7_7_3_51110_1 ` + -ModelDirectory C:\models ` + -ElementId c5adf20f-68d4-5971-a9d3-76a250ef77f0 -PropertyName Encrypted +``` + +Windows only, with .NET Framework 4.8. The script relaunches itself under 32-bit Windows PowerShell +in STA mode because the signed MTMT model assembly is x86 and derives its types from WPF. + +Include controls in both directions, so a run that reports everything refused is distinguishable +from a broken probe and a run that reports everything fine is distinguishable from a probe that +stopped checking: + +- **Positive:** `examples/webshop.tm7` is verified to open. +- **Negative:** `test/ThreatModelForge.Core.Tests/SampleModel.tm7` is verified *not* to open. It + serializes ``, which MTMT refuses outright with + `SerializationException: ValueType 'ThreatModeling.Common.StencilConnectionPort' cannot be null`. + It is a hand-built round-trip fixture, not an openability oracle — a tmforge round trip repairs the + ports and the result opens clean. + +## Facts worth keeping + +Established against MTMT `7.3.51110.1`; re-check when that version moves. + +- **`ObjectModel(String, Boolean, Boolean, Boolean)` overflows the stack** and kills the host process, + destroying buffered output along with it. Use the `(StorageFile, bool)` overload. +- **Elements have no `Properties` member.** Use `PropertyMap`, a `Dictionary` keyed by attribute id + whose values are `ListDisplayAttribute`, `StringDisplayAttribute`, `BooleanDisplayAttribute`, + `CustomStringDisplayAttribute`, or `HeaderDisplayAttribute`. +- **`ListDisplayAttribute.Value` is a `List` typed as `Object`.** Neither `-is [string[]]` nor + `GetType().IsArray` identifies it; both fall through silently and print the whole option list + instead of the selection. Materialize the enumerable and index it by `SelectedIndex`. +- **`GetDrawingSurfaceModels()` returns nothing until the model is processed.** Call + `ProcessModelImmediately(FullModel)`; `ConfigureThreatGeneration` needs a WPF dispatcher a + console host does not have. +- **Duplicate element properties do not stop a model opening.** A file carrying five attributes with + the same display name loads clean, and `PropertyMap` reports one entry. The hazard is not + refusal — it is that the tool and tmforge can resolve the same file to *different values*, which is + how the `set` defect fixed in `DiagramElementHelper` was found. diff --git a/test/ThreatModelForge.Core.Tests/Fixtures/MtmtOpenability/probe-mtmt-openability.ps1 b/test/ThreatModelForge.Core.Tests/Fixtures/MtmtOpenability/probe-mtmt-openability.ps1 new file mode 100644 index 0000000..6a2c760 --- /dev/null +++ b/test/ThreatModelForge.Core.Tests/Fixtures/MtmtOpenability/probe-mtmt-openability.ps1 @@ -0,0 +1,262 @@ +#Requires -Version 5 +<# +.SYNOPSIS +Reports whether the Microsoft Threat Modeling Tool can open each .tm7 in a directory. + +.DESCRIPTION +Loads every model through MTMT's own ObjectModel and reads ModelLoadHasIssues / ModelLoadIssues, +which is what DashboardViewModel inspects before it raises the "coordinates are corrupted" dialog. +A model whose construction throws is a hard refusal: the tool will not open it at all. + +Optionally reads one element property back through MTMT's own PropertyMap, so the answer can be +"the tool sees the value tmforge wrote" rather than only "it opened". + +MTMT 7.3.51110.1 is x86 and derives its model types from WPF, so this relaunches itself under 32-bit +Windows PowerShell in STA mode. + +.PARAMETER PayloadDirectory +A verified MTMT payload directory. See the README for how to obtain one. + +.PARAMETER ModelDirectory +A directory of .tm7 files to probe. + +.PARAMETER OutputPath +Where to write the JSON result. Defaults to openability.json beside the models. + +.PARAMETER ElementId +Optional. The stable id of an element whose property should be read back. + +.PARAMETER PropertyName +Optional. The display name of the property to read back, for example Encrypted. + +.OUTPUTS +JSON on stdout and at OutputPath. Exit code 0 when every model opens, 1 when any does not, so the +script can gate a release check. +#> +[CmdletBinding()] +param( + [Parameter(Mandatory = $true)][string]$PayloadDirectory, + [Parameter(Mandatory = $true)][string]$ModelDirectory, + [string]$OutputPath, + [string]$ElementId, + [string]$PropertyName +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +if ([string]::IsNullOrWhiteSpace($OutputPath)) { + $OutputPath = Join-Path $ModelDirectory 'openability.json' +} + +$PayloadDirectory = [IO.Path]::GetFullPath($PayloadDirectory) +$ModelDirectory = [IO.Path]::GetFullPath($ModelDirectory) +$OutputPath = [IO.Path]::GetFullPath($OutputPath) + +$isSta = [Threading.Thread]::CurrentThread.ApartmentState -eq [Threading.ApartmentState]::STA +if ([IntPtr]::Size -ne 4 -or -not $isSta) { + $x86PowerShell = Join-Path $env:WINDIR 'SysWOW64\WindowsPowerShell\v1.0\powershell.exe' + if (-not (Test-Path -LiteralPath $x86PowerShell)) { + throw '32-bit Windows PowerShell is required because MTMT 7.3.51110.1 is x86.' + } + + $arguments = @( + '-NoProfile', '-Sta', '-ExecutionPolicy', 'Bypass', '-File', $PSCommandPath, + '-PayloadDirectory', $PayloadDirectory, '-ModelDirectory', $ModelDirectory, + '-OutputPath', $OutputPath) + if (-not [string]::IsNullOrWhiteSpace($ElementId)) { $arguments += @('-ElementId', $ElementId) } + if (-not [string]::IsNullOrWhiteSpace($PropertyName)) { $arguments += @('-PropertyName', $PropertyName) } + + & $x86PowerShell @arguments + exit $LASTEXITCODE +} + +function Format-DisplayAttribute { + <# + Renders one MTMT property value. + + A ListDisplayAttribute holds its options in Value as a List typed as Object, so neither + '-is [string[]]' nor GetType().IsArray identifies it; both silently fall through and print the + whole option list instead of the selection. Materialize any non-string enumerable and index it. + #> + param($Attribute) + + $raw = $Attribute.Value + if ($null -eq $raw) { + return '' + } + + if ($raw -is [string] -or -not ($raw -is [System.Collections.IEnumerable])) { + return [string]$raw + } + + $options = @($raw) + $index = -1 + try { $index = [int]$Attribute.SelectedIndex } catch { } + + if ($index -lt 0 -or $index -ge $options.Count) { + return "" + } + + return [string]$options[$index] +} + +function Get-EntityProperty { + <# + Reads one property off one element as MTMT itself resolves it. + + MTMT exposes PropertyMap (a Dictionary keyed by attribute id) rather than the raw attribute list, + which is the interesting part: a dictionary cannot hold two entries for one key, so a file that + carries duplicate attributes still yields a single answer here - and it need not be the answer + tmforge's own reader produces. + + Entirely best-effort. This is supporting evidence for the load verdict, never the verdict itself, + so every failure is reported as a string rather than thrown. + #> + param($Model, $ProcessingModeType, [string]$ElementId, [string]$PropertyName) + + try { + # GetDrawingSurfaceModels() returns nothing until the model has been processed, and + # ConfigureThreatGeneration needs a WPF dispatcher this host does not have. + [void]$Model.ProcessModelImmediately([Enum]::Parse($ProcessingModeType, 'FullModel')) + + foreach ($surface in $Model.GetDrawingSurfaceModels()) { + foreach ($collection in @($surface.Borders, $surface.Lines)) { + foreach ($entity in $collection.Values) { + if ($null -eq $entity) { continue } + + $guid = '' + try { $guid = [string]$entity.Guid } catch { continue } + if ($guid -ne $ElementId) { continue } + + $map = $entity.PropertyMap + if ($null -eq $map) { return '' } + + $seen = @() + foreach ($key in @($map.Keys)) { + $attribute = $map[$key] + if ($null -eq $attribute) { continue } + + $display = '' + try { $display = [string]$attribute.DisplayName } catch { } + if ($display -ne $PropertyName) { continue } + + $seen += Format-DisplayAttribute -Attribute $attribute + } + + if ($seen.Count -eq 0) { return '' } + if ($seen.Count -eq 1) { return $seen[0] } + return ($seen -join ' | ') + " (the tool kept $($seen.Count) entries)" + } + } + } + + return '' + } + catch { + return '' + } +} + +$assemblies = @{} +foreach ($dll in Get-ChildItem -LiteralPath $PayloadDirectory -Filter '*.dll' -Recurse) { + $assemblies[[IO.Path]::GetFileNameWithoutExtension($dll.Name)] = $dll.FullName +} + +foreach ($required in @('ThreatModeling.ExternalStorage.Abstracts', 'ThreatModeling.ExternalStorage.Local', 'ThreatModeling.Model')) { + if (-not $assemblies.ContainsKey($required)) { + throw "The payload directory is missing '$required.dll': '$PayloadDirectory'." + } +} + +$resolver = [ResolveEventHandler] { + param($sender, $eventArgs) + + $name = (New-Object Reflection.AssemblyName($eventArgs.Name)).Name + if ($assemblies.ContainsKey($name)) { + return [Reflection.Assembly]::LoadFrom($assemblies[$name]) + } + + return $null +}.GetNewClosure() + +[AppDomain]::CurrentDomain.add_AssemblyResolve($resolver) +try { + $abstracts = [Reflection.Assembly]::LoadFrom($assemblies['ThreatModeling.ExternalStorage.Abstracts']) + $local = [Reflection.Assembly]::LoadFrom($assemblies['ThreatModeling.ExternalStorage.Local']) + $modelAssembly = [Reflection.Assembly]::LoadFrom($assemblies['ThreatModeling.Model']) + + $storageFileType = $abstracts.GetType('ThreatModeling.ExternalStorage.Abstracts.StorageFile', $true) + $localFileType = $local.GetType('ThreatModeling.ExternalStorage.Local.LocalFile', $true) + $objectModelType = $modelAssembly.GetType('ThreatModeling.Model.ObjectModel', $true) + $processingModeType = $modelAssembly.GetType('ThreatModeling.Model.ModelProcessingMode', $true) + + # Bind the constructors explicitly: PowerShell's params binding would splat a single-element + # array. ObjectModel(String, Boolean, Boolean, Boolean) overflows the stack and kills the host + # process, taking buffered output with it - do not use it. + $localFileCtor = $localFileType.GetConstructor([Type[]]@([string])) + $objectModelCtor = $objectModelType.GetConstructor([Type[]]@($storageFileType, [bool])) + if ($null -eq $localFileCtor -or $null -eq $objectModelCtor) { + throw 'MTMT no longer exposes LocalFile(string) or ObjectModel(StorageFile, bool).' + } + + $models = @(Get-ChildItem -LiteralPath $ModelDirectory -Filter '*.tm7' | Sort-Object Name) + if ($models.Count -eq 0) { + throw "No .tm7 files were found in '$ModelDirectory'." + } + + $results = @() + foreach ($file in $models) { + $record = [ordered]@{ + file = $file.Name + opens = $false + hasLoadIssues = $null + loadIssues = @() + error = $null + observedProperty = $null + } + + try { + $localFile = $localFileCtor.Invoke([object[]]@([string]$file.FullName)) + $model = $objectModelCtor.Invoke([object[]]@($localFile, $false)) + + $hasIssues = [bool]$model.ModelLoadHasIssues + $record.hasLoadIssues = $hasIssues + $record.opens = -not $hasIssues + if ($hasIssues -and $null -ne $model.ModelLoadIssues) { + $record.loadIssues = @($model.ModelLoadIssues | ForEach-Object { [string]$_ }) + } + + if (-not [string]::IsNullOrWhiteSpace($ElementId) -and -not [string]::IsNullOrWhiteSpace($PropertyName)) { + $record.observedProperty = Get-EntityProperty -Model $model -ProcessingModeType $processingModeType ` + -ElementId $ElementId -PropertyName $PropertyName + } + } + catch { + $base = $_.Exception.GetBaseException() + $record.error = $base.GetType().FullName + ': ' + $base.Message + } + + $results += [pscustomobject]$record + } + + $refused = @($results | Where-Object { -not $_.opens }) + $payload = [pscustomobject][ordered]@{ + mtmtPayloadDirectory = $PayloadDirectory + modelDirectory = $ModelDirectory + probed = $results.Count + refused = $refused.Count + models = $results + } + + $json = $payload | ConvertTo-Json -Depth 6 + [IO.File]::WriteAllText($OutputPath, $json) + Write-Output $json + + if ($refused.Count -gt 0) { + exit 1 + } +} +finally { + [AppDomain]::CurrentDomain.remove_AssemblyResolve($resolver) +} diff --git a/test/ThreatModelForge.Editing.Tests/DiagramElementHelperTest.cs b/test/ThreatModelForge.Editing.Tests/DiagramElementHelperTest.cs index 5e01c1f..811dced 100644 --- a/test/ThreatModelForge.Editing.Tests/DiagramElementHelperTest.cs +++ b/test/ThreatModelForge.Editing.Tests/DiagramElementHelperTest.cs @@ -1,6 +1,7 @@ namespace ThreatModelForge.Editing.Tests { using System; + using System.Linq; using Microsoft.VisualStudio.TestTools.UnitTesting; using ThreatModelForge.Editing; using ThreatModelForge.KnowledgeBase; @@ -102,5 +103,83 @@ public void GetCustomPropertiesPrefersCustomOverTypedForSameKey() Assert.AreEqual("TLS", DiagramElementHelper.GetCustomProperties(element)["Protocol"]); } + + /// + /// Verifies that setting a property the tool has already typed moves that typed selection + /// rather than adding a custom attribute beside it. Leaving both would give the element two + /// answers to the same question, and whichever the reader reached first would win. + /// + [TestMethod] + public void SetCustomPropertyMovesAnExistingTypedSelection() + { + StencilRectangle element = TypedElement(); + + DiagramElementHelper.SetCustomProperty(element, "Protocol", "HTTP"); + + Assert.AreEqual("HTTP", DiagramElementHelper.GetCustomProperties(element)["Protocol"]); + Assert.AreEqual(1, element.Properties.OfType().Count()); + Assert.AreEqual(0, element.Properties.OfType().Count()); + } + + /// + /// Verifies that a value the typed property does not offer is still stored, as a custom + /// attribute, so an authored value is preserved rather than silently dropped. + /// + [TestMethod] + public void SetCustomPropertyFallsBackToACustomAttributeForAnUnknownValue() + { + StencilRectangle element = TypedElement(); + + DiagramElementHelper.SetCustomProperty(element, "Protocol", "QUIC"); + + Assert.AreEqual("QUIC", DiagramElementHelper.GetCustomProperties(element)["Protocol"]); + Assert.AreEqual(1, element.Properties.OfType().Count()); + } + + /// + /// Verifies that moving a typed selection clears a custom attribute previously written for the + /// same key. A custom value outranks a typed one when read, so a stale one left behind would + /// override the value just set. + /// + [TestMethod] + public void SetCustomPropertyClearsAStaleCustomValueWhenTheTypedSelectionMoves() + { + StencilRectangle element = TypedElement(); + DiagramElementHelper.SetCustomProperty(element, "Protocol", "QUIC"); + + DiagramElementHelper.SetCustomProperty(element, "Protocol", "HTTP"); + + Assert.AreEqual("HTTP", DiagramElementHelper.GetCustomProperties(element)["Protocol"]); + Assert.AreEqual(0, element.Properties.OfType().Count()); + } + + /// + /// Verifies that a property with no typed counterpart still round-trips as a custom attribute. + /// + [TestMethod] + public void SetCustomPropertyStillUpdatesAnUntypedProperty() + { + StencilRectangle element = new StencilRectangle { Guid = Guid.NewGuid() }; + + DiagramElementHelper.SetCustomProperty(element, "Port", "443"); + DiagramElementHelper.SetCustomProperty(element, "Port", "8443"); + + Assert.AreEqual("8443", DiagramElementHelper.GetCustomProperties(element)["Port"]); + Assert.AreEqual(1, element.Properties.OfType().Count()); + } + + private static StencilRectangle TypedElement() + { + StencilRectangle element = new StencilRectangle { Guid = Guid.NewGuid() }; + element.Properties.Add(new ListDisplayAttribute + { + Name = "Protocol", + DisplayName = "Protocol", + Value = new[] { "Select", "HTTPS", "HTTP" }, + SelectedIndex = 1, + }); + + return element; + } } } diff --git a/test/ThreatModelForge.Editing.Tests/SchemaBackedPropertiesTest.cs b/test/ThreatModelForge.Editing.Tests/SchemaBackedPropertiesTest.cs index 88634fd..4895028 100644 --- a/test/ThreatModelForge.Editing.Tests/SchemaBackedPropertiesTest.cs +++ b/test/ThreatModelForge.Editing.Tests/SchemaBackedPropertiesTest.cs @@ -96,6 +96,36 @@ public void ApplyTypesBooleanPropertyAsYesNoList() Assert.AreEqual("Yes", options[typed.SelectedIndex]); } + /// + /// Verifies that typing a property replaces a typed value already recorded for it rather than + /// adding a second one. Readers take the first typed attribute they find, so a leftover would + /// keep the old value and the element would grow by one attribute per edit. + /// + [TestMethod] + public void ApplyReplacesAnAlreadyTypedPropertyInsteadOfAddingASecond() + { + (ThreatModel model, StencilParallelLines store) = ModelWithStore(); + + // The state a model arrives in after a previous export: the property is already typed, + // with an option list from whatever schema was current then. + store.Properties.Add(new ListDisplayAttribute + { + Name = "Encrypted", + DisplayName = "Encrypted", + Value = new[] { "Select", "No", "At-rest" }, + SelectedIndex = 2, + }); + store.Properties.Add(new CustomStringDisplayAttribute { Value = "Encrypted:TDE" }); + + SchemaBackedProperties.Apply(model, BuildKnowledgeBase("GE.DS")); + + ListDisplayAttribute typed = store.Properties.OfType() + .Single(p => p.DisplayName == "Encrypted"); + string[] options = (string[])typed.Value!; + Assert.AreEqual("TDE", options[typed.SelectedIndex]); + Assert.AreEqual("TDE", DiagramElementHelper.GetCustomProperties(store)["Encrypted"]); + } + /// /// Verifies that a free-text schema property is left as a custom attribute. ///