feat(spp_api_v2): add outgoing API log model, service, and auditor security#38
feat(spp_api_v2): add outgoing API log model, service, and auditor security#38
Conversation
Summary of ChangesHello @jeremi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the API auditing capabilities by introducing a new system for logging outgoing API calls. It provides a detailed record of external interactions, crucial for troubleshooting, compliance, and security analysis. The changes also include a specialized auditor role with restricted access to sensitive log data, ensuring that only authorized personnel can view detailed request and response payloads. This feature set improves the overall observability and security posture of the API V2 module. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive feature for logging outgoing API calls, including a new model, service, views, and security configurations, and adds a new 'Auditor' role to control access to sensitive log data. While the implementation is robust, well-structured, and thoroughly tested, and the code is clean, there are two medium-severity security concerns related to insecure data handling. The promised 'automatic payload masking' is missing from the service implementation, leading to the potential logging of unmasked secrets. Additionally, the full URL of outgoing calls is logged and exposed to low-privileged users, which could leak sensitive information if secrets are passed in query parameters. Addressing these security issues is crucial to prevent data leakage from the logging system itself. There is also one minor suggestion for improving code style in a new test file.
There was a problem hiding this comment.
The OutgoingApiLogService is missing the 'automatic payload masking' functionality mentioned in the pull request description. The current implementation only performs truncation of large payloads via _truncate_payload, but it does not redact sensitive information such as credentials, tokens, or PII from the request and response summaries. Storing unmasked sensitive data in the database, even if restricted to auditors, poses a security risk. Please implement a masking mechanism to redact sensitive keys (e.g., 'Authorization', 'password', 'token') before logging.
| url = fields.Char( | ||
| required=True, | ||
| index=True, | ||
| help="Full URL called", | ||
| ) |
There was a problem hiding this comment.
The url field stores the full URL of outgoing API calls, which may contain sensitive information such as API keys or tokens in query parameters. Unlike the payload fields, the url field is not restricted to the group_api_v2_auditor group, making it visible to any user with 'viewer' access to the logs. Consider adding groups="spp_api_v2.group_api_v2_auditor" to this field or implementing logic to strip sensitive query parameters before logging.
There was a problem hiding this comment.
For better code style and to avoid repeated imports, import json should be moved to the top of the file with other imports. This also applies to the import on line 176.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #38 +/- ##
==========================================
- Coverage 71.31% 61.87% -9.45%
==========================================
Files 299 388 +89
Lines 23618 31578 +7960
==========================================
+ Hits 16844 19538 +2694
- Misses 6774 12040 +5266
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| def _compute_display_name(self): | ||
| for record in self: | ||
| timestamp_str = record.timestamp.strftime("%Y-%m-%d %H:%M") if record.timestamp else "" | ||
| record.display_name = f"{record.http_method} {record.endpoint or 'API Call'} @ {timestamp_str}" |
There was a problem hiding this comment.
Display name fallback uses literal string, not URL
Medium Severity
_compute_display_name falls back to the literal string 'API Call' when endpoint is empty, but the test test_display_name_falls_back_to_url asserts that the URL appears in display_name when endpoint is not set. The computed expression record.endpoint or 'API Call' never references record.url, and url is also missing from the @api.depends decorator. This mismatch means the test will fail, and the intended behavior of showing the URL as a fallback identifier is not implemented.
Additional Locations (1)
Restrict sensitive PII fields in API logs to a dedicated group_api_v2_auditor group. Regular viewers can see log metadata (timestamp, endpoint, status, duration) but not payloads or error details. - Add privilege_api_v2_auditor and group_api_v2_auditor - Auditor implies Viewer; Admin implies Auditor - Restrict outgoing log: request_summary, response_summary, error_detail - Restrict audit log: search_parameters, fields_returned, extensions_returned, ip_address, user_agent, error_detail - Hide payload pages and error_detail in outgoing log form view - Add field-level security tests for both models
Wire up the spp.api.outgoing.log model with manifest registration, ACL rules (viewer read-only, officer/manager read+write), the OutgoingApiLogService wrapper with payload truncation and error resilience, and an "Outgoing API Logs" menu item under API V2.
… API log - Add groups restriction to url field and XML view to limit access to auditors - Add _sanitize_url method to strip sensitive query parameters before logging - Call _sanitize_url in log_call so stored URLs never contain tokens or keys - Add tests covering _sanitize_url and end-to-end URL sanitization in log_call
…urity leak The _compute_display_name method was falling back to record.url when endpoint was not set. Since url has groups="spp_api_v2.group_api_v2_auditor" but display_name is store=True with no groups restriction, the URL value was being persisted into an unrestricted field, bypassing field-level security. Also adds url to @api.depends implicitly by removing the reference entirely. Replace the url fallback with a generic "API Call" string.
jeremi
force-pushed
the
feat/api-outgoing-log
branch
from
ff1b282 to
6438991
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| self.assertTrue(log.display_name) | ||
| self.assertIn("GET", log.display_name) | ||
| self.assertIn("https://example.org/api/test", log.display_name) |
There was a problem hiding this comment.
Test expects URL in display_name but model uses literal fallback
Medium Severity
test_display_name_falls_back_to_url asserts the URL appears in display_name, but _compute_display_name falls back to the literal string 'API Call' when endpoint is not set — not the url field. This test will always fail. Since url is restricted via groups="spp_api_v2.group_api_v2_auditor", using it in the stored display_name would leak sensitive data to non-auditors, so the model fallback is likely correct and the test assertion is wrong.
Additional Locations (1)
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
Reviewed code and tested manually on a running instance. Architecture is clean — model stores, service sanitizes (masking, URL sanitization, truncation). Auditor group field-level security verified working via XML-RPC.
Minor items for follow-up:
- Model
log_calldocstring saysrequest_summary: Request payload (secrets redacted)but the model doesn't redact — only the service does. Misleading docstring. error_detailis not sanitized by the service layer — could leak internal infrastructure details (IPs, hostnames). The field is gated by auditor group in the UI, but consider sanitizing at the service level too.
Neither is a blocker. Good to merge.
3 participants
Summary
spp.api.outgoing.logmodel to track outgoing API requests (URL, method, headers, payload, response, status, duration)OutgoingApiLogServicefor creating and querying log entries with automatic payload maskingTest plan
test_single_module.sh spp_api_v2Note
Medium Risk
Introduces new persistence of external request/response metadata and expands audit/security rules; mistakes in masking or field-group restrictions could leak sensitive data, though changes are well-tested and scoped to logging/UI.
Overview
Adds a new
spp.api.outgoing.logmodel + menu/views to record outgoing HTTP calls (URL/endpoint/method, request/response summaries, status codes, duration, origin context, errors) and expose them in the API V2 configuration UI.Introduces
OutgoingApiLogServiceto write these logs safely viasudo(), while masking common secret/token fields, sanitizing sensitive query params in URLs, and truncating oversized payloads; failures to log are swallowed to avoid breaking the caller.Tightens log data exposure by adding an opt-in
group_api_v2_auditor/privilege_api_v2_auditorand gating sensitive fields on both the existingspp.api.audit.log(IP/user-agent/search params/fields returned/error detail) and the new outgoing log model, with new access rules and tests covering field-levelAccessErrorbehavior.Written by Cursor Bugbot for commit ff1b282. This will update automatically on new commits. Configure here.