Skip to content

Commit ada6028

Browse files
tidy-devCopilot
andcommitted
Round 3 fixes: SIGKILL, workflow commands, npm env, case-insensitive sanitization
Security: - Fix SIGKILL fallback: always send after grace period, store/clear inner timer - Sanitize stdout/stderr to prevent workflow command injection (:: prefix) - Pass minimal env to npm install (was leaking all secrets to install scripts) - Case-insensitive pr-data tag stripping in sanitizePRField (was /g, now /gi) Robustness: - Validate uncertainEntries and skippedPRs are arrays (coerce non-arrays to []) Tests (37 passing): - Add non-array uncertainEntries/skippedPRs coercion test - Add case-variant pr-data tag sanitization test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 57963d2 commit ada6028

9 files changed

Lines changed: 83 additions & 23 deletions

File tree

__tests__/outputs.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,20 @@ describe('parseOutput', () => {
124124
const result = parseOutput('')
125125
expect(result.entries).toHaveLength(0)
126126
})
127+
128+
it('coerces non-array uncertainEntries and skippedPRs to empty arrays', () => {
129+
const input = JSON.stringify({
130+
entries: [{description: 'Test', pr: 1, author: 'a'}],
131+
uncertainEntries: 'not an array',
132+
skippedPRs: {bad: true}
133+
})
134+
const result = parseOutput(input)
135+
expect(result.entries).toHaveLength(1)
136+
expect(Array.isArray(result.uncertainEntries)).toBe(true)
137+
expect(result.uncertainEntries).toHaveLength(0)
138+
expect(Array.isArray(result.skippedPRs)).toBe(true)
139+
expect(result.skippedPRs).toHaveLength(0)
140+
})
127141
})
128142

