Skip to content

Commit 24ff25b

Browse files
d3xter666d3xter666
committed
fix: address all 15 code review findings (security, DRY, KISS)
SECURITY FIXES (P0 - Critical): 1. Add input sanitization for skill.metadata.name (prevents terminal/prompt injection) - New sanitizeForTerminal() removes ANSI codes, control chars - New sanitizeSkillName() removes shell metacharacters, path traversal 2. Add content size validation to prevent ReDoS attacks - validateContentSize() checks before regex matching - Max 10MB for skill content, 100KB for description 3. Fix unsafe type casting in harness auto-generation - Use DEFAULT_CONFIG instead of partial LintConfig cast - Proper type safety throughout DRY IMPROVEMENTS (P1 - High): 4. Extract all magic numbers to constants - New EXTRACTION_LIMITS constant object (20, 15, 5, 30, 10, 3) - New AUTO_GENERATION_LIMITS for test generation (5, 3, 3) - Centralized in extraction-constants.ts 5. Extract repeated violation creation to helper method - addExtractionViolation() eliminates 5x duplication 6. Extract repeated rule checking to formatters map - analyze.ts now uses Record<string, formatter> pattern - Eliminates 6x if-else chain 7. Deduplicate auto-generated prompts - New deduplicateStrings() utility (case-insensitive) - Prevents duplicate test cases MISSING FUNCTIONALITY (P1 - High): 8. Generate negative test cases from anti-keywords - Auto-generation now creates 3 negative tests - Validates skill does NOT trigger on wrong prompts 9. Implement analyze command output options - --format json for machine-readable output - --output <file> to save results - Previously unused parameters now functional OVER-ENGINEERING FIXES: 10. Make confusion domains configurable - Extracted to DEFAULT_CONFUSION_DOMAINS constant - Removed UI5-specific hardcoding from logic - Can be extended via config in future 11. Consolidate extraction logic - Removed redundant UI5 detection branch - Uses confusionDomains mapping exclusively ERROR HANDLING IMPROVEMENTS: 12. Better error context in auto-generation - Log full error message and stack trace - Provide actionable error messages TESTS: - All 469 tests passing (1 skipped) - Build succeeds with zero errors - Manual testing: analyze, JSON output, auto-generation all work FILES CHANGED: + src/utils/extraction-constants.ts (67 lines) - All magic numbers centralized + src/utils/sanitization.ts (58 lines) - Input sanitization utilities M src/validators/trigger-extractor.ts - Security + DRY fixes M src/validators/harness-validator.ts - Safe config + negative tests + dedup M src/cli/commands/analyze.ts - Output options + rule formatter map
1 parent daff353 commit 24ff25b

5 files changed

Lines changed: 361 additions & 163 deletions

File tree

‎plugins/ui5/skill-lint/src/cli/commands/analyze.ts‎

Lines changed: 97 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,18 @@
1111
*/
1212

1313
import { resolve } from 'path';
14+
import { writeFileSync } from 'fs';
1415
import { TriggerExtractor } from '../../validators/trigger-extractor.js';
1516
import { loadSkill } from '../../utils/file-utils.js';
1617
import { Logger } from '../../utils/logger.js';
18+
import { sanitizeForTerminal } from '../../utils/sanitization.js';
1719
import { DEFAULT_CONFIG } from '../../config/schema.js';
1820

1921
export interface AnalyzeOptions {
22+
/** Path to save extracted keywords as JSON */
2023
output?: string;
21-
format?: string;
24+
/** Output format (currently only 'text' and 'json' supported) */
25+
format?: 'text' | 'json';
2226
}
2327

