fix: strip whitespace from instance configuration values on save#8738
fix: strip whitespace from instance configuration values on save#8738rucoder wants to merge 1 commit intomakeplane:previewfrom
Conversation
|
|
📝 WalkthroughWalkthroughTrim leading/trailing whitespace from instance configuration values received via the configuration PATCH endpoint before encrypting/persisting; add unit tests validating trimming for various whitespace cases and that null remains null. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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 |
rucoder
force-pushed
the
fix/trim-instance-configuration-values
branch
from
62d5876 to
058f7a2
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes OAuth authentication failures caused by leading/trailing whitespace being persisted in instance configuration values saved via the God Mode admin UI (e.g., GITHUB_CLIENT_ID).
Changes:
- Trim leading/trailing whitespace from instance configuration values before saving them in
InstanceConfigurationEndpoint.patch().
| value = str( | ||
| request.data.get(configuration.key, configuration.value) | ||
| ).strip() |
There was a problem hiding this comment.
str(...).strip() will coerce non-string inputs (including JSON null) into strings; e.g. null becomes 'None' and gets persisted, which is different from the previous behavior (saving None) and can break “clearing” a config value. Consider only stripping when the incoming value is a str, and preserve None (or explicitly normalize None to an empty string) to avoid storing the literal 'None'.
| value = str( | |
| request.data.get(configuration.key, configuration.value) | |
| ).strip() | |
| value = request.data.get(configuration.key, configuration.value) | |
| if isinstance(value, str): | |
| value = value.strip() |
There was a problem hiding this comment.
Good catch! Updated to use isinstance(value, str) guard instead of str(...) coercion, so None is preserved as-is. Also added a test case for the null scenario.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/views/test_instance_configuration.py (1)
50-132: Good test coverage for the core whitespace trimming fix.The tests comprehensively cover the stated bug fix. Consider adding a test for encrypted configuration values (
is_encrypted=True) to ensure whitespace is stripped before encryption, since the encryption path is distinct in the implementation.Optional: Add test for encrypted value trimming
`@pytest.mark.django_db` def test_patch_strips_whitespace_before_encryption(self, admin_client): """Encrypted values should have whitespace stripped before encryption.""" InstanceConfiguration.objects.create( key="GITHUB_CLIENT_SECRET", value="", category="GITHUB", is_encrypted=True, ) response = admin_client.patch( "/api/instances/configurations/", {"GITHUB_CLIENT_SECRET": " secret-value "}, format="json", ) assert response.status_code == 200 config = InstanceConfiguration.objects.get(key="GITHUB_CLIENT_SECRET") # Value should be encrypted (non-empty and different from input) assert config.value != "" assert config.value != "secret-value" assert config.value != " secret-value "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/unit/views/test_instance_configuration.py` around lines 50 - 132, Add a unit test to verify encrypted configs are trimmed before encryption: inside TestInstanceConfigurationWhitespaceTrimming add a test named test_patch_strips_whitespace_before_encryption that creates an InstanceConfiguration with key "GITHUB_CLIENT_SECRET" and is_encrypted=True, PATCHes "/api/instances/configurations/" with a value containing surrounding whitespace (e.g., " secret-value "), asserts a 200 response, then reloads the InstanceConfiguration and asserts the stored value is non-empty and not equal to the raw input nor the trimmed plain text (to confirm it was trimmed then encrypted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/license/api/views/configuration.py`:
- Around line 48-50: The code currently converts
request.data.get(configuration.key, configuration.value) to a string which turns
JSON null into the literal "None"; update the logic around the assignment to
value (the line using str(...).strip()) to first capture raw_value =
request.data.get(configuration.key, configuration.value) and then set value to
an empty string when raw_value is None, otherwise perform the str(...).strip()
conversion; modify the block that assigns value for configuration updates
(referencing request.data, configuration.key, and the value variable) to
implement this explicit null check.
---
Nitpick comments:
In `@apps/api/plane/tests/unit/views/test_instance_configuration.py`:
- Around line 50-132: Add a unit test to verify encrypted configs are trimmed
before encryption: inside TestInstanceConfigurationWhitespaceTrimming add a test
named test_patch_strips_whitespace_before_encryption that creates an
InstanceConfiguration with key "GITHUB_CLIENT_SECRET" and is_encrypted=True,
PATCHes "/api/instances/configurations/" with a value containing surrounding
whitespace (e.g., " secret-value "), asserts a 200 response, then reloads the
InstanceConfiguration and asserts the stored value is non-empty and not equal to
the raw input nor the trimmed plain text (to confirm it was trimmed then
encrypted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0bbaa2c-e980-48e3-9334-6a40a9120cb3
📒 Files selected for processing (3)
apps/api/plane/license/api/views/configuration.pyapps/api/plane/tests/unit/views/__init__.pyapps/api/plane/tests/unit/views/test_instance_configuration.py
| value = str( | ||
| request.data.get(configuration.key, configuration.value) | ||
| ).strip() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any API calls or frontend code that sends null for configuration values
rg -n "configurations" --type=ts --type=js -C3 | rg -i "null|clear|reset" | head -30
# Check if there's documentation or tests for clearing configuration values
rg -n "InstanceConfiguration" -C2 | rg -i "null|clear|none" | head -20Repository: makeplane/plane
Length of output: 395
🏁 Script executed:
# Read the full context around lines 48-50 in the configuration.py file
cat -n apps/api/plane/license/api/views/configuration.py | sed -n '40,60p'Repository: makeplane/plane
Length of output: 1122
🏁 Script executed:
# Search for the full implementation of this endpoint to understand the context
ast-grep --pattern $'def $_(request):
$$$
for configuration in configurations:
$$$
value = str(
request.data.get(configuration.key, configuration.value)
).strip()
$$$
'Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
# Look for test files related to configuration updates
fd -t f "test" --search-path apps/api | grep -i config | head -20Repository: makeplane/plane
Length of output: 121
🏁 Script executed:
# Check the InstanceConfiguration model definition
rg -n "class InstanceConfiguration" -A 20 apps/apiRepository: makeplane/plane
Length of output: 4982
🏁 Script executed:
# Read the test file for configuration updates
cat -n apps/api/plane/tests/unit/views/test_instance_configuration.pyRepository: makeplane/plane
Length of output: 5109
🏁 Script executed:
# Check if there's request validation/serialization before the patch method
rg -n "class InstanceConfigurationEndpoint" -A 50 apps/api/plane/license/api/views/configuration.py | head -80Repository: makeplane/plane
Length of output: 2291
🏁 Script executed:
# Look for any middleware or validation that filters null values from request.data
rg -n "request\.data" apps/api/plane/license/api/views/configuration.py -B 5 -A 5Repository: makeplane/plane
Length of output: 1169
Handle explicit null values to prevent storing the string "None".
When a user sends null in the JSON payload (e.g., {"GITHUB_CLIENT_ID": null}), the current code converts it to the literal string "None" instead of preserving None or an empty value. Since the model allows null=True, this creates an inconsistency.
Add an explicit null check before the str() conversion:
raw_value = request.data.get(configuration.key, configuration.value)
value = str(raw_value).strip() if raw_value is not None else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/license/api/views/configuration.py` around lines 48 - 50, The
code currently converts request.data.get(configuration.key, configuration.value)
to a string which turns JSON null into the literal "None"; update the logic
around the assignment to value (the line using str(...).strip()) to first
capture raw_value = request.data.get(configuration.key, configuration.value) and
then set value to an empty string when raw_value is None, otherwise perform the
str(...).strip() conversion; modify the block that assigns value for
configuration updates (referencing request.data, configuration.key, and the
value variable) to implement this explicit null check.
Trailing whitespace in OAuth credentials (e.g. GITHUB_CLIENT_ID) causes authentication failures. When a user accidentally pastes a value with leading/trailing spaces into the God Mode admin UI, the spaces are persisted to the database as-is. For OAuth client IDs this results in the identity provider rejecting the request (e.g. GitHub returns 404 because the client_id has a URL-encoded space appended). Strip whitespace from string configuration values before persisting them in InstanceConfigurationEndpoint.patch(). Non-string values (e.g. None) are left unchanged to preserve the ability to clear a config field. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
rucoder
force-pushed
the
fix/trim-instance-configuration-values
branch
from
058f7a2 to
6d7ad1e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/views/test_instance_configuration.py (1)
50-152: Good test coverage for the whitespace trimming scenarios.The tests comprehensively validate the trimming behavior for non-encrypted configurations. The
test_patch_null_value_not_coerced_to_stringtest correctly validates thatNoneis preserved (matching the view'sisinstanceapproach).Consider adding a test for encrypted configurations to ensure the whitespace is stripped before encryption:
📋 Suggested test for encrypted values
`@pytest.mark.django_db` def test_patch_strips_whitespace_before_encryption(self, admin_client): """Encrypted values should have whitespace stripped before encryption.""" InstanceConfiguration.objects.create( key="GITHUB_CLIENT_SECRET", value="", category="GITHUB", is_encrypted=True, ) response = admin_client.patch( "/api/instances/configurations/", {"GITHUB_CLIENT_SECRET": " secret-value "}, format="json", ) assert response.status_code == 200 # The response should contain the decrypted, trimmed value config_data = next( (c for c in response.json() if c["key"] == "GITHUB_CLIENT_SECRET"), None ) assert config_data is not None assert config_data["value"] == "secret-value"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/unit/views/test_instance_configuration.py` around lines 50 - 152, Add a new unit test named test_patch_strips_whitespace_before_encryption that creates an InstanceConfiguration with key "GITHUB_CLIENT_SECRET" and is_encrypted=True, sends a PATCH to "/api/instances/configurations/" with a value containing leading/trailing spaces, asserts a 200 response, then verifies the returned configuration for that key contains the trimmed plaintext value (e.g. "secret-value") to ensure trimming occurs before encryption; reference InstanceConfiguration and the configurations endpoint in the test to locate the relevant behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/plane/tests/unit/views/test_instance_configuration.py`:
- Around line 50-152: Add a new unit test named
test_patch_strips_whitespace_before_encryption that creates an
InstanceConfiguration with key "GITHUB_CLIENT_SECRET" and is_encrypted=True,
sends a PATCH to "/api/instances/configurations/" with a value containing
leading/trailing spaces, asserts a 200 response, then verifies the returned
configuration for that key contains the trimmed plaintext value (e.g.
"secret-value") to ensure trimming occurs before encryption; reference
InstanceConfiguration and the configurations endpoint in the test to locate the
relevant behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28e2478f-26fd-4c90-90fd-3dc072429fc2
📒 Files selected for processing (3)
apps/api/plane/license/api/views/configuration.pyapps/api/plane/tests/unit/views/__init__.pyapps/api/plane/tests/unit/views/test_instance_configuration.py
Summary
Fixes #8737
Trailing whitespace in OAuth credentials (e.g.
GITHUB_CLIENT_ID) causes authentication failures. When a user pastes a value with leading/trailing spaces into the God Mode admin UI, the spaces are persisted to the database as-is. For OAuth client IDs this results in the identity provider rejecting the request — e.g. GitHub returns a 404 because theclient_idparameter has a URL-encoded space (%20) appended.Change
Strip whitespace from all configuration values before persisting them in
InstanceConfigurationEndpoint.patch():Steps to reproduce the bug
client_idin the OAuth URL contains%20Testing
Verified manually on a self-hosted v1.2.3 instance. After applying this fix, the trailing space is stripped and OAuth redirects work correctly.
Summary by CodeRabbit
Bug Fixes
Tests