Skip to content
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

Store payload in Webhook History & make webhook ID clickable to show corresponding payload #3467

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaitjacob
Copy link
Contributor

In this PR,

  1. Webhook Payload is stored in Webhook History table.
  2. The Webhook ID is rendered as a link to open the payload in new tab. The JSON data is passed as a HTML Data Attribute along with the markup.
    image
  3. Payload opened in new tab using javascript function showJson,
    image

Reference,
https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes

Fixed #634



def upgrade():
op.add_column('webhook_history', sa.Column('payload', sa.Text()))

Check warning

Code scanning / vcs-diff-lint

Trailing whitespace Warning

Trailing whitespace
const jsonData = element.getAttribute('data-json');
if(jsonData === "null") return;
const newTab = window.open();
newTab.document.write('<pre>' + JSON.stringify(JSON.parse(jsonData), null, 2) + '</pre>');

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
is reinterpreted as HTML without escaping meta-characters.
@praiskup
Copy link
Member

praiskup commented Oct 8, 2024

Thank you very much for your work on this!

Team: Could someone start a discussion on the Fedora Devel mailing list
regarding whether it's considered "safe enough" to make the payload publicly
available? My initial feeling is that payloads from Forges should be safe
to publish, yet Forges don't typically do so. It would be helpful to get input
from the involved parties and let them help us decide.

@FrostyX
Copy link
Member

FrostyX commented Oct 9, 2024

We also discussed that we could make the payloads downloadable only for project admins. Which is already done because the whole Settings > Integrations page requires login. The problem is that we do daily database dumps and allow anyone to download them. So we would have to exclude this table from the dumps. There is a precedent for this, which is for example a table with user API keys for copr-cli.

We talked that it would make our life easier if the webhook_history.payload column stored not the payload itself but only a link or command where to get it. I did some investigation how that would work for GitHub.

  • I hoped that since I am logged in on GitHub, they would have some magic that would allow me to access auth-only endpoints in the browser. I couldn't find any way to do this
  • They don't allow user+password authentication and require a token. Then curl or gh can be used. But this is more setup steps than I would like
  • I couldn't find a way to point to the website either. There is e.g. https://github.com/fedora-copr/copr/settings/hooks/407079055?tab=deliveries but then I still have to manually search the webhook by UUID

So, not very user-friendly.

@praiskup
Copy link
Member

They don't allow user+password authentication and require a token. Then curl or gh can be used. But this is more setup steps than I would like

If we provide documentation on how to do the boilerplate setup (token?), and the table self-links this documentation, I don't think it is toooo bad. 👍

@jaitjacob jaitjacob marked this pull request as ready for review October 14, 2024 09:00
@jaitjacob jaitjacob marked this pull request as draft October 14, 2024 09:00
@jaitjacob
Copy link
Contributor Author

sorry marked it ready for review by mistake. please ignore.

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.

Provide webhook payload for triggered builds
3 participants