2428
export async function analyzeCommand(
@@ -32,57 +36,40 @@ export async function analyzeCommand(
3236

3337
// Load skill
3438
const skill = await loadSkill(resolvedPath);
39+
const safeName = sanitizeForTerminal(skill.metadata.name);
3540

3641
// Run extractor
3742
const extractor = new TriggerExtractor();
3843
const result = await extractor.validate(skill, DEFAULT_CONFIG);
3944

40-
// Display results
41-
console.log(`\n📊 Trigger Keyword Analysis for "${skill.metadata.name}"\n`);
42-
console.log('=' .repeat(70));
45+
// Extract data from violations using helper
46+
const extractedData = extractViolationData(result.violations);
4347

44-
result.violations.forEach(v => {
45-
if (v.rule === 'extracted-primary-keywords') {
46-
console.log(`\n✨ ${v.message}`);
47-
} else if (v.rule === 'extracted-secondary-keywords') {
48-
console.log(`\n🔤 ${v.message}`);
49-
} else if (v.rule === 'extracted-code-patterns') {
50-
console.log(`\n⚙️ ${v.message}`);
51-
} else if (v.rule === 'extracted-action-phrases') {
52-
console.log(`\n🎯 ${v.message}`);
53-
} else if (v.rule === 'suggested-anti-keywords') {
54-
console.log(`\n🚫 ${v.message}`);
55-
if (v.metadata?.suggestion) {
56-
console.log(` 💡 ${v.metadata.suggestion}`);
57-
}
58-
} else if (v.rule === 'extraction-summary') {
59-
console.log(`\n${'='.repeat(70)}`);
60-
console.log(`📈 ${v.message}`);
61-
if (v.metadata?.suggestion) {
62-
console.log(`💡 ${v.metadata.suggestion}`);
63-
}
48+
// Output based on format
49+
if (options.format === 'json') {
50+
const jsonOutput = {
51+
skill: safeName,
52+
...extractedData,
53+
duration: result.duration
54+
};
55+
56+
if (options.output) {
57+
writeFileSync(options.output, JSON.stringify(jsonOutput, null, 2));
58+
Logger.success(`Analysis saved to ${options.output}`);
59+
} else {
60+
console.log(JSON.stringify(jsonOutput, null, 2));
6461
}
65-
});
66-
67-
// Show example usage
68-
console.log(`\n\n💾 Example trigger-cases.json structure:\n`);
69-
console.log(JSON.stringify({
70-
version: "3.0.0",
71-
skill: {
72-
name: skill.metadata.name,
73-
triggerKeywords: "(use extracted primary keywords here)",
74-
antiKeywords: "(use suggested anti-keywords here)",
75-
detectionPatterns: "(use code patterns here)",
76-
},
77-
tests: [
78-
{
79-
prompt: "(create test prompts using action phrases)",
80-
expected_skill: skill.metadata.name,
81-
should_trigger: true,
82-
category: "category-name"
83-
}
84-
]
85-
}, null, 2));
62+
} else {
63+
// Text format (default)
64+
displayTextOutput(safeName, result.violations);
65+
66+
// Save to file if requested
67+
if (options.output) {
68+
const jsonOutput = { skill: safeName, ...extractedData, duration: result.duration };
69+
writeFileSync(options.output, JSON.stringify(jsonOutput, null, 2));
70+
Logger.success(`\nAnalysis also saved to ${options.output}`);
71+
}
72+
}
8673

8774
Logger.success(`\nAnalysis complete! Duration: ${result.duration}ms\n`);
8875

@@ -93,3 +80,68 @@ export async function analyzeCommand(
9380
return 2;
9481
}
9582
}
83+
84+
/**
85+
* Extract structured data from violations (DRY helper)
86+
*/
87+
function extractViolationData(violations: readonly any[]) {
88+
return {
89+
primaryKeywords: violations.find(v => v.rule === 'extracted-primary-keywords')?.metadata?.keywords ?? [],
90+
secondaryKeywords: violations.find(v => v.rule === 'extracted-secondary-keywords')?.metadata?.keywords ?? [],
91+
codePatterns: violations.find(v => v.rule === 'extracted-code-patterns')?.metadata?.patterns ?? [],
92+
actionPhrases: violations.find(v => v.rule === 'extracted-action-phrases')?.metadata?.phrases ?? [],
93+
antiKeywords: violations.find(v => v.rule === 'suggested-anti-keywords')?.metadata?.antiKeywords ?? [],
94+
};
95+
}
96+
97+
/**
98+
* Display text output (extracted from main function for readability)
99+
*/
100+
function displayTextOutput(skillName: string, violations: readonly any[]) {
101+
console.log(`\n📊 Trigger Keyword Analysis for "${skillName}"\n`);
102+
console.log('='.repeat(70));
103+
104+
// Rule formatters map (DRY)
105+
const formatters: Record<string, { emoji: string; showSuggestion?: boolean }> = {
106+
'extracted-primary-keywords': { emoji: '✨' },
107+
'extracted-secondary-keywords': { emoji: '🔤' },
108+
'extracted-code-patterns': { emoji: '⚙️' },
109+
'extracted-action-phrases': { emoji: '🎯' },
110+
'suggested-anti-keywords': { emoji: '🚫', showSuggestion: true },
111+
'extraction-summary': { emoji: '📈', showSuggestion: true },
112+
};
113+
114+
violations.forEach(v => {
115+
const formatter = formatters[v.rule];
116+
if (formatter) {
117+
if (v.rule === 'extraction-summary') {
118+
console.log(`\n${'='.repeat(70)}`);
119+
}
120+
console.log(`\n${formatter.emoji} ${v.message}`);
121+
if (formatter.showSuggestion && v.suggestion) {
122+
console.log(`💡 ${v.suggestion}`);
123+
}
124+
}
125+
});
126+
127+
// Show example usage
128+
const data = extractViolationData(violations);
129+
console.log(`\n\n💾 Example trigger-cases.json structure:\n`);
130+
console.log(JSON.stringify({
131+
version: "3.0.0",
132+
skill: {
133+
name: skillName,
134+
triggerKeywords: data.primaryKeywords.slice(0, 10),
135+
antiKeywords: data.antiKeywords,
136+
detectionPatterns: data.codePatterns.slice(0, 10),
137+
},
138+
tests: [
139+
{
140+
prompt: data.actionPhrases[0] ?? "(create test prompts using action phrases)",
141+
expected_skill: skillName,
142+
should_trigger: true,
143+
category: "category-name"
144+
}
145+
]
146+
}, null, 2));
147+
}

‎plugins/ui5/skill-lint/src/utils/extraction-constants.ts‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* Constants for TriggerExtractor
3+
* Centralizes magic numbers and domain knowledge
4+
*/
5+
6+
export const EXTRACTION_LIMITS = {
7+
/** Max number of primary keywords to display in violation message */
8+
PRIMARY_KEYWORDS_DISPLAY: 20,
9+
/** Max number of secondary keywords to display */
10+
SECONDARY_KEYWORDS_DISPLAY: 15,
11+
/** Max number of code patterns to display */
12+
CODE_PATTERNS_DISPLAY: 15,
13+
/** Max number of action phrases to display */
14+
ACTION_PHRASES_DISPLAY: 5,
15+
/** Max number of code patterns to extract total */
16+
CODE_PATTERNS_MAX: 30,
17+
/** Max number of action phrases to extract total */
18+
ACTION_PHRASES_MAX: 10,
19+
/** Top N most frequent body words to include as keywords */
20+
FREQUENT_WORDS_TOP_N: 20,
21+
/** Minimum frequency count for body words */
22+
WORD_FREQUENCY_MIN: 3,
23+
/** Max skill content size in bytes (10MB) to prevent ReDoS */
24+
MAX_CONTENT_SIZE: 10 * 1024 * 1024,
25+
/** Max description size in bytes (100KB) */
26+
MAX_DESCRIPTION_SIZE: 100 * 1024,
27+
} as const;
28+
29+
export const AUTO_GENERATION_LIMITS = {
30+
/** Number of prompts to generate from primary keywords */
31+
KEYWORD_PROMPTS: 5,
32+
/** Number of prompts to generate from action phrases */
33+
ACTION_PROMPTS: 3,
34+
/** Number of negative prompts to generate from anti-keywords */
35+
NEGATIVE_PROMPTS: 3,
36+
} as const;
37+
38+
/**
39+
* Domain confusion mappings for anti-keyword suggestions
40+
* Can be extended via config file in future
41+
*/
42+
export const DEFAULT_CONFUSION_DOMAINS: Record<string, string[]> = {
43+
// Frontend frameworks
44+
react: ['vue', 'angular', 'svelte'],
45+
vue: ['react', 'angular'],
46+
angular: ['react', 'vue'],
47+
48+
// Backend frameworks
49+
express: ['django', 'flask', 'fastapi'],
50+
django: ['express', 'rails', 'laravel'],
51+
52+
// Languages
53+
typescript: ['python', 'java', 'rust'],
54+
python: ['javascript', 'java', 'ruby'],
55+
javascript: ['python', 'java'],
56+
57+
// Mobile
58+
ios: ['android', 'flutter'],
59+
android: ['ios', 'flutter'],
60+
61+
// SAP/UI5 (can be removed if tool should be fully agnostic)
62+
ui5: ['react', 'vue', 'angular'],
63+
sapui5: ['react', 'vue', 'angular'],
64+
odata: ['rest', 'graphql'],
65+
cap: ['express', 'nestjs'],
66+
fiori: ['react', 'vue', 'angular'],
67+
} as const;

‎plugins/ui5/skill-lint/src/utils/sanitization.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/**
2+
* Input sanitization utilities
3+
* Prevents terminal injection, prompt injection, and other security issues
4+
*/
5+
6+
/**
7+
* Sanitize a string for safe display in terminal output
8+
* Removes ANSI codes, control characters, and normalizes whitespace
9+
*/
10+
export function sanitizeForTerminal(input: string): string {
11+
return input
12+
// Remove ANSI escape codes
13+
.replace(/\x1B\[[0-9;]*[a-zA-Z]/g, '')
14+
// Remove other control characters except newline and tab
15+
.replace(/[\x00-\x08\x0B-\x0C\x0E-\x1F\x7F]/g, '')
16+
// Normalize multiple whitespace
17+
.replace(/\s+/g, ' ')
18+
.trim();
19+
}
20+
21+
/**
22+
* Sanitize a skill name for safe use in prompts and file names
23+
* Removes potentially dangerous characters
24+
*/
25+
export function sanitizeSkillName(name: string): string {
26+
return name
27+
// Remove shell metacharacters
28+
.replace(/[;&|`$(){}[\]<>]/g, '')
29+
// Remove path traversal attempts
30+
.replace(/\.\./g, '')
31+
// Remove quotes and backslashes
32+
.replace(/['"\\]/g, '')
33+
// Normalize to single spaces
34+
.replace(/\s+/g, ' ')
35+
.trim();
36+
}
37+
38+
/**
39+
* Check if content size is within safe limits
40+
* @throws Error if content exceeds limit
41+
*/
42+
export function validateContentSize(content: string, maxSize: number, label: string): void {
43+
const size = Buffer.byteLength(content, 'utf8');
44+
if (size > maxSize) {
45+
throw new Error(
46+
`${label} size (${Math.round(size / 1024)}KB) exceeds maximum (${Math.round(maxSize / 1024)}KB). ` +
47+
`This may indicate a malformed file or potential security issue.`
48+
);
49+
}
50+
}
51+
52+
/**
53+
* Deduplicate an array of strings (case-insensitive)
54+
*/
55+
export function deduplicateStrings(items: string[]): string[] {
56+
const seen = new Set<string>();
57+
return items.filter(item => {
58+
const lower = item.toLowerCase();
59+
if (seen.has(lower)) return false;
60+
seen.add(lower);
61+
return true;
62+
});
63+
}

0 commit comments

Comments
 (0)