Skip to content

security: verify GitHub webhooks and isolate Chromatic secrets - #18954

Open
pillowtalk-Qy wants to merge 7 commits into
ethereum:devfrom
pillowtalk-Qy:codex/supply-chain-security
Open

security: verify GitHub webhooks and isolate Chromatic secrets#18954
pillowtalk-Qy wants to merge 7 commits into
ethereum:devfrom
pillowtalk-Qy:codex/supply-chain-security

Conversation

@pillowtalk-Qy

@pillowtalk-Qy pillowtalk-Qy commented Aug 1, 2026

Copy link
Copy Markdown

Risk

The good-first-issue endpoint currently accepts unauthenticated JSON and can trigger Discord notifications. The Storybook Chromatic token is also committed in ci.yml, so copied workflows can publish against the ethereum.org project and consume its quota.

This PR fails closed at both boundaries. It must not merge until the administrator rollout below is complete: a missing webhook secret intentionally returns 500, and Chromatic publishing requires repository Secrets that are intentionally unavailable to forks and Dependabot.

Changes

GitHub webhook

  • Verify X-Hub-Signature-256 against the exact request bytes with HMAC-SHA256 and timingSafeEqual.
  • Return 401 for missing, malformed, or incorrect signatures before JSON parsing or Discord side effects.
  • Stream the body with a 1 MiB hard limit; reject oversized deliveries with 413.
  • Return 400 for signed malformed JSON or malformed issue payloads.
  • Preserve signed ping/irrelevant-event handling and the valid good-first-issue Discord path.

Chromatic and fork boundary

  • Replace the committed Storybook token with secrets.CHROMATIC_STORYBOOK_TOKEN.
  • Restrict both secret-backed Chromatic jobs (visual-tests and page-visual-tests) to non-draft PRs whose head repository ID and name match ethereum/ethereum-org-website.
  • Exclude Dependabot, which GitHub treats as a fork trust boundary and does not give Actions secrets.
  • Do not synthesize a successful UI Tests status for an untested fork commit.

Least privilege

  • Set the default workflow GITHUB_TOKEN permission to contents: read.
  • Grant only the additional read permissions needed by release E2E and preserve the existing Lighthouse PR-comment permission.

The duplicate claude-review-translations.yml fix has been removed from this PR and remains owned by #18953. The final diff contains only the security work above and its tests.

Administrator rollout (blocking)

GitHub webhook, before merge

  1. Generate a strong webhook secret.
  2. Add it to the production Netlify environment as GITHUB_WEBHOOK_SECRET.
  3. Configure the same value as the repository webhook secret for /api/gfi-issues-webhook.
  4. Send a GitHub test delivery and confirm a signed ping returns 200.
  5. Merge/deploy only after both sides use the same value.

Setting the environment and webhook secret before deployment is backward-compatible: the current endpoint ignores the new signature header.

Chromatic, before merge

  1. Approve the fork Actions run for final SHA b4dab9c780aaece3d569aad708c0abe05f434b3e so the non-secret checks can execute.
  2. Push or cherry-pick the final SHA to an internal ethereum/ethereum-org-website branch so the trusted visual path can be exercised.
  3. Regenerate/rotate the exposed Storybook project token in Chromatic.
  4. Immediately save the new value as the repository Actions secret CHROMATIC_STORYBOOK_TOKEN.
  5. Re-run the internal PR and confirm Storybook Chromatic, page visual snapshots, and the associated Chromatic App checks are posted successfully.
  6. Confirm the old committed token can no longer publish.

If a Chromatic App check is required, fork PRs cannot merge directly because they intentionally receive no Chromatic secret or result. The secure workflow is to move the reviewed commit to an internal branch and run Chromatic there. Do not create a privileged pull_request_target workflow that marks an untested fork SHA successful.

I could not inspect Actions secrets, webhook configuration, or fork approval policy with contributor credentials, so these remain explicit administrator checks.

Test evidence (final SHA)

  • Targeted webhook suite: 10 passed.
  • Full unit suite: 1000 passed, 1 skipped.
  • pnpm lint: passed.
  • pnpm type-check: passed.
  • USE_MOCK_DATA=true NEXT_PUBLIC_BUILD_LOCALES=en pnpm build: passed; 411 static pages generated.
  • Prettier and git diff --check: passed.
  • actionlint v1.7.12: no new findings versus origin/dev; both report the same pre-existing Lighthouse manifest output typing warning.
  • Final diff against current dev: four files; no claude-review-translations.yml diff.

Rollback

  • Webhook: revert the webhook commits. GitHub may keep sending signed requests because the old handler ignores the signature header.
  • Chromatic: use the existing CHROMATIC_SKIP=** emergency control, then adjust the trusted-source condition or permissions while retaining Secret references. Never restore the exposed token.
  • Rotate any replacement secret if logs or configuration suggest it was disclosed.

