diff --git a/.claude/knowledge/testing-patterns.md b/.claude/knowledge/testing-patterns.md index cecc603..3ea17cc 100644 --- a/.claude/knowledge/testing-patterns.md +++ b/.claude/knowledge/testing-patterns.md @@ -12,3 +12,4 @@ Testing strategies, test infrastructure quirks, how to run/debug specific test s - `gofmt -l .` at repo root is polluted by `.claude/worktrees/` (live agent worktrees) and `.gomodcache/`; scope it to `gofmt -l cmd internal pkg tests changelog.go` or use `make fmt-check`. - **Bidirectional (server→client) MCP tests** live in `tests/mcp/sampling_test.go`: the test acts as the MCP client, advertises `sampling`/`roots` at `initialize`, and answers the server's server-initiated requests. stdio uses interactive `StdinPipe`/`StdoutPipe` (not the replay harness). HTTP opens the `GET /mcp` SSE stream (waits for the `: ready` comment to avoid the attach race), reads the request off the stream, and POSTs the response with the matching `Mcp-Session-Id`. propose_fix verification is expected to fail on the throwaway diff — assert the round trip fired, not a verified patch. The HTTP helper passes `-config` via `CODEGUARD_TEST_HTTP_CONFIG`. - **TS tests can be hijacked by the Node semantic engine**: on hosts with a discoverable `typescript.js` (e.g. VS Code installed), TypeScript targets route through the Node runner instead of the per-file Go path. Tests that must exercise the per-file path (tree-sitter differential tests, corpus TS groups) set `CODEGUARD_TYPESCRIPT_LIB_PATH` to an existing-but-invalid lib to force the fallback. +- Defensive precision positive fixtures should avoid UI-ish names such as `render*` unless the test is explicitly covering UI suppression. The defensive boundary/null rules intentionally skip React/UI helper contexts, so a fixture named like a renderer can stop emitting the server-side defensive finding the test expects. diff --git a/internal/codeguard/checks/quality/quality_defensive.go b/internal/codeguard/checks/quality/quality_defensive.go index c803f55..bd7f108 100644 --- a/internal/codeguard/checks/quality/quality_defensive.go +++ b/internal/codeguard/checks/quality/quality_defensive.go @@ -25,22 +25,24 @@ const ( ) var ( - indexAccessPattern = regexp.MustCompile(`\b([A-Za-z_][\w$]*(?:\.[A-Za-z_][\w$]*)?)\s*\[\s*([^\]\n]+)\s*\]`) - jsonDecodePattern = regexp.MustCompile(`(?i)(json\.Unmarshal|json\.NewDecoder|JSON\.parse|json\.loads|nlohmann::json::parse|decode_json|parseJson)`) - externalCallPattern = regexp.MustCompile(`(?i)(http\.Get|client\.Do|fetch\s*\(|axios\.|requests\.(get|post|put|delete)|curl_easy_perform|httplib::|http_client)`) - resourceReadPattern = regexp.MustCompile(`(?i)(io\.ReadAll|ReadAll|read_to_string|read_to_end|\.read\s*\(|bodyParser|multer|upload|formData\s*\(|request\.body|r\.Body|findMany\s*\(|findFirst\s*\()`) - unsafeDefaultPattern = regexp.MustCompile(`(?i)(getenv|process\.env|os\.environ|std::getenv|config).*?(default|fallback|\|\||!=|,\s*['"]).*?(true|false|allow|disable|skip|insecure)`) - switchLikePattern = regexp.MustCompile(`(?i)\b(switch|match)\b[^{:\n]*(status|state|kind|type)`) - stateAssignmentPattern = regexp.MustCompile(`(?i)(status|state)\s*(?:=|:=|=>)\s*["']?(paid|active|complete|completed|shipped|deleted|approved)["']?`) - authFailOpenPattern = regexp.MustCompile(`(?is)(except|catch)\b[^{:\n]*(?:\{|:)[^}\n]*return\s+(true|allow|nil|none)`) - structStartPattern = regexp.MustCompile(`(?i)\b(type\s+\w+\s+struct|interface\s+\w+|class\s+\w+|struct\s+\w+)`) - boolFieldPattern = regexp.MustCompile(`(?i)\b(bool|boolean)\b`) - stringStateFieldPattern = regexp.MustCompile(`(?i)\b(status|state|kind)\b.*\b(string|str|std::string|String)\b|\b(string|str|std::string|String)\b.*\b(status|state|kind)\b`) - resourceCountGuard = regexp.MustCompile(`(?i)\b(?:count|size|length|len|bytes)\s*(?:<=|<|>|>=)\s*(?:max|limit|quota|cap|[0-9])`) - resourceNamedCountLimit = regexp.MustCompile(`(?i)\b(?:max|limit|quota|cap)[A-Za-z0-9_]*(?:count|size|length|len|bytes)\b`) - sequenceAllocationLine = regexp.MustCompile(`(?i)\b(?:external[_]?id|next[_]?id|sequence|slug|number)\b.*(?:count|max)\s*\+\s*1|(?:count|max)\s*\+\s*1.*\b(?:external[_]?id|next[_]?id|sequence|slug|number)\b`) - jsonReaderSchemaCall = regexp.MustCompile(`(?i)\b(?:read|parse|decode)Json[A-Za-z0-9_]*\s*\([^)\n,]+,\s*[A-Za-z_$][\w$]*(?:Schema|Validator|Codec|Parser)\b`) - prismaTakePattern = regexp.MustCompile(`(?is)\b(?:findMany|findFirst|findUnique|query|search)\s*\([^)]*\btake\s*:`) + indexAccessPattern = regexp.MustCompile(`\b([A-Za-z_][\w$]*(?:\.[A-Za-z_][\w$]*)?)\s*\[\s*([^\]\n]+)\s*\]`) + jsonDecodePattern = regexp.MustCompile(`(?i)(json\.Unmarshal|json\.NewDecoder|JSON\.parse|json\.loads|nlohmann::json::parse|decode_json|parseJson)`) + externalCallPattern = regexp.MustCompile(`(?i)(http\.Get|client\.Do|fetch\s*\(|axios\.|requests\.(get|post|put|delete)|curl_easy_perform|httplib::|http_client)`) + resourceReadPattern = regexp.MustCompile(`(?i)(io\.ReadAll|ReadAll|read_to_string|read_to_end|\.read\s*\(|\.text\s*\(|arrayBuffer\s*\(|bodyParser|multer|formData\s*\(|request\.body|r\.Body)`) + ormCollectionReadPattern = regexp.MustCompile(`(?i)\bfindMany\s*\(`) + unsafeDefaultPattern = regexp.MustCompile(`(?i)(getenv|process\.env|os\.environ|std::getenv|config).*?(default|fallback|\|\||!=|,\s*['"]).*?(true|false|allow|disable|skip|insecure)`) + switchLikePattern = regexp.MustCompile(`(?i)\b(switch|match)\b[^{:\n]*(status|state|kind|type)`) + stateAssignmentPattern = regexp.MustCompile(`(?i)(status|state)\s*(?:=|:=|=>)\s*["']?(paid|active|complete|completed|shipped|deleted|approved)["']?`) + authFailOpenPattern = regexp.MustCompile(`(?is)(except|catch)\b[^{:\n]*(?:\{|:)[^}\n]*return\s+(true|allow|nil|none)`) + structStartPattern = regexp.MustCompile(`(?i)\b(type\s+\w+\s+struct|interface\s+\w+|class\s+\w+|struct\s+\w+)`) + boolFieldPattern = regexp.MustCompile(`(?i)\b(bool|boolean)\b`) + stringStateFieldPattern = regexp.MustCompile(`(?i)\b(status|state|kind)\b.*\b(string|str|std::string|String)\b|\b(string|str|std::string|String)\b.*\b(status|state|kind)\b`) + resourceCountGuard = regexp.MustCompile(`(?i)\b(?:count|size|length|len|bytes)\s*(?:<=|<|>|>=)\s*(?:max|limit|quota|cap|[0-9])`) + resourceNamedCountLimit = regexp.MustCompile(`(?i)\b(?:max|limit|quota|cap)[A-Za-z0-9_]*(?:count|size|length|len|bytes)\b`) + sequenceAllocationLine = regexp.MustCompile(`(?i)\b(?:external[_]?id|next[_]?id|sequence|slug|number)\b.*(?:count|max)\s*\+\s*1|(?:count|max)\s*\+\s*1.*\b(?:external[_]?id|next[_]?id|sequence|slug|number)\b`) + jsonReaderSchemaCall = regexp.MustCompile(`(?i)\b(?:read|parse|decode)Json[A-Za-z0-9_]*\s*\([^)\n,]+,\s*[A-Za-z_$][\w$]*(?:Schema|Validator|Codec|Parser)\b`) + prismaTakePattern = regexp.MustCompile(`(?is)\b(?:findMany|findFirst|findUnique|query|search)\s*\([^)]*\btake\s*:`) + sequenceIndexKeyPattern = regexp.MustCompile(`(?i)^(?:i|j|n|idx|index|offset|position|pos|[A-Za-z_$][\w$]*(?:Index|Idx|Offset|Position|Pos))$`) ) func defensiveBoundaryFindings(env support.Context, file string, fn precisionFunction) []core.Finding { @@ -51,11 +53,11 @@ func defensiveBoundaryFindings(env support.Context, file string, fn precisionFun loweredBody := strings.ToLower(body) findings := make([]core.Finding, 0) - if line, ok := unvalidatedBoundaryInputLine(fn, loweredBody); ok { + if line, ok := unvalidatedBoundaryInputLine(file, fn, loweredBody); ok { findings = append(findings, precisionWarnFinding(env, defensiveUnvalidatedBoundaryInputRuleID, file, line, "boundary input is consumed without validation or schema checks", core.ConfidenceMedium)) } - if line, ok := nullAssumptionLine(fn, loweredBody); ok { + if line, ok := nullAssumptionLine(file, fn, loweredBody); ok { findings = append(findings, precisionWarnFinding(env, defensiveNullAssumptionRuleID, file, line, "nullable boundary value is dereferenced without a nil/null guard", core.ConfidenceMedium)) } @@ -67,7 +69,7 @@ func defensiveBoundaryFindings(env support.Context, file string, fn precisionFun findings = append(findings, precisionWarnFinding(env, defensiveIntegerOverflowRuleID, file, line, "arithmetic on count, size, or length input lacks an overflow bound check", core.ConfidenceMedium)) } - if line, ok := boundsAssumptionLine(fn, loweredBody); ok { + if line, ok := boundsAssumptionLine(file, fn, loweredBody); ok { findings = append(findings, precisionWarnFinding(env, defensiveBoundsAssumptionRuleID, file, line, "indexed access assumes collection bounds without a nearby length check", core.ConfidenceMedium)) } @@ -136,7 +138,10 @@ func structuralStateContainerLine(line string) bool { return strings.Contains(lowered, "struct") || strings.Contains(lowered, "interface") || strings.Contains(lowered, "class") } -func unvalidatedBoundaryInputLine(fn precisionFunction, loweredBody string) (int, bool) { +func unvalidatedBoundaryInputLine(file string, fn precisionFunction, loweredBody string) (int, bool) { + if isUIHelperOrMappingContext(file, fn) || isReactComponentOrHookBoundary(file, fn) { + return 0, false + } if !boundaryFunctionName(fn.Name) && !hasBoundaryParam(fn.Params) { return 0, false } @@ -197,7 +202,10 @@ func hasBoundaryParam(params []support.ParsedParam) bool { return false } -func nullAssumptionLine(fn precisionFunction, loweredBody string) (int, bool) { +func nullAssumptionLine(file string, fn precisionFunction, loweredBody string) (int, bool) { + if isUIHelperOrMappingContext(file, fn) || isReactComponentOrHookBoundary(file, fn) { + return 0, false + } for _, param := range fn.Params { name := strings.ToLower(strings.Trim(param.Name, "*& ")) if name == "" || !nullableParam(param) { @@ -351,7 +359,10 @@ func isSeedOrScriptSourcePath(file string) bool { strings.HasPrefix(base, "cleanup") } -func boundsAssumptionLine(fn precisionFunction, loweredBody string) (int, bool) { +func boundsAssumptionLine(file string, fn precisionFunction, loweredBody string) (int, bool) { + if isUIHelperOrMappingContext(file, fn) || isReactComponentOrHookBoundary(file, fn) { + return 0, false + } if containsAny(loweredBody, []string{"len(", ".length", ".size()", "empty()", "bounds", "range", "count >"}) { return 0, false } @@ -389,7 +400,7 @@ func indexExpressionLooksSequenceAccess(target string, key string, raw string) b if regexp.MustCompile(`^\d+$`).MatchString(loweredKey) { return true } - if containsAny(loweredKey, []string{"index", "idx", "offset", "position", "pos", "i", "j", "n"}) { + if sequenceIndexKeyPattern.MatchString(strings.TrimSpace(strings.Trim(key, `"'`))) { return true } return containsAny(loweredTarget, []string{"array", "list", "slice", "items", "rows", "columns", "chars", "parts", "tokens", "segments", "lines", "values"}) && @@ -468,6 +479,9 @@ func missingSchemaValidationLine(fn precisionFunction, loweredBody string) (int, func missingResourceLimitLine(fn precisionFunction, loweredBody string) (int, bool) { if !resourceReadPattern.MatchString(functionRawBody(fn)) { + if line, ok := missingORMCollectionLimitLine(fn, loweredBody); ok { + return line, true + } return 0, false } if uploadValidationHelperPattern(loweredBody) { @@ -485,6 +499,30 @@ func missingResourceLimitLine(fn precisionFunction, loweredBody string) (int, bo return firstPatternLine(fn, resourceReadPattern), true } +func missingORMCollectionLimitLine(fn precisionFunction, loweredBody string) (int, bool) { + if !ormCollectionReadPattern.MatchString(functionRawBody(fn)) { + return 0, false + } + if resourceLimitProofPattern(loweredBody) { + return 0, false + } + if !ormCollectionReadRequiresExplicitLimit(fn, loweredBody) { + return 0, false + } + return firstPatternLine(fn, ormCollectionReadPattern), true +} + +func ormCollectionReadRequiresExplicitLimit(fn precisionFunction, loweredBody string) bool { + loweredName := strings.ToLower(strings.Trim(fn.Name, "_$")) + if containsAny(loweredName, []string{"search", "list", "page", "feed", "autocomplete"}) { + return true + } + if boundaryFunctionName(fn.Name) && containsAny(loweredBody, []string{"request", "query", "params", "cursor", "page", "search"}) { + return true + } + return containsAny(loweredBody, []string{"cursor", "skip:", "offset:", "searchparams", "query."}) +} + func resourceLimitProofPattern(loweredBody string) bool { if containsAny(loweredBody, []string{ "limitreader", "maxbytes", "max_bytes", "content-length", "contentlength", diff --git a/tests/checks/quality_error_defensive_multilang_test.go b/tests/checks/quality_error_defensive_multilang_test.go index 31ead53..1406960 100644 --- a/tests/checks/quality_error_defensive_multilang_test.go +++ b/tests/checks/quality_error_defensive_multilang_test.go @@ -439,7 +439,7 @@ func TestQualityDefensiveBoundariesDetectMultiLanguageSignals(t *testing.T) { " return payload.user.id;", "}", "", - "export function renderUser(user: User | null): string {", + "export function loadUserName(user: User | null): string {", " return user.name;", "}", "", diff --git a/tests/checks/quality_precision_followup_retune_test.go b/tests/checks/quality_precision_followup_retune_test.go index 6277f6d..7ae251e 100644 --- a/tests/checks/quality_precision_followup_retune_test.go +++ b/tests/checks/quality_precision_followup_retune_test.go @@ -84,9 +84,20 @@ func TestDefensiveBoundsAssumptionDistinguishesDictionaryFromSequenceAccess(t *t "export function envValue(name: string) {", " return process.env[name];", "}", + "export function modelConfig(config: Record, modelKey: string) {", + " return config[modelKey] ?? 'default';", + "}", + "export async function promiseTuple(db: Db, ids: string[]) {", + " const [contracts, risks] = await Promise.all([", + " db.contract.findMany({ where: { id: { in: ids } }, take: 10 }),", + " db.risk.findMany({ where: { id: { in: ids } }, take: 10 }),", + " ]);", + " return { contracts, risks };", + "}", "export function firstSegment(segments: string[]) {", " return segments[0];", "}", + "interface Db { contract: { findMany(input: unknown): Promise }; risk: { findMany(input: unknown): Promise } }", }, "\n")) report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) @@ -94,6 +105,8 @@ func TestDefensiveBoundsAssumptionDistinguishesDictionaryFromSequenceAccess(t *t assertFindingRulePresent(t, report, "Code Quality", "defensive.bounds-assumption") assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:2") assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:5") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:8") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:11") } func TestDefensiveResourceLimitCreditsPrismaTakeAndContentLengthHelpers(t *testing.T) { @@ -117,12 +130,67 @@ func TestDefensiveResourceLimitCreditsPrismaTakeAndContentLengthHelpers(t *testi " const form = await request.formData();", " return form;", "}", + "export async function readRaw(file: File) {", + " return Buffer.from(await file.arrayBuffer());", + "}", + "export function uploadsRoot() {", + " const fromEnv = process.env.LEGAL_OS_UPLOADS_ROOT?.trim();", + " return fromEnv ?? '/tmp/uploads';", + "}", }, "\n")) report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) assertFindingRulePresent(t, report, "Code Quality", "defensive.missing-resource-limit") assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "search-tools.ts") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "unbounded-upload.ts:8") +} + +func TestDefensiveBroadeningSkipsUIBoundsAndInternalORMReads(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "apps/web/components/relationship-tree.tsx"), strings.Join([]string{ + "export function RelationshipTree({ nodes, columns }: Props) {", + " const firstNode = nodes[0];", + " const selectedColumn = columns[activeIndex];", + " return
{firstNode?.id}{selectedColumn?.label}
;", + "}", + "interface Props { nodes: Array<{ id: string }>; columns: Array<{ label: string }>; activeIndex: number }", + }, "\n")) + writeFile(t, filepath.Join(dir, "apps/web/components/use-filter-state.tsx"), strings.Join([]string{ + "export function useFilterState(input?: { query?: string }) {", + " const query = input?.query ?? '';", + " return { query };", + "}", + "export function SearchToolbar({ input }: { input?: { query?: string } }) {", + " return ;", + "}", + }, "\n")) + writeFile(t, filepath.Join(dir, "packages/api/src/lib/legal-roster.ts"), strings.Join([]string{ + "export async function getLegalRoster(db: Db) {", + " const rows = await db.user.findMany({ where: { active: true } });", + " return rows.map((row) => ({ id: row.id, name: row.name }));", + "}", + "interface Db { user: { findMany(input: unknown): Promise> } }", + }, "\n")) + writeFile(t, filepath.Join(dir, "packages/api/src/routers/search-tools.ts"), strings.Join([]string{ + "export async function searchTools(db: Db, query: string) {", + " return db.tool.findMany({ where: { name: { contains: query } } });", + "}", + "export async function searchToolsLimited(db: Db, query: string) {", + " return db.tool.findMany({ where: { name: { contains: query } }, take: TOOL_SEARCH_LIMIT });", + "}", + "declare const TOOL_SEARCH_LIMIT: number;", + "interface Db { tool: { findMany(input: unknown): Promise } }", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "relationship-tree.tsx") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.null-assumption", "use-filter-state.tsx") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.unvalidated-boundary-input", "use-filter-state.tsx") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "legal-roster.ts") + assertCodeQualityRulePresentForPathWithMessage(t, report, "defensive.missing-resource-limit", "search-tools.ts", "resource limit") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "search-tools.ts:4") } func TestFunctionReturnContractAllowsNullableParserLookupAndExistsHelpers(t *testing.T) {