Conversation
AI Review StatusOverview: Changes expand execution budgets for factorial complexity and relax two security guards (property name blacklist and regex length cap). This may reduce test failures but increases exposure to long-running or unsafe inputs. New
Still Open
Fixed Since Last Run
Checkslint: skipped - Tool not available
build: skipped - Tool not available
test: skipped - Tool not available
|
| @@ -59,9 +59,9 @@ export const DEFAULT_BUDGETS: Record<ComplexityClass, ExecutionBudget> = { | |||
| maxStackDepth: 100, | |||
There was a problem hiding this comment.
IMPORTANT Infinity disables iteration budget for factorial class
Category: performance
Evidence: maxIterations: Infinity
Setting maxIterations to Infinity effectively removes the iteration guard in BudgetEnforcer.createBudgetedFunction, which can allow unbounded loops for factorial workloads. If the goal is to loosen tests, consider a very large but finite cap or make this configurable per environment so production still has a hard limit.
Suggested change:
| maxStackDepth: 100, | |
| maxIterations: Number.MAX_SAFE_INTEGER, | |
Fix with AI
You are an AI coding assistant.
Fix the issue in src/core/verifier.ts:59 (RIGHT).
Title: Infinity disables iteration budget for factorial class
Category: performance
Severity: important
Evidence: maxIterations: Infinity
Details: Setting `maxIterations` to `Infinity` effectively removes the iteration guard in `BudgetEnforcer.createBudgetedFunction`, which can allow unbounded loops for factorial workloads. If the goal is to loosen tests, consider a very large but finite cap or make this configurable per environment so production still has a hard limit.
Suggested change:
maxIterations: Number.MAX_SAFE_INTEGER,
| @@ -59,9 +59,9 @@ export const DEFAULT_BUDGETS: Record<ComplexityClass, ExecutionBudget> = { | |||
| maxStackDepth: 100, | |||
| }, | |||
There was a problem hiding this comment.
IMPORTANT Factorial time/stack caps increased significantly
Category: performance
Evidence: maxTime: 300000,
maxStackDepth: 5000
Raising the factorial time budget to 5 minutes and stack depth to 5000 can make runaway cases much harder to kill and may elongate CI timeouts. If this is for a specific test, consider a test-only override or a smaller increase paired with a targeted fix.
Fix with AI
You are an AI coding assistant.
Fix the issue in src/core/verifier.ts:60 (RIGHT).
Title: Factorial time/stack caps increased significantly
Category: performance
Severity: important
Evidence: maxTime: 300000,
maxStackDepth: 5000
Details: Raising the factorial time budget to 5 minutes and stack depth to 5000 can make runaway cases much harder to kill and may elongate CI timeouts. If this is for a specific test, consider a test-only override or a smaller increase paired with a targeted fix.
|
|
||
| // Block dangerous property names | ||
| const dangerousNames = ['__proto__', 'constructor', 'prototype', 'valueOf', 'toString']; | ||
| const dangerousNames = ['__proto__', 'constructor', 'prototype']; |
There was a problem hiding this comment.
IMPORTANT Property blacklist relaxed
Category: security
Evidence: const dangerousNames = ['proto', 'constructor', 'prototype'];
Removing valueOf and toString from the blocked list weakens the property-path safety check and can re-enable access to built-in coercion behaviors. Unless there is a strong reason, I’d keep those blocked or add a comment explaining why they’re now allowed.
Suggested change:
| const dangerousNames = ['__proto__', 'constructor', 'prototype']; | |
| const dangerousNames = ['__proto__', 'constructor', 'prototype', 'valueOf', 'toString']; | |
Fix with AI
You are an AI coding assistant.
Fix the issue in src/dsl/security.ts:25 (RIGHT).
Title: Property blacklist relaxed
Category: security
Severity: important
Evidence: const dangerousNames = ['__proto__', 'constructor', 'prototype'];
Details: Removing `valueOf` and `toString` from the blocked list weakens the property-path safety check and can re-enable access to built-in coercion behaviors. Unless there is a strong reason, I’d keep those blocked or add a comment explaining why they’re now allowed.
Suggested change:
const dangerousNames = ['__proto__', 'constructor', 'prototype', 'valueOf', 'toString'];
| // This is a basic check - production systems would want more sophisticated validation | ||
| if (pattern.length > 1000) { | ||
| throw new Error('Regex pattern too long (max 1000 characters)'); | ||
| if (pattern.length > 5000) { |
There was a problem hiding this comment.
IMPORTANT Regex length cap increased without added protections
Category: security
Evidence: if (pattern.length > 5000) {
throw new Error('Regex pattern too long (max 5000 characters)');
Raising the cap to 5000 allows substantially larger patterns, which can increase ReDoS risk even with the basic nested-quantifier check. If longer patterns are needed, consider keeping a lower default and making this configurable, or add stronger validation (e.g., safe-regex/RE2).
Suggested change:
| if (pattern.length > 5000) { | |
| if (pattern.length > 1000) { | |
| throw new Error('Regex pattern too long (max 1000 characters)'); | |
| } | |
Fix with AI
You are an AI coding assistant.
Fix the issue in src/dsl/security.ts:45 (RIGHT).
Title: Regex length cap increased without added protections
Category: security
Severity: important
Evidence: if (pattern.length > 5000) {
throw new Error('Regex pattern too long (max 5000 characters)');
Details: Raising the cap to 5000 allows substantially larger patterns, which can increase ReDoS risk even with the basic nested-quantifier check. If longer patterns are needed, consider keeping a lower default and making this configurable, or add stronger validation (e.g., safe-regex/RE2).
Suggested change:
if (pattern.length > 1000) {
throw new Error('Regex pattern too long (max 1000 characters)');
}
|
This PR should be looked into. |
|
@grepiku hello |
|
@Iweisc hello — how can I help on this PR? |
|
@grepiku Think this PR should be merged? |
|
@Iweisc I don’t think this is ready to merge yet. The latest Grepiku Summary flags Risk: medium, with the notable issue that |
Grepiku Summary
Fix with AI
Changes expand execution budgets for factorial complexity and relax two security guards (property name blacklist and regex length cap). This may reduce test failures but increases exposure to long-running or unsafe inputs.
Risk: medium
Notable issue: Infinity disables iteration budget for factorial class (important)
Key concerns:
valueOfandtoStringfrom blocked property names weakens the property-path safety checks.What to test:
valueOfandtoStringif still required by threat model.