🛂 fix: Validate types Query Param in People Picker Access Middleware

[danny-avila]

Summary

Fixed an authorization bypass in checkPeoplePickerAccess where the middleware only inspected req.query.type (singular) while the controller exclusively consumed req.query.types (plural). An authenticated caller could send ?types=group to skip type-specific permission enforcement entirely, receiving principal data their role was not permitted to access.

• Replaced the single type-only check with a unified collection step that reads both type and types from req.query and validates every collected principal type against the caller's role permissions before calling next().
• Adjusted the mixed-search fallback condition from !type to requestedTypes.size === 0 so it correctly triggers when neither parameter contributes a recognized type.
• Hoisted VALID_PRINCIPAL_TYPES to a module-level Set constant, eliminating a per-request Object.keys() allocation that previously rebuilt the same three-element structure on every call.
• Updated the catch block error log to include both type and types query values, replacing the previous entry that always showed type = undefined for types-only requests.
• Restored the JSDoc comment with explicit per-type permission requirements (user → VIEW_USERS, group → VIEW_GROUPS, role → VIEW_ROLES, no type → any of the above), which was lost in the initial fix commit.
• Corrected TPrincipalSearchParams in accessPermissions.ts from the stale type?: PrincipalType (singular) to types?: Array<PrincipalType.USER | PrincipalType.GROUP | PrincipalType.ROLE>, aligning the shared type with the actual controller API.
• Corrected TPrincipalSearchResponse in accessPermissions.ts from the stale type?: (singular) to types?: Array<...> | null, where null accurately represents the controller setting typeFilters = null when all provided type values are invalid, distinct from undefined when the parameter is absent entirely.
• Added 8 new unit tests covering: the types-param bypass scenario, comma-separated multi-type strings, array-format types (Express qs parsing), combined type + types params, all-invalid type values falling through to mixed search, empty-string types with no permissions, PrincipalType.PUBLIC treated as mixed search, and a missing .json() assertion on an existing partial-denial test.

Change Type

• Bug fix (non-breaking change which fixes an issue)

Testing

Verified with Jest unit tests in api/server/middleware/checkPeoplePickerAccess.spec.js. All 20 tests pass.

Key scenarios exercised:

• ?types=group with VIEW_GROUPS=false → 403 (the original bypass, now blocked)
• ?types=user,role with VIEW_ROLES=false → 403 on role
• ?types=user,group with both VIEW_USERS=true and VIEW_GROUPS=true → 200
• ?types[]=group&types[]=role (array form) with VIEW_GROUPS=false → 403
• ?type=user&types=group with VIEW_GROUPS=false → 403
• ?types=foobar → mixed-search fallback, next() called when user has any VIEW permission
• ?types= (empty string) with all permissions false → 403
• ?types=public → mixed-search fallback, next() called (PUBLIC absent from valid types)

Test Configuration

• No external services required
• Run from api/ workspace: npx jest checkPeoplePickerAccess

Checklist

• My code adheres to this project's style guidelines
• I have performed a self-review of my own code
• I have commented in any complex areas of my code
• I have made pertinent documentation changes
• My changes do not introduce new warnings
• I have written tests demonstrating that my changes are effective or that my feature works
• Local unit tests pass with my changes

[Copilot]

Pull request overview

This PR closes an authorization gap in the people picker access middleware by ensuring permission checks apply to both the legacy type query param and the controller-supported types param, preventing bypass of type-specific permissions.

Changes:

• Update checkPeoplePickerAccess to collect requested principal types from both type and types and enforce permissions for each requested type.
• Adjust “mixed search” handling to key off “no valid requested types” rather than only “no type”.
• Add test coverage for types as comma-separated strings, arrays, and combined type + types inputs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
api/server/middleware/checkPeoplePickerAccess.js	Enforces per-type permission checks across both type and types query params.
api/server/middleware/checkPeoplePickerAccess.spec.js	Adds regression tests to ensure types cannot bypass type-specific permission checks.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.