From d36d04a9769a9599bfe3d235109a58e755b4ec06 Mon Sep 17 00:00:00 2001 From: Rafid Aslam Date: Thu, 3 Sep 2026 13:08:32 +0700 Subject: [PATCH 1/3] feat: add plan diff information in the comment The comment said whether policies passed and never what was changing. To learn that a pull request creates one bucket, a reviewer had to leave the review and open the job log or the run. It now carries the planned changes and terraform's summary line, rendered from the *masked* plan document rather than from `terraform show` output. That is a security choice, not a convenience one: masking is the only thing keeping a sensitive value out of a public comment, and captured text would not carry it. no-op resources are counted, not listed. A plan against applied infrastructure carries one for every resource in state -- on the demo repository five of six rows -- and listing them buries the one that changed. Replacements are counted separately rather than folded into add and destroy, which is where terraform puts them. "1 to replace" is the number that should make a reviewer look twice, and it disappears when spread across two columns. Markers are +, - and ! only. terraform's ~ means nothing to the diff highlighting, so an update row would render plain and the fence would buy nothing. The block is dropped first when the comment is too long, ahead of any finding. A comment that keeps the diff and loses the violation has failed at its job. `action_summary` moves out of the TUI into a shared module so both surfaces answer the replacement question identically. --- src/tirith/plan_actions.py | 101 ++++++++++++++ src/tirith/platform/check.py | 4 + src/tirith/platform/report.py | 134 +++++++++++++++++- src/tirith/tui/results.py | 21 +-- tests/platform/test_report_plan_block.py | 166 +++++++++++++++++++++++ 5 files changed, 402 insertions(+), 24 deletions(-) create mode 100644 src/tirith/plan_actions.py create mode 100644 tests/platform/test_report_plan_block.py diff --git a/src/tirith/plan_actions.py b/src/tirith/plan_actions.py new file mode 100644 index 00000000..5412399a --- /dev/null +++ b/src/tirith/plan_actions.py @@ -0,0 +1,101 @@ +""" +Terraform plan actions, in vocabulary a human reads. + +Shared because two very different surfaces need the same answer: the TUI's result list and the +pull-request comment. Keeping one implementation is what stops them disagreeing about whether +`["delete", "create"]` is a replacement or two separate operations. +""" + +# What terraform calls the operation, keyed by the exact action tuple it reports. +# +# The replacement pair is ordered, and which way round it is changes the risk rather than the +# wording: delete-then-create has downtime, create-then-delete does not. Reporting both as +# "replace" would hide the difference a reviewer most needs to see. +_ACTION_NAMES = { + ("no-op",): "no change", + ("create",): "create", + ("delete",): "destroy", + ("update",): "update in place", + ("read",): "read", + ("delete", "create"): "replace (destroy first)", + ("create", "delete"): "replace (create first)", +} + +# Which markers a ```diff fence understands. GitHub highlights `+`, `-` and `!` and nothing else -- +# terraform's own `~` for an update renders as plain text, which defeats the point of the fence. So +# update and replace share `!` and the words carry the precision. +_CREATE = "+" +_DESTROY = "-" +_CHANGE = "!" + +_ACTION_MARKERS = { + ("create",): _CREATE, + ("delete",): _DESTROY, + ("update",): _CHANGE, + ("delete", "create"): _CHANGE, + ("create", "delete"): _CHANGE, +} + + +def action_summary(actions): + """ + The planned action, in terraform's own vocabulary. + + An unrecognised combination is joined rather than dropped: a tuple this module has not seen is + still worth showing verbatim, and inventing a friendly name for it would be a guess. + """ + actions = tuple(a for a in actions or () if a) + if not actions: + return "" + return _ACTION_NAMES.get(actions) or ", ".join(actions) + + +def action_marker(actions): + """The diff marker for an action tuple, or "" for no-op and anything unrecognised.""" + actions = tuple(a for a in actions or () if a) + return _ACTION_MARKERS.get(actions, "") + + +def is_no_op(actions): + return tuple(a for a in actions or () if a) == ("no-op",) + + +def plan_counts(resource_changes): + """ + Count a plan the way its summary line reports it. + + Replacements are counted on their own rather than folded into add + destroy. Terraform reports + them inside those two, so this deliberately differs: "2 to replace" is the number that should + make a reviewer look twice, and it disappears when it is spread across the other columns. + """ + counts = {"add": 0, "change": 0, "destroy": 0, "replace": 0, "no_op": 0} + for change in resource_changes or []: + actions = tuple(a for a in ((change.get("change") or {}).get("actions") or []) if a) + if actions in (("delete", "create"), ("create", "delete")): + counts["replace"] += 1 + elif actions == ("create",): + counts["add"] += 1 + elif actions == ("update",): + counts["change"] += 1 + elif actions == ("delete",): + counts["destroy"] += 1 + elif actions == ("no-op",): + counts["no_op"] += 1 + return counts + + +def summary_line(counts): + """ + The one line a reviewer looks for. + + `replace` is only mentioned when there is one, so the common case reads exactly like terraform's + own summary and the unusual case is conspicuous. + """ + parts = [ + f"{counts.get('add', 0)} to add", + f"{counts.get('change', 0)} to change", + f"{counts.get('destroy', 0)} to destroy", + ] + if counts.get("replace"): + parts.append(f"{counts['replace']} to replace") + return "Plan: " + ", ".join(parts) + "." diff --git a/src/tirith/platform/check.py b/src/tirith/platform/check.py index 643bf627..bb27c6d4 100644 --- a/src/tirith/platform/check.py +++ b/src/tirith/platform/check.py @@ -707,6 +707,10 @@ def run_check(opts): limit=opts.markdown_limit, cost_breakdown=cost_breakdown, commit=opts.sha, + # The masked document, never the raw one -- see report.render_plan_block. + plan=plan, + source_dir=opts.source_dir, + workflow_id=opts.workflow_id, ) try: with open(opts.output_markdown, "w") as f: diff --git a/src/tirith/platform/report.py b/src/tirith/platform/report.py index ffb15cd5..1862634f 100644 --- a/src/tirith/platform/report.py +++ b/src/tirith/platform/report.py @@ -7,6 +7,8 @@ import html +from .. import plan_actions + FAIL = "FAIL" WARN = "WARN" PASS = "PASS" @@ -24,6 +26,98 @@ _ICONS = {FAIL: "❌", WARN: "⚠️", APPROVAL_REQUIRED: "⏳", PASS: "✅", UNKNOWN: "❓"} +# How many changed resources render inline before the block is collapsed. Small changes should be +# readable without a click; large ones must not push the findings off the screen. +PLAN_INLINE_LIMIT = 12 + +# A hard cap on rows, independent of the comment limit. A thousand-resource plan would otherwise +# consume the whole budget and take the findings down with it during truncation. +PLAN_ROW_LIMIT = 50 + + +def _fence_safe(value): + """ + Make a plan-derived string safe to place inside a ``` fence. + + The same class of bug `_code` exists for, one layer out. A resource address comes from the plan, + and the plan comes from terraform a pull-request author controls -- `for_each` keys make + `aws_s3_bucket.demo["```"]` a legal address. Inside a fence a triple backtick does not merely + close an inline span, it closes the whole block, and everything after it renders as markdown: a + forged "all policies passed" banner, a stray hiding the real findings, a link whose + text and href disagree. + + Backticks are removed rather than escaped, because there is no escape that works inside a fence. + Newlines would fabricate extra rows, so they go too. + """ + text = str(value) + text = text.replace("`", "").replace("\r", " ").replace("\n", " ") + if len(text) > 200: + text = text[:197] + "..." + return text + + +def render_plan_block(plan): + """ + The planned changes, as a diff-fenced list plus terraform's summary line. + + Rendered from the *masked* plan document, never from `terraform show` output. The masking is the + only thing keeping a sensitive value out of a public pull-request comment, and text captured from + terraform would not carry it. See redact.redact_plan. + + `no-op` resources are counted, not listed. A plan against applied infrastructure carries one for + every resource in state, and listing them buries the handful that changed. + """ + if not isinstance(plan, dict): + return [] + + changes = plan.get("resource_changes") + if not isinstance(changes, list) or not changes: + return [] + + counts = plan_actions.plan_counts(changes) + + rows = [] + for change in changes: + if not isinstance(change, dict): + continue + actions = (change.get("change") or {}).get("actions") or [] + if plan_actions.is_no_op(actions): + continue + marker = plan_actions.action_marker(actions) + if not marker: + continue + address = _fence_safe(change.get("address") or change.get("type") or "") + if not address: + continue + rows.append(f"{marker} {address:<48} {_fence_safe(plan_actions.action_summary(actions))}".rstrip()) + + dropped = 0 + if len(rows) > PLAN_ROW_LIMIT: + dropped = len(rows) - PLAN_ROW_LIMIT + rows = rows[:PLAN_ROW_LIMIT] + + summary = plan_actions.summary_line(counts) + if counts.get("no_op"): + summary += f" {counts['no_op']} unchanged" + + if not rows: + # Nothing is changing, so there is no list to show -- but the line saying so is still worth + # having, otherwise the comment looks like it simply forgot to mention the plan. + return [summary, ""] + + fence = ["```diff"] + rows + (["", f"… and {dropped} more"] if dropped else []) + ["```"] + + if len(rows) > PLAN_INLINE_LIMIT: + block = [ + f"
Show plan — {len(rows)} changed resources", + "", + ] + fence + ["", "
"] + else: + block = fence + + return block + ["", summary, ""] + + def summarize(policy_results): """ @@ -256,7 +350,16 @@ def render_cost(breakdown): def render_markdown( - policy_results, run_status, run_url, marker=None, limit=COMMENT_LIMIT, cost_breakdown=None, commit=None + policy_results, + run_status, + run_url, + marker=None, + limit=COMMENT_LIMIT, + cost_breakdown=None, + commit=None, + plan=None, + source_dir=None, + workflow_id=None, ): """ Render the results as markdown, truncating detail before the summary table. @@ -277,8 +380,17 @@ def render_markdown( f"## 🛡️ {headline(counts, verdict_value)}", "", ] - if commit: - header += [f"Scanned commit {_html(_short_commit(commit))}", ""] + if commit or source_dir or workflow_id: + # One line of provenance. `dir` and the workflow matter when a matrix posts several comments + # on one pull request: today only comment-tag distinguishes them, and that is invisible. + bits = [] + if commit: + bits.append(f"Scanned commit {_html(_short_commit(commit))}") + if source_dir: + bits.append(f"dir {_html(source_dir)}") + if workflow_id: + bits.append(f"workflow {_html(workflow_id)}") + header += [f"{' · '.join(bits)}", ""] if verdict_value == "errored": # Two different reasons land here, and saying the wrong one is worse than saying nothing: @@ -306,18 +418,28 @@ def render_markdown( detail_sections = [_render_detail(f) for f in findings if f["result"] in (FAIL, APPROVAL_REQUIRED, WARN, UNKNOWN)] - body = "\n".join(header + table + detail_sections + footer) + plan_block = render_plan_block(plan) + + body = "\n".join(header + plan_block + table + detail_sections + footer) if len(body) <= limit: return body - # Drop detail sections from the end until it fits, keeping the summary table intact -- the + # The plan goes first, before any finding is touched. It is context; the findings are the point, + # and a comment that keeps the diff while dropping the violation has failed at its job. + if plan_block: + plan_block = [] + body = "\n".join(header + plan_block + table + detail_sections + footer) + if len(body) <= limit: + return body + + # Then drop detail sections from the end until it fits, keeping the summary table intact -- the # table is the part a reviewer scans first. kept = list(detail_sections) while kept and len(body) > limit: kept.pop() omitted = len(detail_sections) - len(kept) note = [f"", f"_… and {omitted} more finding(s). See the full run in StackGuardian._", ""] - body = "\n".join(header + table + kept + note + footer) + body = "\n".join(header + plan_block + table + kept + note + footer) if len(body) > limit: # Even the table is too large; truncate hard rather than risk a 422. diff --git a/src/tirith/tui/results.py b/src/tirith/tui/results.py index 2b6cc0e0..ee753869 100644 --- a/src/tirith/tui/results.py +++ b/src/tirith/tui/results.py @@ -17,6 +17,8 @@ from typing import Any, Dict, Iterator, List, NamedTuple, Optional +from .. import plan_actions + # The tri-state a check reports. `None` means skipped, and skipped is not a pass -- the engine # is careful about this distinction (see the --fail-on-error commentary in cli.py) and so is # every count here. @@ -55,24 +57,7 @@ def is_empty(self) -> bool: @property def action_summary(self) -> str: """The planned action, in terraform's own vocabulary.""" - actions = [a for a in self.actions if a] - if not actions: - return "" - if actions == ["no-op"]: - return "no change" - if actions == ["create"]: - return "create" - if actions == ["delete"]: - return "destroy" - if actions == ["update"]: - return "update in place" - # terraform expresses a replacement as an ordered pair, and which way round it is - # changes the risk: delete-then-create has downtime, create-then-delete does not. - if actions == ["delete", "create"]: - return "replace (destroy first)" - if actions == ["create", "delete"]: - return "replace (create first)" - return ", ".join(actions) + return plan_actions.action_summary(self.actions) @property def label(self) -> str: diff --git a/tests/platform/test_report_plan_block.py b/tests/platform/test_report_plan_block.py new file mode 100644 index 00000000..db2552b7 --- /dev/null +++ b/tests/platform/test_report_plan_block.py @@ -0,0 +1,166 @@ +""" +The plan block in the pull-request comment. + +The load-bearing test here is the injection one. Everything else describes what a reviewer sees; +that one describes what an attacker cannot do. +""" + +from tirith.platform import report + + +def _change(address, actions, resource_type="aws_s3_bucket"): + return {"address": address, "type": resource_type, "change": {"actions": list(actions)}} + + +def _plan(*changes): + return {"resource_changes": list(changes)} + + +def _render(plan, **kwargs): + kwargs.setdefault("run_url", "https://example.invalid/run") + return report.render_markdown({}, "COMPLETED", kwargs.pop("run_url"), plan=plan, **kwargs) + + +# --- what a reviewer sees --------------------------------------------------------------------- + + +def test_a_create_is_listed_and_counted(): + body = _render(_plan(_change("aws_s3_bucket.analytics", ["create"]))) + assert "+ aws_s3_bucket.analytics" in body + assert "Plan: 1 to add, 0 to change, 0 to destroy." in body + + +def test_no_op_resources_are_counted_but_not_listed(): + """ + The reason the list is readable at all. + + A plan against applied infrastructure carries a no-op for every resource in state. Listing them + would bury the one that changed -- on the demo repository that is five of six rows. + """ + plan = _plan( + _change("aws_s3_bucket.analytics", ["create"]), + _change("aws_s3_bucket.artifacts", ["no-op"]), + _change("aws_kms_key.artifacts", ["no-op"], "aws_kms_key"), + ) + body = _render(plan) + assert "+ aws_s3_bucket.analytics" in body + assert "aws_s3_bucket.artifacts" not in body.split("```")[1] + assert "2 unchanged" in body + + +def test_a_replacement_is_one_row_and_its_own_count(): + """ + Terraform folds replacements into add and destroy. We do not: "1 to replace" is the number a + reviewer should look twice at, and it vanishes when spread across the other columns. + """ + body = _render(_plan(_change("aws_iam_role.deploy", ["delete", "create"], "aws_iam_role"))) + assert body.count("aws_iam_role.deploy") == 1 + assert "replace (destroy first)" in body + assert "1 to replace" in body + assert "0 to add, 0 to change, 0 to destroy" in body + + +def test_update_and_replace_use_a_marker_the_fence_understands(): + """ + `~` is terraform's marker for an update and means nothing to GitHub's diff highlighting, so a + `~` row renders plain and the fence buys nothing. `!` is the one that colours. + """ + body = _render( + _plan( + _change("aws_kms_key.artifacts", ["update"], "aws_kms_key"), + _change("aws_iam_role.deploy", ["create", "delete"], "aws_iam_role"), + ) + ) + fence = body.split("```diff")[1].split("```")[0] + assert "~" not in fence + assert "! aws_kms_key.artifacts" in fence + assert "! aws_iam_role.deploy" in fence + + +def test_a_plan_with_nothing_changing_still_says_so(): + body = _render(_plan(_change("aws_s3_bucket.artifacts", ["no-op"]))) + assert "Plan: 0 to add, 0 to change, 0 to destroy." in body + assert "```diff" not in body + + +def test_a_document_with_no_resource_changes_renders_no_block(): + assert "```diff" not in _render({"format_version": "1.2"}) + assert "```diff" not in _render(None) + + +def test_small_plans_are_inline_and_large_ones_collapse(): + small = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(report.PLAN_INLINE_LIMIT)]) + assert "
Show plan" not in _render(small) + + large = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(report.PLAN_INLINE_LIMIT + 1)]) + assert "
Show plan" in _render(large) + + +def test_the_row_list_is_capped(): + plan = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(report.PLAN_ROW_LIMIT + 25)]) + body = _render(plan) + assert "… and 25 more" in body + # The count still reflects the whole plan, not just what was shown. + assert f"Plan: {report.PLAN_ROW_LIMIT + 25} to add" in body + + +# --- what an attacker cannot do ---------------------------------------------------------------- + + +def test_an_address_cannot_escape_the_fence(): + """ + The one that matters. + + A pull-request author controls the terraform, therefore the addresses. `for_each` keys make + aws_s3_bucket.demo["```"] a legal address, and inside a fence a triple backtick closes the whole + block -- everything after it would render as markdown, which is how you forge a passing verdict + in a comment a reviewer trusts. Same class of bug `_code` was written for, one layer out. + """ + evil = 'aws_s3_bucket.x["```\\n\\n## 🛡️ Tirith — all policies passed\\n\\n```diff"]' + body = _render(_plan(_change(evil, ["create"]))) + + # Exactly one fence was opened and one closed: the block is still a block. + assert body.count("```diff") == 1 + assert body.count("```") == 2 + assert "all policies passed" not in body.split("```")[2] + + +def test_a_newline_in_an_address_cannot_fabricate_rows(): + body = _render(_plan(_change("aws_s3_bucket.x\\n- aws_s3_bucket.production", ["create"]))) + fence = body.split("```diff")[1].split("```")[0] + assert len([line for line in fence.strip().splitlines() if line.strip()]) == 1 + + +def test_masked_values_stay_masked(): + """ + The block is rendered from the masked document, so anything redact.py replaced is already gone. + This pins that the renderer does not go looking for the original elsewhere in the plan. + """ + plan = _plan(_change("aws_db_instance.main", ["create"], "aws_db_instance")) + plan["resource_changes"][0]["change"]["after"] = {"password": "__SG_REDACTED__"} + body = _render(plan) + assert "hunter2" not in body + + +# --- truncation ------------------------------------------------------------------------------- + + +def test_the_plan_is_dropped_before_any_finding(): + """ + Ordering, not just presence. The plan is context; the findings are the point. + """ + results = { + "policy-a": [ + { + "result": "FAIL", + "rule_name": "a-rule", + "evaluations": {"fails": [{"description": "x" * 4000}]}, + } + ] + } + plan = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(20)]) + body = report.render_markdown( + results, "COMPLETED", "https://example.invalid/run", plan=plan, limit=3000 + ) + assert "```diff" not in body + assert "policy-a" in body From 346cfc2a9c469fe67d4c2f573670a78eb83be2dc Mon Sep 17 00:00:00 2001 From: Rafid Aslam Date: Thu, 3 Sep 2026 14:06:51 +0700 Subject: [PATCH 2/3] fix: the unchanged count is plain text, not Every other in the reporter wraps a whole line -- the cost line, the context line, the footer. This one wrapped a fragment mid-line, gluing an HTML tag onto a line that otherwise reads as terraform output. The count itself stays: it is what explains why the list is shorter than the plan, so a reviewer who knows there are seven resources does not wonder where six went. It just does not need to shout. Also strengthens the newline guard test to use a real newline rather than an escaped one. Terraform escapes newlines in a for_each key into an inert literal, so the original test was checking the harmless case; a hand-written state or a different tool can still carry a real one, and one real newline in an address is one forged row in the diff. --- src/tirith/platform/report.py | 6 ++++- tests/platform/test_report_plan_block.py | 28 +++++++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/tirith/platform/report.py b/src/tirith/platform/report.py index 1862634f..591b9a45 100644 --- a/src/tirith/platform/report.py +++ b/src/tirith/platform/report.py @@ -98,7 +98,11 @@ def render_plan_block(plan): summary = plan_actions.summary_line(counts) if counts.get("no_op"): - summary += f" {counts['no_op']} unchanged" + # Plain text, not . Every other in this module wraps a whole line; wrapping a + # fragment mid-line glues an HTML tag onto a line that otherwise reads as terraform output. + # The count still earns its place -- it is what explains why the list is shorter than the + # plan -- but it does not need to shout, and a sentence is quieter than a tag. + summary += f" {counts['no_op']} unchanged." if not rows: # Nothing is changing, so there is no list to show -- but the line saying so is still worth diff --git a/tests/platform/test_report_plan_block.py b/tests/platform/test_report_plan_block.py index db2552b7..d39c446b 100644 --- a/tests/platform/test_report_plan_block.py +++ b/tests/platform/test_report_plan_block.py @@ -48,6 +48,24 @@ def test_no_op_resources_are_counted_but_not_listed(): assert "2 unchanged" in body +def test_the_unchanged_count_is_plain_text(): + """ + Not wrapped in . + + Every other in the reporter wraps a whole line -- the cost line, the context line, the + footer. Wrapping a fragment mid-line glues an HTML tag onto a line that otherwise reads as + terraform output. The count still earns its place; it just does not need a tag. + """ + plan = _plan( + _change("aws_s3_bucket.analytics", ["create"]), + _change("aws_s3_bucket.artifacts", ["no-op"]), + ) + body = _render(plan) + summary = [line for line in body.splitlines() if line.startswith("Plan:")][0] + assert summary == "Plan: 1 to add, 0 to change, 0 to destroy. 1 unchanged." + assert "" not in summary + + def test_a_replacement_is_one_row_and_its_own_count(): """ Terraform folds replacements into add and destroy. We do not: "1 to replace" is the number a @@ -126,9 +144,17 @@ def test_an_address_cannot_escape_the_fence(): def test_a_newline_in_an_address_cannot_fabricate_rows(): - body = _render(_plan(_change("aws_s3_bucket.x\\n- aws_s3_bucket.production", ["create"]))) + """ + A real newline, not the escaped kind. + + Terraform escapes newlines in a for_each key into a literal backslash-n, which is inert. But the + plan document is not always terraform's: a hand-written state or another tool could carry a real + one, and one real newline in an address is one forged row in the diff. + """ + body = _render(_plan(_change("aws_s3_bucket.x\n- aws_s3_bucket.production", ["create"]))) fence = body.split("```diff")[1].split("```")[0] assert len([line for line in fence.strip().splitlines() if line.strip()]) == 1 + assert "aws_s3_bucket.production" in fence # kept, but on the same row def test_masked_values_stay_masked(): From e4544d0c4bcddd15b4aa90c8bbee16978e4b3cad Mon Sep 17 00:00:00 2001 From: Rafid Aslam Date: Fri, 4 Sep 2026 22:44:05 +0700 Subject: [PATCH 3/3] Show attribute-level changes in the plan diff (#292) * feat: show attribute-level changes in the plan block The resource list said what was changing. This says what about it. Rendered from the masked plan document, as before -- `before`, `after`, `after_unknown` and the `*_sensitive` trees carry everything needed, including `replace_paths`, which names the one attribute whose change is costing a destroy and recreate. That line is the most consequential thing in a plan review and no amount of resource-level summary conveys it. What each action shows differs on purpose. A create names the values the author chose and counts the computed ones, because a wall of "(known after apply)" is not information. An update and a replace show only what changed. A destroy shows nothing: the resource is going away, its former values do not inform the decision, and they do not belong in a public comment. Two things this had to get right, both found by writing it wrong first. Attribute values are as author-controlled as addresses -- more so, being the literal text of their terraform. A first draft interpolated them raw and a value of three backticks closed the block: five fence terminators where there should be two, with a forged verdict heading rendering as markdown after it. Keys and values now both go through _fence_safe. The guard belongs on both or it protects neither. Sensitivity is reported as a tree mirroring the value, not a boolean: {"triggers_replace": [false]} means the element is NOT sensitive. A truthy test on that list says the opposite, and duly printed "(sensitive value)" over a value that was never secret. _contains_true walks the tree and treats any true anywhere as marked -- conservative on purpose, since for sensitivity it over-hides rather than leaks. The cap is now counted in lines rather than resources, because a resource brings its attributes with it and it is the line count that decides readability. * test: cover module addresses and the module for_each injection surface Modules need no code change -- terraform flattens them into the same resource_changes list, so nesting shows up only as a longer address. These tests pin that down so it stays true, and add the one genuinely new attack surface: a module for_each key lands before the resource part of the address, so a guard written around resource names rather than whole addresses would miss it. * feat: always expand the plan block, and drop detail before resources Two changes to how an oversized plan is handled. The block is no longer collapsed behind a
above twelve resources. The plan is the thing this block was added to show, and putting it behind a click makes the common case a reviewer who never sees it. And when the plan does not fit, detail gives way before resources: every attribute row is dropped so that every resource stays named, and only a bare list that is still too long is cut. A reviewer can act on "this is being destroyed" without knowing which field changed, but not the reverse. Detail is dropped wholesale rather than at the cut-off point -- trimming there would annotate the first few resources and leave the rest bare, which reads as though the later ones had nothing to say. --- src/tirith/plan_actions.py | 111 ++++++++ src/tirith/platform/report.py | 102 +++++-- tests/platform/test_report_plan_attributes.py | 265 ++++++++++++++++++ tests/platform/test_report_plan_block.py | 114 +++++++- 4 files changed, 561 insertions(+), 31 deletions(-) create mode 100644 tests/platform/test_report_plan_attributes.py diff --git a/src/tirith/plan_actions.py b/src/tirith/plan_actions.py index 5412399a..69956e4b 100644 --- a/src/tirith/plan_actions.py +++ b/src/tirith/plan_actions.py @@ -6,6 +6,8 @@ `["delete", "create"]` is a replacement or two separate operations. """ +import json + # What terraform calls the operation, keyed by the exact action tuple it reports. # # The replacement pair is ordered, and which way round it is changes the risk rather than the @@ -99,3 +101,112 @@ def summary_line(counts): if counts.get("replace"): parts.append(f"{counts['replace']} to replace") return "Plan: " + ", ".join(parts) + "." + + +# Rendered in place of a value we must not or cannot print. Both are terraform's own wording, so a +# reader who knows plan output needs no translation. +SENSITIVE = "(sensitive value)" +UNKNOWN = "(known after apply)" + + +def _contains_true(node): + """ + Whether a sensitivity or unknown-ness tree marks anything at all. + + These trees mirror the *shape* of the value rather than being booleans: terraform reports + `{"triggers_replace": [false]}`, where the list is structure and the `false` is the answer. A + truthy test on the node therefore says "sensitive" for a list that says the opposite -- which is + how a first attempt printed "(sensitive value)" over a value that was never secret. + + Any `True` anywhere means the whole value is treated as marked. That is deliberately + conservative: for sensitivity it over-hides rather than leaks, and for unknown-ness it prefers + "we cannot show this" to printing half a value as if it were whole. + """ + if node is True: + return True + if isinstance(node, dict): + return any(_contains_true(v) for v in node.values()) + if isinstance(node, list): + return any(_contains_true(v) for v in node) + return False + + +def attribute_changes(change, limit=8): + """ + Per-attribute changes for one resource, as (marker, key, before, after, forces_replacement). + + `before` and `after` are already rendered to strings here, because whether a value may be shown + at all is plan semantics -- sensitivity and unknown-ness live in the document -- while *how* to + make a string safe for the surface it lands on belongs to the renderer. + + What each action shows, and why it differs: + + create every attribute with a known value. The unknown ones are what terraform fills in, and + a wall of "(known after apply)" says nothing a reader can act on, so they are counted. + update only what changed. That is the whole question being asked. + replace the same, plus which attribute forced it -- the most useful line in a plan review. + delete nothing. The resource is going away; its former values neither inform the decision nor + belong in a public comment. + """ + actions = tuple(a for a in ((change or {}).get("actions") or []) if a) + if not actions or actions == ("no-op",) or actions == ("delete",): + return [], 0, 0 + + before = (change.get("before") or {}) if isinstance(change.get("before"), dict) else {} + after = (change.get("after") or {}) if isinstance(change.get("after"), dict) else {} + unknown = change.get("after_unknown") or {} + after_sensitive = change.get("after_sensitive") + after_sensitive = after_sensitive if isinstance(after_sensitive, dict) else {} + before_sensitive = change.get("before_sensitive") + before_sensitive = before_sensitive if isinstance(before_sensitive, dict) else {} + forces = {path[0] for path in (change.get("replace_paths") or []) if path} + + creating = actions == ("create",) + rows = [] + hidden_unknown = 0 + + for key in sorted(set(before) | set(after) | (set(unknown) if isinstance(unknown, dict) else set())): + node = unknown.get(key) if isinstance(unknown, dict) else None + is_unknown = _contains_true(node) + old, new = before.get(key), after.get(key) + + if not is_unknown and old == new: + continue + + if creating: + if is_unknown: + # Counted rather than printed: on a create these are every computed attribute, and + # naming them crowds out the values the author actually chose. + hidden_unknown += 1 + continue + rows.append(("+", key, None, _render_value(new, after_sensitive.get(key)), key in forces)) + continue + + rows.append( + ( + "~", + key, + _render_value(old, before_sensitive.get(key)), + UNKNOWN if is_unknown else _render_value(new, after_sensitive.get(key)), + key in forces, + ) + ) + + dropped = max(0, len(rows) - limit) + return rows[:limit], dropped, hidden_unknown + + +def _render_value(value, sensitivity_node): + if _contains_true(sensitivity_node): + return SENSITIVE + if value is None: + return "null" + if isinstance(value, str): + return f'"{value}"' + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, (int, float)): + return str(value) + # Nested structures are compacted rather than rendered as a tree. A full nested diff is a much + # larger feature, and a compact form still answers "did this change and roughly to what". + return json.dumps(value, separators=(",", ":"), sort_keys=True) diff --git a/src/tirith/platform/report.py b/src/tirith/platform/report.py index 591b9a45..7ccfd8fd 100644 --- a/src/tirith/platform/report.py +++ b/src/tirith/platform/report.py @@ -26,13 +26,14 @@ _ICONS = {FAIL: "❌", WARN: "⚠️", APPROVAL_REQUIRED: "⏳", PASS: "✅", UNKNOWN: "❓"} -# How many changed resources render inline before the block is collapsed. Small changes should be -# readable without a click; large ones must not push the findings off the screen. -PLAN_INLINE_LIMIT = 12 - -# A hard cap on rows, independent of the comment limit. A thousand-resource plan would otherwise -# consume the whole budget and take the findings down with it during truncation. -PLAN_ROW_LIMIT = 50 +# A hard cap on lines, independent of the comment limit. A thousand-resource plan would otherwise +# consume the whole budget and take the findings down with it during truncation. Counted in lines +# rather than resources because each resource brings its changed attributes with it. +# +# The block is never collapsed behind a
. It was, above twelve resources -- but the plan is +# the thing the comment was extended to show, and putting it behind a click means the common case is +# a reviewer who never sees it. What gives way under this cap is detail, not visibility. +PLAN_LINE_LIMIT = 60 def _fence_safe(value): @@ -56,6 +57,40 @@ def _fence_safe(value): return text +def _render_attributes(change): + """ + The per-attribute lines under one resource row. + + Every key and every value goes through `_fence_safe`, and that is the whole reason this is a + separate function rather than an f-string at the call site. An attribute *value* is as + author-controlled as an address -- more so, since it is the literal text of their terraform -- and + a first version of this passed values through raw. A value of "```" closed the block and let the + rest of the comment render as markdown, reopening exactly the hole the address guard was written + to close. The guard belongs on both or it protects neither. + """ + rows, dropped, hidden_unknown = plan_actions.attribute_changes(change) + + lines = [] + for marker, key, before, after, forces in rows: + rendered_key = _fence_safe(key) + if before is None: + line = f" {marker} {rendered_key} = {_fence_safe(after)}" + else: + line = f" {marker} {rendered_key} = {_fence_safe(before)} -> {_fence_safe(after)}" + if forces: + # Terraform's own wording, and the most consequential thing on the row: it names the one + # attribute whose change is costing a destroy and recreate. + line += " # forces replacement" + lines.append(line) + + trailer = [] + if dropped: + trailer.append(f" … and {dropped} more changed attribute(s)") + if hidden_unknown: + trailer.append(f" … and {hidden_unknown} computed attribute(s), known after apply") + return lines + trailer + + def render_plan_block(plan): """ The planned changes, as a diff-fenced list plus terraform's summary line. @@ -76,7 +111,7 @@ def render_plan_block(plan): counts = plan_actions.plan_counts(changes) - rows = [] + entries = [] for change in changes: if not isinstance(change, dict): continue @@ -89,12 +124,11 @@ def render_plan_block(plan): address = _fence_safe(change.get("address") or change.get("type") or "") if not address: continue - rows.append(f"{marker} {address:<48} {_fence_safe(plan_actions.action_summary(actions))}".rstrip()) + row = f"{marker} {address:<48} {_fence_safe(plan_actions.action_summary(actions))}".rstrip() + entries.append((row, _render_attributes(change.get("change") or {}))) - dropped = 0 - if len(rows) > PLAN_ROW_LIMIT: - dropped = len(rows) - PLAN_ROW_LIMIT - rows = rows[:PLAN_ROW_LIMIT] + listed_resources = len(entries) + rows, hidden_detail, dropped_resources = _fit_plan_rows(entries) summary = plan_actions.summary_line(counts) if counts.get("no_op"): @@ -109,17 +143,41 @@ def render_plan_block(plan): # having, otherwise the comment looks like it simply forgot to mention the plan. return [summary, ""] - fence = ["```diff"] + rows + (["", f"… and {dropped} more"] if dropped else []) + ["```"] + notes = [] + if hidden_detail: + notes += ["", f"… attribute detail omitted: {hidden_detail} more line(s) than a comment can carry"] + if dropped_resources: + notes += ["", f"… and {dropped_resources} more resource(s), truncated"] + + return ["```diff"] + rows + notes + ["```", "", summary, ""] + + +def _fit_plan_rows(entries): + """ + Fit the plan into PLAN_LINE_LIMIT lines, giving up detail before resources. + + Returns (rows, hidden_detail_lines, dropped_resources). + + The order of sacrifice is the whole point. A resource row says *what is happening to your + infrastructure*; an attribute row elaborates on one. So an oversized plan loses every attribute + row before it loses a single resource, and only a resource list that is still too long after that + gets cut off the end. + + Detail is dropped wholesale rather than from the cut-off point onward. Trimming at the boundary + would annotate the first handful of resources and leave the rest bare, which reads as though the + later ones had nothing to say -- the most misleading shape available, since the untouched-looking + ones are exactly where a reviewer stops looking. + """ + full = [line for row, attributes in entries for line in [row] + attributes] + if len(full) <= PLAN_LINE_LIMIT: + return full, 0, 0 - if len(rows) > PLAN_INLINE_LIMIT: - block = [ - f"
Show plan — {len(rows)} changed resources", - "", - ] + fence + ["", "
"] - else: - block = fence + bare = [row for row, _ in entries] + hidden_detail = len(full) - len(bare) + if len(bare) <= PLAN_LINE_LIMIT: + return bare, hidden_detail, 0 - return block + ["", summary, ""] + return bare[:PLAN_LINE_LIMIT], hidden_detail, len(bare) - PLAN_LINE_LIMIT diff --git a/tests/platform/test_report_plan_attributes.py b/tests/platform/test_report_plan_attributes.py new file mode 100644 index 00000000..bfb6e434 --- /dev/null +++ b/tests/platform/test_report_plan_attributes.py @@ -0,0 +1,265 @@ +""" +Attribute-level detail in the plan block. + +Two of these matter more than the rest. `test_an_attribute_value_cannot_escape_the_fence` covers the +hole this feature opens -- a first draft passed values through raw and a value of "```" closed the +block. `test_a_sensitive_value_is_never_printed` covers the one where a bug leaks rather than merely +looks wrong. +""" + +from tirith import plan_actions +from tirith.platform import report + + +def _render(changes): + return report.render_markdown( + {}, "COMPLETED", "https://example.invalid/run", plan={"resource_changes": changes} + ) + + +def _fence(body): + return body.split("```diff")[1].split("```")[0] + + +# --- what a reviewer sees --------------------------------------------------------------------- + + +def test_an_update_shows_only_what_changed(): + body = _render( + [ + { + "address": "terraform_data.x", + "type": "terraform_data", + "change": { + "actions": ["update"], + "before": {"input": "before", "untouched": "same"}, + "after": {"input": "after", "untouched": "same"}, + }, + } + ] + ) + fence = _fence(body) + assert '~ input = "before" -> "after"' in fence + assert "untouched" not in fence + + +def test_a_create_shows_known_values_and_counts_the_computed_ones(): + """ + A wall of "(known after apply)" tells a reader nothing they can act on, so those are counted + while the values the author actually chose are named. + """ + body = _render( + [ + { + "address": "aws_s3_bucket.b", + "type": "aws_s3_bucket", + "change": { + "actions": ["create"], + "before": None, + "after": {"bucket": "demo-x"}, + "after_unknown": {"arn": True, "id": True}, + }, + } + ] + ) + fence = _fence(body) + assert '+ bucket = "demo-x"' in fence + assert "arn" not in fence + assert "2 computed attribute(s), known after apply" in fence + + +def test_a_destroy_lists_no_attributes(): + """ + The resource is going away. Its former values do not inform the decision, and printing them puts + the old state of a public comment's worth of infrastructure into the comment. + """ + body = _render( + [ + { + "address": "aws_db_instance.old", + "type": "aws_db_instance", + "change": {"actions": ["delete"], "before": {"password": "hunter2"}, "after": None}, + } + ] + ) + fence = _fence(body) + assert "- aws_db_instance.old" in fence + assert "hunter2" not in fence + assert "password" not in fence + + +def test_the_attribute_that_forces_a_replacement_is_named(): + """The most consequential line in a plan review: which change is costing a destroy.""" + body = _render( + [ + { + "address": "terraform_data.r", + "type": "terraform_data", + "change": { + "actions": ["delete", "create"], + "before": {"triggers_replace": ["v1"], "input": "same"}, + "after": {"triggers_replace": ["v2"], "input": "same"}, + "replace_paths": [["triggers_replace"]], + }, + } + ] + ) + fence = _fence(body) + assert "# forces replacement" in fence + assert [line for line in fence.splitlines() if "forces replacement" in line][0].lstrip().startswith( + "~ triggers_replace" + ) + + +# --- sensitivity, where a bug leaks ----------------------------------------------------------- + + +def test_a_sensitive_value_is_never_printed(): + body = _render( + [ + { + "address": "aws_db_instance.main", + "type": "aws_db_instance", + "change": { + "actions": ["update"], + "before": {"password": "hunter2"}, + "after": {"password": "hunter3"}, + "before_sensitive": {"password": True}, + "after_sensitive": {"password": True}, + }, + } + ] + ) + fence = _fence(body) + assert "hunter2" not in fence and "hunter3" not in fence + assert "(sensitive value)" in fence + + +def test_a_shape_shaped_sensitivity_tree_is_not_read_as_true(): + """ + terraform reports sensitivity as a tree mirroring the value, not a boolean: + {"triggers_replace": [false]} means the element is NOT sensitive. A truthy test on that list + says the opposite, and a first draft duly printed "(sensitive value)" over a value that was + never secret -- hiding information for no reason. + """ + assert plan_actions._contains_true({"triggers_replace": [False]}) is False + assert plan_actions._contains_true({"triggers_replace": [True]}) is True + assert plan_actions._contains_true(True) is True + assert plan_actions._contains_true({}) is False + + body = _render( + [ + { + "address": "terraform_data.r", + "type": "terraform_data", + "change": { + "actions": ["update"], + "before": {"triggers_replace": ["v1"]}, + "after": {"triggers_replace": ["v2"]}, + "after_sensitive": {"triggers_replace": [False]}, + "before_sensitive": {"triggers_replace": [False]}, + }, + } + ] + ) + assert "(sensitive value)" not in _fence(body) + assert '["v1"]' in _fence(body) + + +# --- what an attacker cannot do --------------------------------------------------------------- + + +def test_an_attribute_value_cannot_escape_the_fence(): + """ + The hole this feature opens. + + An attribute value is the literal text of the author's terraform, so it is at least as + controlled as an address. A first draft interpolated values raw, and a value of "```" closed the + block: measured five fence terminators where there should be two, with a forged heading rendering + as markdown after it. + """ + payload = "```\n\n## Tirith — all policies passed\n\n```diff" + body = _render( + [ + { + "address": "terraform_data.probe", + "type": "terraform_data", + "change": {"actions": ["create"], "before": None, "after": {"input": payload}}, + } + ] + ) + assert body.count("```diff") == 1 + assert body.count("```") == 2 + assert "`" not in _fence(body) + assert "all policies passed" not in body.split("```")[2] + + +def test_an_attribute_name_cannot_escape_the_fence(): + body = _render( + [ + { + "address": "terraform_data.probe", + "type": "terraform_data", + "change": {"actions": ["create"], "before": None, "after": {"a```b": "x"}}, + } + ] + ) + assert body.count("```") == 2 + assert "`" not in _fence(body) + + +def test_a_newline_in_a_value_cannot_fabricate_rows(): + body = _render( + [ + { + "address": "terraform_data.probe", + "type": "terraform_data", + "change": { + "actions": ["create"], + "before": None, + "after": {"input": "x\n- aws_db_instance.production"}, + }, + } + ] + ) + fence = _fence(body) + # One resource row plus exactly one attribute row. + assert len([line for line in fence.strip().splitlines() if line.strip()]) == 2 + + +# --- bounds ----------------------------------------------------------------------------------- + + +def test_attributes_per_resource_are_capped(): + after = {f"attr_{i:02d}": f"v{i}" for i in range(20)} + body = _render( + [ + { + "address": "terraform_data.wide", + "type": "terraform_data", + "change": {"actions": ["create"], "before": None, "after": after}, + } + ] + ) + assert "more changed attribute(s)" in _fence(body) + + +def test_the_block_is_capped_in_lines_not_resources(): + changes = [ + { + "address": f"terraform_data.r{i}", + "type": "terraform_data", + "change": { + "actions": ["create"], + "before": None, + "after": {"a": "1", "b": "2", "c": "3"}, + }, + } + for i in range(40) + ] + body = _render(changes) + fence = _fence(body) + assert len(fence.strip().splitlines()) <= report.PLAN_LINE_LIMIT + 3 + # 40 resources fit; 40 resources plus three attributes each do not. The detail is what gives way. + assert "attribute detail omitted" in fence + assert "terraform_data.r39" in fence diff --git a/tests/platform/test_report_plan_block.py b/tests/platform/test_report_plan_block.py index d39c446b..2afd4dba 100644 --- a/tests/platform/test_report_plan_block.py +++ b/tests/platform/test_report_plan_block.py @@ -106,20 +106,101 @@ def test_a_document_with_no_resource_changes_renders_no_block(): assert "```diff" not in _render(None) -def test_small_plans_are_inline_and_large_ones_collapse(): - small = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(report.PLAN_INLINE_LIMIT)]) - assert "
Show plan" not in _render(small) +def test_the_plan_is_never_collapsed(): + """ + The plan is the thing this block was added to show. Hiding it behind a click makes the common + case a reviewer who never opens it, which is the same as not rendering it at all. + """ + large = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(40)]) + body = _render(large) + assert "
Show plan" not in body + assert "```diff" in body + assert "+ aws_s3_bucket.b39" in body + + +def test_detail_is_given_up_before_any_resource(): + """ + The order of sacrifice. Every resource stays listed and every attribute row goes, rather than + the other way round -- a reviewer can act on "this is being destroyed" without knowing which + field changed, but not the reverse. + """ + changes = [ + { + "address": f"aws_s3_bucket.b{i}", + "type": "aws_s3_bucket", + "change": {"actions": ["update"], "before": {"acl": "private"}, "after": {"acl": "public"}}, + } + for i in range(report.PLAN_LINE_LIMIT - 5) + ] + body = _render({"resource_changes": changes}) + fence = body.split("```diff")[1].split("```")[0] - large = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(report.PLAN_INLINE_LIMIT + 1)]) - assert "
Show plan" in _render(large) + # Every resource is still named... + for i in range(report.PLAN_LINE_LIMIT - 5): + assert f"aws_s3_bucket.b{i} " in fence + # ...and no resource was cut off the end. + assert "more resource(s), truncated" not in body + # ...but the attribute rows that would not fit are gone, and said to be gone. + assert '~ acl = "private" -> "public"' not in fence + assert "attribute detail omitted" in fence -def test_the_row_list_is_capped(): - plan = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(report.PLAN_ROW_LIMIT + 25)]) +def test_detail_is_dropped_wholesale_not_from_the_cut_off_point(): + """ + Trimming at the boundary would annotate the first few resources and leave the rest bare, which + reads as though the later ones had nothing to say -- and the bare-looking ones are exactly where + a reviewer stops looking. + """ + changes = [ + { + "address": f"aws_s3_bucket.b{i}", + "type": "aws_s3_bucket", + "change": {"actions": ["update"], "before": {"acl": "private"}, "after": {"acl": "public"}}, + } + for i in range(report.PLAN_LINE_LIMIT) + ] + fence = _render({"resource_changes": changes}).split("```diff")[1].split("```")[0] + assert "acl" not in fence # not "some of them kept their attributes" + + +def test_resources_are_only_cut_when_the_bare_list_still_does_not_fit(): + total = report.PLAN_LINE_LIMIT + 25 + plan = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(total)]) body = _render(plan) - assert "… and 25 more" in body + assert "more resource(s), truncated" in body # The count still reflects the whole plan, not just what was shown. - assert f"Plan: {report.PLAN_ROW_LIMIT + 25} to add" in body + assert f"Plan: {total} to add" in body + + +def test_module_resources_are_listed_like_any_other(): + """ + Modules need no special handling, and this test exists to keep it that way. + + terraform flattens every module resource into the same flat `resource_changes` list the root + ones land in -- nesting shows up only as a longer address. So the renderer stays module-blind by + construction, and the risk is that someone later "adds module support" and special-cases it. + """ + body = _render( + _plan( + _change("module.storage.aws_s3_bucket.b", ["create"]), + _change("module.outer.module.inner.aws_s3_bucket.deep", ["update"]), + _change("module.replica[1].aws_s3_bucket.r", ["delete"]), + ) + ) + assert "+ module.storage.aws_s3_bucket.b" in body + assert "! module.outer.module.inner.aws_s3_bucket.deep" in body + assert "- module.replica[1].aws_s3_bucket.r" in body + assert "Plan: 1 to add, 1 to change, 1 to destroy." in body + + +def test_a_long_module_address_goes_ragged_rather_than_truncated(): + """ + Nested module addresses routinely pass the column width. Losing the tail of an address would + make two resources indistinguishable, so the column gives way instead -- ugly beats ambiguous. + """ + address = "module.platform.module.networking.module.subnets.aws_subnet.private_az_c" + body = _render(_plan(_change(address, ["create"]))) + assert address in body # --- what an attacker cannot do ---------------------------------------------------------------- @@ -157,6 +238,21 @@ def test_a_newline_in_an_address_cannot_fabricate_rows(): assert "aws_s3_bucket.production" in fence # kept, but on the same row +def test_a_module_for_each_key_cannot_escape_the_fence(): + """ + A surface the resource-level probe above does not reach. + + A module `for_each` key is author-controlled just like a resource one, but it lands *before* the + resource part of the address: module.tenant["```"].aws_s3_bucket.b. A guard that sanitised + resource names rather than whole addresses would pass this straight through. + """ + evil = 'module.tenant["```\n\n## 🛡️ Tirith — all policies passed\n\n```diff"].aws_s3_bucket.b' + body = _render(_plan(_change(evil, ["create"]))) + assert body.count("```diff") == 1 + assert body.count("```") == 2 + assert "all policies passed" not in body.split("```")[2] + + def test_masked_values_stay_masked(): """ The block is rendered from the masked document, so anything redact.py replaced is already gone.