fix(proxy): restore pusher email from the token cache - #1713
sankalpsthakur wants to merge 2 commits into
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
8f35edf to
f6f62eb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1713 +/- ##
=======================================
Coverage 86.05% 86.06%
=======================================
Files 101 101
Lines 5571 5574 +3
Branches 995 997 +2
=======================================
+ Hits 4794 4797 +3
Misses 526 526
Partials 251 251 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
checkUserPushPermission keys on action.userEmail. The token cache only stored the username, so a cache hit left the last committer's email in place and the permission check ran against the wrong user. Store email with the cached identity, and run resolveUserFromToken on tag pushes as well as branch pushes. Related to finos#1400 (cache-hit and tag-chain slices; GHES / unmatched gitAccount paths are unchanged). Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
f6f62eb to
23be2ce
Compare
jescalada
left a comment
There was a problem hiding this comment.
@coopernetes Wondering if this looks appropriate and how it affects your pusher identity work 🤔
|
Thanks @sankalpsthakur for the contribution. The cache-hit path leaving the committer email in place is a real bug, and adding the resolver to the tag chain is correct. Appreciate raising this. After reviewing this change and considering the underlying flaw in how users are resolved from tokens today (only affecting the auth decision on some paths, and an incomplete fix for the problems described in #1400), we should pursue a more complete and direct fix where commit metadata (committer, tagger, author) is not considered at all for git-proxy authorization decisions as part of a git push. This is due to the simple fact that it is client controlled and it is too simple to forge in order to elevate permissions onto a repo a particular user isn't intended to have push access to. There is a more complete fix on the way which will refactor this completely. The git-proxy user can (and should) contain the additional, non-permission-affecting information such as email addresses but only for attribution checks and never for authorization. It touches the same three files this PR changes, so this would be superseded on merge either way. @jescalada I'd suggest we hold this in favour of the larger change. Once it lands, if there's anything still missing, I'd be happy to accept a contribution then. Until then, we're going to revisit these assumptions rather than continue living with them and patching around it. |
Summary
checkUserPushPermissionlooks up the pusher byaction.userEmail. After #1604,resolveUserFromTokenmaps a GitHub PAT to a GitProxy user on a cold cache, but the token cache only stored the username. On a cache hit the username was overwritten and the last-commit committer email fromparsePushwas left in place, so the permission check ran against the wrong user (the remaining path on #1400 that @jescalada reproduced). Tag pushes never ranresolveUserFromTokenat all.{ username, email }and restore both on hitresolveUserFromTokenfirst on the tag-push chain, same as branch pushesDoes not close #1400: GHES / non-github.com providers and unmatched
gitAccountare unchanged.Validation
npx vitest run test/processors/resolveUserFromToken.test.ts test/chain.test.ts test/testCheckUserPushPermission.test.tsRelated to #1400
AI/LLM disclosure
AI coding tools assisted with the change and this description.