Skip to content

fix: strip whitespace from instance configuration values#8744

Open
okxint wants to merge 1 commit intomakeplane:previewfrom
okxint:fix/strip-whitespace-instance-config
Open

fix: strip whitespace from instance configuration values#8744
okxint wants to merge 1 commit intomakeplane:previewfrom
okxint:fix/strip-whitespace-instance-config

Conversation

@okxint
Copy link

@okxint okxint commented Mar 10, 2026

Strip whitespace from config values before persisting. Closes #8737

Summary by CodeRabbit

  • Bug Fixes
    • Improved configuration value handling with stricter input normalization to ensure consistent data processing and storage.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

📝 Walkthrough

Walkthrough

The instance configuration endpoint now strips leading and trailing whitespace from configuration values before persisting them to the database. This prevents accidental whitespace in pasted credentials (e.g., OAuth Client IDs) from being stored verbatim.

Changes

Cohort / File(s) Summary
Configuration Value Normalization
apps/api/plane/license/api/views/configuration.py
Modified InstanceConfigurationEndpoint.patch to coerce request value to string and strip surrounding whitespace before encryption or direct assignment.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A rabbit's keen eye caught the space,
Trailing whispers in every place,
Now stripped and clean, the values gleam,
OAuth flows like a rabbit's dream! 🐰✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is minimal but references the linked issue. The template requires more detailed sections (Type of Change, Test Scenarios, etc.) which are largely absent. Expand the description to include the type of change (bug fix), test scenarios explaining validation of whitespace stripping, and more context on the fix.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: stripping whitespace from instance configuration values before persistence.
Linked Issues check ✅ Passed The code change directly addresses issue #8737 by stripping whitespace from configuration values in the InstanceConfigurationEndpoint.patch() method as required.
Out of Scope Changes check ✅ Passed The single-line change is narrowly scoped to fix the root cause identified in issue #8737, with no extraneous modifications.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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`:
- Line 48: When building the new config value, don't call str(...) on a JSON
null; instead read the raw = request.data.get(configuration.key,
configuration.value) and if raw is None set value to an empty string (or other
empty/unset sentinel) before calling .strip(); this prevents JSON null from
becoming the literal "None" in persisted config and aligns with
get_configuration_value() semantics. Ensure you update the assignment around
request.data.get(configuration.key, configuration.value) (referenced by
configuration.key and configuration.value) to handle raw is None explicitly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e07f9fde-d22f-4ff3-bd46-379d4b142a8e

📥 Commits

Reviewing files that changed from the base of the PR and between 6627282 and 91f8ea2.

📒 Files selected for processing (1)
  • apps/api/plane/license/api/views/configuration.py

bulk_configurations = []
for configuration in configurations:
value = request.data.get(configuration.key, configuration.value)
value = str(request.data.get(configuration.key, configuration.value)).strip()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Handle null explicitly before stringifying.

str(...).strip() turns a JSON null clear request into the literal "None". Since get_configuration_value() reads persisted values back verbatim, that string will propagate into runtime config instead of behaving like an empty/unset value.

Suggested fix
-            value = str(request.data.get(configuration.key, configuration.value)).strip()
+            raw_value = request.data.get(configuration.key, configuration.value)
+            value = "" if raw_value is None else str(raw_value).strip()
📝 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
value = str(request.data.get(configuration.key, configuration.value)).strip()
raw_value = request.data.get(configuration.key, configuration.value)
value = "" if raw_value is None else str(raw_value).strip()
🤖 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` at line 48, When building
the new config value, don't call str(...) on a JSON null; instead read the raw =
request.data.get(configuration.key, configuration.value) and if raw is None set
value to an empty string (or other empty/unset sentinel) before calling
.strip(); this prevents JSON null from becoming the literal "None" in persisted
config and aligns with get_configuration_value() semantics. Ensure you update
the assignment around request.data.get(configuration.key, configuration.value)
(referenced by configuration.key and configuration.value) to handle raw is None
explicitly.

@okxint
Copy link
Author

okxint commented Mar 12, 2026

Hey, friendly follow-up on this — let me know if there's anything I should adjust. Happy to iterate on feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 Bug: OAuth login fails when instance configuration values have trailing whitespace

2 participants