🐛 Embed screenshots as base64 in issues; GHA processes into images#4327
🐛 Embed screenshots as base64 in issues; GHA processes into images#4327clubanderson merged 1 commit intomainfrom
Conversation
#4220) The previous approach uploaded screenshots to .github/screenshots/ via the GitHub Contents API, which requires push access to the repo. This meant any user without write access (i.e., every non-org-member running localhost) got a 404 on screenshot upload — the feature never worked for external users. New approach: - Backend embeds screenshots as base64 data URIs in collapsible <details> blocks with machine-readable markers (<!-- screenshot-base64:N -->) - Issue is created with the base64 data inline — no push access needed - New GHA workflow (process-screenshots.yml) triggers on issues.opened, finds the markers, decodes base64, commits images to .github/screenshots/ using the workflow's GITHUB_TOKEN (which has write access), and replaces the base64 blocks with rendered  markdown - Frontend messages updated: "will render as an image shortly" instead of "uploaded and attached" - Token permission warnings for Contents scope removed (no longer needed) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
kubestellar-prow
bot
added
the
size/L
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
There was a problem hiding this comment.
Pull request overview
This PR changes how screenshots are attached to feedback/feature-request GitHub issues so that non-org-members can submit screenshots without needing repo push access, by embedding base64 data URIs in the issue body and using a GitHub Actions workflow to commit images and rewrite the issue body to image markdown.
Changes:
- Frontend: update screenshot success/failure messaging to reflect “processed shortly” behavior.
- Backend: embed screenshots as base64
<details>blocks with machine-readable markers in the created issue body. - CI/Automation: add an
issues.openedGitHub Actions workflow to decode embedded screenshots, commit them under.github/screenshots/, and update the issue body with rendered image links.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| web/src/components/feedback/FeedbackModal.tsx | Updates UI messaging around screenshot attachment; removes token-permission guidance that assumed Contents API uploads. |
| web/src/components/feedback/FeatureRequestModal.tsx | Updates UI messaging around screenshot attachment; trims unused GitHub token constants import. |
| pkg/api/handlers/feedback.go | Replaces Contents API screenshot upload with base64 embedding blocks in the issue body. |
| .github/workflows/process-screenshots.yml | New workflow to extract base64 screenshots from issues, commit images, and rewrite the issue body. |
| // Validate the data URI format | ||
| parts := strings.SplitN(dataURI, ",", 2) | ||
| if len(parts) != 2 { | ||
| ssResult.Failed++ | ||
| log.Printf("[Feedback] Failed to upload screenshot %d: %v", i+1, err) | ||
| if strings.Contains(err.Error(), "404") { | ||
| log.Printf("[Feedback] Hint: a 404 from the GitHub Contents API usually means the token lacks 'Contents: Read and write' permission. "+ | ||
| "Ensure FEEDBACK_GITHUB_TOKEN has this scope for %s/%s.", repoOwner, repoName) | ||
| } | ||
| log.Printf("[Feedback] Screenshot %d: invalid data URI format", i+1) | ||
| continue | ||
| } | ||
| ssResult.Uploaded++ |
There was a problem hiding this comment.
The new screenshot “validation” only checks that the data URI contains a comma, then increments Uploaded. If the URI isn’t actually data:image/<type>;base64,... (or contains invalid base64), the issue will still report screenshots as “attached”, but the process-screenshots.yml regex won’t match / the workflow will fail to commit the image.
Consider validating that the header matches the exact format the workflow expects (including data:image/*;base64,) and validating/normalizing base64 (e.g., padding + decode check like uploadScreenshotToGitHub already does) before counting it as uploaded/embedded.
| # Only run if the issue body contains our base64 screenshot marker | ||
| if: contains(github.event.issue.body, '<!-- screenshot-base64:') |
There was a problem hiding this comment.
This workflow grants contents: write and runs on issues.opened. That means anyone who can open an issue (including first-time external users) can cause the workflow to commit arbitrary blobs into the repo under .github/screenshots/ by including the marker/base64 block, effectively escalating “issue write” into “repo write”.
Please add a trust gate (e.g., only process issues opened by a specific bot/service account, or require author_association to be MEMBER/OWNER, or verify an HMAC/signature emitted by the backend) and/or move storage to a safer target (separate repo/branch) to avoid unintended write access and repo-bloat abuse.
| # Only run if the issue body contains our base64 screenshot marker | |
| if: contains(github.event.issue.body, '<!-- screenshot-base64:') | |
| # Only run for trusted issue authors when the issue body contains our | |
| # base64 screenshot marker. This prevents arbitrary repository writes from | |
| # untrusted external users who can open issues. | |
| if: > | |
| contains(github.event.issue.body, '<!-- screenshot-base64:') && | |
| ( | |
| github.event.issue.author_association == 'OWNER' || | |
| github.event.issue.author_association == 'MEMBER' || | |
| github.event.issue.author_association == 'COLLABORATOR' | |
| ) |
| // Find all screenshot-base64 markers | ||
| const markerRegex = /<!-- screenshot-base64:(\d+) -->\n<details>\n<summary>Screenshot \d+ \(processing\.\.\.\)<\/summary>\n\n```\n(data:image\/[^;]+;base64,[A-Za-z0-9+/=\n]+)\n```\n\n<\/details>/g; | ||
|
|
||
| let match; | ||
| const screenshots = []; | ||
| while ((match = markerRegex.exec(body)) !== null) { | ||
| screenshots.push({ | ||
| fullMatch: match[0], | ||
| index: parseInt(match[1]), | ||
| dataUri: match[2].replace(/\n/g, ''), |
There was a problem hiding this comment.
markerRegex is extremely strict (exact newlines, exact <summary> text, and a restricted base64 character class). Small formatting changes on the backend, CRLF normalization, or URL-safe base64 (-/_) would prevent matches and leave large base64 blobs stuck in the issue body.
Consider making the parsing more resilient: locate each <!-- screenshot-base64:N --> marker first, then parse the following fenced code block with more flexible newline handling (\r?\n) and a broader base64 matcher, instead of matching the entire <details> block verbatim.
| // Find all screenshot-base64 markers | |
| const markerRegex = /<!-- screenshot-base64:(\d+) -->\n<details>\n<summary>Screenshot \d+ \(processing\.\.\.\)<\/summary>\n\n```\n(data:image\/[^;]+;base64,[A-Za-z0-9+/=\n]+)\n```\n\n<\/details>/g; | |
| let match; | |
| const screenshots = []; | |
| while ((match = markerRegex.exec(body)) !== null) { | |
| screenshots.push({ | |
| fullMatch: match[0], | |
| index: parseInt(match[1]), | |
| dataUri: match[2].replace(/\n/g, ''), | |
| // Find all screenshot-base64 markers, then parse the following fenced | |
| // code block more flexibly so minor formatting changes do not prevent | |
| // cleanup of embedded base64 content. | |
| const markerRegex = /<!-- screenshot-base64:(\d+) -->/g; | |
| const fencedDataUriRegex = /```[^\r\n]*\r?\n\s*(data:image\/[^;]+;base64,[A-Za-z0-9+/_=\r\n-]+)\s*\r?\n```/; | |
| let match; | |
| const screenshots = []; | |
| while ((match = markerRegex.exec(body)) !== null) { | |
| const markerStart = match.index; | |
| const markerEnd = markerRegex.lastIndex; | |
| const remainingBody = body.slice(markerEnd); | |
| const fencedMatch = remainingBody.match(fencedDataUriRegex); | |
| if (!fencedMatch || fencedMatch.index === undefined) { | |
| continue; | |
| } | |
| const fencedStart = markerEnd + fencedMatch.index; | |
| const fencedEnd = fencedStart + fencedMatch[0].length; | |
| const detailsCloseIndex = body.indexOf('</details>', fencedEnd); | |
| const fullMatchEnd = detailsCloseIndex === -1 | |
| ? fencedEnd | |
| : detailsCloseIndex + '</details>'.length; | |
| screenshots.push({ | |
| fullMatch: body.slice(markerStart, fullMatchEnd), | |
| index: parseInt(match[1], 10), | |
| dataUri: fencedMatch[1].replace(/[\r\n]/g, ''), |
| // Decode base64 to verify it's valid | ||
| const content = b64Content; | ||
|
|
||
| // Commit the image to the repo | ||
| const filePath = `.github/screenshots/${requestId}/screenshot-${ss.index}.${ext}`; | ||
| console.log(`Uploading ${filePath}...`); | ||
|
|
||
| const { data } = await github.rest.repos.createOrUpdateFileContents({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| path: filePath, | ||
| message: `Add screenshot ${ss.index} for issue #${issueNumber}`, | ||
| content: content, | ||
| committer: { | ||
| name: 'github-actions[bot]', | ||
| email: '41898282+github-actions[bot]@users.noreply.github.com', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The script comment says “Decode base64 to verify it's valid”, but the code never decodes/validates it (const content = b64Content). If the input is malformed or missing padding, createOrUpdateFileContents will fail; also there’s no size limit, so a large base64 payload could cause excessive repo growth.
Suggested fixes: actually validate/normalize base64 (e.g., decode with Buffer.from(..., 'base64'), enforce a max byte size, then re-encode to base64 for the API), and consider handling reruns gracefully (if the file already exists, fetch its sha before calling createOrUpdateFileContents).
| import { FETCH_DEFAULT_TIMEOUT_MS, COPY_FEEDBACK_TIMEOUT_MS } from '../../lib/constants' | ||
| import { FEEDBACK_UPLOAD_TIMEOUT_MS } from '../../lib/constants/network' | ||
| import { GITHUB_TOKEN_MANAGE_URL, GITHUB_TOKEN_FINE_GRAINED_PERMISSIONS, GITHUB_TOKEN_CLASSIC_SCOPE } from '../../lib/constants/github-token' | ||
| // github-token constants no longer needed here — screenshots are embedded as base64 |
There was a problem hiding this comment.
There are still comments in this file that describe the old “upload screenshots to GitHub and embed as markdown images” behavior, but screenshots are now embedded as base64 in the issue body and later processed by GitHub Actions. Please update the surrounding documentation/comments (including the top-of-file header comment) to match the new two-step flow to avoid misleading future maintainers.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 2 code suggestion(s) and 3 general comment(s). @copilot Please apply all of the following code review suggestions:
Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
Fixes #4220 — screenshot uploads never worked for non-org-members because the Contents API requires push access to the repo.
Root cause
The backend uploaded screenshots via
PUT /repos/{owner}/{repo}/contents/which requires write/push access. Any user without org membership (i.e., every localhost user) got a 404. This was never a token scope issue — it was a fundamental permissions issue.New approach (two-step)
Backend embeds screenshots as base64 data URIs in the issue body inside collapsible
<details>blocks with machine-readable markers:No push access needed — just Issues write permission.
GHA workflow (
process-screenshots.yml) triggers onissues.opened:<!-- screenshot-base64:N -->markers.github/screenshots/using the workflow'sGITHUB_TOKEN(which has write access)markdownOther changes
uploadScreenshotToGitHubfunction retained but no longer called from the issue creation pathTest plan
<details>block<details>blocks, no workflow trigger