Skip to content

Show PR links directly in Slack notifications - #38

Merged
KhushiSharma22 merged 1 commit into
mainfrom
codex/slack-visible-pr-link
Sep 28, 2026
Merged

KhushiSharma22 merged 1 commit into
mainfrom
codex/slack-visible-pr-link

Conversation

@KhushiSharma22

Copy link
Copy Markdown
Collaborator

Slack PR notifications currently expose the GitHub URL only through the action button. Show the full clickable PR URL directly below the title/author details, and include it in the notification fallback text so it is also available in notification previews and accessible text.

Applies to draft, ready-for-review, and merged notifications. Keeps the existing action button; contributor-provided titles remain in plain-text blocks, while only the trusted GitHub URL uses Slack link formatting.

Validation: all 47 contributor automation tests pass, with the existing payload test checking visible and fallback links for all three notification stages. git diff --check passes. Live Slack rendering remains to be checked after merge.

@github-actions

Copy link
Copy Markdown

Thanks for contributing to the Cyberwave Python SDK, @KhushiSharma22!

A repository maintainer will follow up. When this PR is ready, we request a maintainer review and run an automated advisory review. CI and human review remain the source of truth.

Please describe what changed and how you tested it. See the contribution guide or ask for help on Discord. Drafts are welcome.

Merged human contributions receive a Discord thank-you. To opt out, check the PR template's Discord opt-out box or add <!-- cyberwave:no-celebration --> to the PR description.

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Slack team notifications

  • opened: sent
  • merged: sent

@github-actions

Copy link
Copy Markdown

Automated advisory review · Claude

Commit: 9a0a603785bf6b7f47e5bb73a566af1fe7970806. Human review and CI are still required.

This review covers two patch excerpts: a Slack payload builder in a GitHub Actions script and its accompanying test. The only substantive change is adding an mrkdwn-typed section block containing the PR URL, and appending the URL to the fallback text field. One concrete defect is visible in the new mrkdwn block when the PR title contains Slack mrkdwn metacharacters injected via the URL path segment, but on closer inspection the URL is constructed from trusted repository metadata and a numeric PR number, so that specific vector is low-risk. However, there is a real structural bug in the test's section-index assumptions versus the actual block order produced by the updated code.

  • Medium · .github/scripts/tests/test_contributors.py: The test asserts sections[1]["text"] is the new mrkdwn URL block and sections[2]["text"]["type"] is plain_text for the action string. After the patch the payload block list is: header, details section (plain_text), URL section (mrkdwn), action section (plain_text), actions. Filtering for type==section yields index 0=details, index 1=URL, index 2=action, which matches the assertions. However, the old assertion 'all sections are plain_text' was intentionally verifying that no untrusted input could inject an mrkdwn block. The replacement assertion only checks sections[0] and sections[2], silently allowing an attacker-controlled mrkdwn block to appear in any other position (e.g., if a future code change moves untrusted content into an mrkdwn block). More critically, the test title still claims it verifies 'untrusted title is only plain_text and cannot override url', but it no longer asserts that sections[0] does not contain the injected mrkdwn payload from the title (it only checks the type field, not the absence of the injection string). A PR title of '<!channel>' would pass sections[0] type check while still containing the injection in the text value. The fix is to add assertNotIn('<
  • Medium · .github/scripts/contributors.py: The new mrkdwn block uses Slack angle-bracket link syntax '<{url}>' where url is composed of github.repository concatenated with pr['number']. The pr dict comes from GitHub webhook payloads. Although pr['number'] is an integer in normal GitHub events, if the repository field were ever sourced from untrusted input (e.g., a fork's self-reported name in certain workflow triggers), a repository string containing '|evil label' could turn the rendered Slack link into '<org/repo|evil label>/pull/42', changing the display text of the hyperlink in Slack while keeping the raw URL intact. The consequence is limited to display-name spoofing in the mrkdwn block. The fix is to validate or sanitize github.repository before embedding it in the mrkdwn string, for example by stripping or rejecting any '|' or '>' characters, or by constructing the link with the numeric ID only.

Coverage: 2 of 2 changed files; patch excerpts only. Generated REST code, lockfiles, notebooks, SVGs, missing patches and files beyond the 40-file / 60,000-character budget are excluded. GitHub may truncate individual patches.

@KhushiSharma22
KhushiSharma22 merged commit c6f8ad1 into main Sep 28, 2026
10 checks passed
@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Thanks @KhushiSharma22 — your contribution has been merged!

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.

1 participant