feat: add security fixes, quality tools, and infrastructure improvements #13

jschulte · 2025-11-26T16:15:17Z

Security fixes:

Fix prototype pollution timing in state-manager.ts (sanitize BEFORE validation)
Add temp file cleanup to prevent orphaned files accumulation
Add ReDoS protection to file-searcher.ts globToRegex function
Add file size limits to feature-analyzer.ts

New tools:

stackshift_spec_quality: Analyze spec quality with scores for completeness,
testability, and clarity
stackshift_spec_diff: Compare specifications between directories

New infrastructure:

error-handler.ts: Standardized error wrapping and safe execution utilities
logger.ts: Structured logging with levels, JSON/pretty output modes
spec-alignment.yml: GitHub Actions workflow for CI/CD spec validation

New slash commands:

/stackshift.quality: Analyze specification quality
/stackshift.diff: Compare specifications between directories

Test updates:

Updated state-manager test to verify sanitize-first behavior

All 446 tests pass.

Security fixes: - Fix prototype pollution timing in state-manager.ts (sanitize BEFORE validation) - Add temp file cleanup to prevent orphaned files accumulation - Add ReDoS protection to file-searcher.ts globToRegex function - Add file size limits to feature-analyzer.ts New tools: - stackshift_spec_quality: Analyze spec quality with scores for completeness, testability, and clarity - stackshift_spec_diff: Compare specifications between directories New infrastructure: - error-handler.ts: Standardized error wrapping and safe execution utilities - logger.ts: Structured logging with levels, JSON/pretty output modes - spec-alignment.yml: GitHub Actions workflow for CI/CD spec validation New slash commands: - /stackshift.quality: Analyze specification quality - /stackshift.diff: Compare specifications between directories Test updates: - Updated state-manager test to verify sanitize-first behavior All 446 tests pass.

Performance optimizations: - Pre-compile regex patterns at module level in gap-analyzer.ts - Move common words to Set for O(1) lookup - Use structured logger instead of console.log Error handling improvements: - Integrate error-handler.ts into analyze.ts tool - Add logging to previously silent catch blocks - Use consistent error wrapping pattern New utilities: - validation.ts: Centralized input validation with common validators - maxLength, minLength, range, pattern, oneOf - safeString, safeGlobPattern, safePath - validateOrThrow, createObjectValidator - Sanitization helpers (sanitizeString, escapeHtml) All 446 tests pass.

Copilot

Pull request overview

This PR adds comprehensive security improvements, quality analysis tools, and infrastructure enhancements to the StackShift MCP server. It addresses prototype pollution vulnerabilities, adds ReDoS protection, implements structured logging and error handling, and introduces two new specification quality tools with GitHub Actions integration.

Key Changes:

Security fixes for prototype pollution, temp file cleanup, ReDoS protection, and file size limits
New quality analysis tools (spec-quality and spec-diff) with scoring algorithms for completeness, testability, and clarity
Infrastructure modules for validation, error handling, and structured logging

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
mcp-server/src/utils/validation.ts	New validation module with string length, pattern, and safety validators
mcp-server/src/utils/state-manager.ts	Security fix to sanitize JSON before validation and add temp file cleanup
mcp-server/src/utils/logger.ts	New structured logging module with level-based filtering and environment-aware output
mcp-server/src/utils/error-handler.ts	New error handling utilities with standardized wrapping and safe execution
mcp-server/src/utils/tests/state-manager.test.ts	Updated test to verify sanitize-first behavior
mcp-server/src/tools/spec-quality.ts	New tool to analyze spec quality with scoring on completeness, testability, clarity
mcp-server/src/tools/spec-diff.ts	New tool to compare specifications between directories
mcp-server/src/tools/analyze.ts	Integration of new error handler for consistent error management
mcp-server/src/index.ts	Registration of new tools and slash commands
mcp-server/src/analyzers/utils/file-searcher.ts	Added ReDoS protection to glob pattern processing
mcp-server/src/analyzers/gap-analyzer.ts	Performance improvements with pre-compiled regex patterns
mcp-server/src/analyzers/feature-analyzer.ts	Added file size limits and improved error handling
.github/workflows/spec-alignment.yml	New GitHub Actions workflow for CI/CD spec validation
.claude/commands/stackshift.quality.md	Documentation for quality analysis slash command
.claude/commands/stackshift.diff.md	Documentation for spec diff slash command

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-26T16:20:10Z

mcp-server/src/utils/error-handler.ts

+    }
+
+    // This path is only taken when rethrow is explicitly false
+    throw wrappedError;


