Skip to content

fix: cleanup EPERM and shrink oversized step summary in post.js - #6

Merged
tsigouris007 merged 1 commit into
mainfrom
fixups
Jul 10, 2026
Merged

fix: cleanup EPERM and shrink oversized step summary in post.js#6
tsigouris007 merged 1 commit into
mainfrom
fixups

Conversation

@tsigouris007

Copy link
Copy Markdown
Contributor

The events file is written via sudo tee so it ends up root-owned, and /tmp's sticky bit blocks the unprivileged post-step from unlinking it directly.

The step summary wa one markdown row per raw event, which blew past GitHub's 1MB limit on noisy runs with lots of repeated short-lived connections. Now it's one row per distinct connection (grouped by proto/ips/port/domain/process/uid) with a Count, a condensed source port list, and a first/last seen time range.

@gpapakyriakopoulos gpapakyriakopoulos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes based on the security-sensitive CI/action review. The PR is narrow and the summary-size direction is good, but I think we should address these before merging.

  1. [P1] Security signal is weakened in the step summary. The new grouping key omits process.pid and process.exit_code, and the rendered columns omit both too. That means failed TCP attempts can be merged with successful ones, and a process can blend into a benign-looking row by setting its short comm/process name. The full NDJSON still has the data, but the summary is the first audit surface. Please add exit-code/status counts and PID visibility, or avoid grouping across process.exit_code; include distinct PID count/list if full PID grouping is too noisy. Affected area: src/post.js, around groupEvents() and the summary columns.

  2. [P2] Truncated summaries point to undefined. printStepSummary(events, fullDataLocation) uses fullDataLocation in the truncation note, but the call passes only enriched. In high-cardinality runs, the summary will say full data is in undefined, which is bad during incident review. Please compute/pass s3Uri or the job log before printing, or print after the upload/log fallback location is known.

  3. [P2] Markdown summary fields are not escaped. Process names and PTR-derived domains are inserted directly into markdown table rows. A monitored process can control process.name, including table-breaking characters, which can distort or hide suspicious rows in the security report. Please escape |, backslashes, CR/LF, and other markdown-sensitive content before joining rows.

Verification performed locally: fetched PR ref 2ea860e; node --check passed for src/post.js; Go unit tests passed with a temp GOCACHE via go test ./internal/.... I did not find a blocking performance regression; aggregation should usually reduce summary output, though it still does O(groups log groups) sorting.

The events file is written via `sudo tee` so it ends up root-owned,
and /tmp's sticky bit blocks the unprivileged post-step from unlinking
it directly.

The step summary was one markdown row per raw event, which blew
past GitHub's 1MB limit on noisy runs with lots of repeated short-lived
connections. Now it's one row per distinct connection (grouped by
proto/ips/port/domain/process/uid) with a Count, a condensed source
port list, and a first/last seen time range.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@tsigouris007
tsigouris007 merged commit a4a2673 into main Jul 10, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants