Skip to content

Commit c70a881

Browse files
tidy-devCopilot
andcommitted
Security hardening, bug fixes, and unit tests
Security: - Remove shell(cat/head/tail/grep/wc/jq) and --allow-all-paths from Copilot CLI Only shell(git) is granted — prevents secret exfiltration via prompt injection - Pass minimal env to subprocess (PATH, HOME, GITHUB_TOKEN only) No longer spreads process.env which leaked all secrets - Add prompt injection armor: security notice, <pr-data> delimiters, code-fenced PR bodies, heading/tag sanitization - Remove dead temp file write that left sensitive data on disk Bug fixes: - Fix GITHUB_TOKEN fallback: COPILOT_GITHUB_TOKEN || GITHUB_TOKEN || '' - Fix JSON parser: search for valid {"entries"} objects instead of greedy regex Handles AI output with braces before the JSON block - Fix squash regex in github-api strategy: use $ anchor + first line only - Warn when GitHub compare API may truncate at 250 commits - Add prompt size warning when over 100K chars - Add 5-minute timeout for Copilot CLI execution - Validate entries field is an array before returning Tests (34 passing): - outputs.test.ts: parseOutput edge cases, formatAsMarkdown grouping - prompt.test.ts: security armor, sanitization, truncation, code fences - inputs.test.ts: input parsing, defaults, validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 11915c6 commit c70a881

17 files changed

Lines changed: 830 additions & 156 deletions

__tests__/inputs.test.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// Mock @actions/core before any imports
2+
jest.mock('@actions/core', () => ({
3+
warning: jest.fn(),
4+
debug: jest.fn(),
5+
info: jest.fn(),
6+
getInput: jest.fn()
7+
}))
8+
9+
import * as core from '@actions/core'
10+
import {getInputs} from '../src/inputs'
11+
12+
const mockGetInput = core.getInput as jest.MockedFunction<typeof core.getInput>
13+
14+
describe('getInputs', () => {
15+
beforeEach(() => {
16+
jest.clearAllMocks()
17+
// Default mock implementation — no inputs set
18+
mockGetInput.mockReturnValue('')
19+
})
20+
21+
it('requires base-ref', () => {
22+
mockGetInput.mockImplementation((name: string) => {
23+
if (name === 'base-ref') return ''
24+
return ''
25+
})
26+
expect(() => getInputs()).toThrow('base-ref is required')
27+
})
28+
29+
it('parses all inputs correctly', () => {
30+
mockGetInput.mockImplementation((name: string) => {
31+
switch (name) {
32+
case 'base-ref':
33+
return 'v1.0'
34+
case 'head-ref':
35+
return 'v2.0'
36+
case 'instructions':
37+
return 'guide.md'
38+
case 'model':
39+
return 'gpt-4'
40+
case 'pr-strategy':
41+
return 'github-api'
42+
default:
43+
return ''
44+
}
45+
})
46+
const inputs = getInputs()
47+
expect(inputs.baseRef).toBe('v1.0')
48+
expect(inputs.headRef).toBe('v2.0')
49+
expect(inputs.model).toBe('gpt-4')
50+
expect(inputs.prStrategy).toBe('github-api')
51+
})
52+
53+
it('defaults head-ref to HEAD', () => {
54+
mockGetInput.mockImplementation((name: string) => {
55+
if (name === 'base-ref') return 'v1.0'
56+
return ''
57+
})
58+
const inputs = getInputs()
59+
expect(inputs.headRef).toBe('HEAD')
60+
})
61+
62+
it('defaults pr-strategy to merge-commits', () => {
63+
mockGetInput.mockImplementation((name: string) => {
64+
if (name === 'base-ref') return 'v1.0'
65+
return ''
66+
})
67+
const inputs = getInputs()
68+
expect(inputs.prStrategy).toBe('merge-commits')
69+
})
70+
71+
it('rejects invalid pr-strategy', () => {
72+
mockGetInput.mockImplementation((name: string) => {
73+
if (name === 'base-ref') return 'v1.0'
74+
if (name === 'pr-strategy') return 'invalid'
75+
return ''
76+
})
77+
expect(() => getInputs()).toThrow('Invalid pr-strategy')
78+
})
79+
})

