security: harden workflow security - API allowlist, input validation, idempotency

[lml2468]

Summary

This PR applies security and best-practice fixes identified in a full workflow audit by codex (gpt-5.5).

Changes

🔴 High Priority

API allowlist in Octo notification workflows

• Files: octo-ci-status.yml, octo-issue-feed.yml, octo-pr-feed.yml
• Previously api_base_url could be set by any caller repo to an arbitrary URL, risking OCTO_BOT_TOKEN exfiltration
• Now validated against a hardcoded allowlist before use

🟡 Medium Priority

workflow_id made required in octo-ci-status.yml

• Previously optional; without it, run history was fetched globally and filtered by name, risking incorrect previous-run detection

Explicit sort for previous-run selection in octo-ci-status.yml

• Added explicit descending sort instead of relying on API return order

Input sanitization in octo-issue-feed.yml and octo-pr-feed.yml

• Added sanitize_text() for titles and authors to prevent IM message injection
• Added require_group_id() with regex validation for group IDs

issue-welcome.yml improvements

• Added timeout-minutes: 3
• Added payload guard for non-issues events
• Added idempotency marker to prevent duplicate welcome comments on rerun

Audit

Full audit performed by codex (gpt-5.5). Fixes implemented by claude-opus-4-6.

[yujiawei]

Code Review — PR #19 (.github)

Summary

This PR applies security hardening to the org's reusable GitHub Actions workflows: an API URL allowlist to prevent secret exfiltration, input sanitization against IM message injection, idempotency for the welcome bot, and correctness fixes for CI status detection. The changes are well-motivated and address real vulnerabilities.

Verdict: Approve — the security fixes are correct and important. Minor issues noted below.

---

Findings

✅ Correct and Important

Change	Assessment
API allowlist (all 3 notification workflows)	Critical fix. Prevents OCTO_BOT_TOKEN from being sent to attacker-controlled URLs via crafted api_base_url input. Implementation is correct — rstrip('/') normalizes before strict set-membership check.
sanitize_text() for titles/authors	Addresses newline injection in IM messages. The primary attack vector (layout manipulation via CR/LF) is neutralized.
require_group_id() hex validation	Prevents malformed group IDs from reaching the API. Regex is strict and correct.
Idempotency marker in issue-welcome.yml	Prevents duplicate welcome comments on workflow re-runs. Clean implementation using a hidden HTML comment marker.
Explicit sort for previous-run selection	Correctness fix — relying on implicit API return order was fragile.
timeout-minutes: 3 on issue-welcome	Good practice for a reusable workflow.
Payload guard (!issue)	Fail-fast on misconfiguration. Appropriate use of core.setFailed.

⚠️ P1 — Breaking Change: workflow_id now required

File: octo-ci-status.yml, lines 22–24

Making workflow_id required (was optional with default: 0) means any existing caller workflow that doesn't pass this input will fail at the GitHub Actions level with a "missing required input" error.

Recommendation: Verify all caller workflows across Mininglamp-OSS repos already pass workflow_id before merging, or coordinate a deployment plan. If any caller doesn't pass it today, they'll break on merge.

⚠️ P2 — sanitize_text() scope

Files: octo-issue-feed.yml, octo-pr-feed.yml

The function strips CR/LF and truncates, which handles the primary injection vector (newline-based layout manipulation). However:

• Unicode control characters (U+0000–U+001F beyond CR/LF, U+200B–U+200F, etc.) are not stripped
• Any IM-specific formatting syntax (if Octo IM supports markdown-like syntax) is not escaped

This is acceptable for now since the primary attack is newline injection, but consider hardening later if the IM client interprets other control sequences.

⚠️ P2 — import re placement

Files: octo-issue-feed.yml (line ~80 in new), octo-pr-feed.yml (line ~97 in new)

The import re is placed between two function definitions rather than at the top with import os, json, sys, .... This works but is non-idiomatic — readers expect imports at the top of the script.

⚠️ P2 — Code duplication

sanitize_text() and require_group_id() are copy-pasted identically between octo-issue-feed.yml and octo-pr-feed.yml. Given these are inline Python scripts inside YAML, this is understandable, but worth noting for future maintenance. If the sanitization logic needs updating, both files must be changed in lockstep.

