|
| 1 | +--- |
| 2 | +applyTo: "{**/plugins/**/*.ts,**/packages/**/*.ts}" |
| 3 | +excludeAgent: "coding-agent" |
| 4 | +--- |
| 5 | + |
| 6 | +# Security & Authorization Review Guidelines |
| 7 | + |
| 8 | +> **Critical Security Requirement:** All API routes in Kibana must have proper authorization checks. Authorization is not optional, even for `internal` routes. |
| 9 | +
|
| 10 | +## Review Style |
| 11 | +- Be specific and actionable in feedback |
| 12 | +- Explain the "why" behind recommendations |
| 13 | +- Acknowledge good patterns when you see them |
| 14 | +- Ask clarifying questions when code intent is unclear |
| 15 | + |
| 16 | +## Authorization Configuration |
| 17 | + |
| 18 | +Routes use the `security` configuration in `KibanaRouteOptions`: |
| 19 | + |
| 20 | +```ts |
| 21 | +router.get({ |
| 22 | + path: '/api/path', |
| 23 | + security: { |
| 24 | + authz: { |
| 25 | + requiredPrivileges: ['<privilege_1>', '<privilege_2>'], |
| 26 | + }, |
| 27 | + }, |
| 28 | + ... |
| 29 | +}, handler); |
| 30 | +``` |
| 31 | + |
| 32 | +## Critical Review Checklist |
| 33 | + |
| 34 | +Focus on these three key areas when reviewing API authorization: |
| 35 | + |
| 36 | +### 1. Branching Logic with AuthzResult |
| 37 | + |
| 38 | +**REQUIRED:** If a route handler branches its logic based on user privileges (e.g., returns different data or features), it MUST use `request.authzResult` to check which privileges were granted. Do NOT use `capabilities.resolveCapabilities()` or other authorization checks for branching—`authzResult` is the single source of truth. |
| 39 | + |
| 40 | +**Patterns to search for:** |
| 41 | +- Routes with `anyRequired` or complex privilege configurations (OR logic) |
| 42 | +- Handlers that return different data or behavior based on user privileges |
| 43 | +- Handlers that conditionally expose features or data based on permissions |
| 44 | +- Handlers that check `capabilities.resolveCapabilities()` or similar methods for authorization decisions |
| 45 | +- Functions that check permissions/privileges and return boolean values used for branching |
| 46 | + |
| 47 | +** Correct Implementation:** |
| 48 | +```ts |
| 49 | +router.get({ |
| 50 | + path: '/api/path', |
| 51 | + security: { |
| 52 | + authz: { |
| 53 | + requiredPrivileges: ['privilege_3', { anyRequired: ['privilege_1', 'privilege_2'] }], |
| 54 | + }, |
| 55 | + }, |
| 56 | + ... |
| 57 | +}, (context, request, response) => { |
| 58 | + // CORRECT: Use request.authzResult to check which privileges were granted |
| 59 | + const authzResult = request.authzResult; |
| 60 | + // { |
| 61 | + // "<privilege_3>": true, |
| 62 | + // "<privilege_1>": true, |
| 63 | + // "<privilege_2>": false |
| 64 | + // } |
| 65 | + |
| 66 | + // Branch logic based on specific privileges |
| 67 | + if (authzResult.privilege_1) { |
| 68 | + // User has privilege_1, return enhanced data |
| 69 | + return response.ok({ body: ... }); |
| 70 | + } else if (authzResult.privilege_2) { |
| 71 | + // User has privilege_2, return basic data |
| 72 | + return response.ok({ body: ... }); |
| 73 | + } |
| 74 | + |
| 75 | + // Fallback (should not reach here if security config is correct) |
| 76 | + return response.ok({ body: { data: ... } }); |
| 77 | +}); |
| 78 | +``` |
| 79 | + |
| 80 | +**Flag using Capabilities Instead of AuthzResult:** |
| 81 | +```ts |
| 82 | + // WRONG: Using capabilities.resolveCapabilities() for authorization branching |
| 83 | + const canReadDecryptedParams = async (routeContext: RouteContext) => { |
| 84 | + const { request, server } = routeContext; |
| 85 | + |
| 86 | + const capabilities = await server.coreStart.capabilities.resolveCapabilities(request, { |
| 87 | + capabilityPath: 'my_capability.*', |
| 88 | + }); |
| 89 | + |
| 90 | + return capabilities.my_capability?.canReadParams ?? false; |
| 91 | + }; |
| 92 | + |
| 93 | + // In handler: |
| 94 | + if (await canReadDecryptedParams(routeContext)) { |
| 95 | + return getDecryptedParams(routeContext, paramId); |
| 96 | + } else { |
| 97 | + return getBasicParams(routeContext, paramId); |
| 98 | + } |
| 99 | + |
| 100 | + // CORRECT: Use authzResult instead |
| 101 | + router.get({ |
| 102 | + path: '/api/params', |
| 103 | + security: { |
| 104 | + authz: { |
| 105 | + requiredPrivileges: [{ anyRequired: ['read_params_decrypted', 'read_params'] }], |
| 106 | + }, |
| 107 | + }, |
| 108 | + }, (context, request, response) => { |
| 109 | + if (request.authzResult.read_params_decrypted) { |
| 110 | + return getDecryptedParams(routeContext, paramId); |
| 111 | + } else { |
| 112 | + return getBasicParams(routeContext, paramId); |
| 113 | + } |
| 114 | + }); |
| 115 | +``` |
| 116 | + |
| 117 | +### 2. Opt-Out Authorization Reason |
| 118 | + |
| 119 | +**When reviewing:** Check all routes with `authz: { enabled: false }` or `AuthzDisabled.*` usage. Verify they use predefined reasons when applicable, or provide specific context. |
| 120 | + |
| 121 | +**If a route opts out of authorization, use predefined `AuthzOptOutReason` enum or `AuthzDisabled` helpers:** |
| 122 | + |
| 123 | +```ts |
| 124 | +import { AuthzDisabled, AuthzOptOutReason } from '@kbn/core-security-server'; |
| 125 | + |
| 126 | +// CORRECT: Use predefined reason for Saved Objects client |
| 127 | +router.get({ |
| 128 | + path: '/api/path', |
| 129 | + security: { |
| 130 | + authz: AuthzDisabled.delegateToSOClient, |
| 131 | + }, |
| 132 | + ... |
| 133 | +}, handler); |
| 134 | + |
| 135 | +// CORRECT: Use predefined reason enum |
| 136 | +router.get({ |
| 137 | + path: '/api/path', |
| 138 | + security: { |
| 139 | + authz: { |
| 140 | + enabled: false, |
| 141 | + reason: AuthzOptOutReason.DelegateToSOClient, |
| 142 | + }, |
| 143 | + }, |
| 144 | + ... |
| 145 | +}, handler); |
| 146 | + |
| 147 | +// CORRECT: Custom string when no predefined reason applies |
| 148 | +router.get({ |
| 149 | + path: '/api/health', |
| 150 | + security: { |
| 151 | + authz: { |
| 152 | + enabled: false, |
| 153 | + reason: 'This route is a health check endpoint that returns no sensitive information', |
| 154 | + }, |
| 155 | + }, |
| 156 | + ... |
| 157 | +}, handler); |
| 158 | +``` |
| 159 | + |
| 160 | +**Flag if:** `reason` is: |
| 161 | + -`"Opt out from authorization"` (too generic, no context) |
| 162 | + - `"This route does not need authorization"` (no explanation why) |
| 163 | + - `"Authorization not required"` (no context provided) |
| 164 | + - `Authorization is delegated to SO Client` (semantically equivalent to the already predefined reason `AuthzOptOutReason.DelegateToSOClient`) |
| 165 | + |
| 166 | +## Summary |
| 167 | + |
| 168 | +**Key Points for Reviewers:** |
| 169 | + |
| 170 | +1. **Privilege names must follow `<operation>_<subject>` convention** |
| 171 | + - Incorrect privilege names: |
| 172 | + - `read-entity-a`: Uses `-` instead of `_` |
| 173 | + - `delete_entity-a`: Mixes `_` and `-` |
| 174 | + - `entity_manage`: Places the subject name before the operation |
| 175 | + - Correct privilege names: |
| 176 | + - `read_entity_a` |
| 177 | + - `delete_entity_a` |
| 178 | + - `manage_entity` |
| 179 | +2. **Routes with privilege-based branching MUST use `request.authzResult`** |
| 180 | +3. **Opt-out routes must have clear, specific reasons with context (not just "Opt out from authorization")* |
| 181 | + |
| 182 | +## References |
| 183 | + |
| 184 | +- [Kibana API Authorization Documentation](dev_docs/key_concepts/api_authorization.mdx) |
| 185 | +- [Kibana HTTP API Design Guidelines](dev_docs/contributing/kibana_http_api_design_guidelines.mdx) |
| 186 | + |
0 commit comments