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

Fix: Tolerate any string values in github app events array schema #118

Merged

Conversation

jmehnle
Copy link
Contributor

@jmehnle jmehnle commented Jul 20, 2024

GitHub generates check_suite webhook events with check_suite.app.events containing illegal event names. I filed this upstream at github/rest-api-description#3775.

E.g., I'm seeing events like:

{
  "check_suite": {
    ...
    "app": {
      ...
      "events": [
        "branch_protection_configuration",  // <-- Oops!
        "branch_protection_rule",
        "check_run",
        "check_suite",
        "issue_comment",
        "member",
        "membership",
        "merge_group",
        "organization",
        "pull_request",
        "pull_request_review",
        "pull_request_review_comment",
        "pull_request_review_thread",
        "push",
        "repository",
        "repository_ruleset",  // <-- Oops!
        "status",
        "team",
        "team_add",
        "workflow_job",
        "workflow_run"
      ]
    },
    ...

I believe the best course of action would be for GitHubKit to not validate the check_suite.app.events array element values.

Is this an appropriate change?

BTW, is it possible to disable validation in webhook event parsing? My presumption is that as long as the GitHub signature on the event checks out, then the event is trustworthy. Currently my GitHub application is unable to process critical events because GitHub started including these unspecified values in this kinda-unimportant field, and I would really like to go back to processing these events.

Tolerate any string values in `check_suite.app.events` array.

This works around github/rest-api-description#3775.
Copy link

codecov bot commented Jul 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.60%. Comparing base (7e66386) to head (013d514).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #118   +/-   ##
=======================================
  Coverage   17.60%   17.60%           
=======================================
  Files        4797     4797           
  Lines      222565   222565           
=======================================
  Hits        39191    39191           
  Misses     183374   183374           
Flag Coverage Δ
unittests 17.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yanyongyu
Copy link
Owner

BTW, is it possible to disable validation in webhook event parsing?

As mentioned in the readme, you could only verify the webhook body by calling githubkit.webhooks.verify and using json to parse the event as a python dict.

For webhook parsing in githubkit, it will validate the event body to prevent runtime errors caused by type errors, data missing, etc. You can choose to verify signature only and use the event body directly to skip the validation.

@yanyongyu yanyongyu added schema schema related WebHook labels Jul 20, 2024
@jmehnle
Copy link
Contributor Author

jmehnle commented Jul 20, 2024

Thanks. I'm considering your suggestion of avoiding githubkit.webhooks.parse altogether, but given GitHub's tendency of violating their own spec it seems in practice this renders githubkit.webhooks.parse useless. :-(

@yanyongyu
Copy link
Owner

It seems many webhook event types related with github app contain the events field. I will search for these fields and make a workaround.

pyproject.toml Outdated Show resolved Hide resolved
@yanyongyu yanyongyu changed the title Work around GitHub schema bug: Tolerate any string values in check_suite.app.events array Fix: Tolerate any string values in github app events array schema Jul 20, 2024
@yanyongyu yanyongyu added the bug Something isn't working label Jul 20, 2024
@yanyongyu yanyongyu merged commit 756f83a into yanyongyu:master Jul 20, 2024
34 checks passed
@jmehnle
Copy link
Contributor Author

jmehnle commented Jul 20, 2024

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working schema schema related WebHook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants