Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .claude/knowledge/testing-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
84 changes: 61 additions & 23 deletions internal/codeguard/checks/quality/quality_defensive.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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))
}
Expand All @@ -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))
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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"}) &&
Expand Down Expand Up @@ -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) {
Expand All @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion tests/checks/quality_error_defensive_multilang_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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;",
"}",
"",
Expand Down
68 changes: 68 additions & 0 deletions tests/checks/quality_precision_followup_retune_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,29 @@ func TestDefensiveBoundsAssumptionDistinguishesDictionaryFromSequenceAccess(t *t
"export function envValue(name: string) {",
" return process.env[name];",
"}",
"export function modelConfig(config: Record<string, string>, 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<unknown[]> }; risk: { findMany(input: unknown): Promise<unknown[]> } }",
}, "\n"))

report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript"))

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) {
Expand All @@ -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 <div>{firstNode?.id}{selectedColumn?.label}</div>;",
"}",
"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 <button>{input?.query ?? 'Search'}</button>;",
"}",
}, "\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<Array<{ id: string; name: string }>> } }",
}, "\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<unknown[]> } }",
}, "\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) {
Expand Down
Loading