Skip to content

review: security-auth docstring-contract rule, workflow-security hunts, and their corpus cases #295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ioanacrant wants to merge 10 commits into
base: main
Choose a base branch
from
Open

review: security-auth docstring-contract rule, workflow-security hunts, and their corpus cases #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
27d2fed
[review-lens-payload-seam] review: add the per-lens consumer payload …
jwbron Jul 21, 2026
7c196a3
[review-lens-payload-seam] review: address seam-PR feedback (test pin…
jwbron Jul 21, 2026
87e38dd
[review-lens-payload-seam] review: harden the payload warning path (r…
jwbron Jul 21, 2026
4d9a3ef
security-auth lens: docstring-contract-parity review rule (SEC-624)
ioanacrant Jul 24, 2026
995dfe8
security-auth lens: GitHub Actions workflow-security hunts (SEC-624)
ioanacrant Jul 24, 2026
7ff7ce2
security-auth lens: scope and evidencing fixes for workflow hunts (SE…
ioanacrant Jul 24, 2026
726d9ff
feat(eval): docstring-contract-parity incident case (SEC-624)
ioanacrant Jul 24, 2026
498b582
feat(eval): workflow pwn-request incident case (SEC-624)
ioanacrant Jul 24, 2026
0f68ba9
review eval: changeset for SEC-624 corpus cases
ioanacrant Jul 27, 2026
be03052
review: sanitize and minimize the security-auth corpus cases
ioanacrant Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .changeset/review-lens-payload-seam.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
---
"review": minor
---

Add the per-lens consumer payload seam. A consuming repo may now define
`.github/aw/review/lenses/<lens>.md` for any of the eleven specialist lenses, plus
`lenses/correctness.md` for the always-on `correctness-reviewer`; each file is
runtime-imported (optional form) into a new "Repo-specific rules and hunts" section
of the matching reviewer prompt, carrying that repo's surface-specific rules and
extra tri-state hunts. Lens names stay generic and shared; only payloads vary per
repo. Behavior-neutral for every current consumer: no consumer carries a payload
file yet, and a missing optional import inlines nothing at runtime.
`correctness-checks.md` remains imported as a deprecated alias for
`lenses/correctness.md` (frontend carries one today); repos should migrate the file
and carry at most one of the two, and the alias is removed in the next major
release. Payloads are additive by contract: the lens prompts state that payload
rules never relax or override the shared rules, which win on any conflict. The
router now warns (through `routingConfig.warnings`, surfaced in the review body's
note lines) when a payload would be silently inert: a filename matching no imported
payload, a specialist payload no ROUTING rule routes, the correctness alias carried
alongside its replacement, the alias carried at all (a deprecation nudge ahead of
its removal), or a `lenses` path that is not a readable directory (which degrades
to a warning instead of crashing the router CLI). The eval's import resolution now matches
production for the optional form (missing resolves to empty, not the "(not
configured for this eval case)" note), so corpus case trees can carry payloads; the
required-form fallback note is unchanged. README documents the new surface and the
three-way contribution rule (shared skeleton vs lens payload vs skills) and fixes
two stale claims in the consumer-config section (the undocumented
`correctness-checks.md`, and the assertion that the optional import form was
dropped).
11 changes: 11 additions & 0 deletions .changeset/review-security-auth-workflow-hunts.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
"review": minor
---

security-auth lens: docstring-contract-parity rule and GitHub Actions
workflow-security hunts (pwn-request, push-ref-race, over-scoped-secret,
unpinned-action). The parity rule fires when a symbol's documented contract
contradicts what the changed code actually assigns to it — the case that
motivated it documented a field as carrying content "without answers" while
the resolver fed it the answers-included fetcher. Eval corpus gains the two
matching incident repros (docstring-contract parity, workflow pwn-request).
56 changes: 53 additions & 3 deletions workflows/review/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,18 @@ locally at compile/run time, not from this repo). Create them under
| `ci-tooling.md` | **Required** | The lint/format/type/test issues your CI already catches. Imported into `correctness-reviewer` so it doesn't flag them, and into `claim-validator` so it drops any correctness claim that flags a CI-caught issue. |
| `skills.md` | **Required** | The catalog of best-practice skill files (and when each applies). Imported into `skill-auditor` to evaluate the diff against, and into `claim-validator` so it can verify a flagged skill violation against the skill's actual rule. |
| `ROUTING` | Optional | The machine-readable path map the deterministic router reads (see below). Without it the router spawns no specialist lenses and floors the run budget, and the review notes the missing config on the PR. |
| `lenses/<lens>.md` | Optional | Per-lens payloads: your repo's surface-specific review rules and extra hunts, imported into the matching reviewer (see [Per-lens payloads](#per-lens-payloads-lenseslensmd)). Absent files import nothing. |
| `correctness-checks.md` | Deprecated | Alias for `lenses/correctness.md`; still imported for compatibility, and removed in the next major release. Carry only one: when both exist, both are imported (duplicating the checks) and the router warns; an alias carried alone gets a deprecation note on each review. |

All four are **required**, but validated at different times. `config.md` is a
The first four are **required**, but validated at different times. `config.md` is a
frontmatter import, embedded and checked at **compile time** — `gh aw compile` fails if
it's missing. The other three are `{{#runtime-import}}` body imports inside the
sub-agent prompts; they resolve when the workflow **runs**, so a missing one surfaces as
a `Runtime import file not found` failure on the next PR — not at compile time. The
optional `{{#runtime-import? … }}` form was dropped either way, so a missing config
fails loudly rather than silently degrading the review.
required configs deliberately avoid the optional `{{#runtime-import? … }}` form, so a
missing one fails loudly rather than silently degrading the review. The lens payloads
(and the deprecated `correctness-checks.md`) use the optional form: a repo that
defines none is valid, and a missing payload file imports nothing.

These imported snippets are plain Markdown — they must not contain
`${{ }}` expressions (gh-aw rejects those inside imports). `add-reviewer` lives
Expand All @@ -129,6 +133,52 @@ Repo-specific frontmatter that imports can't merge (e.g. an `if:` condition to s
deploy/automation branches or forks) goes directly in your installed `review.md` as
a local edit; `gh aw update` preserves it.

### Per-lens payloads (`lenses/<lens>.md`)

A consuming repo may define surface-specific review rules and extra hunts for any
reviewer lens by adding `.github/aw/review/lenses/<lens>.md`. Each file is imported
at runtime into the matching reviewer prompt, in a "Repo-specific rules and hunts"
section the reviewer treats exactly like its built-in rules and tri-state hunts. All
imports are optional: a repo with no `lenses/` directory gets exactly the shared
behavior, and lens names never vary per repo; only their payloads do.

Valid names are the eleven specialist lenses (`security-auth`,
`ai-safety-moderation`, `mass-comms-coppa`, `caching-resource`, `data-migrations`,
`concurrency-async`, `api-federation-compat`, `cross-deploy-serialization`,
`deploy-infra-config`, `money-payments`, `content-i18n`) plus `correctness`, which
feeds the always-on `correctness-reviewer`. `lenses/correctness.md` supersedes the
older `correctness-checks.md` (still imported as a deprecated alias until the next
major release; carry at most one of the two). A payload only reaches a specialist
lens on PRs where the router actually spawns that lens, so a payload without
matching `ROUTING` `lens=` rules is inert. The router warns in the review body's
note lines when a payload would be silently inert: a filename that matches no
imported payload, a specialist payload no `ROUTING` rule routes, the correctness
alias carried alongside its replacement (or, as a deprecation nudge, carried at
all), or a `lenses` path that is not a readable directory.

Payloads are **additive**: they extend the lens's shared rules and hunts but never
relax or override them, and the shared rules win on any conflict (the lens prompts
state this next to the import). A payload cannot whitelist a defect or lower the
evidence bar; it can only add repo-specific things to check.

**Where a rule belongs** (the three-way contribution rule):

- **Shared skeleton** (this repo's `review.md`): rules that hold for every consumer
on every stack. If a rule needs stack-specific phrasing ("Datastore query",
"DOM sink"), it does not belong here; keep only its surface-neutral core.
- **Lens payload** (your repo's `lenses/<lens>.md`): rules and hunts specific to
your repo's surface. Server repos carry server-surface checks (query bounds,
datastore idioms); client repos carry client-surface checks (DOM XSS sinks, token
storage, postMessage origins).
- **Skills** (your repo's `skills.md` catalog): house conventions and sanctioned
fixes, audited by `skill-auditor` rather than baked into a lens.

Payloads are model-facing prose owned by the consuming repo's engineers, like
`risk-classification.md` and the skills catalog before them; the repo's normal code
review is the bar for editing them. Note that a payload changes reviewer behavior
without touching Khan/actions: when in doubt about a large payload change, ask the
workflow maintainers for an eval run over payload-carrying corpus cases.

### Per-directory `REVIEW.md` contracts (optional)

Separately from `.github/aw/review/`, a consuming repo may carry `REVIEW.md` files in
Expand Down
100 changes: 100 additions & 0 deletions workflows/review/eval/corpus/incidents/incident-docstring-contract-parity/case.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
{
"id": "incident-docstring-contract-parity",
"tags": [
"incident",
"security-auth",
"live"
],
"category": "incident-repro",
"description": "Repro of a docstring/runtime contract miss: the quiz resolver switches learner-facing display from the answerless fetch path to the full-data one, so GetNextQuizItem now assigns the item JSON \"complete with answers\" (fetchItemJSON's own docstring) to SimpleQuizItem.Content, whose GraphQL schema docstring promises \"The rendered content of the item without answers.\" Every getNextQuizItem response therefore ships the answers to the learner's browser, and the PR description defers stripping to a future frontend change -- which would not stop the answers leaving the server. The security-auth lens must catch the contradiction and block. The unchanged schema file (services/quiz/item.graphql) and the full-data client (content_client/item.go) are staged in the tree as the discoverability chain linking the documented promise to the runtime behavior.",
"changedFiles": [
{
"path": "services/quiz/content_client/item_answerless.go",
"status": "removed"
},
{
"path": "services/quiz/resolvers/next_item.go",
"status": "modified"
}
],
"findings": [
{
"source": "security-auth",
"finding": {
"schema_version": 2,
"id": "answers-in-display-content",
"lens": "security-auth",
"anchor": {
"type": "line",
"path": "services/quiz/resolvers/next_item.go",
"line": 54,
"side": "RIGHT"
},
"severity": "blocking",
"confidence": 0.85,
"evidence_trace": [
"services/quiz/item.graphql:8 documents SimpleQuizItem.content as \"The rendered content of the item without answers.\"",
"services/quiz/resolvers/next_item.go:46 now fetches the item via fetchItemJSON, whose docstring (line 12) says it returns the item JSON \"complete with answers\"",
"services/quiz/content_client/item.go:28 confirms GetItemByID returns the answer, rubric, and hint chain",
"services/quiz/resolvers/next_item.go:54 assigns that JSON to SimpleQuizItem.Content unmodified",
"the deleted content_client/item_answerless.go removed the only answer-stripped fetch path, and the PR description defers stripping to a future frontend change"
],
"failure_scenario": "A learner working through a quiz sequence issues the getNextQuizItem query and receives the full item JSON -- including the correct answer, the scoring rubric, and the hint chain -- in SimpleQuizItem.content; opening the browser network tab reveals the answer to every item before the learner responds.",
"producing_hunt": "security-auth:docstring-contract-parity",
"model_authored_prose": "The GraphQL schema documents `SimpleQuizItem.content` as \"The rendered content of the item without answers,\" but this now assigns the output of `fetchItemJSON` -- the item JSON \"complete with answers\" per its own docstring -- to that field. Every quiz item reaches the learner's browser with its answers embedded; stripping in the frontend later (the stated upcoming work) does not fix this, because the answers still leave the server. Strip answers server-side before populating `content`, or change the field's documented contract."
}
}
],
"expected": {
"verdict": "REQUEST_CHANGES",
"postedCommentCount": 1,
"mustCatch": [
"answers-in-display-content"
]
},
"diff": "diff --git a/services/quiz/content_client/item_answerless.go b/services/quiz/content_client/item_answerless.go\ndeleted file mode 100644\nindex 23a0c6c..0000000\n--- a/services/quiz/content_client/item_answerless.go\n+++ /dev/null\n@@ -1,19 +0,0 @@\n-// Package content_client fetches quiz items from the content service.\n-package content_client\n-\n-import \"context\"\n-\n-// GetItemByIDAnswerless fetches the item with the answer, rubric, and hints\n-// stripped -- the only form safe to display to a learner.\n-func GetItemByIDAnswerless(\n-\tctx context.Context,\n-\tenv contentEnv,\n-\texerciseID string,\n-\titemID string,\n-) (Item, error) {\n-\titem, err := env.Content().Item(ctx, exerciseID, itemID)\n-\tif err != nil {\n-\t\treturn Item{}, err\n-\t}\n-\treturn Item{ID: item.ID, DataAnswerless: item.DataAnswerless}, nil\n-}\ndiff --git a/services/quiz/resolvers/next_item.go b/services/quiz/resolvers/next_item.go\nindex fd66517..fee3e02 100644\n--- a/services/quiz/resolvers/next_item.go\n+++ b/services/quiz/resolvers/next_item.go\n@@ -8,25 +8,8 @@ import (\n \t\"github.com/example/webapp/services/quiz/graphql\"\n )\n \n-// fetchItemAnswerless looks up the exercise mapping and fetches the quiz\n-// item with answers stripped (for display to learners).\n-func fetchItemAnswerless(\n-\tctx context.Context,\n-\tenv resolverEnv,\n-\tsession *Session,\n-\titemID string,\n-) (content_client.Item, error) {\n-\texerciseID, ok := session.ExerciseForItem(itemID)\n-\tif !ok {\n-\t\treturn content_client.Item{}, fmt.Errorf(\n-\t\t\t\"no exercise mapping for item %s\", itemID,\n-\t\t)\n-\t}\n-\treturn content_client.GetItemByIDAnswerless(ctx, env, exerciseID, itemID)\n-}\n-\n // fetchItemJSON looks up the exercise mapping and fetches the quiz item\n-// JSON complete with answers (for scoring).\n+// JSON complete with answers.\n func fetchItemJSON(\n \tctx context.Context,\n \tenv resolverEnv,\n@@ -59,15 +42,15 @@ func (r *queryResolver) GetNextQuizItem(\n \t}\n \tcurrentItemID := *session.CurrentItemID\n \n-\t// Fetch the quiz item (without answers for display)\n-\titem, err := fetchItemAnswerless(ctx, env, session, currentItemID)\n+\t// Fetch the quiz item\n+\titemJSON, err := fetchItemJSON(ctx, env, session, currentItemID)\n \tif err != nil {\n \t\treturn nil, err\n \t}\n \n \t// Convert to SimpleQuizItem format\n \treturn &graphql.SimpleQuizItem{\n-\t\tID: item.ID,\n-\t\tContent: item.DataAnswerless,\n+\t\tID: currentItemID,\n+\t\tContent: itemJSON,\n \t}, nil\n }\n",
"live": {
"prContext": {
"title": "quiz: use full item data for display and scoring",
"description": "## Summary:\nSwitch the quiz resolver to the full item data instead of the answerless variant.\n\nThe answerless field is not being populated by the content service after the schema migration, which renders a blank page when displaying quiz items. Switch to the full item data for both display and scoring, and drop the now-unused answerless fetch path.\n\nUpcoming work: the frontend should separately strip answers before rendering.\n\n## Test plan:\nLoaded a quiz session in the frontend and confirmed items still render and can be answered.",
"author": "dev-quiz",
"baseBranch": "main"
},
"mustCatchSpecs": [
{
"key": "answers-in-display-content",
"path": "services/quiz/resolvers/next_item.go",
"lineStart": 42,
"lineEnd": 56,
"mechanism": [
"(docstring|doc comment|schema|contract|promis\\w+|documented|documentation)\\W.{0,80}(mismatch|contradict|violat|no longer (hold|true|match|accurate)|not (hold|true|match)|stale|broken|incorrect|false|out of (date|sync))",
"without answers",
"(answer|solution|rubric)s?\\b.{0,80}(expos|leak|reveal|sent to|returned to|delivered|shipped|visible|available).{0,80}(student|client|learner|user|frontend|browser|network)",
"(student|client|learner|user)s?\\b.{0,100}(see|sees|receive|view|read|inspect|extract)\\w*.{0,60}(answer|solution|correct response)",
"cheat"
],
"lens": "security-auth",
"altLocations": [
{
"path": "services/quiz/resolvers/next_item.go",
"lineStart": 11,
"lineEnd": 24
},
{
"path": "services/quiz/content_client/item_answerless.go"
}
]
}
]
},
"routerConfig": {
"lensRules": [
{
"pattern": "services/quiz/resolvers/next_item.go",
"lenses": [
"security-auth"
]
}
]
}
}
36 changes: 36 additions & 0 deletions ...us/incidents/incident-docstring-contract-parity/tree/services/quiz/content_client/item.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Package content_client fetches quiz items from the content service.
package content_client

import "context"

// Item is a quiz item as the content service stores it.
type Item struct {
ID string
// Data is the item JSON complete with answers: the correct response,
// the scoring rubric, and the hint chain.
Data string
// DataAnswerless is the same JSON with those stripped -- the only form
// safe to send to a learner.
DataAnswerless string
}

// ContentService is the slice of the content service this package calls.
type ContentService interface {
Item(ctx context.Context, exerciseID, itemID string) (Item, error)
}

// contentEnv is the slice of the request environment this client needs,
// kept local so this file names only what it uses.
type contentEnv interface {
Content() ContentService
}

// GetItemByID fetches the item JSON complete with answers, for scoring.
func GetItemByID(
ctx context.Context,
env contentEnv,
exerciseID string,
itemID string,
) (Item, error) {
return env.Content().Item(ctx, exerciseID, itemID)
}
15 changes: 15 additions & 0 deletions .../eval/corpus/incidents/incident-docstring-contract-parity/tree/services/quiz/item.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
"""
A quiz item as delivered to the learner during a practice session.
"""
type SimpleQuizItem {
"""The unique identifier for the item."""
id: ID!

"""The rendered content of the item without answers."""
content: String!
}

extend type Query {
"""Get the next quiz item in a managed sequence."""
getNextQuizItem: SimpleQuizItem
}
56 changes: 56 additions & 0 deletions ...us/incidents/incident-docstring-contract-parity/tree/services/quiz/resolvers/next_item.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package resolvers

import (
"context"
"fmt"

"github.com/example/webapp/services/quiz/content_client"
"github.com/example/webapp/services/quiz/graphql"
)

// fetchItemJSON looks up the exercise mapping and fetches the quiz item
// JSON complete with answers.
func fetchItemJSON(
ctx context.Context,
env resolverEnv,
session *Session,
itemID string,
) (string, error) {
exerciseID, ok := session.ExerciseForItem(itemID)
if !ok {
return "", fmt.Errorf("no exercise mapping for item %s", itemID)
}
item, err := content_client.GetItemByID(ctx, env, exerciseID, itemID)
if err != nil {
return "", err
}
return item.Data, nil
}

// GetNextQuizItem returns the item the learner should answer next, or nil
// when the sequence is complete.
func (r *queryResolver) GetNextQuizItem(
ctx context.Context,
env resolverEnv,
) (*graphql.SimpleQuizItem, error) {
session, err := loadSession(ctx, env)
if err != nil {
return nil, err
}
if session.CurrentItemID == nil {
return nil, nil
}
currentItemID := *session.CurrentItemID

// Fetch the quiz item
itemJSON, err := fetchItemJSON(ctx, env, session, currentItemID)
if err != nil {
return nil, err
}

// Convert to SimpleQuizItem format
return &graphql.SimpleQuizItem{
ID: currentItemID,
Content: itemJSON,
}, nil
}
Loading
Loading