__tests__/outputs.test.ts

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
// Mock @actions/core before any imports
2+
jest.mock('@actions/core', () => ({
3+
warning: jest.fn(),
4+
debug: jest.fn(),
5+
info: jest.fn(),
6+
setOutput: jest.fn()
7+
}))
8+
9+
import {parseOutput, formatAsMarkdown, ParsedOutput} from '../src/outputs'
10+
11+
describe('parseOutput', () => {
12+
it('parses clean JSON output', () => {
13+
const input = JSON.stringify({
14+
entries: [
15+
{description: 'Add feature X', pr: 123, author: 'octocat'}
16+
],
17+
uncertainEntries: [],
18+
skippedPRs: []
19+
})
20+
const result = parseOutput(input)
21+
expect(result.entries).toHaveLength(1)
22+
expect(result.entries[0].description).toBe('Add feature X')
23+
expect(result.entries[0].pr).toBe(123)
24+
})
25+
26+
it('finds JSON embedded in surrounding text', () => {
27+
const input =
28+
'Here is my analysis of the PRs:\n\n' +
29+
JSON.stringify({
30+
entries: [{description: 'Fix bug', pr: 456, author: 'mona'}],
31+
uncertainEntries: []
32+
}) +
33+
'\n\nDone!'
34+
const result = parseOutput(input)
35+
expect(result.entries).toHaveLength(1)
36+
expect(result.entries[0].pr).toBe(456)
37+
})
38+
39+
it('handles text with braces before the actual JSON', () => {
40+
// This was a known bug — greedy regex matched the wrong opening brace
41+
const input =
42+
'I analyzed {5 changes} and found interesting results.\n\n' +
43+
JSON.stringify({
44+
entries: [{description: 'Update deps', pr: 789, author: 'bot'}],
45+
uncertainEntries: []
46+
})
47+
const result = parseOutput(input)
48+
expect(result.entries).toHaveLength(1)
49+
expect(result.entries[0].pr).toBe(789)
50+
})
51+
52+
it('handles JSON with code blocks containing braces', () => {
53+
const input =
54+
'The diff shows `if (x) { return }` patterns.\n' +
55+
JSON.stringify({
56+
entries: [{description: 'Refactor conditionals', pr: 100, author: 'dev'}],
57+
uncertainEntries: []
58+
})
59+
const result = parseOutput(input)
60+
expect(result.entries).toHaveLength(1)
61+
})
62+
63+
it('returns empty results for no JSON', () => {
64+
const result = parseOutput('No JSON here at all')
65+
expect(result.entries).toHaveLength(0)
66+
expect(result.uncertainEntries).toHaveLength(0)
67+
expect(result.skippedPRs).toHaveLength(0)
68+
})
69+
70+
it('returns empty results for invalid JSON with "entries"', () => {
71+
const result = parseOutput('{entries: not valid json}')
72+
expect(result.entries).toHaveLength(0)
73+
})
74+
75+
it('returns empty results when entries is not an array', () => {
76+
const input = JSON.stringify({entries: 'not an array'})
77+
const result = parseOutput(input)
78+
expect(result.entries).toHaveLength(0)
79+
})
80+
81+
it('handles missing uncertainEntries and skippedPRs gracefully', () => {
82+
const input = JSON.stringify({
83+
entries: [{description: 'Test', pr: 1, author: 'a'}]
84+
})
85+
const result = parseOutput(input)
86+
expect(result.entries).toHaveLength(1)
87+
expect(result.uncertainEntries).toHaveLength(0)
88+
expect(result.skippedPRs).toHaveLength(0)
89+
})
90+
91+
it('handles nested JSON strings in descriptions', () => {
92+
const input = JSON.stringify({
93+
entries: [
94+
{
95+
description: 'Parse {"key": "value"} objects correctly',
96+
pr: 42,
97+
author: 'dev'
98+
}
99+
],
100+
uncertainEntries: []
101+
})
102+
const result = parseOutput(input)
103+
expect(result.entries).toHaveLength(1)
104+
expect(result.entries[0].pr).toBe(42)
105+
})
106+
107+
it('parses entries with tags', () => {
108+
const input = JSON.stringify({
109+
entries: [
110+
{
111+
description: 'Add new command',
112+
pr: 10,
113+
author: 'dev',
114+
tag: '✨ Features'
115+
}
116+
],
117+
uncertainEntries: []
118+
})
119+
const result = parseOutput(input)
120+
expect(result.entries[0].tag).toBe('✨ Features')
121+
})
122+
123+
it('handles empty string input', () => {
124+
const result = parseOutput('')
125+
expect(result.entries).toHaveLength(0)
126+
})
127+
})
128+
129+
describe('formatAsMarkdown', () => {
130+
it('formats flat entries without tags', () => {
131+
const output: ParsedOutput = {
132+
entries: [
133+
{description: 'Add feature X', pr: 123, author: 'octocat'},
134+
{description: 'Fix bug Y', pr: 456, author: 'mona'}
135+
],
136+
uncertainEntries: [],
137+
skippedPRs: []
138+
}
139+
const md = formatAsMarkdown(output)
140+
expect(md).toBe(
141+
'- Add feature X (#123)\n- Fix bug Y (#456)'
142+
)
143+
})
144+
145+
it('groups entries by tag when tags are present', () => {
146+
const output: ParsedOutput = {
147+
entries: [
148+
{description: 'New command', pr: 1, author: 'a', tag: '✨ Features'},
149+
{description: 'Fix crash', pr: 2, author: 'b', tag: '🐛 Fixes'},
150+
{description: 'Another feature', pr: 3, author: 'c', tag: '✨ Features'}
151+
],
152+
uncertainEntries: [],
153+
skippedPRs: []
154+
}
155+
const md = formatAsMarkdown(output)
156+
expect(md).toContain('### ✨ Features')
157+
expect(md).toContain('### 🐛 Fixes')
158+
expect(md).toContain('- New command (#1)')
159+
expect(md).toContain('- Another feature (#3)')
160+
expect(md).toContain('- Fix crash (#2)')
161+
})
162+
163+
it('puts untagged entries under "Other" when some entries have tags', () => {
164+
const output: ParsedOutput = {
165+
entries: [
166+
{description: 'Tagged', pr: 1, author: 'a', tag: '✨ Features'},
167+
{description: 'Untagged', pr: 2, author: 'b'}
168+
],
169+
uncertainEntries: [],
170+
skippedPRs: []
171+
}
172+
const md = formatAsMarkdown(output)
173+
expect(md).toContain('### ✨ Features')
174+
expect(md).toContain('### Other')
175+
expect(md).toContain('- Untagged (#2)')
176+
})
177+
178+
it('includes uncertain entries section', () => {
179+
const output: ParsedOutput = {
180+
entries: [{description: 'Sure thing', pr: 1, author: 'a'}],
181+
uncertainEntries: [
182+
{
183+
description: 'Maybe this?',
184+
pr: 2,
185+
author: 'b',
186+
reason: 'PR body was empty'
187+
}
188+
],
189+
skippedPRs: []
190+
}
191+
const md = formatAsMarkdown(output)
192+
expect(md).toContain('### Needs Review')
193+
expect(md).toContain('Maybe this?')
194+
expect(md).toContain('_PR body was empty_')
195+
})
196+
197+
it('handles empty entries', () => {
198+
const output: ParsedOutput = {
199+
entries: [],
200+
uncertainEntries: [],
201+
skippedPRs: []
202+
}
203+
const md = formatAsMarkdown(output)
204+
expect(md).toBe('')
205+
})
206+
207+
it('preserves tag insertion order', () => {
208+
const output: ParsedOutput = {
209+
entries: [
210+
{description: 'Fix A', pr: 1, author: 'a', tag: '🐛 Fixes'},
211+
{description: 'Feature B', pr: 2, author: 'b', tag: '✨ Features'},
212+
{description: 'Fix C', pr: 3, author: 'c', tag: '🐛 Fixes'}
213+
],
214+
uncertainEntries: [],
215+
skippedPRs: []
216+
}
217+
const md = formatAsMarkdown(output)
218+
// Fixes should come before Features because first entry was a Fix
219+
const fixesIdx = md.indexOf('### 🐛 Fixes')
220+
const featuresIdx = md.indexOf('### ✨ Features')
221+
expect(fixesIdx).toBeLessThan(featuresIdx)
222+
})
223+
})

0 commit comments

Comments
 (0)