fix(security): manager loaders prefetch GuardOrganization#343
fix(security): manager loaders prefetch GuardOrganization#343Nikola-BS wants to merge 2 commits into
Conversation
Atama-Heitekava
left a comment
Atama-Heitekava
left a comment
There was a problem hiding this comment.
Critical problem
Leak cross organisations
Description
No verification if the caller is a member of the asked organisation in the endpoint getActiveOrganization. The user can ask permission to access a ressource, be granted access to this ressource, and read another one.
Suggested change
Add a verification of the member organisation (like it has been done in the stats.getAll endpoint.
Major problem
2 path to decide which organisation
Description
2 different verifications for the organisation :
- active org : used by most of the project
- slug uRL : new system introduce in this branch
It must be harmonized
Suggested changes
Make sure that organizationProcedure verify the organisation from the slug of the route, and verify memberhip one time, in the middleware (updating context.organizationId value to be used).
Minor problems
Oversize fetch
Description
findBySlugWithDetails fetch every members, with their user cards, and every invitations, just to solve slug --> id and test membership. It is really heavy,
Suggested change
Dedicate another endpoint that return only the concerned id
Test
Descritpion
No test on the critical errror above (all with the same orga), and the slug is replace by an id in the test
| ); | ||
| if (!org) throw new ORPCError('NOT_FOUND'); | ||
|
|
||
| const isMember = org.members.some((m) => m.user.id === context.user.id); |
There was a problem hiding this comment.
the isMember verification must also be added to the getActiveOrganization endpoint
2 participants
close #323