-
Notifications
You must be signed in to change notification settings - Fork 0
Return empty rather than forbidden on no access #190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
avoid leaking information about presence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modifies the API’s behavior to prevent leaking information by returning empty responses instead of forbidden errors when access is not allowed. Key changes include updating HTTP status codes and response bodies in tests, adjusting error handling and owner checks in the API endpoints, and removing the forbidden error helper.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| broker/test/api/api-handler_test.go | Updated tests to reflect empty responses rather than forbidden errors for unauthorized access. |
| broker/api/api-handler.go | Revised error and access handling logic for multiple endpoints to return empty responses or not found errors. |
adamdickmeiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also remove '403': # Example error response from the open-api.yaml file to indicate it's no longer a possible response.
adamdickmeiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One condition could be simplified.
Co-authored-by: Adam Dickmeiss <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the API behavior to return an empty response (or a not found error) instead of a forbidden error when access is not granted, in order to avoid information leakage about resource presence. Key changes include:
- Modifying test assertions in the API handler tests to expect empty responses or a not found error.
- Updating error handling in the API endpoints to return an empty JSON array or a not found error instead of a forbidden error.
- Introducing a helper function (httpGetTrans) in tests for unmarshalling transaction lists.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| broker/test/api/api-handler_test.go | Tests updated to check for empty responses and changed status codes. |
| broker/api/api-handler.go | API endpoints updated error handling logic to return empty or not found responses instead of forbidden. |
Files not reviewed (1)
- broker/oapi/open-api.yaml: Language not supported
avoid leaking information about presence