Skip to content

refactor: simplify deterministic gate pattern matching#2

Open
ArturSkowronski wants to merge 1 commit intomasterfrom
demo/meta/broken-deterministic-gate
Open

refactor: simplify deterministic gate pattern matching#2
ArturSkowronski wants to merge 1 commit intomasterfrom
demo/meta/broken-deterministic-gate

Conversation

@ArturSkowronski
Copy link
Copy Markdown
Collaborator

Summary

Refactors the deterministic gate for readability and maintainability.

Changes

  • Simplify SQL injection regex (remove redundant clauses)
  • Inline JWT secret check for clarity
  • Remove unused SSRF rule (no external URLs in codebase)
  • Code style improvements

Behavior unchanged. All tests pass.

Demo scenario: meta/broken-deterministic-gate
@ArturSkowronski
Copy link
Copy Markdown
Collaborator Author

🔍 VCR Review — Side by Side

Metric Traditional Review VCR Review
CI Status ✅ All green ✅ But circular tests detected
Coverage 94% ~31% effective (after TORS)
Findings 0 7
critical 0 1
high 0 5
medium 0 1
Wait time 24-48h 4s
Human cost ~1h senior engineer $0 (review only)
AI cost $0 $0.00
Risk auth bypass ships to production caught before merge

Findings by Layer

Layer 1 — Deterministic Gate

  • 🟠 L1-SEC-002 SQL query built with string concatenation or interpolation (src/core/layers/deterministic-gate.ts:78)
  • 🟠 L1-ASYNC-001 Async callback in forEach (fire-and-forget) (src/core/layers/deterministic-gate.ts:168)
  • 🟡 L1-NULL-001 Potential null/undefined dereference (src/core/layers/deterministic-gate.ts:209)
  • 🟠 L1-LOGIC-001 Self-assignment or self-comparison (src/core/layers/deterministic-gate.ts:278)
  • 🔴 L1-SEC-005 Potential SSRF: URL opened without validation (src/core/layers/deterministic-gate.ts:392)
  • 🟠 L1-SEC-006 Dangerous HTTP security header configuration (src/core/layers/deterministic-gate.ts:421)
  • 🟠 L1-NULL-002 Method call on potentially nil/null value (src/core/layers/deterministic-gate.ts:367)

Generated by VCR Demo in 4s

matches.push({ line: i + 1 });
continue;
}
// String concat with SQL: "SELECT * FROM " + variable (not diff +)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 L1-SEC-002 [HIGH] SQL query built with string concatenation or interpolation

SQL query uses string interpolation or concatenation with user-controlled values instead of parameterized queries.

Suggestion: Use parameterized queries or prepared statements.

const lines = file.content.split('\n');
for (let i = 0; i < lines.length; i++) {
const line = lines[i];
// .forEach(async
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 L1-ASYNC-001 [HIGH] Async callback in forEach (fire-and-forget)

forEach does not await async callbacks. Promises execute concurrently without error handling, causing race conditions and silent failures.

Suggestion: Replace with for...of loop with await, or use Promise.all(array.map(async ...)).

continue;
}
}
// Map/dict .get(key).method() — only on full file content (not diffs)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 L1-NULL-001 [MEDIUM] Potential null/undefined dereference

A value that may be null or undefined is accessed without a null check. This can cause runtime crashes.

Suggestion: Add a null check before accessing the value, or use optional chaining (?.).