ℹ️ Minor — Idempotency race condition

The issue-welcome.yml idempotency check (paginate → check → post) has a theoretical TOCTOU race if two workflow runs fire concurrently for the same issue. Worst case is a duplicate welcome comment, which is harmless. No action needed.

---

Verification Checklist

• ✅ Allowlist logic is correct (strict set membership after normalization)
• ✅ sanitize_text() handles the primary injection vector
• ✅ require_group_id() regex [0-9a-f]{32} is correct for Octo group IDs
• ✅ Explicit sort uses correct key and direction
• ✅ Idempotency marker is invisible to users (HTML comment)
• ⚠️ Breaking change on workflow_id — verify callers before merge

---

Suggestion

For the import re placement, consider moving it to the top:

import os, json, re, sys, time, urllib.request, urllib.error

This is a style nit, not a blocker.

---

Overall: solid security hardening. The fixes correctly address the identified vulnerabilities. Approve with the caveat that the workflow_id breaking change should be coordinated with caller repos.

[lml2468]

Review Response

Thanks @yujiawei for the thorough review!

P1 — workflow_id breaking change
Verified all caller repos before merging: octo-server, octo-adapters, octo-matter, octo-smart-summary, octo-admin all already pass workflow_id. No callers will break.

P2 — import re placement
Fixed in c713912 — merged re into the top-level import line and removed the mid-script standalone import re in both octo-issue-feed.yml and octo-pr-feed.yml.

- import os, json, sys, time, urllib.request, urllib.error
+ import os, json, re, sys, time, urllib.request, urllib.error

P2 — Code duplication / sanitize_text() scope
Acknowledged. Acceptable for inline Python-in-YAML; tracked for future improvement.

Ready to merge. 🙏

[yujiawei]

Code Review — PR #19 (.github)

Verdict: COMMENTED — fixes are sound and well-implemented. No blocking issues. A few observations below worth considering before or shortly after merge.

Summary

The four changes (API allowlist, workflow_id required, explicit sort, input sanitization, idempotency marker) all address real issues from the prior audit. Implementation matches the design, defaults are kept consistent, and the diff is minimal. Verified end-to-end against the head commit.

What works well