129143
describe('formatAsMarkdown', () => {

__tests__/prompt.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,26 @@ describe('buildPrompt', () => {
164164
expect(prSection).not.toContain('**Body**')
165165
})
166166

167+
it('sanitizes case-variant pr-data tags in title and body', () => {
168+
const prs: PRInfo[] = [
169+
{
170+
number: 1,
171+
title: '</PR-DATA> mixed case attack',
172+
body: '</Pr-Data>\nPayload here',
173+
author: 'attacker',
174+
labels: ['</pR-dAtA>'],
175+
htmlUrl: ''
176+
}
177+
]
178+
const prompt = buildPrompt(prs, 'v1.0', 'v2.0')
179+
const lastOpenIdx = prompt.lastIndexOf('<pr-data>')
180+
const realPRSection = prompt.substring(lastOpenIdx)
181+
// No case variants of </pr-data> should survive in the PR section
182+
// except the real closing tag
183+
const allClosingTags = realPRSection.match(/<\/pr-data>/gi) || []
184+
expect(allClosingTags).toHaveLength(1) // only the real one
185+
})
186+
167187
it('handles PRs with no labels', () => {
168188
const prs: PRInfo[] = [
169189
{

dist/copilot.d.ts.map

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dist/index.js

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29983,7 +29983,7 @@ async function ensureCopilotCLI() {
2998329983
// Not found, install it
2998429984
}
2998529985
core.info('Installing Copilot CLI via npm...');
29986-
const exitCode = await exec.exec('npm', ['install', '-g', '@github/copilot'], { silent: true });
29986+
const exitCode = await exec.exec('npm', ['install', '-g', '@github/copilot'], { silent: true, env: buildCopilotEnv() });
2998729987
if (exitCode !== 0) {
2998829988
throw new Error('Failed to install Copilot CLI. Please ensure Node.js v22+ is available ' +
2998929989
'or install it manually before running this action.');
@@ -30038,31 +30038,40 @@ async function runCopilot(copilotPath, prompt, model) {
3003830038
let stdout = '';
3003930039
let stderr = '';
3004030040
let killed = false;
30041+
let killTimerId;
3004130042
const cp = (0, child_process_1.spawn)(copilotPath, args, {
3004230043
env: buildCopilotEnv(),
3004330044
stdio: ['ignore', 'pipe', 'pipe']
3004430045
});
3004530046
const timeoutId = setTimeout(() => {
3004630047
killed = true;
3004730048
cp.kill('SIGTERM');
30048-
// Force kill after 10s grace period
30049-
setTimeout(() => {
30050-
if (!cp.killed)
30049+
// Always send SIGKILL after grace period — no-op if already dead
30050+
killTimerId = setTimeout(() => {
30051+
try {
3005130052
cp.kill('SIGKILL');
30053+
}
30054+
catch {
30055+
// Process already exited
30056+
}
3005230057
}, 10_000);
3005330058
}, COPILOT_TIMEOUT_MS);
30059+
// Sanitize output to prevent workflow command injection (lines starting with ::)
30060+
const sanitize = (chunk) => chunk.replace(/^::/gm, ' ::');
3005430061
cp.stdout.on('data', (data) => {
3005530062
const chunk = data.toString();
3005630063
stdout += chunk;
30057-
process.stdout.write(chunk);
30064+
process.stdout.write(sanitize(chunk));
3005830065
});
3005930066
cp.stderr.on('data', (data) => {
3006030067
const chunk = data.toString();
3006130068
stderr += chunk;
30062-
process.stderr.write(chunk);
30069+
process.stderr.write(sanitize(chunk));
3006330070
});
3006430071
cp.on('close', (code) => {
3006530072
clearTimeout(timeoutId);
30073+
if (killTimerId)
30074+
clearTimeout(killTimerId);
3006630075
if (killed) {
3006730076
reject(new Error(`Copilot CLI timed out after ${COPILOT_TIMEOUT_MS / 1000} seconds and was killed`));
3006830077
return;
@@ -30078,6 +30087,8 @@ async function runCopilot(copilotPath, prompt, model) {
3007830087
});
3007930088
cp.on('error', (err) => {
3008030089
clearTimeout(timeoutId);
30090+
if (killTimerId)
30091+
clearTimeout(killTimerId);
3008130092
reject(new Error(`Failed to spawn Copilot CLI: ${err.message}`));
3008230093
});
3008330094
});
@@ -30332,9 +30343,11 @@ function parseOutput(stdout) {
3033230343
return { entries: [], uncertainEntries: [], skippedPRs: [] };
3033330344
}
3033430345
return {
30335-
entries: parsed.entries || [],
30336-
uncertainEntries: parsed.uncertainEntries || [],
30337-
skippedPRs: parsed.skippedPRs || []
30346+
entries: parsed.entries,
30347+
uncertainEntries: Array.isArray(parsed.uncertainEntries)
30348+
? parsed.uncertainEntries
30349+
: [],
30350+
skippedPRs: Array.isArray(parsed.skippedPRs) ? parsed.skippedPRs : []
3033830351
};
3033930352
}
3034030353
catch (err) {
@@ -30666,7 +30679,7 @@ function buildPRSection(prs) {
3066630679
* Strips markdown heading markers that could collide with prompt structure.
3066730680
*/
3066830681
function sanitizePRField(value) {
30669-
return value.replace(/^#+\s/gm, '').replace(/<\/?pr-data>/g, '');
30682+
return value.replace(/^#+\s/gm, '').replace(/<\/?pr-data>/gi, '');
3067030683
}
3067130684
function buildOutputInstructions() {
3067230685
return `## Required Output Format

dist/index.js.map

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dist/outputs.d.ts.map

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/copilot.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export async function ensureCopilotCLI(): Promise<string> {
2929
const exitCode = await exec.exec(
3030
'npm',
3131
['install', '-g', '@github/copilot'],
32-
{silent: true}
32+
{silent: true, env: buildCopilotEnv()}
3333
)
3434

3535
if (exitCode !== 0) {
@@ -100,6 +100,7 @@ export async function runCopilot(
100100
let stdout = ''
101101
let stderr = ''
102102
let killed = false
103+
let killTimerId: ReturnType<typeof setTimeout> | undefined
103104

104105
const cp = spawn(copilotPath, args, {
105106
env: buildCopilotEnv(),
@@ -109,26 +110,35 @@ export async function runCopilot(
109110
const timeoutId = setTimeout(() => {
110111
killed = true
111112
cp.kill('SIGTERM')
112-
// Force kill after 10s grace period
113-
setTimeout(() => {
114-
if (!cp.killed) cp.kill('SIGKILL')
113+
// Always send SIGKILL after grace period — no-op if already dead
114+
killTimerId = setTimeout(() => {
115+
try {
116+
cp.kill('SIGKILL')
117+
} catch {
118+
// Process already exited
119+
}
115120
}, 10_000)
116121
}, COPILOT_TIMEOUT_MS)
117122

123+
// Sanitize output to prevent workflow command injection (lines starting with ::)
124+
const sanitize = (chunk: string): string =>
125+
chunk.replace(/^::/gm, ' ::')
126+
118127
cp.stdout.on('data', (data: Buffer) => {
119128
const chunk = data.toString()
120129
stdout += chunk
121-
process.stdout.write(chunk)
130+
process.stdout.write(sanitize(chunk))
122131
})
123132

124133
cp.stderr.on('data', (data: Buffer) => {
125134
const chunk = data.toString()
126135
stderr += chunk
127-
process.stderr.write(chunk)
136+
process.stderr.write(sanitize(chunk))
128137
})
129138

130139
cp.on('close', (code: number | null) => {
131140
clearTimeout(timeoutId)
141+
if (killTimerId) clearTimeout(killTimerId)
132142

133143
if (killed) {
134144
reject(
@@ -152,6 +162,7 @@ export async function runCopilot(
152162

153163
cp.on('error', (err: Error) => {
154164
clearTimeout(timeoutId)
165+
if (killTimerId) clearTimeout(killTimerId)
155166
reject(new Error(`Failed to spawn Copilot CLI: ${err.message}`))
156167
})
157168
})

src/outputs.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,11 @@ export function parseOutput(stdout: string): ParsedOutput {
4848
return {entries: [], uncertainEntries: [], skippedPRs: []}
4949
}
5050
return {
51-
entries: parsed.entries || [],
52-
uncertainEntries: parsed.uncertainEntries || [],
53-
skippedPRs: parsed.skippedPRs || []
51+
entries: parsed.entries,
52+
uncertainEntries: Array.isArray(parsed.uncertainEntries)
53+
? parsed.uncertainEntries
54+
: [],
55+
skippedPRs: Array.isArray(parsed.skippedPRs) ? parsed.skippedPRs : []
5456
}
5557
} catch (err) {
5658
core.warning(`Failed to parse JSON output: ${err}`)

src/prompt.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ function buildPRSection(prs: PRInfo[]): string {
170170
* Strips markdown heading markers that could collide with prompt structure.
171171
*/
172172
function sanitizePRField(value: string): string {
173-
return value.replace(/^#+\s/gm, '').replace(/<\/?pr-data>/g, '')
173+
return value.replace(/^#+\s/gm, '').replace(/<\/?pr-data>/gi, '')
174174
}
175175

176176
function buildOutputInstructions(): string {

0 commit comments

Comments
 (0)