refactor: logging with retention + API token hardening#9148
Merged
sriramveeraghanta merged 6 commits intoMay 27, 2026
Conversation
…text logging - Make `allowed_rate_limit` read-only on APITokenSerializer so users can no longer raise their own API token rate limit via PATCH (GHSA-xfgr-2x3f-g2cf). - Stop persisting API keys in plaintext in APITokenLogMiddleware: store a SHA-256 hash as the token identifier and redact sensitive request headers (X-Api-Key, Authorization, Cookie) before logging (GHSA-r5p8-cj3q-38cc).
Logs are now written to and cleared from PostgreSQL only; MongoDB is no longer used as a log sink or archive. - Drop the MongoDB write/archival paths from the API request logger, the webhook log writer, and the cleanup tasks; Postgres is the sole sink. - Cleanup tasks now hard-delete expired rows in batches via `all_objects` (rows are removed immediately, not soft-deleted). - Add env-backed, per-log-type retention settings: API activity logs (API_ACTIVITY_LOG_RETENTION_DAYS, default 14), webhook logs (WEBHOOK_LOG_RETENTION_DAYS, default 14), email logs (EMAIL_LOG_RETENTION_DAYS, default 7). HARD_DELETE_AFTER_DAYS no longer drives any log cleanup. - Delete settings/mongo.py, remove MONGO_DB_* settings and the plane.mongo loggers, and drop the pymongo dependency.
sriramveeraghanta
requested review from
dheeru0198 and
pablohashescobar
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR removes MongoDB integration across Plane's logging and cleanup infrastructure, migrating entirely to PostgreSQL-only persistence. It adds SHA-256 HMAC identifiers for API tokens and header redaction in middleware logging, establishes retention-day settings for log expiration, and refactors the cleanup task to batch-delete via primary keys. ChangesMongoDB Removal and PostgreSQL Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sriramveeraghanta
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes MongoDB as a logging sink (request logs, webhook logs, email logs now persist only to PostgreSQL) and adds per-log-type retention windows with hard deletes via the generic process_cleanup_task. It also addresses two security advisories: APITokenSerializer.allowed_rate_limit becomes read-only so users can't escalate their own throttle (GHSA-xfgr-2x3f-g2cf), and APITokenLogMiddleware now stores a SHA-256 hash of the API key and redacts X-Api-Key / Authorization / Cookie headers (GHSA-r5p8-cj3q-38cc).
Changes:
- Drop MongoDB code paths (
settings/mongo.py,pymongo,plane.mongologgers,mock_mongodbfixture, archival branches in cleanup/logger/webhook tasks) and slimprocess_cleanup_taskto PostgreSQL hard-delete-by-id batches keyed off newAPI_ACTIVITY_LOG_RETENTION_DAYS/WEBHOOK_LOG_RETENTION_DAYS/EMAIL_LOG_RETENTION_DAYSsettings. - Harden token logging in
APITokenLogMiddleware: hashX-Api-Keyfortoken_identifierand redact sensitive request headers; drop legacy mongo payload fromprocess_logs. - Mark
allowed_rate_limitas read-only onAPITokenSerializerand add a contract test verifying PATCH cannot change it.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/requirements/base.txt | Drops pymongo dependency. |
| apps/api/plane/settings/common.py | Removes MONGO_DB_* settings; adds three retention-day settings. |
| apps/api/plane/settings/local.py / production.py | Removes plane.mongo logger. |
| apps/api/plane/settings/mongo.py | Deletes the MongoDB connection singleton. |
| apps/api/plane/middleware/logger.py | Hashes token_identifier, redacts sensitive headers, drops mongo payload. |
| apps/api/plane/bgtasks/logger_task.py | Reduces process_logs to PostgreSQL-only persistence. |
| apps/api/plane/bgtasks/webhook_task.py | save_webhook_log writes directly to PostgreSQL only. |
| apps/api/plane/bgtasks/cleanup_task.py | Generic cleanup now hard-deletes batches by id using new retention settings. |
| apps/api/plane/app/serializers/api.py | Adds allowed_rate_limit to read_only_fields. |
| apps/api/plane/tests/contract/app/test_api_token.py | Adds PATCH read-only test for allowed_rate_limit. |
| apps/api/plane/tests/unit/middleware/test_logger.py | New unit tests for hashing + header redaction. |
| apps/api/plane/tests/unit/bg_tasks/test_cleanup_task.py | New unit tests for retention + hard-delete behavior. |
| apps/api/plane/tests/conftest_external.py | Removes mock_mongodb fixture. |
| apps/api/plane/tests/README.md | Updates docs to remove MongoDB references. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/middleware/test_logger.py (1)
38-58: ⚡ Quick winAdd explicit coverage for
AuthorizationandCookieredaction.This suite only proves
X-API-Keyredaction today. Since the middleware now treats three headers as sensitive, please includeAuthorizationandCookiein the request and assert their raw values never appear inlog_data["headers"].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/unit/middleware/test_logger.py` around lines 38 - 58, Update the test_sensitive_headers_are_redacted test (and/or the helper _captured_log_data) to include Authorization and Cookie headers in the constructed request (use HTTP_AUTHORIZATION and HTTP_COOKIE when calling request_factory.get) so the middleware processes them alongside X-API-Key; then assert the raw Authorization and Cookie values do not appear in log_data["headers"] and that each sensitive header is replaced/marked (e.g., contains "[REDACTED]") in the returned log_data; keep existing assertions for X-API-Key and token hashing in test_token_identifier_is_hashed_not_plaintext unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/bgtasks/cleanup_task.py`:
- Around line 58-66: The batch delete loop currently catches and logs exceptions
from model.all_objects.filter(id__in=ids).delete() using log_exception(e) but
suppresses them, causing the task to report success; change this so failures are
not swallowed — either re-raise the exception after logging (raise) or collect
per-model errors and, after the loop, raise a composite exception so the task
fails/retries; update the code paths around log_exception(e), delete_result
handling and the final total_deleted/return logic to ensure an exception is
propagated when a delete fails.
In `@apps/api/plane/bgtasks/logger_task.py`:
- Around line 33-37: The Celery task process_logs removed the old mongo_log
argument which will break mixed-version deployments and queued jobs; restore
backward compatibility by updating process_logs to accept the legacy parameter
(e.g., mongo_log: Optional[Any] = None) or a catch-all **kwargs, and simply
ignore that extra input while still calling log_to_postgres(log_data) so
existing callers and enqueued tasks won’t raise TypeError during rollout.
In `@apps/api/plane/settings/common.py`:
- Around line 405-414: Validate and reject negative retention settings at
startup for API_ACTIVITY_LOG_RETENTION_DAYS, WEBHOOK_LOG_RETENTION_DAYS, and
EMAIL_LOG_RETENTION_DAYS: after parsing each env var to int, check if value < 0
and if so raise a clear startup error (e.g., ValueError or SystemExit) with a
message naming the offending variable so the process fails fast; update the
initialization logic that sets these constants to perform these checks before
the app continues.
---
Nitpick comments:
In `@apps/api/plane/tests/unit/middleware/test_logger.py`:
- Around line 38-58: Update the test_sensitive_headers_are_redacted test (and/or
the helper _captured_log_data) to include Authorization and Cookie headers in
the constructed request (use HTTP_AUTHORIZATION and HTTP_COOKIE when calling
request_factory.get) so the middleware processes them alongside X-API-Key; then
assert the raw Authorization and Cookie values do not appear in
log_data["headers"] and that each sensitive header is replaced/marked (e.g.,
contains "[REDACTED]") in the returned log_data; keep existing assertions for
X-API-Key and token hashing in test_token_identifier_is_hashed_not_plaintext
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbf61211-58cc-4ca1-8f20-94aec2efb4ff
📒 Files selected for processing (15)
apps/api/plane/app/serializers/api.pyapps/api/plane/bgtasks/cleanup_task.pyapps/api/plane/bgtasks/logger_task.pyapps/api/plane/bgtasks/webhook_task.pyapps/api/plane/middleware/logger.pyapps/api/plane/settings/common.pyapps/api/plane/settings/local.pyapps/api/plane/settings/mongo.pyapps/api/plane/settings/production.pyapps/api/plane/tests/README.mdapps/api/plane/tests/conftest_external.pyapps/api/plane/tests/contract/app/test_api_token.pyapps/api/plane/tests/unit/bg_tasks/test_cleanup_task.pyapps/api/plane/tests/unit/middleware/test_logger.pyapps/api/requirements/base.txt
💤 Files with no reviewable changes (5)
- apps/api/requirements/base.txt
- apps/api/plane/settings/mongo.py
- apps/api/plane/tests/conftest_external.py
- apps/api/plane/settings/local.py
- apps/api/plane/settings/production.py
Address CodeQL "weak hashing of sensitive data" by hashing the API key with a SECRET_KEY-keyed HMAC instead of a bare SHA-256. The identifier is a non-reversible tokenization of a high-entropy key (not password storage); keying it also prevents precomputing the digest from a known key value.
- process_logs accepts extra kwargs so jobs enqueued by an older release (with a mongo_log arg) don't fail during a rolling deploy. - Log-cleanup batch delete failures are logged and skipped rather than aborting the run, so a single bad batch can't block the rest. - Extend logger middleware test to assert Authorization and Cookie headers are redacted; add a test that a failing cleanup batch is swallowed.
Negative (or unparseable) retention values would compute a future cutoff and delete every log row. The retention settings now fall back to their defaults in that case via a shared `_retention_days` helper.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/middleware/test_logger.py (1)
68-72: ⚡ Quick winAssert explicit redaction per sensitive header (not just value absence).
Line 68–72 can still pass if a sensitive header is omitted entirely instead of being redacted. Parse
log_data["headers"]and assert those header keys are present with"[REDACTED]"values.Proposed test hardening
+import ast import hashlib import hmac from unittest.mock import Mock, patch @@ def test_sensitive_headers_are_redacted(self, middleware, request_factory): log_data = self._captured_log_data(middleware, request_factory) + headers = ast.literal_eval(log_data["headers"]) + lowered = {k.lower(): v for k, v in headers.items()} # None of the sensitive header values may appear in the logged headers. assert self.API_KEY not in log_data["headers"] assert self.AUTHORIZATION not in log_data["headers"] assert self.COOKIE not in log_data["headers"] assert "[REDACTED]" in log_data["headers"] + assert lowered["x-api-key"] == "[REDACTED]" + assert lowered["authorization"] == "[REDACTED]" + assert lowered["cookie"] == "[REDACTED]"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/unit/middleware/test_logger.py` around lines 68 - 72, The test currently only checks that sensitive values aren't present in log_data["headers"], which could pass if headers were omitted; update the assertions in the test (in test_logger.py) to parse log_data["headers"] and assert that each sensitive header key (API_KEY, AUTHORIZATION, COOKIE) exists in the headers map and that its value equals the string "[REDACTED]" instead of only asserting the raw values are absent; keep the check for "[REDACTED]" but replace the value-absence assertions with explicit key-presence-and-value-equals checks against log_data["headers"].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/tests/unit/bg_tasks/test_cleanup_task.py`:
- Around line 129-143: Update the test
TestProcessCleanupTaskErrorHandling.test_batch_delete_failure_is_swallowed to
also assert that the error is logged: patch or monkeypatch the log_exception
function used by process_cleanup_task (or the module-level logger) with a mock
before calling process_cleanup_task(lambda: iter([1,2,3]), _BoomModel, "Boom"),
call the function, then assert the mock was called once (or with an exception
argument) in addition to asserting no exception was raised; reference
process_cleanup_task and log_exception in the test so the logging call is
validated.
---
Nitpick comments:
In `@apps/api/plane/tests/unit/middleware/test_logger.py`:
- Around line 68-72: The test currently only checks that sensitive values aren't
present in log_data["headers"], which could pass if headers were omitted; update
the assertions in the test (in test_logger.py) to parse log_data["headers"] and
assert that each sensitive header key (API_KEY, AUTHORIZATION, COOKIE) exists in
the headers map and that its value equals the string "[REDACTED]" instead of
only asserting the raw values are absent; keep the check for "[REDACTED]" but
replace the value-absence assertions with explicit key-presence-and-value-equals
checks against log_data["headers"].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c61e80a3-e151-4279-af60-d20a77f9fb53
📒 Files selected for processing (6)
apps/api/plane/bgtasks/cleanup_task.pyapps/api/plane/bgtasks/logger_task.pyapps/api/plane/settings/common.pyapps/api/plane/tests/unit/bg_tasks/test_cleanup_task.pyapps/api/plane/tests/unit/middleware/test_logger.pyapps/api/plane/tests/unit/settings/test_retention.py
Comment on lines
+129
to
+143
| @pytest.mark.unit | ||
| class TestProcessCleanupTaskErrorHandling: | ||
| def test_batch_delete_failure_is_swallowed(self): | ||
| """A failing batch is logged and skipped; the run does not raise.""" | ||
|
|
||
| class _BoomManager: | ||
| @staticmethod | ||
| def filter(**kwargs): | ||
| raise RuntimeError("db unavailable") | ||
|
|
||
| class _BoomModel: | ||
| all_objects = _BoomManager() | ||
|
|
||
| # Should not raise even though the delete blows up. | ||
| process_cleanup_task(lambda: iter([1, 2, 3]), _BoomModel, "Boom") |
Contributor
There was a problem hiding this comment.
Assert the error is logged, not just swallowed.
Line 132 states “logged and skipped”, but the test currently only verifies “does not raise”. Please also assert log_exception is called so this resilience contract can’t regress silently.
✅ Suggested test tightening
+from unittest.mock import patch
@@
class TestProcessCleanupTaskErrorHandling:
def test_batch_delete_failure_is_swallowed(self):
"""A failing batch is logged and skipped; the run does not raise."""
@@
- # Should not raise even though the delete blows up.
- process_cleanup_task(lambda: iter([1, 2, 3]), _BoomModel, "Boom")
+ with patch("plane.bgtasks.cleanup_task.log_exception") as mocked_log_exception:
+ # Should not raise even though the delete blows up.
+ process_cleanup_task(lambda: iter([1, 2, 3]), _BoomModel, "Boom")
+ mocked_log_exception.assert_called_once()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.unit | |
| class TestProcessCleanupTaskErrorHandling: | |
| def test_batch_delete_failure_is_swallowed(self): | |
| """A failing batch is logged and skipped; the run does not raise.""" | |
| class _BoomManager: | |
| @staticmethod | |
| def filter(**kwargs): | |
| raise RuntimeError("db unavailable") | |
| class _BoomModel: | |
| all_objects = _BoomManager() | |
| # Should not raise even though the delete blows up. | |
| process_cleanup_task(lambda: iter([1, 2, 3]), _BoomModel, "Boom") | |
| from unittest.mock import patch | |
| `@pytest.mark.unit` | |
| class TestProcessCleanupTaskErrorHandling: | |
| def test_batch_delete_failure_is_swallowed(self): | |
| """A failing batch is logged and skipped; the run does not raise.""" | |
| class _BoomManager: | |
| `@staticmethod` | |
| def filter(**kwargs): | |
| raise RuntimeError("db unavailable") | |
| class _BoomModel: | |
| all_objects = _BoomManager() | |
| with patch("plane.bgtasks.cleanup_task.log_exception") as mocked_log_exception: | |
| # Should not raise even though the delete blows up. | |
| process_cleanup_task(lambda: iter([1, 2, 3]), _BoomModel, "Boom") | |
| mocked_log_exception.assert_called_once() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/plane/tests/unit/bg_tasks/test_cleanup_task.py` around lines 129 -
143, Update the test
TestProcessCleanupTaskErrorHandling.test_batch_delete_failure_is_swallowed to
also assert that the error is logged: patch or monkeypatch the log_exception
function used by process_cleanup_task (or the module-level logger) with a mock
before calling process_cleanup_task(lambda: iter([1,2,3]), _BoomModel, "Boom"),
call the function, then assert the mock was called once (or with an exception
argument) in addition to asserting no exception was raised; reference
process_cleanup_task and log_exception in the test so the logging call is
validated.
pablohashescobar
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Removes MongoDB as a log sink and hardens API token handling.
Logging / retention
all_objects(removed immediately, not soft-deleted).API_ACTIVITY_LOG_RETENTION_DAYS(default 14)WEBHOOK_LOG_RETENTION_DAYS(default 14)EMAIL_LOG_RETENTION_DAYS(default 7)HARD_DELETE_AFTER_DAYSno longer drives any log cleanup (still used bydeletion_task.pyfor purging soft-deleted records).settings/mongo.py, removes theMONGO_DB_*settings andplane.mongologgers, and drops thepymongodependency.API token hardening
allowed_rate_limitis now read-only onAPITokenSerializer, so a user can no longer raise their own token's rate limit viaPATCH(GHSA-xfgr-2x3f-g2cf).APITokenLogMiddlewareno longer stores API keys in plaintext: it records a SHA-256 hash as the token identifier and redacts sensitive request headers (X-Api-Key,Authorization,Cookie) before logging (GHSA-r5p8-cj3q-38cc).Type of Change
Screenshots and Media (if applicable)
Test Scenarios
PATCH /api/users/api-tokens/{id}/withallowed_rate_limitset to a large value → response 200 but the stored value is unchanged.X-Api-Keyheader → the persistedAPIActivityLogrow stores a SHA-256 hash (not the raw key) and the logged headers show[REDACTED]for sensitive headers.APIActivityLog/WebhookLog/EmailNotificationLogrows older than their retention window, run the respective cleanup task, and confirm the rows are hard-deleted while fresh rows survive.Automated coverage added/passing:
plane/tests/contract/app/test_api_token.py::...::test_patch_cannot_modify_allowed_rate_limitplane/tests/unit/middleware/test_logger.py(hash + header redaction)plane/tests/unit/bg_tasks/test_cleanup_task.py(hard-delete + retention for API/webhook/email logs)References
Summary by CodeRabbit
Bug Fixes
Chores
Security / Privacy
Tests