Unresolved

  • Administrator-only secret creation, token rotation, webhook configuration, fork Actions approval, and trusted internal-branch validation.
  • Fork PRs intentionally need an internal maintainer branch for trusted visual coverage.
  • Delivery replay/idempotency is not added here; a legitimate GitHub redelivery can still create a duplicate Discord notification.
  • The current dev version of claude-review-translations.yml has a pre-existing invalid multiline if expression. Its fix is deliberately excluded here and remains in ci: enforce a production release gate #18953.

@netlify

netlify Bot commented Aug 1, 2026

Copy link
Copy Markdown

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit b4dab9c
🔍 Latest deploy log https://app.netlify.com/projects/ethereumorg/deploys/6a722567fa5b6f0008a73b71
😎 Deploy Preview https://deploy-preview-18954.ethereum.it
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 59 (🟢 up 3 from production)
Accessibility: 95 (no change from production)
Best Practices: 100 (no change from production)
SEO: 98 (no change from production)
PWA: 60 (no change from production)
View the detailed breakdown and full score reports
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added the tooling 🔧 Changes related to tooling of the project label Aug 1, 2026
@pillowtalk-Qy

Copy link
Copy Markdown
Author

Live CI note: GitHub created a no-job failure for the unchanged .github/workflows/claude-review-translations.yml. actionlint reproduces it on origin/dev at line 62: a comment is embedded inside the multiline job if expression and GitHub rejects the expression parser input. This is pre-existing and outside this PR diff; run: https://github.com/ethereum/ethereum-org-website/actions/runs/30701334871. It should be fixed in the release-reliability workflow follow-up, not folded into this supply-chain change.

@pillowtalk-Qy

Copy link
Copy Markdown
Author

Security follow-up db4eb7267c is pushed. It moves the disabled-trigger comments out of the if: | expression, so the latest SHA no longer creates the no-job claude-review-translations.yml validation failure seen on a28514b. It also adds explicit malformed-signature and malformed-issue-payload regressions.

Evidence on the latest SHA:

  • targeted webhook suite: 10 passed
  • full unit suite: 978 passed, 1 skipped
  • lint, type-check, Prettier, YAML parsing, actionlint and git diff --check: passed
  • English mock production build: 411 static pages
  • Netlify deploy preview, Header rules, Redirect rules and GitGuardian: passed

The remaining CI run is awaiting fork-workflow approval: https://github.com/ethereum/ethereum-org-website/actions/runs/30709016390

Do not merge until an administrator completes both rollout blocks in the PR body: configure the matching production/repository webhook secret and verify a signed ping; rotate the exposed Chromatic token, add CHROMATIC_STORYBOOK_TOKEN, and validate the trusted internal-branch visual checks.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🔎 First-pass review — 🔧 Needs work

Solid supply-chain hardening, but it carries external prerequisites that must be met at merge. Three parts: (1) CI hardening — adds permissions: contents: read at workflow + job level and restricts the Chromatic visual-tests job to non-fork branches of this repo; (2) Chromatic token — moves the hardcoded projectToken: fee8e66c9916 to secrets.CHROMATIC_STORYBOOK_TOKEN; (3) Webhook auth — adds HMAC-SHA256 signature verification (security.ts: timing-safe compare, 1 MiB body cap, strict payload type-guards) to app/api/gfi-issues-webhook, with a thorough unit test that includes GitHub's documented test vector. The webhook code is well-written and fail-closed.

Before merge (blocking — merging without these breaks live behavior):

  • Create the CHROMATIC_STORYBOOK_TOKEN secret first, or visual-tests publishes with an empty token. Also confirm the intended policy that fork PRs no longer get Storybook Chromatic.
  • Configure GITHUB_WEBHOOK_SECRET on the deployment and the GitHub webhook — the route now returns 500 when it's unset and 401 on bad signatures, so GFI Discord notifications stop until the secret is set. (The previously hardcoded token in git history should be rotated regardless.)

Non-blocking: same claude-review-translations.yml edit as #18952/#18953 (three-way conflict — consolidate).

Analysis

Lane: tooling + code (.github/workflows/{ci,claude-review-translations}.yml, app/api/gfi-issues-webhook/{route.ts,security.ts}, tests/unit/api/gfi-issues-webhook.spec.ts). Base reported as dev.

Checked: verifyGitHubWebhookSignature uses crypto.timingSafeEqual with a length guard and validates the sha256=<64hex> shape; readWebhookBody streams with a byte cap and honors content-length; payload type-guards run before use; the unit test asserts 401/400/413/500 paths and the HMAC vector. No unused vars, no raw Intl.*. Metadata integrity-filtered — diff-only review. Fork PR from a first-time contributor touching secrets + CI, so an engineer should confirm the secret/webhook rollout before merge.

Routing: needs dev approval 🧑💻.

Generated by PR Backlog Sweeper · 495.5 AIC · ⌖ 31.9 AIC · ⊞ 6K ·

@pillowtalk-Qy

pillowtalk-Qy commented Aug 4, 2026

Copy link
Copy Markdown
Author