The function always throws regardless of the rethrow option. Line 105 is unreachable and contradicts the comment. If rethrow is false, the function should return a default value or the error should be handled differently. Either remove lines 104-105 or change the logic to return instead of throw when rethrow is false.

Suggested change

throw wrappedError;

return undefined as unknown as T;

Copilot · 2025-11-26T16:20:11Z

mcp-server/src/analyzers/gap-analyzer.ts

+    // Reset regex lastIndex for global pattern (required for reuse)
+    DEPENDENCY_PATTERN.lastIndex = 0;
+


Resetting lastIndex on a global regex is correct practice, but the regex is defined with the 'gi' flags on line 41, making it stateful. However, the code uses .match() on line 735 which doesn't use lastIndex. The .match() method with a global regex returns all matches at once. Either remove the lastIndex reset (since .match() doesn't use it) or use .exec() in a loop if you need stateful matching.

Suggested change

// Reset regex lastIndex for global pattern (required for reuse)

DEPENDENCY_PATTERN.lastIndex = 0;

Copilot · 2025-11-26T16:20:12Z

mcp-server/src/utils/validation.ts

+ * Create a safe string validator (no dangerous characters)
+ */
+export function safeString(): Validator<string> {
+  const dangerousChars = /[<>&'";\x00-\x1f]/;


The safeString validator includes semicolon as a dangerous character, but sanitizeString (line 285) doesn't remove semicolons. This inconsistency means a string could pass validation but not be properly sanitized, or vice versa. Consider aligning the character sets between validation and sanitization, or document why they differ.

Copilot · 2025-11-26T16:20:12Z

mcp-server/src/tools/spec-quality.ts

+  }
+
+  // Check for passive voice (simple heuristic)
+  const passiveMatches = content.match(/\b(is|are|was|were|be|been|being)\s+\w+ed\b/gi) || [];


This regex could cause ReDoS with pathological input containing many 'is/are/was/were' followed by non-word characters. The \w+ed pattern can backtrack excessively. Consider using a non-backtracking approach or limiting the input size more strictly (currently 2MB). A safer pattern would be: /\b(is|are|was|were|be|been|being)\s+\w+?ed\b/gi with non-greedy quantifier.

Suggested change

const passiveMatches = content.match(/\b(is|are|was|were|be|been|being)\s+\w+ed\b/gi) || [];

const passiveMatches = content.match(/\b(is|are|was|were|be|been|being)\s+\w+?ed\b/gi) || [];

Copilot · 2025-11-26T16:20:12Z

mcp-server/src/analyzers/utils/file-searcher.ts

+      .replace(/\*/g, '[^/]*?') // * matches anything except / (non-greedy)
+      .replace(/§DOUBLESTAR§/g, '.*?') // ** matches anything including / (non-greedy)


While adding non-greedy quantifiers (? instead of ) is good for ReDoS protection, the patterns [^/]? and .? still have potential for backtracking in certain edge cases (e.g., 'a*b' pattern against 'aaaaaac'). Consider documenting this limitation or using possessive quantifiers if the regex engine supports them, or validate that the non-greedy approach combined with the length/wildcard limits on lines 265-274 is sufficient protection.

Copilot · 2025-11-26T16:20:13Z

mcp-server/src/tools/spec-quality.ts

+
+  // Check for incomplete markers
+  for (const marker of INCOMPLETE_MARKERS) {
+    const count = (content.match(new RegExp(escapeRegex(marker), 'gi')) || []).length;


Creating a new RegExp inside a loop (for each marker) is inefficient. Consider pre-compiling the regexes for incomplete markers at module level, similar to how TESTABLE_PATTERNS and UNTESTABLE_PATTERNS are defined. This would improve performance when analyzing multiple specs.

Copilot · 2025-11-26T16:20:14Z

mcp-server/src/analyzers/gap-analyzer.ts

-      .filter(w => w.length > 3 && !commonWords.includes(w));
+      .replace(NON_ALPHANUM_PATTERN, ' ')
+      .split(WHITESPACE_PATTERN)
+      .filter(w => w.length > 3 && !COMMON_WORDS.has(w));


The magic number '3' for minimum word length is not documented. Consider extracting this to a named constant (e.g., MIN_KEYWORD_LENGTH = 3) at the module level to make it clear why 3-letter words are filtered out and to make it easier to adjust in the future.

Copilot · 2025-11-26T16:20:14Z

mcp-server/src/utils/validation.ts

+ * Create a safe glob pattern validator
+ */
+export function safeGlobPattern(): Validator<string> {
+  return (value: string) => {
+    // Check for pathological patterns
+    const consecutiveWildcards = (value.match(/\*[^/*]*\*/g) || []).length;
+    if (consecutiveWildcards > 3) {
+      return 'pattern has too many consecutive wildcards (potential ReDoS)';
+    }
+    if (value.length > 500) {
+      return 'pattern is too long (max 500 characters)';
+    }
+    return true;
+  };


The safeGlobPattern validator uses different logic (line 103 regex and > 3 threshold) than the file-searcher's ReDoS protection (line 271 in file-searcher.ts uses the same regex but same threshold). While they use the same threshold, having duplicate ReDoS detection logic in two places creates a maintenance burden. Consider extracting this validation to a shared utility function to ensure consistency.

Suggested change

* Create a safe glob pattern validator

*/

export function safeGlobPattern(): Validator<string> {

return (value: string) => {

// Check for pathological patterns

const consecutiveWildcards = (value.match(/\*[^/*]*\*/g) || []).length;

if (consecutiveWildcards > 3) {

return 'pattern has too many consecutive wildcards (potential ReDoS)';

}

if (value.length > 500) {

return 'pattern is too long (max 500 characters)';

}

return true;

};

* Shared logic for glob pattern ReDoS protection

*/

export function isSafeGlobPattern(value: string): true | string {

// Check for pathological patterns

const consecutiveWildcards = (value.match(/\*[^/*]*\*/g) || []).length;

if (consecutiveWildcards > 3) {

return 'pattern has too many consecutive wildcards (potential ReDoS)';

}

if (value.length > 500) {

return 'pattern is too long (max 500 characters)';

}

return true;

}

/**

* Create a safe glob pattern validator

*/

export function safeGlobPattern(): Validator<string> {

return (value: string) => isSafeGlobPattern(value);

Copilot · 2025-11-26T16:20:14Z

mcp-server/src/index.ts

 import { createConstitutionToolHandler } from './tools/create-constitution.js';
 import { createFeatureSpecsToolHandler } from './tools/create-feature-specs.js';
 import { createImplPlansToolHandler } from './tools/create-impl-plans.js';
+import { specQualityTool, executeSpecQuality } from './tools/spec-quality.js';


Unused import specQualityTool.

Suggested change

import { specQualityTool, executeSpecQuality } from './tools/spec-quality.js';

import { executeSpecQuality } from './tools/spec-quality.js';

Copilot · 2025-11-26T16:20:15Z

mcp-server/src/index.ts

 import { createFeatureSpecsToolHandler } from './tools/create-feature-specs.js';
 import { createImplPlansToolHandler } from './tools/create-impl-plans.js';
+import { specQualityTool, executeSpecQuality } from './tools/spec-quality.js';
+import { specDiffTool, executeSpecDiff } from './tools/spec-diff.js';


Unused import specDiffTool.

Suggested change

import { specDiffTool, executeSpecDiff } from './tools/spec-diff.js';

import { executeSpecDiff } from './tools/spec-diff.js';

claude added 2 commits November 24, 2025 21:14

Copilot AI review requested due to automatic review settings November 26, 2025 16:15

Copilot started reviewing on behalf of jschulte November 26, 2025 16:15 View session

Copilot finished reviewing on behalf of jschulte November 26, 2025 16:17

Copilot AI reviewed Nov 26, 2025

View reviewed changes

jschulte merged commit 8469a4c into main Nov 29, 2025
17 of 20 checks passed

jschulte deleted the claude/codebase-review-01LBVxLho4aFbMniXLniNnpH branch November 29, 2025 20:06

		// Reset regex lastIndex for global pattern (required for reuse)
		DEPENDENCY_PATTERN.lastIndex = 0;

	const passiveMatches = content.match(/\b(is\|are\|was\|were\|be\|been\|being)\s+\w+ed\b/gi) \|\| [];
	const passiveMatches = content.match(/\b(is\|are\|was\|were\|be\|been\|being)\s+\w+?ed\b/gi) \|\| [];

		.replace(/\/g, '[^/]?') // * matches anything except / (non-greedy)
		.replace(/§DOUBLESTAR§/g, '.?') // * matches anything including / (non-greedy)

	import { specQualityTool, executeSpecQuality } from './tools/spec-quality.js';
	import { executeSpecQuality } from './tools/spec-quality.js';

	import { specDiffTool, executeSpecDiff } from './tools/spec-diff.js';
	import { executeSpecDiff } from './tools/spec-diff.js';

Uh oh!

feat: add security fixes, quality tools, and infrastructure improvements #13

feat: add security fixes, quality tools, and infrastructure improvements #13

Uh oh!

Conversation

jschulte commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants