Skip to content

Commit f9c9b7a

Browse files
fix(status): apply the KICS verdict first and fail when the results cannot be read (#176)
* add: unit e2e test suite, ci workflow and coverage badge * add: explanatory comment to write permissions * update: test coverage to match real use cases and known bug count * add: known-bug missing file * add: correct badge * fix: input queries and bom params * fix: only enable boolean switches when the input is "true" * fix: escape pipes and all newlines in query table cells * fix: skip the PR comment when the event is not a pull request * fix: post the summary without the queries table when the comment is too large * test: make the fake kics write results world-readable like KICS v2.1.20 * fix: keep a scan path containing spaces as a single argument * fix: keep path-valued inputs containing spaces as single arguments * fix: remove the results.json KICS wrote into the workspace when json was not requested * fix: honor the type input as an alias of platform_type * docs: set ignore-on-exit in the config file SARIF example * fix: apply the KICS verdict first and fail when the results cannot be read * fix: apply the KICS verdict first and fail when the results cannot be read
1 parent eef2554 commit f9c9b7a

4 files changed

Lines changed: 20 additions & 24 deletions

File tree

‎src/commenter.js‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -138,17 +138,13 @@ async function postPRComment(results, repo, prNumber, octokit, commentWithQuerie
138138
message = createComment(results) + "_The queries table was omitted: it exceeds the GitHub comment size limit._\n";
139139
}
140140

141-
const {data: comments} = await octokit.rest.issues.listComments({
141+
const comments = await octokit.paginate(octokit.rest.issues.listComments, {
142142
...repo,
143143
issue_number: prNumber,
144+
per_page: 100,
144145
});
145146

146-
const comment = comments.find((comment) => {
147-
return (
148-
comment.user.login === "github-actions[bot]" &&
149-
comment.body.startsWith("![kics-logo](")
150-
);
151-
});
147+
const comment = comments.find((comment) => comment.body?.startsWith("![kics-logo]("));
152148

153149
if (comment) {
154150
await octokit.rest.issues.updateComment({

‎src/main.js‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@ async function main() {
6060
const outputFormats = process.env.INPUT_OUTPUT_FORMATS;
6161
const exitCode = process.env.KICS_EXIT_CODE
6262

63+
// the scan verdict comes first so a reporting failure can never hide it
64+
setWorkflowStatus(exitCode);
65+
6366
try {
6467
const octokit = github.getOctokit(githubToken);
6568
let context = {};
@@ -80,7 +83,13 @@ async function main() {
8083
enableComments = enableComments ? enableComments : "false"
8184
enableJobsSummary = enableJobsSummary ? enableJobsSummary : "false"
8285

83-
const parsedResults = readJSON(outputPath.resultsJSONFile);
86+
let parsedResults;
87+
try {
88+
parsedResults = readJSON(outputPath.resultsJSONFile);
89+
} catch (e) {
90+
core.setFailed(`Could not read the KICS results: ${e.message}`);
91+
return;
92+
}
8493
if (enableAnnotations.toLocaleLowerCase() === "true") {
8594
annotator.annotateChangesWithResults(parsedResults);
8695
}
@@ -91,7 +100,6 @@ async function main() {
91100
await commenter.postJobSummary(parsedResults, commentsWithQueries.toLocaleLowerCase() === "true", excludedColumnsForCommentsWithQueries);
92101
}
93102

94-
setWorkflowStatus(exitCode);
95103
await cleanupOutput(outputPath.resultsJSONFile, outputFormats);
96104
} catch (e) {
97105
console.error(e);

‎test/e2e/action.test.js‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,9 @@ const assert = require('node:assert/strict')
55
const { runAction } = require('../helpers/run-action')
66
const { startFakeGitHub, humanComment } = require('../helpers/fake-github')
77
const { kicsResults, query, finding } = require('../helpers/fixtures')
8-
const { knownBug } = require('../helpers/known-bug')
98

109
const KICS_FOUND_RESULTS = '50'
1110

12-
const SILENT_FAILURE =
13-
'main() swallows every exception, so a KICS failure is never reported when GitHub or results.json cannot be used'
14-
1511
test('pull request comment', async (t) => {
1612
await t.test('is not posted unless enable_comments is on (the default)', async (t) => {
1713
const fake = await startFakeGitHub(t)
@@ -171,19 +167,19 @@ test('workflow status follows the KICS exit code', async (t) => {
171167
assert.equal(run.code, 0)
172168
})
173169

174-
await t.test('fails when KICS fails and the PR comment cannot be posted', knownBug(SILENT_FAILURE), async (t) => {
170+
await t.test('fails when KICS fails and the PR comment cannot be posted', async (t) => {
175171
const fake = await startFakeGitHub(t, { failWith: 500 })
176172
const run = await runAction(t, { results: kicsResults(), kicsExitCode: KICS_FOUND_RESULTS, inputs: { enable_comments: 'true' }, apiUrl: fake.url })
177173
assert.equal(run.code, 1, 'a GitHub API outage must not turn a failed scan into a green build')
178174
})
179175

180-
await t.test('fails when KICS failed and wrote no results.json', knownBug(SILENT_FAILURE), async (t) => {
176+
await t.test('fails when KICS failed and wrote no results.json', async (t) => {
181177
const run = await runAction(t, { kicsExitCode: '126' })
182178
assert.equal(run.code, 1, run.stdout + run.stderr)
183179
assert.ok(run.errors.length > 0)
184180
})
185181

186-
await t.test('reports a missing results.json instead of passing silently', knownBug(SILENT_FAILURE), async (t) => {
182+
await t.test('reports a missing results.json instead of passing silently', async (t) => {
187183
const run = await runAction(t, { kicsExitCode: '0', inputs: { enable_comments: 'true' } })
188184
assert.notEqual(run.code, 0, 'the scan produced no report, which is not a success')
189185
})

‎test/unit/commenter.test.js‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,13 @@ const path = require('node:path')
88
const { postPRComment, postJobSummary } = require('../../src/commenter')
99
const { startFakeGitHub, humanComment, MAX_COMMENT_LENGTH } = require('../helpers/fake-github')
1010
const { kicsResults, query, finding } = require('../helpers/fixtures')
11-
const { knownBug } = require('../helpers/known-bug')
1211

1312
const repo = { owner: 'o', repo: 'r' }
1413
const PR = 1
1514
const COMMENTS_PATH = '/repos/o/r/issues/1/comments'
1615
const kicsBody = '![kics-logo](https://example.com/logo.png)\nold report'
1716
const noExclusions = []
1817

19-
// Pending fix: PR #163 (fix-duplicated-comment). Remove the `knownBug` markers once it is merged.
20-
const PR_163 = 'PR #163: comment lookup must paginate and match on the KICS marker, not the author'
21-
2218
async function post(fake, results = kicsResults(), { withQueries = false, excluded = noExclusions, prNumber = PR } = {}) {
2319
await postPRComment(results, repo, prNumber, fake.octokit, withQueries, excluded)
2420
}
@@ -81,7 +77,7 @@ test('PR comment lifecycle', async (t) => {
8177
})
8278

8379
test('finding the existing KICS comment in a busy thread', async (t) => {
84-
await t.test('updates it when buried past the first 30 comments', knownBug(PR_163), async (t) => {
80+
await t.test('updates it when buried past the first 30 comments', async (t) => {
8581
const comments = Array.from({ length: 40 }, (_, i) => humanComment(i))
8682
comments.push({ id: 1000, user: { login: 'github-actions[bot]' }, body: kicsBody })
8783
const fake = await startFakeGitHub(t, { comments })
@@ -92,7 +88,7 @@ test('finding the existing KICS comment in a busy thread', async (t) => {
9288
assert.ok(!fake.calls().some((c) => c.startsWith('POST')), 'must not create a duplicate')
9389
})
9490

95-
await t.test('updates it when it is on page 3 of 100+ comments per page', knownBug(PR_163), async (t) => {
91+
await t.test('updates it when it is on page 3 of 100+ comments per page', async (t) => {
9692
const comments = Array.from({ length: 250 }, (_, i) => humanComment(i))
9793
comments.push({ id: 1000, user: { login: 'github-actions[bot]' }, body: kicsBody })
9894
const fake = await startFakeGitHub(t, { comments })
@@ -104,13 +100,13 @@ test('finding the existing KICS comment in a busy thread', async (t) => {
104100
assert.ok(fake.requests.filter((r) => r.method === 'GET').length <= 3, 'lists 100 comments per page, not 30')
105101
})
106102

107-
await t.test('updates it when it was posted by an author other than github-actions[bot]', knownBug(PR_163), async (t) => {
103+
await t.test('updates it when it was posted by an author other than github-actions[bot]', async (t) => {
108104
const fake = await startFakeGitHub(t, { comments: [{ id: 5, user: { login: 'my-app[bot]' }, body: kicsBody }] })
109105
await post(fake)
110106
assert.deepEqual(fake.writes().map((r) => `${r.method} ${r.path}`), ['PATCH /repos/o/r/issues/comments/5'])
111107
})
112108

113-
await t.test('updates the oldest KICS comment when duplicates already exist', knownBug(PR_163), async (t) => {
109+
await t.test('updates the oldest KICS comment when duplicates already exist', async (t) => {
114110
const fake = await startFakeGitHub(t, {
115111
comments: [
116112
{ id: 10, user: { login: 'someone' }, body: kicsBody },

0 commit comments

Comments
 (0)