Follow-up to the automated first-pass review: the code-side findings are addressed on final SHA b4dab9c780aaece3d569aad708c0abe05f434b3e.

  • Merged current dev without rewriting branch history.
  • Removed the duplicate claude-review-translations.yml change; that fix remains owned by ci: enforce a production release gate #18953.
  • Kept the webhook change scoped to raw-body HMAC verification, a 1 MiB streaming cap, fail-closed responses, payload validation, and regression tests.
  • Replaced the committed Storybook token with CHROMATIC_STORYBOOK_TOKEN and restricted both secret-backed Chromatic jobs to trusted same-repository, non-Dependabot PRs.
  • Kept default Actions permissions at contents: read, with narrow job-level grants only where needed.

Final local evidence: targeted webhook tests 10/10; full unit suite 1000 passed and 1 skipped; lint, type-check, Prettier, git diff --check, and English production build passed. actionlint reports no new finding versus current dev; both trees have the same pre-existing Lighthouse manifest output typing warning.

@wackerow @pettinarip, the remaining work is administrator-only and blocks merge:

  1. Approve the fork Actions run for the final SHA so non-secret CI can run.
  2. Configure the same strong GITHUB_WEBHOOK_SECRET in the production Netlify environment and the GitHub webhook, then verify a signed ping returns 200.
  3. Rotate the exposed Storybook Chromatic token, add the replacement as the repository Actions secret CHROMATIC_STORYBOOK_TOKEN, and confirm the old token can no longer publish.
  4. Move/cherry-pick the final SHA to a trusted internal branch and verify both Chromatic jobs plus the associated App checks.
  5. Re-run the full check set on this final SHA after the configuration is complete.

Please do not merge before those steps are complete: without the webhook secret the route deliberately fails closed, and without the new Chromatic secret the trusted Storybook publish cannot work. The invalid claude-review-translations.yml expression currently inherited from dev is intentionally outside this PR and may still produce a separate no-job workflow failure until #18953 is resolved.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🔎 First-pass review — 🔧 Needs work

Re-review of final SHA b4dab9c. The code-side findings from the Aug 4 review are resolved: the duplicate claude-review-translations.yml edit is gone (now owned by #18953), the webhook is scoped to raw-body HMAC-SHA256 verification (timing-safe compare, 1 MiB streaming cap, strict payload type-guards, fail-closed 401/413/500), the hardcoded Chromatic projectToken is replaced by secrets.CHROMATIC_STORYBOOK_TOKEN, and both Chromatic jobs (visual-tests, page-visual-tests) are restricted to trusted same-repo, non-Dependabot PRs. Default Actions perms are contents: read. Clean, well-tested work.

What remains is admin-only and blocks merge — merging without these breaks live behavior:

  • Create the CHROMATIC_STORYBOOK_TOKEN repo secret and rotate the previously-committed token, or the visual jobs publish with an empty/exposed token.
  • Set GITHUB_WEBHOOK_SECRET on the deployment + GitHub webhook — the route now 500s when unset and 401s on bad signatures, so GFI Discord notifications stop until it's configured.
  • Approve the fork Actions run for the final SHA so non-secret CI can report.
Analysis

Lane: tooling + code (.github/workflows/ci.yml, app/api/gfi-issues-webhook/{route,security}.ts, unit test). Base dev.

Checked: verifyGitHubWebhookSignature validates the sha256=<64hex> shape + length-guarded timingSafeEqual; readWebhookBody honors content-length and caps by bytes; payload type-guards run before use; the unit test covers the HMAC vector + 401/400/413/500 paths and asserts Discord isn't called on rejection. No duplicate workflow edit remains.

Fork PR from a first-time contributor touching secrets + CI. Routing: needs dev approval 🧑💻.

Generated by PR Backlog Sweeper · 407.6 AIC · ⌖ 29.4 AIC · ⊞ 6K ·

@pillowtalk-Qy

Copy link
Copy Markdown
Author

Admin follow-up on final SHA b4dab9c780aaece3d569aad708c0abe05f434b3e: the Aug 5 automated re-review confirmed that the code-side findings are resolved, and this branch has not received further code changes.

@wackerow @pettinarip, could you confirm whether the blocking rollout is planned or completed?

  • Configure the same strong GITHUB_WEBHOOK_SECRET in the production Netlify environment and the GitHub webhook, then verify a signed ping returns 200.
  • Rotate the previously committed Storybook Chromatic token, create the repository secret CHROMATIC_STORYBOOK_TOKEN, and confirm the old token can no longer publish.
  • Approve the final fork Actions run so non-secret CI can report.
  • Move/cherry-pick the final SHA to a trusted internal branch and verify both secret-backed Chromatic jobs plus their App checks.
  • Re-run the complete check set on the final SHA after configuration.

Please do not merge before those steps are confirmed: the webhook intentionally fails closed without its secret, and trusted Storybook publishing requires the replacement Secret. No additional code work is proposed on #18954.

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

Labels

tooling 🔧 Changes related to tooling of the project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant