-
Notifications
You must be signed in to change notification settings - Fork 0
fix: the engine only knew Node (3.5.1) #10
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.
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # 006: `auth-2` reports middleware as something to confirm, not as a failure | ||
|
|
||
| **Decision.** `auth-2` no longer calls a request handler unguarded just because the word "auth" is missing from the file. It now recognizes three ways a handler can be authenticated, and when it finds request middleware, it drops from a HIGH failure to a LOW warning that names the middleware and lists its public route patterns. | ||
|
|
||
| **Why this came up.** On a live application, `auth-2` reported "18 of 25 request handlers never mention an auth check" as a HIGH finding. Reading the flagged files, at least 14 of them were authenticated. Three mechanisms the check could not see: middleware that guards every route not on a public list, webhook routes that verify a cryptographic signature because a machine is calling and cannot carry a browser session, and routes that compare a shared secret from the environment. What was at stake: a HIGH finding that is wrong about half the time is worse than no finding, because it teaches people to scroll past the real ones. The same report contained a genuine critical finding, `auth-3`, sitting underneath eighteen false ones. | ||
|
|
||
| **Options.** | ||
| 1. Delete `auth-2` until it can be right. Cost: a genuinely unguarded handler, which is the most common way a small app leaks data, goes unreported entirely. | ||
| 2. Teach it the three mechanisms and keep the HIGH severity. Cost: a file scan still cannot prove a middleware route pattern covers a given path, because those patterns are globs evaluated when a request arrives. The check would still be asserting something it cannot know. | ||
| 3. Teach it the three mechanisms, and when middleware exists, change what the finding claims: not "these are unguarded" but "these rely on middleware, confirm the matcher covers them". Drop the severity, and print the public patterns so a person can check in about a minute. Cost: a route that middleware genuinely misses is now a LOW warning rather than a HIGH failure, and a hurried reader may skip it. | ||
|
|
||
| **What we chose and why.** Option 3. Joint call: the handoff leaned this way and Claude implemented it. The deciding argument is the toolkit's own rule, that a tool must never claim more than it checked. The tool checked that middleware exists. It did not check that the matcher covers these paths. So that is exactly what the finding should say. The severity follows the certainty, not the topic. | ||
|
|
||
| **What we gave up.** Loudness on a real class of bug. If someone adds a route that their middleware matcher happens to exclude, this now arrives as a LOW warning in a list, not a HIGH finding at the top. We accepted that because the alternative was eighteen false HIGH findings burying one true CRITICAL. | ||
|
|
||
| **How we'll know if this was right.** On the live test application the finding goes from 18 flagged to 9, and each of the 9 is genuinely middleware-dependent rather than authenticated some other way. Longer term: nobody reports a handler that this warning mentioned and they dismissed, which turned out to be open. | ||
|
|
||
| **What actually happened.** | ||
| (Tarik fills this in.) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # 007: Node and Next.js and Python are supported. Everything else gets the checks that do not care. | ||
|
|
||
| **Decision.** The tool no longer claims to work on any repository regardless of language. Checks split into two kinds. Checks that read the repository itself, is there CI, does it gate pull requests, is `.env` ignored, is there a runbook with real words in it, run on anything. Checks that need to understand the stack are supported on Node, Next.js and Python only, and on any other language they say what they could not check instead of guessing. | ||
|
|
||
| **Why this came up.** Five separate bugs turned out to be one bug wearing different hats: the engine only really knew Node. A helper returned a fake empty `package.json`, which is truthy in Python, so a Go repository was told "package.json exists but does not define a test script" about a file that was not there. The lint-step check knew `eslint` and `ruff` but not `mypy` or `golangci-lint`. The health-endpoint check could only find a route that was a file path, which is how Next.js declares one and how no other framework does. Sixty-eight tests caught none of it, because every fixture in the suite was a JavaScript repository. What was at stake: the tool's entire value is that it does not overstate what it looked at, and the sentence "it works on any git repo regardless of language/framework" was the tool overstating where it looked. | ||
|
|
||
| **Options.** | ||
| 1. Keep the "any language" claim and keep patching. Cost: every new language is an edit in five files, the claim stays false in between, and the tool earns a reputation for lying about ecosystems it never supported. | ||
| 2. Support Node, Next.js, Python and Go properly. Cost: Go is a fourth surface to keep correct, and Go is not who this tool is for. Its audience is self-taught developers shipping AI-assisted apps, which in practice means Next.js and Python. | ||
| 3. Support Node, Next.js and Python. Split the checks so the language-agnostic ones still run everywhere, and make "no rules for Go" an explicit, named outcome that is left out of the score rather than counted as a failure. | ||
|
|
||
| **What we chose and why.** Option 3. Tarik's call, after research into how seven mature audit tools handle the same problem. OpenSSF Scorecard has a third outcome besides pass and fail, an inconclusive result scored `-1`, used when a check does not apply or the repository's language is unsupported, and it is excluded from the aggregate rather than penalized. Checkov has the same idea under the name `UNKNOWN`. That is the honest shape, and this codebase already had the machinery: an `n/a` status that is dropped from scoring. | ||
|
|
||
| **What we gave up.** The broad claim, which was the more impressive-sounding one. A Go or Ruby user now gets a smaller report. That is the correct trade: a smaller true report beats a larger one with invented findings in it. | ||
|
|
||
| **How we'll know if this was right.** A Go repository's report contains no statement about a file that does not exist, and names what it skipped. Adding a language later is one PR that adds an applicability tag and a fixture pair, not an edit across five files. | ||
|
|
||
| **What actually happened.** | ||
| (Tarik fills this in.) | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete or remove the outcome sections before merge. Both decision records still contain unresolved editorial placeholders. Replace each with the actual validation outcome or remove the section so the committed records are complete.
📍 Affects 2 files
docs/decisions/007-supported-languages.md#L19-L19(this comment)docs/decisions/006-auth-2-middleware-and-signatures.md#L18-L19🤖 Prompt for AI Agents