[SECUR-104] fix: Arbitrary Modification of API Token Rate Limits

[sangeethailango]

Description

Update the patch endpoint for the Personal Access token endpoint with right permissions.

Summary by CodeRabbit

• New Features

  • API tokens now display additional metadata including active status, last usage time, and user type information.

• Bug Fixes

  • API token values and user types are now protected from accidental modification.

[makeplane]

Linked to Plane Work Item(s)

• [SECUR-104] Arbitrary Modification of API Token Rate Limits

This comment was auto-generated by Plane

[coderabbitai]

📝 Walkthrough

Walkthrough

API token serializer now exposes three additional read-only fields (is_active, last_used, user_type). Contract tests updated to validate the detail endpoint and enforce immutability constraints on token properties during PATCH operations.

Changes

Cohort / File(s)	Summary
Serializer Updates apps/api/plane/app/serializers/api.py	Expanded APITokenSerializer.Meta.read_only_fields to include is_active, last_used, and user_type.
Contract Tests apps/api/plane/tests/contract/app/test_api_token.py	Updated URL endpoint references from api-tokens to api-tokens-details for detail operations. Added three new test cases enforcing immutability: test_patch_cannot_modify_token, test_patch_cannot_modify_user_type, and test_patch_cannot_modify_service_token.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Three fields now shine in read-only grace,
API tokens show their hidden face,
Tests ensure what should not change,
A patch that cannot rearrange! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title references 'Rate Limits' but the PR actually fixes arbitrary modification of API token properties (is_active, last_used, user_type) and adds immutability constraints via PATCH.	Update the title to accurately reflect the actual changes, such as 'fix: Add immutability constraints to API token PATCH endpoint' or 'fix: Prevent arbitrary modification of API token properties'.
Description check	⚠️ Warning	The description is minimal and lacks required sections. It does not address the Type of Change, Test Scenarios, or References sections from the template, and provides insufficient detail about what permissions were updated and why.	Expand the description to include all template sections: specify the type of change, detail the test scenarios performed, list related issues/references, and clarify which permissions were modified and the security impact.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-api-token-arbitary-modifications

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/api/plane/app/serializers/api.py (1)

15-24: ⚠️ Potential issue | 🔴 Critical

Add allowed_rate_limit and is_service to read_only_fields.

The serializer is used for PATCH updates (apps/api/plane/app/views/api.py), and allowed_rate_limit is currently writable. This allows authenticated users to modify their own API token rate limits, keeping the SECUR-104 vector open. Additionally, is_service should also be protected. Add both to read_only_fields:

🔒 Proposed fix

         read_only_fields = [
             "token",
             "expired_at",
             "created_at",
             "updated_at",
             "workspace",
             "user",
+            "allowed_rate_limit",
+            "is_service",
             "is_active",
             "last_used",
             "user_type",
         ]

🧹 Nitpick comments (1)

apps/api/plane/tests/contract/app/test_api_token.py (1)

336-385: Add a PATCH immutability test for allowed_rate_limit.

Given SECUR-104, this is the most important field to lock down; adding a test here will prevent regressions.

✅ Suggested test

     `@pytest.mark.django_db`
     def test_patch_cannot_modify_user_type(self, session_client, create_user, create_api_token_for_user):
         """Test that user_type cannot be modified via PATCH"""
         # Arrange
         session_client.force_authenticate(user=create_user)
         url = reverse("api-tokens-details", kwargs={"pk": create_api_token_for_user.pk})
         update_data = {"user_type": 1}

         # Act
         response = session_client.patch(url, update_data, format="json")

         # Assert
         assert response.status_code == status.HTTP_200_OK
         create_api_token_for_user.refresh_from_db()
         assert create_api_token_for_user.user_type == 0

+    `@pytest.mark.django_db`
+    def test_patch_cannot_modify_allowed_rate_limit(self, session_client, create_user, create_api_token_for_user):
+        """Test that allowed_rate_limit cannot be modified via PATCH"""
+        # Arrange
+        session_client.force_authenticate(user=create_user)
+        url = reverse("api-tokens-details", kwargs={"pk": create_api_token_for_user.pk})
+        original_rate_limit = create_api_token_for_user.allowed_rate_limit
+        update_data = {"allowed_rate_limit": "9999/min"}
+
+        # Act
+        response = session_client.patch(url, update_data, format="json")
+
+        # Assert
+        assert response.status_code == status.HTTP_200_OK
+        create_api_token_for_user.refresh_from_db()
+        assert create_api_token_for_user.allowed_rate_limit == original_rate_limit