Skip to content

fix: tighten require_repo_name regex and add issue.user null guard#22

Open
lml2468 wants to merge 1 commit into
mainfrom
fix/workflow-o1-o5-polish
Open

fix: tighten require_repo_name regex and add issue.user null guard#22
lml2468 wants to merge 1 commit into
mainfrom
fix/workflow-o1-o5-polish

Conversation

@lml2468
Copy link
Copy Markdown
Contributor

@lml2468 lml2468 commented May 17, 2026

Follow-up to #19

Addresses two non-blocking observations from the final approved review:

O1 — require_repo_name() permits . and ..

The previous regex [A-Za-z0-9._-]{1,100} accepted . and .. as valid repo names. While neither is a valid GitHub repo name and would just 404, the whole point of the validation is to make path-traversal impossible by construction.

Fix: Require a leading alphanumeric and explicitly reject . / ..:

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

O5 — issue.user null guard

issue.user is theoretically nullable for ghost/deleted accounts. Added a null check before accessing issue.user.type.

Files changed

  • .github/workflows/octo-ci-status.yml
  • .github/workflows/octo-issue-feed.yml
  • .github/workflows/octo-pr-feed.yml
  • .github/workflows/issue-welcome.yml

O1 — require_repo_name() now rejects '.' and '..' and requires a
leading alphanumeric, making path-traversal impossible by construction.

O5 — issue-welcome.yml guards against null issue.user (ghost/deleted
accounts) before reading issue.user.type.

Follow-up to #19 (merged).
Copy link
Copy Markdown

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

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

Code Review — PR #22 (.github)

Follow-up to #19 addressing two non-blocking observations (O1, O5). Small, focused, low-risk diff: +5/-4 across 4 workflow files.

Verdict: APPROVED

Both fixes are correct and address the stated observations. One minor edge-case worth noting but not blocking.

1. Verification

Item Status Evidence
O1: regex requires leading alphanumeric octo-ci-status.yml:85, octo-issue-feed.yml:93, octo-pr-feed.yml:110 all updated to r'[A-Za-z0-9][A-Za-z0-9._-]{0,99}'
O1: explicit . / .. rejection ... or val in {'.', '..'} added in all 3 files
O1: length bound preserved (1–100 chars) 1 leading char + {0,99} trailing = 1–100, matches old {1,100}
O1: all 3 octo-*.yml updated consistently Identical regex in all three call sites
O5: null guard on issue.user issue-welcome.yml:28if (issue.user && issue.user.type === 'Bot')
Path-traversal made impossible by construction . and .. can no longer pass validation; URL composition at octo-ci-status.yml:111,128,133 is now safe by construction

2. Findings

P2 — .github repo itself cannot use these reusable workflows

The new regex requires a leading alphanumeric, which rejects .github as a valid REPO_NAME. Since GitHub does allow the special .github repo name (it's literally the name of this repo), any future caller wanting to notify about events in the .github repo would be rejected.

In practice this is unlikely to matter — these workflows are typically invoked from feature repos (octo-server, octo-deployment, etc.), not the meta-repo. But it's worth being aware of: the trade-off here is "path-traversal-impossible by construction" vs. "supports the .github edge case." The PR explicitly chose the former, which is the right call for a defense-in-depth security control.

If supporting .github ever becomes needed, the cleaner fix would be an explicit allowlist of leading-dot exceptions rather than loosening the regex.

Nit — val in {'.', '..'} is redundant with the new regex

The leading-alphanumeric requirement already rejects both . (single char fails [A-Za-z0-9]) and .. (first . fails [A-Za-z0-9]). The explicit set check is defensive belt-and-suspenders — not harmful, and arguably good for making intent obvious to the next reader. Leave as-is.

O5 semantics — null user treated as non-bot ✅

if (issue.user && issue.user.type === 'Bot') causes ghost/deleted-account issues to fall through to the welcome path rather than being skipped as bots. That's the right default: a deleted human account opening an issue should still get the welcome message logic (which itself handles missing user gracefully downstream, since the body composition doesn't depend on issue.user).

3. Suggestions

  • None blocking. Consider adding a short inline comment to the regex itself explaining the leading-alphanumeric requirement is intentional anti-traversal (the function docstring covers it, but the regex looks fiddly enough that a future reader may "simplify" it back).

4. Additional observations (out of scope)

  • octo-pr-feed.yml:111,128,133 and equivalents: the hardcoded org Mininglamp-OSS/{repo} in the URL is the second layer of defense — even if repo validation were ever bypassed, the org prefix prevents arbitrary owner/repo injection. Good defense-in-depth.
  • require_group_id (octo-pr-feed.yml:101 etc.) correctly notes "this is not an authorization check" — purely format validation. No action needed; just calling out that the comment is well-written and accurate.

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