ci: guard that runbook commands do not hardcode a prefixed resource name - #117
Merged
Conversation
Lambda, SNS topic, DLQ, IAM role/policy, EventBridge rule and alarm names all derive from ResourcePrefix, and the runbook tells operators NOT to deploy at the `cdr` default — a live askkaifbot service already owns those names in ap-southeast-1. A command hardcoding `cdr-<suffix>` therefore sends the reader at a resource they do not have, or at somebody else's production, during an incident. That shipped twice for the EventBridge rule (fixed in #116). scripts/check_runbook_prefix.py derives the prefixed-name suffixes from src/template.yaml and terraform/main.tf at runtime (13 of them) rather than hand-listing them — pitfall #61's lesson applied — and flags a `cdr-<suffix>` only where it is passed to a command. Prose, parameter tables and S3 tag names are deliberately untouched: the runbook must be able to advise against the `cdr` default, and a guard that flags its own advice gets silenced, not fixed. Section 8 legitimately inspects the FOREIGN cdr-lambda, where $PREFIX would be wrong; that declares itself with a reasoned `<!-- prefix-literal-ok: … -->` marker. An empty reason fails, and so does a marker inside a fenced block — an HTML comment there renders literally and splits a line-continued command. Five negative controls run, two of which caught real guard bugs: the first regex missed `--dimensions Name=FunctionName,Value=cdr-lambda` (the form the runbook's own CloudWatch commands use), and the harness initially counted a help-text line, making a failing control look like it fired. Recorded as pitfall #62. 454 tests pass; all 8 CI guards pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the last item from the Codex review — the one I deferred out of #116 because it needed a real false-positive story rather than a grep.
Why this is a security guard, not a docs nit
Lambda, SNS topic, DLQ, IAM role/policy, EventBridge rule and alarm names all derive from
ResourcePrefix. The runbook tells operators not to deploy at thecdrdefault, because a live askkaifbot service already owns those names in ap-southeast-1. So a command hardcodingcdr-<suffix>is wrong for precisely the readers who followed the runbook's own advice — best caseResourceNotFoundExceptionmid-incident, worst case they inspect an unrelated production service and draw conclusions from it.That shipped twice for the EventBridge rule (fixed in #116). This makes the class non-recurring.
What it checks
Suffixes are derived from
src/template.yamlandterraform/main.tfat runtime — 13 of them — never hand-listed. That is pitfall #61's lesson applied one commit later: the #60 cap audit enumerated from memory, missed one, and shipped a fail-open. A resource added to either IaC file is covered without touching this script.A
cdr-<suffix>is flagged only where it is passed to a command:Prose, parameter tables and S3 tag names (
cdr-status=sanitised) are deliberately not flagged. The runbook must be able to say "do not leaveResourcePrefixat thecdrdefault" and showcdr-staging-source-<alias>as a suggested value — a guard that flags its own advice gets silenced rather than fixed.The exemption
Section 8's
ResourceExistenceCheckblock investigates the foreigncdr-lambdathe reader did not deploy. There the literal is correct and$PREFIX-lambdawould be actively wrong. No syntactic rule tells that apart from a bug, so the doc declares it inline:A marker with no reason fails. A marker inside a fenced block fails — an HTML comment there is not a comment; it renders literally and lands mid-command when the command is line-continued (I wrote it that way first, which is how I found it). Every exemption is printed in the summary, so a growing pile is visible rather than quietly normal.
Negative controls — two caught real guard bugs
A guard never shown failing is an assumption (#57). Five controls run:
4dc275a^)Value=cdr-lambdaTwo bugs this caught in my own guard:
--opt, so an injected--dimensions Name=FunctionName,Value=cdr-lambda— the form the runbook's own CloudWatch commands use — passed clean. Now matches the whole argument run.grep -c 'hardcodes', which also matched the help text, so a failing control looked like it fired. A guard shown failing by a broken harness is worse than one never shown failing, because it looks like evidence.Verification
🤖 Generated with Claude Code