Bugfix/broken access control on observability REST APIs (IDOR)#590
Bugfix/broken access control on observability REST APIs (IDOR)#590
Conversation
fix(frontend): resolve typescript any types and hook dependency bugs
Fixing the Credentials DB Consistency Issue
merge from head branch
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughChangesModified
ImpactUsers can now only retrieve observability logs and metrics for runtimes they are authorized to access. The change maintains existing API response shapes and behavior for authorized users. WalkthroughAdds request-scoped user context extraction and runtime-level authorization to observability endpoints. The service now reads the incoming Authorization header, converts it to a Changes
Sequence DiagramsequenceDiagram
participant Client
participant ObservabilityService
participant AuthModule
participant StorageService
participant Adapter
Client->>ObservabilityService: HTTP request (with Authorization header)
ObservabilityService->>AuthModule: extractUserContextV2(Authorization)
AuthModule-->>ObservabilityService: UserContextV2
ObservabilityService->>ObservabilityService: resolveRuntimeIds()
ObservabilityService-->>ObservabilityService: runtimeIdList
loop per runtimeId
ObservabilityService->>StorageService: hasAccessToRuntime(userId, runtimeId)
StorageService-->>ObservabilityService: accessGranted / accessDenied
end
ObservabilityService->>ObservabilityService: filterRuntimeIdsForUser() -> filtered runtimeIdList
ObservabilityService->>ObservabilityService: apply component/environment filters
ObservabilityService->>Adapter: invoke with filtered runtime IDs
Adapter-->>ObservabilityService: adapter response
ObservabilityService-->>Client: HTTP response
Estimated code review effortHigh | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
Co-authored-by: wso2-engineering[bot] <229087779+wso2-engineering[bot]@users.noreply.github.com>
Co-authored-by: wso2-engineering[bot] <229087779+wso2-engineering[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
icp_server/observability_service.bal (2)
176-184: Extract the user context beforeresolveRuntimeIdsto fail fast.
resolveRuntimeIdsalready hits the DB (potentially multiplestorage:getRuntimescalls under a component×environment cross-product). If the Authorization header is malformed orextractUserContextV2fails, that work is wasted and the same logic is duplicated across the logs and metrics handlers.Moving the user-context extraction above
resolveRuntimeIdsin both resource functions also narrows the auth-vs-data ordering: every request that reaches the storage layer will have already produced a validateduserId, which simplifies future auditing/logging and reduces the DoS surface for tokens that decode but are semantically invalid.♻️ Suggested ordering (logs handler)
resource function post logs(http:Request request, types:ICPLogEntryRequest logRequest) returns types:LogEntriesResponse|http:Response|error { log:printDebug("Received log request: " + logRequest.toString()); + types:UserContextV2 userContext = check extractUserFromObservabilityRequest(request); + // Transform ICPLogEntryRequest to LogEntryRequest by resolving component/environment filters to runtime IDs string[] runtimeIdList = check resolveRuntimeIds({ componentId: logRequest.componentId, componentIdList: logRequest.componentIdList, environmentId: logRequest.environmentId, environmentList: logRequest.environmentList }); - - types:UserContextV2 userContext = check extractUserFromObservabilityRequest(request); runtimeIdList = check filterRuntimeIdsForUser(userContext.userId, runtimeIdList);Apply the analogous change at lines 262-270 for the metrics handler.
Also applies to: 262-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@icp_server/observability_service.bal` around lines 176 - 184, Move the user extraction to run before DB work: call extractUserFromObservabilityRequest(request) and assign types:UserContextV2 userContext before invoking resolveRuntimeIds so malformed/invalid auth fails fast; then pass userContext.userId into filterRuntimeIdsForUser as you already do. Apply the same reorder in the metrics handler (the analogous block that calls resolveRuntimeIds and filterRuntimeIdsForUser) so extraction of userContext always happens prior to resolveRuntimeIds.
134-147: N+1 database roundtrips per request underfilterRuntimeIdsForUser.
storage:hasAccessToRuntimeissues ~3 DB queries per runtime (runtime lookup + integration-access view + optional environment restriction). Called sequentially in this loop, a query that resolved M runtimes will incur ~3M synchronous roundtrips before the adapter is even called, which can dominate request latency for broad component/environment filters and contradicts the "keep performance stable" goal stated in the PR objectives.Consider a single batch authorization check that scopes directly in SQL, e.g. an
IN (?, ?, ...)join againstv_user_integration_access(plus environment restriction) that returns the subset of accessible runtime IDs in one roundtrip. This also avoids partial-failure states where some checks succeed and a later one errors out, leaving the caller unsure whether partial filtering occurred.♻️ Sketch (pseudo-API)
-isolated function filterRuntimeIdsForUser(string userId, string[] runtimeIds) returns string[]|error { - string[] filtered = []; - foreach string rid in runtimeIds { - boolean|error allowed = storage:hasAccessToRuntime(userId, rid); - if allowed is error { - return allowed; - } - if allowed { - filtered.push(rid); - } - } - return filtered; -} +isolated function filterRuntimeIdsForUser(string userId, string[] runtimeIds) returns string[]|error { + if runtimeIds.length() == 0 { + return runtimeIds; + } + // Single DB roundtrip returning only the subset of runtimeIds the user can access. + return storage:filterAccessibleRuntimes(userId, runtimeIds); +}Implementation on the storage side would be roughly:
SELECT r.runtime_uuid FROM runtimes r JOIN v_user_integration_access v ON v.integration_uuid = r.integration_uuid AND v.user_uuid = ? LEFT JOIN <env_restriction_view> e ON e.user_uuid = v.user_uuid AND e.integration_uuid = r.integration_uuid WHERE r.runtime_uuid IN (?, ?, ...) AND (r.env_uuid IS NULL OR e.env_uuid IS NULL OR e.env_uuid = r.env_uuid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@icp_server/observability_service.bal` around lines 134 - 147, The current filterRuntimeIdsForUser loops and calls storage:hasAccessToRuntime per runtime causing N+1 DB roundtrips; replace this with a single batch storage call that returns the subset of accessible runtime IDs in one query (eg. add/use a storage method like storage:getAccessibleRuntimeIds(userId, runtimeIds) or storage:filterAccessibleRuntimes(userId, runtimeIds) which implements an IN(...) join against v_user_integration_access with the environment restriction), then have filterRuntimeIdsForUser call that single method, propagate any error directly, and return the resulting string[] to avoid partial-success states and make authorization one roundtrip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@icp_server/observability_service.bal`:
- Around line 176-184: Move the user extraction to run before DB work: call
extractUserFromObservabilityRequest(request) and assign types:UserContextV2
userContext before invoking resolveRuntimeIds so malformed/invalid auth fails
fast; then pass userContext.userId into filterRuntimeIdsForUser as you already
do. Apply the same reorder in the metrics handler (the analogous block that
calls resolveRuntimeIds and filterRuntimeIdsForUser) so extraction of
userContext always happens prior to resolveRuntimeIds.
- Around line 134-147: The current filterRuntimeIdsForUser loops and calls
storage:hasAccessToRuntime per runtime causing N+1 DB roundtrips; replace this
with a single batch storage call that returns the subset of accessible runtime
IDs in one query (eg. add/use a storage method like
storage:getAccessibleRuntimeIds(userId, runtimeIds) or
storage:filterAccessibleRuntimes(userId, runtimeIds) which implements an IN(...)
join against v_user_integration_access with the environment restriction), then
have filterRuntimeIdsForUser call that single method, propagate any error
directly, and return the resulting string[] to avoid partial-success states and
make authorization one roundtrip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 177c2044-fdf7-4250-990d-5988caba42a7
📒 Files selected for processing (1)
icp_server/observability_service.bal
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Purpose
Goals
Automation tests
Test environment
OS: Windows 10
Database: MySQL (Docker, mysql-db from icp_server/docker-compose.local.yml)
Backend runtime: Ballerina (inside Docker image wso2/icp_server)
Learning
Reused existing authorization patterns already present in the codebase (JWT validation via service config + auth:extractUserContextV2 and storage access checks such as hasAccessToRuntime) to keep the fix consistent and low-risk.