Commit 71b7152
github: eager bot-user-ID auto-detection (slice of upstream 9824d33) (#128)
* github: eager bot-user-ID auto-detection (slice of upstream 9824d33)
Port the github slice of upstream `9824d33` (PR #441 adapter-hardening
pass) so `is_me` checks work and self-reply loops are prevented in
multi-tenant deployments.
Adds `_detect_bot_user_id` mirroring upstream's exact sequence and
fallback:
- try `users.getAuthenticated` (`GET /user`) first — works for PAT mode
and (returns the bot user) for installation tokens too
- on failure, fall back to `apps.getAuthenticated` (`GET /app`) and
resolve the bot user via `users.getByUsername`
(`GET /users/{slug}[bot]`)
Detection runs eagerly on the first webhook for an installation (before
dispatch, so the very first reply sees a populated bot id) and is cached
on `self._bot_user_id` so it's fetched once per process, not per webhook.
`initialize()` and `remove_reaction()` now route through the same method.
`_github_api_request` gains an `installation_id` override so detection
can authenticate for the webhook's installation in multi-tenant mode
(reuses existing `_get_installation_token` plumbing).
Tests (mock the GitHub API client via AsyncMock):
- first webhook for a new installation populates `_bot_user_id` before
message handling proceeds
- a message authored by the bot itself is filtered as `is_me`
- PAT path uses the `users.getAuthenticated` (`GET /user`) path
- second webhook for same installation does not re-fetch (cache hit)
Refs #98. github slice of the 4-part `9824d33` security pass.
https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
* fix(github): auth /app with App JWT and serialize bot-id detection
Addresses two PR #128 review findings on bot-user-ID auto-detection:
HIGH (Gemini): GET /app must use an App JWT, not an installation token.
_detect_bot_user_id's fallback calls apps.getAuthenticated (GET /app),
but _github_api_request unconditionally exchanged the App JWT for an
installation token whenever _app_credentials was set. GitHub's /app
endpoint rejects installation tokens (401/403), so bot-id detection was
broken for the GitHub-App/installation case (the primary /user path also
403s on installation tokens, so the /app fallback is what must work).
Special-case path == "/app" in the auth-selection branch to authenticate
with self._generate_app_jwt() directly; all other App-auth requests keep
the installation-token exchange (and its RuntimeError when no installation
id is resolvable). /users/{slug}[bot] is a public lookup and works fine
with the installation token.
MEDIUM (Gemini): race condition + await-in-try in _detect_bot_user_id.
Concurrent webhooks could all trigger redundant detection before the first
cached the result. Add an asyncio.Lock (_detect_lock, created in __init__)
and double-checked locking: fast-path early-return on cache hit, then
acquire the lock and re-check inside it before running the detection
sequence with each await inside its try block. Best-effort semantics
(errors logged, not raised) preserved.
Tests (tests/test_github_dispatch.py):
- TestAppEndpointAuthSelection.test_app_endpoint_uses_jwt_not_installation_token:
fakes the HTTP layer so the real auth-selection logic runs; asserts /app
is sent "Bearer APP_JWT" (not the installation token) while /user and
/users/{slug}[bot] use the installation token. Fails against the broken
code (where /app went through installation-token exchange).
- TestConcurrentDetectionFetchesOnce.test_concurrent_detect_fetches_once:
fires 8 concurrent _detect_bot_user_id calls with a slow mocked /user;
asserts the API is hit exactly once. Fails without the lock (8 calls).
https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 316953b commit 71b7152
2 files changed
Lines changed: 422 additions & 23 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
120 | 120 | | |
121 | 121 | | |
122 | 122 | | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
123 | 127 | | |
124 | 128 | | |
125 | 129 | | |
| |||
201 | 205 | | |
202 | 206 | | |
203 | 207 | | |
204 | | - | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
205 | 243 | | |
206 | | - | |
207 | | - | |
| 244 | + | |
| 245 | + | |
208 | 246 | | |
209 | | - | |
| 247 | + | |
210 | 248 | | |
211 | 249 | | |
212 | | - | |
| 250 | + | |
213 | 251 | | |
214 | 252 | | |
| 253 | + | |
215 | 254 | | |
216 | | - | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
217 | 259 | | |
218 | | - | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
219 | 279 | | |
220 | 280 | | |
221 | 281 | | |
| |||
306 | 366 | | |
307 | 367 | | |
308 | 368 | | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
309 | 376 | | |
310 | 377 | | |
311 | 378 | | |
| |||
629 | 696 | | |
630 | 697 | | |
631 | 698 | | |
632 | | - | |
633 | | - | |
634 | | - | |
635 | | - | |
636 | | - | |
637 | | - | |
| 699 | + | |
| 700 | + | |
| 701 | + | |
| 702 | + | |
638 | 703 | | |
639 | 704 | | |
640 | 705 | | |
| |||
1059 | 1124 | | |
1060 | 1125 | | |
1061 | 1126 | | |
1062 | | - | |
| 1127 | + | |
| 1128 | + | |
| 1129 | + | |
1063 | 1130 | | |
1064 | 1131 | | |
1065 | 1132 | | |
1066 | 1133 | | |
| 1134 | + | |
| 1135 | + | |
| 1136 | + | |
| 1137 | + | |
| 1138 | + | |
1067 | 1139 | | |
1068 | 1140 | | |
1069 | 1141 | | |
1070 | | - | |
| 1142 | + | |
1071 | 1143 | | |
1072 | | - | |
1073 | | - | |
1074 | | - | |
1075 | | - | |
1076 | | - | |
1077 | | - | |
1078 | | - | |
1079 | | - | |
| 1144 | + | |
| 1145 | + | |
| 1146 | + | |
| 1147 | + | |
| 1148 | + | |
| 1149 | + | |
| 1150 | + | |
| 1151 | + | |
| 1152 | + | |
| 1153 | + | |
| 1154 | + | |
| 1155 | + | |
| 1156 | + | |
| 1157 | + | |
| 1158 | + | |
| 1159 | + | |
| 1160 | + | |
| 1161 | + | |
| 1162 | + | |
1080 | 1163 | | |
1081 | 1164 | | |
1082 | 1165 | | |
| |||
0 commit comments