Skip to content

feat: trigger audited webhook pings - #240

Merged
ecarreras merged 1 commit into
mainfrom
feat/webhook-ping-action
Oct 6, 2026
Merged

ecarreras merged 1 commit into
mainfrom
feat/webhook-ping-action

Conversation

@giscebot

@giscebot giscebot commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • add an administrator-only action to request a fresh GitHub webhook ping
  • validate the stored URL against the exact api.github.com organization/repository hook path before invoking gh api
  • audit requested, successful and failed ping actions with the administrator identity
  • keep operational CLI errors out of browser responses
  • add a Send ping control, progress feedback, delayed refresh and recent administrative actions to hook detail
  • document required webhook write permissions for the operational GitHub identity

Validation

  • pytest -q — 410 passed
  • npm test -- --run — 62 passed
  • npm run build

Refs #191

Requested by: @ecarreras

@giscebot
giscebot added this pull request to stack #235 October 5, 2026 09:42
@giscebot
giscebot requested a review from ecarreras October 5, 2026 09:43
@giscebot giscebot self-assigned this Oct 5, 2026
@ecarreras
ecarreras requested a review from pilipilisbot October 5, 2026 17:51

@pilipilisbot pilipilisbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the webhook ping action end-to-end and this looks ready.

What I checked:

  • Backend endpoint is admin-only, uses the stored hook metadata, validates the exact https://api.github.com/{org|repo}/.../hooks/{id}/pings path before invoking gh api, and does not expose raw CLI stderr/stdout to the browser.
  • Audit flow records requested/succeeded/failed actions and the hook detail endpoint returns recent administrative actions.
  • SQLite schema addition is created through the normal packaged schema path.
  • Dashboard wiring exposes the action only when a ping URL is known, gives pending/result feedback, refreshes detail state, and keeps the generated static bundle updated.
  • Docs cover the operational gh permission requirement and the browser/CLI credential separation.

Validation:

  • GitHub checks are green: pytest (3.11), pytest (3.12), and dashboard.
  • Local focused backend check in a temporary venv: pytest tests/test_webhook.py tests/test_backend.py -q -> 95 passed, 1 existing Starlette/httpx deprecation warning.

No blocking findings from my side.

Base automatically changed from feat/webhook-socket-ingress to main October 6, 2026 00:11
Let dashboard administrators request a fresh GitHub hook ping through the operational gh identity. Validate the stored API path, audit every result, refresh hook details, and avoid exposing CLI failures to the browser.\n\nRefs #191\n\nCo-authored-by: Eduard Carreras <ecarreras@gisce.net>
@giscebot
giscebot force-pushed the feat/webhook-ping-action branch from b86b920 to df47c43 Compare October 6, 2026 05:17
@ecarreras
ecarreras merged commit c511609 into main Oct 6, 2026
3 checks passed
@ecarreras
ecarreras deleted the feat/webhook-ping-action branch October 6, 2026 05:19
@giscebot

giscebot commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Post-merge audit found one confidentiality mismatch that needs a follow-up fix.

github_webhook_hook_ping stores raw gh stderr/stdout in webhook_hook_actions.detail on failure. The hook-detail endpoint then selects detail and serializes it into recent_actions, so GET /api/webhooks/github/hooks/{hook_id} returns that raw CLI failure to the browser. This contradicts both the PR's stated guarantee and docs/operations.md (“raw CLI errors ... are not returned to the browser”). The current failure test only checks the POST response; its subsequent assertion that the GET response contains permission denied: sensitive detail actually demonstrates the leak.

Recommended fix: omit the private detail field from the hook-detail response (or introduce a separately sanitized public message), retain the raw value only for server-side audit, and add a regression assertion that the GET response cannot contain the sensitive stderr text.

Integration audit: final head df47c43 is patch-equivalent to the approved change apart from rebased context, merge commit c511609 has the same tree as the final head, all three GitHub checks are green, and the focused backend/webhook suite passes locally (108 tests).

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.

3 participants