Skip to content

Commit a84cee4

Browse files
aidandaly24aidandaly24
authored
feat(workflows): add closed-PR comment redirect (#1328)
* feat(workflows): add closed-PR comment redirect for COE AI-7 Add closed-pr-comment.yml: detects comments on closed PRs by non-maintainers, posts a redirect to the issue tracker, and pages oncall via the existing SLACK_WEBHOOK_URL. Update slack-issue-notification.yml: switch to injection-safe env+toJSON pattern and emit a uniform 20-key payload (with event_type discriminator) so the Slack workflow can branch on event type. COE AI-7: an external user reported the SDK SPII OTEL leak by commenting on a closed revert PR; oncall does not monitor closed-PR comments, so the report was missed for ~20 hours. * fix(workflows): address reviewer feedback on closed-PR redirect - Gate Slack notification on already_posted so oncall is paged only on the first external comment per PR. Subsequent comments don't page — the redirect comment has already directed the commenter to issues. - Add per-PR concurrency group so the marker-comment dedup is race-free (cancel-in-progress: false to ensure later runs still execute against the just-posted marker). - Inline steps.pr_state.outputs.state in the Slack payload directly rather than routing it through env: to avoid step-level evaluation ambiguity. The value is enum-typed (merged|closed) so injection-safe. * refactor(workflows): consolidate GitHub->Slack notifications into one file Address review feedback: - Merge closed-pr-comment.yml and slack-issue-notification.yml into a single github-slack-notifications.yml with two triggers (issues + issue_comment) and two jobs. One central place to control the GitHub -> Slack integration, as requested in review. - Remove internal incident references from the public workflow; the header now describes what the workflow does, not why it was created. No behavior change: both jobs emit the same 20-key payload the Slack workflow already consumes, gated and deduped exactly as before.
1 parent a2bd0a7 commit a84cee4

2 files changed

Lines changed: 220 additions & 24 deletions

File tree

‎.github/workflows/github-slack-notifications.yml‎

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,220 @@
1+
name: GitHub Slack Notifications
2+
3+
# Central GitHub -> Slack integration point. Two triggers feed one shared
4+
# Slack workflow (via SLACK_WEBHOOK_URL), which branches on event_type:
5+
# - issues opened -> notify oncall of a new issue
6+
# - comments on closed PRs -> redirect the commenter to open an issue,
7+
# and notify oncall (closed-PR comments are
8+
# otherwise easy to miss)
9+
# Every payload sends the same key set so the Slack workflow can branch
10+
# reliably; fields that don't apply to an event are sent empty.
11+
12+
on:
13+
issues:
14+
types: [opened]
15+
issue_comment:
16+
types: [created]
17+
18+
jobs:
19+
notify-issue-opened:
20+
if: github.event_name == 'issues'
21+
runs-on: ubuntu-latest
22+
permissions: {}
23+
steps:
24+
- name: Send issue details to Slack
25+
# Attacker-controlled fields are passed through env: rather than
26+
# interpolated into the YAML payload, to prevent workflow injection.
27+
# For issue_opened, the issue_* fields carry the data and the
28+
# pr_*/comment_* fields are empty.
29+
env:
30+
REPOSITORY: ${{ github.repository }}
31+
CREATED_AT: ${{ github.event.issue.created_at }}
32+
ISSUE_NUMBER: ${{ github.event.issue.number }}
33+
ISSUE_TITLE: ${{ github.event.issue.title }}
34+
ISSUE_URL: ${{ github.event.issue.html_url }}
35+
ISSUE_AUTHOR: ${{ github.event.issue.user.login }}
36+
ISSUE_BODY: ${{ github.event.issue.body }}
37+
LABELS: ${{ join(github.event.issue.labels.*.name, ', ') }}
38+
uses: slackapi/slack-github-action@v2.1.1
39+
with:
40+
webhook: ${{ secrets.SLACK_WEBHOOK_URL }}
41+
webhook-type: webhook-trigger
42+
payload: |
43+
event_type: "issue_opened"
44+
repository: "${{ env.REPOSITORY }}"
45+
created_at: "${{ env.CREATED_AT }}"
46+
issue_number: "${{ env.ISSUE_NUMBER }}"
47+
issue_title: ${{ toJSON(env.ISSUE_TITLE) }}
48+
issue_url: "${{ env.ISSUE_URL }}"
49+
issue_author: "${{ env.ISSUE_AUTHOR }}"
50+
issue_body: ${{ toJSON(env.ISSUE_BODY) }}
51+
labels: ${{ toJSON(env.LABELS) }}
52+
pr_number: ""
53+
pr_title: ""
54+
pr_url: ""
55+
pr_author: ""
56+
pr_state: ""
57+
pr_closed_at: ""
58+
pr_merged_at: ""
59+
comment_id: ""
60+
comment_url: ""
61+
comment_author: ""
62+
comment_body: ""
63+
64+
closed-pr-comment-redirect:
65+
# Only fire on comments left on PRs (issue_comment fires for issues too)
66+
# that are already closed, and skip comments left by bots.
67+
if: >-
68+
github.event_name == 'issue_comment' && github.event.issue.pull_request != null && github.event.issue.state ==
69+
'closed' && github.event.comment.user.type != 'Bot'
70+
runs-on: ubuntu-latest
71+
permissions:
72+
pull-requests: write
73+
issues: read
74+
# Serialize per-PR so the marker-comment dedup is race-free (otherwise two
75+
# rapid-fire comments could both see "no marker" and both post a redirect).
76+
# cancel-in-progress is false so the second run still executes after the
77+
# first finishes -- we want to evaluate dedup against the just-posted marker.
78+
concurrency:
79+
group: closed-pr-comment-${{ github.event.issue.number }}
80+
cancel-in-progress: false
81+
steps:
82+
- name: Check commenter permission
83+
id: perm
84+
uses: actions/github-script@v9
85+
with:
86+
script: |
87+
// External users on private repos can 404 here; treat any
88+
// failure as "not a maintainer" so the redirect still fires.
89+
let permission = 'none';
90+
try {
91+
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
92+
owner: context.repo.owner,
93+
repo: context.repo.repo,
94+
username: context.payload.comment.user.login,
95+
});
96+
permission = data.permission;
97+
} catch (err) {
98+
core.info(`Permission lookup failed for ${context.payload.comment.user.login}: ${err.message}. Treating as non-maintainer.`);
99+
}
100+
const skip = ['admin', 'maintain', 'write'].includes(permission);
101+
core.setOutput('skip', String(skip));
102+
core.info(`Commenter ${context.payload.comment.user.login} permission=${permission} skip=${skip}`);
103+
104+
- name: Check for existing redirect comment
105+
id: existing
106+
if: steps.perm.outputs.skip != 'true'
107+
uses: actions/github-script@v9
108+
with:
109+
script: |
110+
// Marker we embed in our reply so we don't double-post on the same PR.
111+
const marker = '<!-- closed-pr-comment-redirect -->';
112+
const comments = await github.paginate(
113+
github.rest.issues.listComments,
114+
{
115+
owner: context.repo.owner,
116+
repo: context.repo.repo,
117+
issue_number: context.payload.issue.number,
118+
per_page: 100,
119+
}
120+
);
121+
const alreadyPosted = comments.some(c => c.body && c.body.includes(marker));
122+
core.setOutput('already_posted', String(alreadyPosted));
123+
124+
- name: Post redirect comment
125+
if: steps.perm.outputs.skip != 'true' && steps.existing.outputs.already_posted != 'true'
126+
# The Slack notification is the load-bearing part of this job. If
127+
# posting the bot reply fails (rate limit, transient error), don't
128+
# block the Slack notification.
129+
continue-on-error: true
130+
uses: actions/github-script@v9
131+
env:
132+
# Repos with issue templates use /issues/new/choose; repos without
133+
# templates should change this to /issues/new.
134+
ISSUES_NEW_URL: https://github.com/${{ github.repository }}/issues/new/choose
135+
with:
136+
script: |
137+
const commenter = context.payload.comment.user.login;
138+
const issuesNewUrl = process.env.ISSUES_NEW_URL;
139+
const body = [
140+
'<!-- closed-pr-comment-redirect -->',
141+
'',
142+
`Thanks for the report, @${commenter} — feedback like this is exactly`,
143+
"how we catch the things we missed. Because this PR is already",
144+
"closed, the team won't see follow-up comments here.",
145+
'',
146+
'Would you mind opening a new issue so we can track it properly?',
147+
issuesNewUrl,
148+
'',
149+
'If this is a security issue, please report it privately via',
150+
'https://aws.amazon.com/security/vulnerability-reporting/ instead',
151+
'of a public issue.',
152+
].join('\n');
153+
154+
await github.rest.issues.createComment({
155+
owner: context.repo.owner,
156+
repo: context.repo.repo,
157+
issue_number: context.payload.issue.number,
158+
body,
159+
});
160+
161+
- name: Compute PR state
162+
id: pr_state
163+
if: steps.perm.outputs.skip != 'true'
164+
uses: actions/github-script@v9
165+
with:
166+
script: |
167+
// PRs surface as `issue` events; merged_at is null when closed-not-merged.
168+
const mergedAt = context.payload.issue.pull_request &&
169+
context.payload.issue.pull_request.merged_at;
170+
core.setOutput('state', mergedAt ? 'merged' : 'closed');
171+
172+
- name: Notify Slack
173+
# Notify oncall only on the FIRST external comment per PR (gated by
174+
# already_posted). Subsequent comments on the same PR don't notify --
175+
# the redirect comment has already directed the commenter to open an
176+
# issue, and issues notify oncall via the issue path. This bounds
177+
# notification volume regardless of how chatty a thread becomes.
178+
if: steps.perm.outputs.skip != 'true' && steps.existing.outputs.already_posted != 'true'
179+
# Attacker-controlled fields are passed through env: rather than
180+
# interpolated into the YAML payload, to prevent workflow injection.
181+
# For closed-PR comments, the issue_* fields are empty (this isn't
182+
# an issue) and the pr_*/comment_* fields carry the real data.
183+
env:
184+
REPOSITORY: ${{ github.repository }}
185+
CREATED_AT: ${{ github.event.comment.created_at }}
186+
PR_NUMBER: ${{ github.event.issue.number }}
187+
PR_TITLE: ${{ github.event.issue.title }}
188+
PR_URL: ${{ github.event.issue.html_url }}
189+
PR_AUTHOR: ${{ github.event.issue.user.login }}
190+
PR_CLOSED_AT: ${{ github.event.issue.closed_at }}
191+
PR_MERGED_AT: ${{ github.event.issue.pull_request.merged_at }}
192+
COMMENT_ID: ${{ github.event.comment.id }}
193+
COMMENT_URL: ${{ github.event.comment.html_url }}
194+
COMMENT_AUTHOR: ${{ github.event.comment.user.login }}
195+
COMMENT_BODY: ${{ github.event.comment.body }}
196+
uses: slackapi/slack-github-action@v2.1.1
197+
with:
198+
webhook: ${{ secrets.SLACK_WEBHOOK_URL }}
199+
webhook-type: webhook-trigger
200+
payload: |
201+
event_type: "closed_pr_comment"
202+
repository: "${{ env.REPOSITORY }}"
203+
created_at: "${{ env.CREATED_AT }}"
204+
issue_number: ""
205+
issue_title: ""
206+
issue_url: ""
207+
issue_author: ""
208+
issue_body: ""
209+
labels: ""
210+
pr_number: "${{ env.PR_NUMBER }}"
211+
pr_title: ${{ toJSON(env.PR_TITLE) }}
212+
pr_url: "${{ env.PR_URL }}"
213+
pr_author: "${{ env.PR_AUTHOR }}"
214+
pr_state: "${{ steps.pr_state.outputs.state }}"
215+
pr_closed_at: "${{ env.PR_CLOSED_AT }}"
216+
pr_merged_at: "${{ env.PR_MERGED_AT }}"
217+
comment_id: "${{ env.COMMENT_ID }}"
218+
comment_url: "${{ env.COMMENT_URL }}"
219+
comment_author: "${{ env.COMMENT_AUTHOR }}"
220+
comment_body: ${{ toJSON(env.COMMENT_BODY) }}

‎.github/workflows/slack-issue-notification.yml‎

Lines changed: 0 additions & 24 deletions
This file was deleted.

0 commit comments

Comments
 (0)