• API allowlist (octo-ci-status.yml:191-199, octo-issue-feed.yml:105-112, octo-pr-feed.yml:127-134): Constructed identically in all three files, fail-closed with sys.exit(2), default value (https://im.deepminer.com.cn/api) is in the allowlist so no behavior regression. This is the highest-value fix in the PR.
• Explicit sorted(..., reverse=True) on previous-run selection (octo-ci-status.yml:141-148): Removes silent reliance on GitHub's documented but undocumented-in-contract default ordering. Correct fix.
• require_group_id() regex ^[0-9a-f]{32}$ (octo-issue-feed.yml:79-84, octo-pr-feed.yml:96-101): Tight, fail-closed, matches the format of the hardcoded IDs already in the file (151a45970e1546afa9e947ac36a5c4e5, etc.). Good.
• Idempotency marker in issue-welcome.yml:23-33: HTML-comment marker is invisible in rendered Markdown; restricting the lookup to c.user.type === 'Bot' correctly prevents a malicious user comment from suppressing the welcome.
• Payload guard in issue-welcome.yml:19-22: Defensive, fails loudly with core.setFailed, good UX for misconfigured callers.
• timeout-minutes: 3 on issue-welcome.yml:9 aligns with the 5-minute timeouts already on the feed workflows.

Observations (non-blocking)

1. workflow_id becoming required is a breaking change for callers — call it out in release notes

octo-ci-status.yml:21-24 removes default: 0 and flips required: false → true. Any existing caller workflow in the org that calls this reusable workflow without passing workflow_id will start failing immediately on next dispatch with a workflow validation error.

The audit-driven motivation is sound (without workflow_id, history is fetched globally and filtered by name, which can cross-pollute when two workflows share a name). But:

• Recommend an explicit note in the merge commit / release notes / v1 tag bump so caller repos know to update before this lands.
• The new description says ${{ github.event.workflow_run.workflow_id }}. Worth verifying that this is what callers actually have available — for callers triggered by workflow_run, the correct expression is ${{ github.event.workflow_run.workflow_id }} (which the new description matches), but the prior PR description text said ${{ github.workflow_run.workflow_id }} (no .event.), so any caller that copy-pasted from the old description will be wrong.

If feasible, grep dependent caller repos before merging, or stage this behind a version tag bump (v2) so existing @v1 consumers are not surprised.

2. sanitize_text() covers title and author but not labels or repo name

octo-issue-feed.yml:90-94 joins labels with ', ' directly:

labels = json.loads(os.environ.get('ISSUE_LABELS', '[]'))
labels_part = ' · 🏷️ ' + ', '.join(labels) if labels else ''

A label containing \n would inject newlines into the IM message exactly the same way an unsanitized title would. GitHub does allow newlines in label names in principle (rare in practice, but possible via the API). Suggest:

labels = [sanitize_text(l, max_len=64) for l in json.loads(os.environ.get('ISSUE_LABELS', '[]'))]

Similarly, REPO_NAME (octo-issue-feed.yml:96, octo-pr-feed.yml:118) is interpolated into both the message body and — in octo-issue-feed.yml:165 — into a constructed URL (f'https://github.com/Mininglamp-OSS/{repo}/issues/{num}'). Caller-controlled, low risk in practice (repo names can't contain \n), but for symmetry with the title/author hardening it would be cheap to sanitize once at the top.

Not a blocker — the realistic injection vectors (issue/PR title and author display name) are the ones that matter, and those are covered.

3. existingComments pagination runs even for bot-opened issues

issue-welcome.yml:24-33 paginates all comments before the bot-skip check at line 36. For repos that get bot-opened issues frequently (Dependabot, Renovate), this is a wasted API round-trip per event. Trivial fix: move the issue.user.type === 'Bot' check above the pagination block. Performance only, not correctness.

4. Concurrent-trigger race in idempotency check

The marker check at issue-welcome.yml:30-33 is read-then-write without synchronization. If issues.opened and issues.labeled (or two reruns) fire close enough together that both runs read before either writes, both will post. In practice this is unlikely because:

• The default caller pattern only triggers on opened.
• GitHub serializes most triggers per-issue closely enough.

The marker still defends against the dominant case (manual reruns and retries). Worth being aware of, not worth fixing unless duplicates start being reported.

5. Minor consistency: PROJECT_GROUP_ID is validated in feeds but not in octo-ci-status.yml

octo-ci-status.yml:82 still uses plain require_env('PROJECT_GROUP_ID'), while octo-issue-feed.yml:157 and octo-pr-feed.yml:180 use the new require_group_id. Same input source, same target API — the validation should match. Cheap to add.

Things I checked and confirmed are fine

• API allowlist is constructed by reading the env var via os.environ.get(..., default) (not require_env), so the default still applies — confirmed callers that don't pass api_base_url continue to work.
• The marker <!-- octo-issue-welcome:v1 --> is prepended on issue-welcome.yml:132 exactly once and stripped from rendered Markdown. The :v1 suffix gives room to bump if the welcome content materially changes.
• sanitize_text(s, max_len=80) for author is appropriate — GitHub login is bounded at 39 chars but display name (pr_author could be either) can be longer; 80 is a safe ceiling without truncating realistic names.
• sort(reverse=True) correctly returns the most-recent run as older[0] after the filter; verified the filter still excludes current['id'] so it cannot return the current run as previous.

Recommendation

Ship it. The breaking change on workflow_id is the only meaningful operational concern — coordinate with caller repos (or bump a major tag) and the rest is solid hardening.

[lml2468]

Follow-up: Review Observations Addressed (a1e459e)

Thanks for the additional observations @yujiawei! All actionable items fixed:

Obs #2 — Labels not sanitized ✅

# Before
labels = json.loads(os.environ.get('ISSUE_LABELS', '[]'))
# After
labels = [sanitize_text(l, max_len=64) for l in json.loads(os.environ.get('ISSUE_LABELS', '[]'))]

Obs #3 — Bot check before paginate ✅
Moved issue.user.type === 'Bot' guard above octokit.paginate() — bot-opened issues now skip the API round-trip entirely.

Obs #5 — PROJECT_GROUP_ID validation in octo-ci-status.yml ✅
Added require_group_id() function (+ import re) and replaced require_env('PROJECT_GROUP_ID') with require_group_id('PROJECT_GROUP_ID') for consistency across all three feed workflows.

Obs #1 (workflow_id) — Already verified: all 5 caller repos pass workflow_id. No action needed.
Obs #4 (TOCTOU race) — Acknowledged, harmless worst-case. No action needed per reviewer's assessment.

[yujiawei]

Code Review — PR #19 (.github)

Summary

Solid security hardening pass. Each change is justified by a concrete threat (token exfil via attacker-supplied api_base_url, IM message injection via newlines in titles/authors, false-positive "CI broken" alerts from globally-scoped run history, duplicate welcome comments on workflow rerun). The implementation is clean, scoped, and consistent across the three notification workflows. No P0/P1 issues found; a few P2 polish items below.

1. Verification of stated fixes

Change	Verdict	Evidence
API allowlist on api_base_url	✅	octo-ci-status.yml:200-206, octo-issue-feed.yml:105-111, octo-pr-feed.yml:127-133 — token can no longer be sent to arbitrary URLs
workflow_id made required	✅	octo-ci-status.yml:21-24. Verified all 5 known callers (octo-server, octo-adapters, octo-admin, octo-smart-summary, octo-matter) already pass ${{ github.event.workflow_run.workflow_id }}, so no caller breakage when v1 is retagged
Explicit descending sort for previous-run selection	✅	octo-ci-status.yml:149-157 — no longer relies on GitHub API ordering
sanitize_text() for titles/authors/labels	✅	octo-issue-feed.yml:73-76,91,98,100; octo-pr-feed.yml:90-93,120,122. Strips CR/LF, caps length
require_group_id() regex validation	✅	octo-ci-status.yml:73-78,90; octo-issue-feed.yml:79-84,157; octo-pr-feed.yml:96-101,179-180 — [0-9a-f]{32} matches the format used by all known callers
issue-welcome.yml timeout + payload guard + idempotency	✅	issue-welcome.yml:9, 19-22, 29-39, 132 — all three additions present and structurally correct

2. Findings

P2 — existingComments.some(c => c.user.type === 'Bot' …) is over-narrow

issue-welcome.yml:36 restricts the idempotency lookup to Bot-authored comments. The marker <!-- octo-issue-welcome:v1 --> is already unique enough on its own — the user.type === 'Bot' predicate adds no security and creates a latent footgun: if a future caller swaps secrets.GITHUB_TOKEN for a PAT (so user.type === 'User'), idempotency silently breaks and duplicate welcomes start posting on every workflow rerun. Recommend dropping the Bot filter:

if (existingComments.some(c => c.body && c.body.includes(marker))) { … }

P2 — Welcome idempotency is not race-safe

The paginate(listComments) → createComment sequence has no concurrency guard. Two near-simultaneous issues.opened deliveries for the same issue (rare but observed during re-dispatch / replay scenarios on GitHub) would each see zero existing markers and both post. Add a job-level concurrency group:

jobs:
  welcome:
    concurrency:
      group: welcome-${{ github.event.issue.number }}
      cancel-in-progress: false

P2 — api_base_url input is now effectively cosmetic

After the allowlist (octo-ci-status.yml:200-202, etc.), the only value that passes validation is the hardcoded default https://im.deepminer.com.cn/api. Two options:

1. Drop the api_base_url input from workflow_call.inputs entirely (cleanest — removes the misleading knob).
2. Keep it but expand the description to "Only the production Octo IM endpoint is currently allowed; pass an unsupported value and the workflow will fail."

The current state where operators can pass the input but only one value works is the worst of both worlds.

P2 — Validation order in octo-issue-feed.yml

require_group_id('PROJECT_GROUP_ID') (line 157) runs before the first send() (line 158), so no partial-send concern — but the variable is created far from the sends it gates. For symmetry with octo-pr-feed.yml:179-180 (where both group IDs are validated together right before sending) and to make the gating obvious to future readers, consider hoisting proj_gid = require_group_id(...) up to where the other env reads happen (alongside repo, num, title, etc. around line 96). Pure readability nit.

P2 — PR body wording inconsistency (no code impact)

PR description references ${{ github.workflow_run.workflow_id }}, but the in-file description at octo-ci-status.yml:24 correctly uses ${{ github.event.workflow_run.workflow_id }} (the form callers actually use). Not a bug — only worth a one-character fix if you amend the PR body.

3. Additional observations (out of scope but noted)

• The single-element allowlist set (ALLOWED_API_BASES = {'https://im.deepminer.com.cn/api'}) is duplicated across three files. If a second IM environment is ever needed (staging?), it will need to be updated in three places. Not actionable now; flag for future refactor.
• require_group_id regex [0-9a-f]{32} validates format but not authority — any well-formed hex string passes. This is by design (defense against typo/quoting bugs, not authorization), but worth being explicit about in a comment so future maintainers don't mistake it for an allowlist.
• urllib.request.Request(send_url, …) uses HTTPS, but urlopen does not pin certificates. Acceptable for an internal IM endpoint; flag only if threat model includes MITM on the GHA runner egress path.

4. Verdict

COMMENTED — the security fixes are correct, well-scoped, and backed by real threats. All in-org callers remain compatible. The P2 items above are polish (idempotency robustness, input cleanup, readability) and do not block merging this PR. Recommend addressing the Bot-filter and concurrency items either in this PR or as an immediate follow-up before retagging v1.

[lml2468]

Follow-up: All Review Observations Addressed (b2226ec)

Thanks @yujiawei for the continued feedback! All remaining actionable P2 items are now fixed:

P2 — Bot-type filter too narrow in idempotency check ✅
Dropped c.user.type === 'Bot' && from the predicate. The unique marker <!-- octo-issue-welcome:v1 --> is sufficient — no assumption on commenter type.

P2 — Welcome idempotency race (TOCTOU) ✅
Added concurrency: block to the welcome job:

concurrency:
  group: welcome-${{ github.event.issue.number }}
  cancel-in-progress: false

Concurrent triggers for the same issue are now serialized; the second run sees the marker and exits cleanly.

P2 — api_base_url input description ✅
Updated in all three feed workflows to make the allowlist restriction explicit:

"Only the production endpoint is allowlisted; any other value will cause the workflow to fail."

P2 — proj_gid validation order in octo-issue-feed.yml ✅
Hoisted proj_gid = require_group_id('PROJECT_GROUP_ID') up alongside the other env reads (after author = ...), so all input validation happens at the top before message construction.

Ready for re-review.

[yujiawei]

Code Review — PR #19 (.github)

Summary

The change is a focused, defensive hardening of four reusable workflows. It addresses a real and credible exfiltration vector (api_base_url was previously caller-controlled and the secret OCTO_BOT_TOKEN was forwarded to whatever host the caller named), tightens input validation for group IDs and free-text fields used in IM messages, and adds idempotency + a payload guard to the welcome workflow. Scope is appropriate, code is uniform across the three Python jobs, and no behavior of the existing happy path appears to regress.

I verified all five known callers (octo-adapters, octo-admin, octo-smart-summary, octo-matter, octo-server) and the new workflow_id: required: true does not break them — they each already pass ${{ github.event.workflow_run.workflow_id }}. Existing callers also pin to @v1, and the v1 tag currently points at 6db6243, not main — so this change does not affect production callers until the tag is moved.

Verdict by item

Change	Status	Notes
api_base_url allowlist (3 files)	✅	Correct token-exfil mitigation. Caller can no longer redirect OCTO_BOT_TOKEN to an attacker host.
workflow_id made required in octo-ci-status.yml	✅	All current callers already pass it; description is clear. Eliminates the silent "wrong workflow" failure mode.
Explicit descending sort in previous-run selection	✅	GitHub's run list is documented descending, but relying on it was fragile. ISO timestamp lex sort is correct here.
sanitize_text() on titles, authors, labels	✅	Strips CR/LF and truncates — sufficient if the IM renders as plain text. See note below.
require_group_id() ([0-9a-f]{32})	✅	All known callers pass conforming IDs (I checked each).
issue-welcome.yml payload guard + idempotency marker + concurrency	✅	cancel-in-progress: false combined with the comment-scan marker gives real exactly-once semantics across reruns/replays.

Findings

P1 — none

P2 (suggestions, non-blocking)

1. Allowlist comparison is case-sensitive on the host.
_api_base not in ALLOWED_API_BASES does a strict string compare. Hostnames are case-insensitive per RFC 3986, so https://IM.deepminer.com.cn/api would be rejected by the allowlist but accepted by urllib. That's defensive in the right direction (false-rejects are safe), so this isn't a bug — just worth noting that any future "alternate casing" issue would manifest as a hard failure rather than a silent bypass. Optional: _api_base.lower() not in {b.lower() for b in ALLOWED_API_BASES} if you want to be lenient on case.

2. sanitize_text only strips \r and \n.
Other control characters (\t, \x00-\x1f other than CR/LF, and bidi overrides like U+202E) survive. If the IM renders messages as plain text this is fine; if it ever supports any kind of markup, a bidi-override attack on a PR title could visually flip the URL. Low likelihood, low blast radius — file under "consider when the IM rendering layer changes."

3. repo is used to build the GitHub API URL with a hardcoded Mininglamp-OSS/ prefix.

f'https://api.github.com/repos/Mininglamp-OSS/{repo}/actions/...'

If a caller from outside the org ever invoked this and passed repo = "../some-other-org/foo", they could pivot the lookup. This is theoretical given org-scoped reusable workflows and the trusted caller set, but a one-line re.fullmatch(r'[A-Za-z0-9._-]{1,100}', repo) would close it for free.

4. Idempotency marker via body.includes(marker) is a substring check.
Realistic collision risk is essentially zero (<!-- octo-issue-welcome:v1 --> is an HTML comment), but exact-match (c.body.startsWith(marker)) would be marginally safer if a future template ever quoted the marker in a code block. Not worth fixing now.

5. Pre-existing, out of scope for this PR — flagging for awareness only.
octo-pr-feed.yml on main declares feed_group_id: required: true, but none of the five caller workflows pass it. This is not introduced by this PR (it predates this branch) and does not affect production callers (they pin @v1, which doesn't have this input). It will become a problem the moment v1 is moved forward. Worth tracking as a follow-up before the next tag bump.

Things I specifically checked and that are fine

• _api_base parsing handles trailing slash, query strings, and fragments correctly: only the exact production URL passes.
• Hardcoded group IDs in scripts (151a45..., 4ade98...) are all 32-char lowercase hex; they are not validated by require_group_id but don't need to be — they're literals.
• The triage-bot message in octo-issue-feed.yml only interpolates repo and num from caller inputs, both of which come from github.event.* and are trusted.
• The new earlier extraction of proj_gid in octo-issue-feed.yml is functionally equivalent and just fails faster on bad input — good change.
• The concurrency group key uses github.event.issue.number, which is safe because the payload guard runs after the job starts but the key is evaluated by the runner — and the issue number is integer-typed by GitHub.
• The older list sort by created_at is correct: ISO-8601 with Z suffix is lex-sortable.

Recommendation

Approve / comment. The PR does exactly what its title says, the code is consistent across the three Python jobs, no caller breaks, and the security improvement is real. The suggestions above are all P2 and can be picked up later or ignored.

[lml2468]

Review Feedback Resolution (fc8bdb8)

Addressed all remaining P2 items from the 4 review rounds by @yujiawei:

Changes in this push

Fix	Files	Review reference
require_repo_name() validation — prevents path traversal in GitHub API URL construction (../some-other-org/foo)	ci-status, issue-feed, pr-feed	Review 4 §P2.3
sanitize_text() hardened — now strips all C0 control characters (U+0000-U+001F) + DEL via regex, not just CR/LF	issue-feed, pr-feed	Reviews 2 §Obs.2, 4 §P2.2
Case-insensitive API allowlist — _api_base.lower() not in {b.lower() for b in ...} per RFC 3986 hostname semantics	ci-status, issue-feed, pr-feed	Review 4 §P2.1
Docstrings on require_group_id() — clarifies format-only validation, not authorization	ci-status, issue-feed, pr-feed	Review 4 §Obs note
Safety comment on triage URL — documents that repo is pre-validated	issue-feed	Review 4 §P2.3

Already fixed in prior iterations (confirmed present)

• ✅ import re moved to top-level imports (review 1 §P2)
• ✅ concurrency: group: welcome-${{ github.event.issue.number }} added (review 2 §Obs.4)
• ✅ Labels sanitized: [sanitize_text(l, max_len=64) for l in ...] (review 2 §Obs.2)
• ✅ require_group_id used in octo-ci-status.yml (review 2 §Obs.5)
• ✅ Bot-filter removed from idempotency check (review 3 §P2)
• ✅ Bot-skip check runs before pagination (review 2 §Obs.3)
• ✅ api_base_url description updated (review 3 §P2)
• ✅ Validation order in issue-feed hoisted up (review 3 §P2)

Noted but intentionally not changed

• Code duplication across inline Python scripts — acknowledged, inherent to YAML-embedded scripts
• TOCTOU race in welcome — mitigated by concurrency group; residual race is harmless (worst case: duplicate welcome)
• api_base_url input kept — single-value allowlist is documented in description; dropping the input would be a breaking interface change

All 4 review rounds fully addressed.

[yujiawei]

Code Review — PR #19 (.github)

Summary

This PR delivers a focused, well-scoped security hardening of three reusable notification workflows plus the issue-welcome workflow. The four review rounds visible in the commit history have meaningfully tightened the design: API allowlist, format-only group-id validation, control-character stripping, repo-name validation, idempotency marker, concurrency, and timeouts are all in place. The code is consistent across the three notification workflows (same helper names and behavior), which is exactly what you want for a shared utility.

I have no blocking concerns. A handful of small observations and one deployment-coordination note are listed below.

Verification of stated changes

Claim	Result	Evidence
API allowlist prevents OCTO_BOT_TOKEN exfil via api_base_url	✅	octo-ci-status.yml:210-217, octo-issue-feed.yml:118-125, octo-pr-feed.yml:139-146 — exact-match check after rstrip('/') and lower-case comparison; any non-allowlisted value sys.exit(2) before the token is ever placed in a request header.
workflow_id is now required	✅	octo-ci-status.yml:21-24 (required: true, default removed).
Explicit descending sort for previous-run selection	✅	octo-ci-status.yml:159-166 — sorted(..., key=lambda r: r['created_at'], reverse=True). ISO 8601 Z-suffixed timestamps sort correctly lexicographically.
sanitize_text() for issue/PR titles, authors, and labels	✅	octo-issue-feed.yml:73-78, 103, 110, 112; octo-pr-feed.yml:90-95, 132, 134. Strips C0 controls and DEL, truncates to max_len.
require_group_id() format validation	✅	All three workflows; 32-char lowercase hex; docstring explicitly clarifies this is not authorization.
require_repo_name() for path-traversal hardening	✅	All three workflows; applied to REPO_NAME everywhere it is later interpolated into a URL (octo-ci-status.yml:97, 111, octo-issue-feed.yml:108, 178, octo-pr-feed.yml:130).
Issue-welcome: timeout-minutes: 3, concurrency, payload guard, idempotency marker	✅	issue-welcome.yml:9-12, 22-25, 32-42, 132-135.

All claims in the PR description are accurate against the diff at fc8bdb8.

Observations

O1 — require_repo_name regex permits . and .. (P2, defense-in-depth)

re.fullmatch(r'[A-Za-z0-9._-]{1,100}', val) accepts . and ... Neither is a valid GitHub repository name, but both would be interpolated unmodified into:

• https://api.github.com/repos/Mininglamp-OSS/{repo}/actions/runs/{run_id} (octo-ci-status.yml:111)
• https://github.com/Mininglamp-OSS/{repo}/issues/{num} (octo-issue-feed.yml:178)

urllib.request does not normalize .. segments, so the request reaches the API as-is and GitHub's server-side path normalization decides what happens. In practice this just 404s and no secret leaks, so it is not an active exploit — but since the whole point of require_repo_name() is to make path-traversal impossible by construction, it is worth tightening:

if not re.fullmatch(r'[A-Za-z0-9][A-Za-z0-9._-]{0,99}', val) or val in {'.', '..'}:

or equivalently disallow ., .., and a leading non-alphanumeric. Pure cleanup; non-blocking.

O2 — Case-insensitive comparison on the URL path is slightly over-broad (P3)

_api_base.lower() not in {b.lower() for b in ALLOWED_API_BASES} lowercases the entire URL. RFC 3986 defines only the scheme and host as case-insensitive; the path component is case-sensitive. Today this is harmless because the single allowlisted entry is already lowercase, but if a future entry adds a path with mixed case (e.g. /Api/v2), the comparison would incorrectly accept /api/v2 and reject /API/v2. If you ever generalize the allowlist, prefer splitting via urllib.parse.urlsplit and lowercasing only scheme + netloc. Non-blocking.

O3 — Breaking change for callers that do not yet pass workflow_id (P1, coordination)

Making workflow_id required: true (octo-ci-status.yml:21-24) is the right correctness fix — relying on name-only filtering across a global run list is genuinely fragile. However, any caller in a sibling repo that still invokes the old form (no workflow_id:) will fail with a workflow_call input error the moment this lands. Two suggestions:

1. Land or stage matching caller updates (in octo-server, octo-deployment, etc.) before merging this, or at least mention the cutover in the PR description so on-call knows what to expect.
2. Consider whether a workflow_dispatch-style soft-fail (warning + sys.exit(0)) for one rotation would smooth the rollout — though "loud break" is also defensible for a security/correctness fix.

This is a process concern, not a code defect.

O4 — Hardcoded feed group ID in octo-issue-feed.yml (P2, consistency)

octo-pr-feed.yml now takes feed_group_id as a required input (good), but octo-issue-feed.yml:170 and :180 still hardcode '151a45970e1546afa9e947ac36a5c4e5' for the feed and triage channels. This PR isn't claiming to fix that, so it's out of scope — flagging only because the asymmetry will trip up the next person who reads both files. Worth a follow-up to parameterize the issue-feed channel(s) the same way.

O5 — issue.user null guard (P3)

issue-welcome.yml:27 reads issue.user.type. issue.user is theoretically nullable (ghost / deleted account). Cheap guard: if (issue.user && issue.user.type === 'Bot'). Extremely unlikely to fire in practice.

O6 — Welcome idempotency interaction with quoted replies (informational)

The marker <!-- octo-issue-welcome:v1 --> is placed at the top of the body. If a human quotes the welcome comment in a reply, GitHub preserves the HTML comment verbatim, so a subsequent re-run still detects the marker via existingComments.some(...) and correctly skips. This is the right behavior; flagging for the reader because it is non-obvious and worth keeping in mind if the marker format ever changes.

Things worth calling out positively

• require_group_id()'s docstring explicitly says "Validate format only … this is not an authorization check." That kind of intent-recording inline is exactly what prevents the next contributor from mistaking format validation for an access-control gate.
• sanitize_text() strips the full C0 range plus DEL rather than just \r\n, which is what you actually want against a chat-protocol injection surface — tabs, BEL, BS, etc. are all unsafe in untrusted IM payloads.
• Concurrency on issue-welcome uses cancel-in-progress: false, which is the correct choice for an idempotent post — the second run will read the marker the first run left and exit cleanly. Cancelling would have created a race window where neither run completed.
• Explicit sorted(..., reverse=True) for previous-run selection removes a latent ordering assumption on the GitHub API response. Even though the API today returns descending, depending on undocumented order is the kind of bug that bites you 18 months later.
• permissions: {} on the two feed workflows is correctly minimal — they only need OCTO_BOT_TOKEN (a secret, not a token scope), not GITHUB_TOKEN perms.

Verdict

The four review iterations show in the result. Security posture is materially improved, the helper functions are uniform across the three workflows, and every claim in the PR description is supported by the diff. The observations above are either P2/P3 polish or a deployment-coordination note (O3) about the workflow_id cutover; none are blocking.

Recommend merging after confirming caller workflows in the consuming repos pass workflow_id.