yanyongyu / githubkit

The modern, all-batteries-included GitHub SDK for Python, including rest api, graphql, webhooks, like octokit!
https://yanyongyu.github.io/githubkit/
MIT License
177 stars 25 forks source link

Bug: fix `require_app_auth` matches unexpected routes #42

Closed udangel-r7 closed 1 year ago

udangel-r7 commented 1 year ago

APP_ROUTES contains an r"/app" entry to match for the /app route. This will also match repositories which start with app causing require_app_auth to return true.

This change adjusts the /app regex to match at the beginning of the string and not anywhere within the url.path

yanyongyu commented 1 year ago

The app auth routes are copied from https://github.com/octokit/auth-app.js/blob/1c0d530c6b85890853b860b16f0c11e106bb676f/src/requires-app-auth.ts#L1-L22.

The regex in the octokit is https://github.com/octokit/auth-app.js/blob/1c0d530c6b85890853b860b16f0c11e106bb676f/src/requires-app-auth.ts#L45. We may need to change the regex at L29 instead.

Seems this change should be applied:

- APP_AUTH_REGEX = re.compile(rf"(?:{'|'.join(APP_ROUTES)})[^/]*$", re.I)
+ APP_AUTH_REGEX = re.compile(rf"^(?:{'|'.join(APP_ROUTES)})$", re.I)

Could you please update this regex instead and test it? I cannot make a pr review comment on unchanged lines.

brainiac commented 11 months ago

This change breaks github enterprise, because urls begin with /api/v3, this regex no longer matches for the /app endpoint

yanyongyu commented 11 months ago

🤔 That's my mistake. octokit matches the path by replacing the base url first:

https://github.com/octokit/auth-app.js/blob/c0068e06081a5d930799285a7c79c9c948b676b6/src/hook.ts#L43