Skip to content

feat: Add JSON body support for incoming webhook parameters #2100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented May 22, 2025

  • Implemented support for passing incoming webhook parameters in the JSON
    request body.
  • Marked the legacy method using URL query parameters for secrets as insecure.
  • Added a warning log when the legacy method is detected.
  • Updated documentation to describe the new recommended method and the
    deprecated legacy method.
  • Updated end-to-end tests to cover both legacy and new methods.

Fixes #1093
JIRA: https://issues.redhat.com/browse/SRVKP-7678

Signed-off-by: Chmouel Boudjnah [email protected]

@Copilot Copilot AI review requested due to automatic review settings May 22, 2025 11:40
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for specifying incoming webhook parameters in the JSON request body, marks the legacy URL query parameter method as insecure, and updates tests and documentation accordingly.

  • Introduced separate tests for legacy and JSON body methods in both GitLab and GitHub webhook tests.
  • Updated the listener logic in the adapter to detect incoming webhook parameters from both query and JSON body, logging a warning for legacy usage.
  • Revised documentation to clearly indicate the recommended JSON body method and deprecate the legacy query parameter approach.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/gitlab_incoming_webhook_test.go Added tests for both legacy and JSON body methods for GitLab incoming webhooks
test/github_incoming_test.go Updated tests to support legacy and JSON body approaches for GitHub incoming webhooks
pkg/adapter/incoming_test.go Replaced hardcoded HTTP methods with http.MethodPost and added tests for legacy warnings
pkg/adapter/incoming.go Updated the webhook detection logic to handle JSON body parsing and legacy warnings
docs/content/docs/guide/incoming_webhook.md Updated documentation to describe both legacy (deprecated) and JSON body methods
Comments suppressed due to low confidence (1)

test/gitlab_incoming_webhook_test.go:98

  • [nitpick] The variable name 'incomingSecreteValue' appears to have a typo. Consider renaming it to 'incomingSecretValue' for improved clarity.
opts.ControllerURL, randomedString, randomedString, "pipelinerun-incoming", incomingSecreteValue

@chmouel chmouel added the feature New feature or request label May 22, 2025
@chmouel chmouel requested a review from zakisk May 28, 2025 06:07
@zakisk
Copy link
Contributor

zakisk commented May 28, 2025

/lgtm

Copy link

@pipelines-as-code pipelines-as-code bot left a comment

Choose a reason for hiding this comment

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

Congrats @chmouel your PR Has been approved 🎉

✅ Pull Request Approved

Approval Status:

  • Required Approvals: 1
  • Current Approvals: 1

👥 Reviewers Who Approved:

Reviewer Permission Level Approval Status
@zakisk admin

📝 Next Steps

  • Ensure all required checks pass
  • Comply with branch protection rules
  • Request a maintainer to merge using the /merge command (or merge it
    directly if you have repository permission).

Automated by the PAC Boussole 🧭

* Implemented support for passing incoming webhook parameters in the JSON
request body.
* Marked the legacy method using URL query parameters for secrets as insecure.
* Added a warning log when the legacy method is detected.
* Updated documentation to describe the new recommended method and the
deprecated legacy method.
* Updated end-to-end tests to cover both legacy and new methods.

Fixes openshift-pipelines#1093
JIRA: https://issues.redhat.com/browse/SRVKP-7678

Signed-off-by: Chmouel Boudjnah <[email protected]>
@chmouel chmouel force-pushed the issue-1093-use-post-for-incoming-webhook branch from fd51934 to 9f5b520 Compare May 28, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Development

Successfully merging this pull request may close these issues.

Switch incoming webhook shared secret to use Post data instad of query parameters
2 participants