feat: block PAT from cross-org endpoints and fix principal.ID resolution#1516
feat: block PAT from cross-org endpoints and fix principal.ID resolution#1516
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughChanges introduce subject ID resolution via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/v1beta1connect/user.go (1)
346-353:⚠️ Potential issue | 🟡 MinorValidate
subjectIDis non-empty before update.
ResolveSubject()returns(p.ID, p.Type)whenp.PATis nil. If somehow a PAT principal has a nil PAT field,subjectIDwould be the PAT's ID (not the user ID), which is the bug this change aims to fix.Additionally, if
subjectIDis empty (edge case),userService.Updatewould fall through toUpdateByName("")per the relevant code snippet atcore/user/service.go:112-123, potentially causing unexpected behavior.Consider adding validation:
🛡️ Proposed fix
subjectID, _ := principal.ResolveSubject() + if subjectID == "" { + return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) + } updatedUser, err := h.userService.Update(ctx, user.User{ ID: subjectID,internal/api/v1beta1connect/preferences.go (1)
188-198:⚠️ Potential issue | 🟡 MinorValidate
subjectIDis non-empty before creating preferences.Same concern as in
user.go: ifsubjectIDresolves to empty string (edge case), preferences would be created with an emptyResourceID. Per the relevant code snippet atinternal/store/postgres/preference_repository.go:1-50, empty strings are stored directly without validation.🛡️ Proposed fix
subjectID, _ := principal.ResolveSubject() + if subjectID == "" { + return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) + } var createdPreferences []preference.Preference for _, prefBody := range req.Msg.GetBodies() {
🧹 Nitpick comments (1)
pkg/server/connect_interceptors/authorization.go (1)
36-56: Consider PAT check consistency for streaming handlers.The PAT denial check is only applied in
WrapUnary(line 60-67) but not inWrapStreamingHandler. If any streaming endpoints should be blocked for PAT principals in the future, they won't be protected.This is likely fine for now since the blocked endpoints (org/session operations) appear to be unary RPCs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d65d24f7-133d-48e5-83d0-50f1ccd851af
📒 Files selected for processing (3)
internal/api/v1beta1connect/preferences.gointernal/api/v1beta1connect/user.gopkg/server/connect_interceptors/authorization.go
Coverage Report for CI Build 24075403743Coverage decreased (-0.001%) to 41.146%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Description:
Summary
principal.IDusage in handlers where PAT ID was incorrectly used as user IDProblem
Endpoints in
authorizationSkipEndpointsbypass authorization entirely. When called with a PAT:principal.IDis the PAT ID, not the user IDprincipal.IDdirectly, which is the PAT ID instead of the user IDChanges
Interceptor deny list (
authorization.go)Added
patDeniedEndpointsmap checked before the authz logic. When principal is PAT and endpoint is in the deny list, returnsPermissionDenied. Runs afterAuthenticationInterceptorso principal is already cached in context.Blocked endpoints:
Handler fixes
Replaced
principal.IDwithprincipal.ResolveSubject()which correctly returns user ID for PAT, user ID for user, and serviceuser ID for serviceuser:UpdateCurrentUser(user.go) — was updating with PAT ID as user IDCreateCurrentUserPreferences(preferences.go) — was storing preferences under PAT IDListCurrentUserPreferences(preferences.go) — was querying preferences by PAT IDTest plan
go build ./...passesmake lint-fix— 0 issuesPermissionDeniedPermissionDenied