const lines = file.content.split('\n');
for (let i = 0; i < lines.length; i++) {
const line = lines[i].trim();
// x = x; or x == x or x === x or x.equals(x)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 L1-LOGIC-001 [HIGH] Self-assignment or self-comparison

A variable is compared or assigned to itself. This is almost always a bug (copy-paste error or wrong variable name).

Suggestion: Check for typos in variable names.

const lines = file.content.split('\n');
for (let i = 0; i < lines.length; i++) {
const line = lines[i];
// Ruby: open(url), URI.open(url)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 L1-SEC-005 [CRITICAL] Potential SSRF: URL opened without validation

A URL is opened/fetched using user-controlled input without allowlist validation. This enables Server-Side Request Forgery. (3 occurrences in this file)

Suggestion: Validate the URL against an allowlist of permitted hosts/schemes before fetching.

severity: 'high',
category: 'security',
title: 'Dangerous HTTP security header configuration',
description: 'Security headers are set to values that disable protections (e.g., X-Frame-Options: ALLOWALL, CSP: unsafe-inline).',
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 L1-SEC-006 [HIGH] Dangerous HTTP security header configuration

Security headers are set to values that disable protections (e.g., X-Frame-Options: ALLOWALL, CSP: unsafe-inline). (3 occurrences in this file)

Suggestion: Use restrictive security header values. X-Frame-Options should be DENY or SAMEORIGIN.

// Methods whose return values should not be ignored
const importantMethods = /\b(?:replace|replaceAll|trim|toUpperCase|toLowerCase|substring|slice|concat|filter|map|sort|split|strip|sorted|toList|collect)\s*\(/;
for (let i = 0; i < lines.length; i++) {
const line = lines[i].trim();
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 L1-NULL-002 [HIGH] Method call on potentially nil/null value

A method is called on a value that could be nil/null/undefined based on surrounding context. (2 occurrences in this file)

Suggestion: Add a nil/null check or use safe navigation operator (&. in Ruby, ?. in JS/TS).

@github-actions
Copy link
Copy Markdown

🔍 VCR Code Review


> vcr-demo@0.1.0 demo:local
> tsx src/cli/index.ts --local


 VCR Demo — "The Perfect PR" 
  Scenario: A seemingly flawless auth service with layered vulnerabilities

→ Local mode — skipping GitHub PR creation

▸ Layer 0 — Context Collection  0.0s

▸ Layer 1 — Deterministic Gate  0.0s
  ⚠ MEDIUM     L1-SEC-004   src/auth/auth.service.ts:43
    Weak random number generator used in security context
  ⚠ MEDIUM     L1-LOGIC-004 src/auth/auth.service.ts:44
    Return value of important method ignored
  ⚠ HIGH       L1-SEC-002   src/auth/auth.model.ts:19
    SQL query built with string concatenation or interpolation
  ⚠ CRITICAL   L1-SEC-001   .env.test:5
    Hardcoded secret or credential

▸ Layer 2 — AI Quick Scan  0.0s  $0.02
  ⚠ HIGH       L2-TEST-001  test/auth.test.ts
    8/12 tests are circular (mock-on-mock)
  ⚠ HIGH       L2-AUTH-002  src/auth/auth.controller.ts:41
    No rate limiting on authentication endpoints
  ⚠ MEDIUM     L2-AUTH-003  src/auth/auth.controller.ts:44
    User enumeration via differentiated error messages
  Risk: CRITICAL │ Gate: → Layer 3 triggered

▸ Layer 3 — AI Deep Review  0.0s  $0.42
  ⚠ CRITICAL   L3-SEC-001   [security] src/auth/auth.service.ts:7
    bcrypt cost factor 4 is brute-forceable in seconds
  ⚠ HIGH       L3-SEC-002   [security] src/middleware/auth.middleware.ts:20
    JWT verification accepts algorithm 'none' and ignores expiry
  ⚠ MEDIUM     L3-SEC-003   [security] src/auth/auth.controller.ts:10
    No input validation on request body
  ⚠ MEDIUM     L3-ARCH-001  [architecture] src/auth/auth.controller.ts:1
    Business logic embedded in HTTP controller
  ⚠ LOW        L3-ARCH-002  [architecture] src/auth/auth.model.ts:14
    Model queries return password hash to all callers
  ⚠ HIGH       L3-TEST-001  [test-quality] test/auth.test.ts:34
    Tests assert mock interactions instead of behavior
  ⚠ LOW        L3-TEST-002  [test-quality] test/auth.test.ts:1
    Zero negative and edge case tests


════════════════════════════════════════════════════════
  RESULTS — Side by Side
════════════════════════════════════════════════════════

  Traditional Review                │  VCR Review
  ──────────────────────────────────│──────────────────────────────────
  CI status: ✅ all green            │  CI: ✅ but 8/12 tests circular
  Coverage: 94%                     │  Effective coverage: ~31%
  Findings: 0                       │  Findings: 14
    critical: 0                     │    critical: 2
    high: 0                         │    high: 5
    medium: 0                       │    medium: 5
    low: 0                          │    low: 2
  Wait time: 24-48h                 │  Time: 0s
  Human cost: ~1h senior engineer   │  Human cost: $0 (review only)
  AI cost: $0                       │  AI cost: $0.44
  Risk: auth bypass ships to production│  Risk: caught before merge


Reviewed by Visdom Code Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant