Repository navigation
fix: certificate lookup by name leaks names of unreadable signed cert… - #1222
Conversation
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
| return unfilteredNames | ||
| } | ||
| val actor = userContextHolder.userContext?.actor | ||
| val paths = permissionCheckingService.findAllPathsByActor(actor.toString()) |
There was a problem hiding this comment.
AI reviewer found:
userContextHolder.userContext?.actor.toString() turns a missing actor into the literal string null, and we then query findAllPathsByActor("null"). Two problems:
- With no user context, we look up ACLs for a principal literally named
nullinstead of failing. Anyone with that name would match. checkPermissionsByNameusesactor!!and fails fast, so the two paths behave differently for the same missing input.
Suggest resolving the actor once with a null check (?: throw ..., or return an empty list), not stringifying a nullable. filterPermissions (line 279) has the same flaw. Fixing both here would be cheap, or we can track it as a follow-up.
There was a problem hiding this comment.
Fixed. filterNamesByPermissions and both existing filterPermissions functions, in DefaultCertificatesHandler and DefaultCredentialsHandler, now resolve the actor the same way checkPermissionsByName does. HTTP clients see no difference: UserContextInterceptor always sets the user context, or rejects the request, before these handlers run.
d4db9f8 to
9d3f42b
Compare
…cate list - Only include certificates the caller can read in the `signs` field of GET /certificates?name= when ACLs are enforced. - Require an actor when filtering certificates and credentials by permissions, instead of looking up permissions for the string "null". AI-Tool-Used: Claude AI-Tool-Use-Level: High AI-Code-Category: Production
9d3f42b to
cf0263f
Compare
dlinsley
left a comment
dlinsley
left a comment
There was a problem hiding this comment.
Thanks for the fixes and tests!
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
…ificates
signsfield of GET /certificates?name= when ACLs are enforced.ai-assisted=yes
TNZGOV-15094