Skip to content

refactor(auth): drop the 14 unreachable commenter-auth catch wrappers - #449

Open
mrbobbytables wants to merge 1 commit into
cncf:mainfrom
mrbobbytables:remove-unreachable-auth-wrappers
Open

mrbobbytables wants to merge 1 commit into
cncf:mainfrom
mrbobbytables:remove-unreachable-auth-wrappers

Conversation

@mrbobbytables

Copy link
Copy Markdown
Member

Description

checkOrgMember, checkCollaborator and checkIssueComments (src/utils/auth.ts) each wrap their whole body in try { … } catch { core.warning(…); return false }, so they never reject. The fourteen try { … } catch (e) { throw new Error('could not check … auth: ' + e) } wrappers around them were therefore unreachable from the shipped bundle and were kept green only by unit tests that vi.mocked the callee to reject (or made core.warning throw) — pinning error messages the action can never emit, including the misspelt couldn ot check commentor Auth.

This PR removes them, as recommended in #386:

  • close.ts, reopen.ts, lock.ts, milestone.ts, retitle.ts, unassign.ts, labels/remove.ts: call the probe directly (close/reopen become a single isAuthor || collaborator || closePolicyAllows short-circuit chain, same evaluation order as before)
  • assign.ts, cc.ts, uncc.ts: drop the could not get authorized users wrappers
  • auth.ts: drop the wrapper in getOrgCollabCommentUsers and the three in checkCommenterAuth; getOrgCollabCommentUsers now reuses checkCommenterAuth per user (same three probes in the same order, so identical API traffic). Also fixes the @param args → user jsdoc mismatch on checkCommenterAuth that npm run lint warned about.
  • Delete the 15 mock-only unit cases (__tests__/issueCommentTest/errorPaths.test.ts ×8, approveMilestoneErrorPaths.test.ts ×1, __tests__/label/errorPaths.test.ts ×1, __tests__/utils/auth.test.ts ×4 logger down) and reword the vi.mock lead-in comments that described the removed branches. The mocks themselves stay — the remaining cases still use mockResolvedValueOnce.
  • dist/index.js repacked.

No behaviour change: every removed catch was dead, and every probe call sequence is unchanged.

Fixes #386

Testing

  • npm run all — build, lint:fix, lint, pack, test: 151 files / 2075 tests pass; lint has 0 errors (1 pre-existing jsdoc warning in src/issueComment/cc.ts:81, not touched here)
  • npm run test:coverage — 100 % lines / 99.7 % branches / 100 % functions / 100 % statements, so every line in the touched files is still reached by the unit suite without the deleted mock-rejection cases
  • npm run test:coverage:e2e — 72 files / 452 tests pass; src/ e2e lines 98.73 % → 99.28 %; the only remaining 0-hit line in the touched files is the unrelated auth.ts:24

Checklist

  • I ran npm run all to lint and build my code
  • I ran npm run pack and committed dist/
  • Commits are signed off (DCO)
  • Note any new dependencies these changes bring in — none
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas — nothing non-obvious added; the removed code had the comments
  • I have made corresponding changes to the documentation — none needed (no user-facing behaviour or docs reference the removed messages)
  • My changes generates no new warnings (please describe new warnings if unavoidable) — removes one lint warning, adds none

checkOrgMember, checkCollaborator and checkIssueComments each wrap their
whole body in try/catch and resolve false on any failure, so they never
reject. The fourteen `try { … } catch (e) { throw new Error('could not
check … auth: ' + e) }` wrappers around them — in close, reopen, lock,
milestone, retitle, unassign, assign, cc, uncc, labels/remove and in
auth.ts (getOrgCollabCommentUsers, checkCommenterAuth) — were dead code
kept green only by unit tests that mocked the callee to reject, pinning
error messages the action can never emit (including the misspelt
"couldn ot check commentor Auth").

Call the probes directly, have getOrgCollabCommentUsers reuse
checkCommenterAuth (same three probes in the same order, so identical
API traffic), and delete the fifteen mock-only unit cases. Repack dist.

Fixes cncf#386

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Killen <bkillen@linuxfoundation.org>
@github-actions

Copy link
Copy Markdown
Contributor

Please add a kind label with /kind failing-test or /kind cleanup.

@hivecommons-hive

Copy link
Copy Markdown
Contributor
9e11e3a

⚠️ Sentinel alert — maintainer review required

Hive flagged this PR (author @mrbobbytables, head 9e11e3a9b6b2) because it matches behaviors that can override security controls, escalate privileges or damage the codebase. This is a heuristic, not an accusation — a maintainer should confirm the change is intended before it merges.

  • test_removal — removes 136 test lines while adding 6 (Deletes test files or guts test coverage)
    • __tests__/issueCommentTest/approveMilestoneErrorPaths.test.ts
    • __tests__/issueCommentTest/errorPaths.test.ts
    • __tests__/label/errorPaths.test.ts
    • __tests__/utils/auth.test.ts

Hive added the sentinel-alert label. While it is present, Hive will not approve this PR, apply LGTM/approval labels, or merge it through any auto-merge lane. Remove the label once reviewed; Hive will not re-apply it unless new commits are pushed. Tune paths and behaviors under sentinel in hive.yaml or the dashboard Security tab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[quality] 14 commenter-auth catch arms (10 commands + auth.ts:181/210/217/224) are unreachable — only module mocks in unit tests keep them covered

1 participant