Skip to content

fix(admin): honour escape sequences in settings form string fields - #8239

Open
JohnMcLear wants to merge 3 commits into
developfrom
fix/8211-admin-settings-escapes
Open

JohnMcLear wants to merge 3 commits into
developfrom
fix/8211-admin-settings-escapes

Conversation

@JohnMcLear

Copy link
Copy Markdown
Member

Fixes #8211

Root cause

The /admin/settings form view edits string values through single-line <input type="text"> widgets:

  • Plain strings (StringInput) were shown decoded. Browsers strip line breaks from an input's value, so defaultPadText showed as Welcome to Etherpad!This pad text…. Typing \n was then JSON-encoded again by jsonc-parser's modify(), which wrote \\n to settings.json. The pad then showed a literal \n.
  • Env placeholder defaults (EnvPill, e.g. "${DEFAULT_PAD_TEXT:Line 1\nLine 2}") were shown in raw escaped form, sliced straight from the JSON text, but re-escaped on save. So \n became \\n. This is exactly what the issue reports.

Raw mode wasn't affected: it sends the textarea text through unchanged.

Fix

  • New admin/src/components/settings/stringEscapes.ts, with escapeForInput and unescapeFromInput.
  • Both widgets now show values in JSON-escaped form: a newline shows as \n and a backslash as \\. Quotes and / stay readable.
  • Before writing, both widgets decode what the user typed. So Welcome\n\ntest\n is saved as "Welcome\n\ntest\n", the same bytes you'd get by editing settings.json by hand.
  • While the user is still typing, an incomplete or invalid escape (such as a trailing \ or \q) isn't saved. The input gets aria-invalid until the text decodes, and on blur it resets to the last valid value.

Tests

  • Admin unit tests (admin/src/components/settings/__tests__/stringEscapes.test.ts) cover escape/unescape, invalid escapes, round-trips, and the jsonc write, checking that the file gets \n and not \\n. pnpm test in admin/: 27/27 pass.
  • Two new Playwright tests in adminsettings.spec.ts (chromium-admin):
    • #8211 … form string field: before the fix it failed with Received string: "Welcome to Etherpad!This pad text…" (newlines dropped). After the fix it passes: the field shows \n, and a value typed with \n, \\ and " round-trips to the expected decoded JSON value.
    • #8211 … env placeholder default: before the fix it failed, and the file contained "defaultPadText": "${DEFAULT_PAD_TEXT:Welcome\\n\\ntest\\n}". After the fix it passes, with ${DEFAULT_PAD_TEXT:Welcome\n\ntest\n}.
  • Every non-restart test in adminsettings.spec.ts passes locally (14/14). I left out the restart tests: on my local server started with --settings <custom path>, the in-process restart reloads without the admin user. That's specific to my setup and unrelated to this change; CI uses the root settings.json.
  • Backend mocha: 1673 passing. vitest: 840 passing. tsc --noEmit: clean. The admin build (tsc && vite build) is clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_012kA75NPq8nGRidAwhPXeCi

The settings form view rendered string values in single-line <input>s.
Browsers strip line breaks from an input's value, so values such as
defaultPadText lost their newlines, and anything typed (e.g. `\n`) was
JSON-encoded again on save, producing `\\n` in settings.json and a literal
backslash-n in the pad text. Env placeholder defaults were shown in raw
escaped form but re-escaped on save, with the same result.

String inputs and env placeholder defaults now show values in their
JSON-escaped form and decode typed escape sequences before writing, so
`Welcome\n\ntest\n` is stored exactly like a hand-edited settings.json.
Incomplete escapes (a trailing backslash while typing) are not propagated.

Fixes #8211

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012kA75NPq8nGRidAwhPXeCi
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Preserve escape sequences in admin settings string fields

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Displays settings strings with JSON-style escapes so single-line inputs preserve control
 characters.
• Decodes valid typed escapes before saving and rejects incomplete or invalid sequences.
• Adds unit and browser regressions for plain strings and environment placeholder defaults.
Diagram

graph TD
  A["Settings JSON"] --> B["Form Values"] --> C["Escape Helper"] --> D["String Input"] --> F["Unescape Helper"] --> G["Change Handler"] --> A
  C --> E["Env Pill"] --> F
Loading
High-Level Assessment

The centralized reversible conversion helpers are the appropriate approach because both affected widgets require identical escape semantics. Relying directly on JSON serialization would unnecessarily escape readable quotes and slashes, while multiline controls would change the established single-line settings UI.

Files changed (5) +241 / -13

Bug fix (3) +122 / -13
stringEscapes.tsAdd reversible input escape helpers +65/-0

Add reversible input escape helpers

• Introduces helpers that render control characters and backslashes in JSON-style escaped form while leaving quotes and slashes readable. The decoder accepts standard JSON escapes and returns null for invalid or incomplete sequences.

admin/src/components/settings/stringEscapes.ts

EnvPill.tsxDecode escaped environment placeholder defaults before saving +12/-2

Decode escaped environment placeholder defaults before saving

• Normalizes raw placeholder defaults for consistent form display and decodes edited escape sequences before invoking the settings change handler. Invalid or incomplete drafts remain local and are not persisted.

admin/src/components/settings/widgets/EnvPill.tsx

StringInput.tsxPreserve escaped string drafts in single-line inputs +45/-11

Preserve escaped string drafts in single-line inputs

• Converts decoded settings strings to escaped display text and maintains a focused draft to avoid disrupting partial input. Valid drafts are decoded before propagation, while invalid drafts receive 'aria-invalid' and reset on blur.

admin/src/components/settings/widgets/StringInput.tsx

Tests (2) +119 / -0
stringEscapes.test.tsTest string escape conversion and JSONC persistence +45/-0

Test string escape conversion and JSONC persistence

• Adds regression coverage for control-character escaping, readable quotes and slashes, typed escape decoding, invalid input rejection, and arbitrary-string round trips. Verifies that JSONC writes decoded newlines as '\n' rather than literal '\\n'.

admin/src/components/settings/tests/stringEscapes.test.ts

adminsettings.spec.tsAdd browser regressions for settings escape handling +74/-0

Add browser regressions for settings escape handling

• Adds Playwright coverage proving ordinary string fields preserve existing newlines and round-trip typed newlines, backslashes, and quotes. Also verifies environment placeholder defaults save newline escapes without double-escaping and restores the original settings afterward.

src/tests/frontend-new/admin-spec/adminsettings.spec.ts

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 17, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Valid closing braces disappear ✓ Resolved 🐞 Bug ≡ Correctness
Description
EnvPill decodes a raw \u007d default into } and then the added sanitize(decoded) call
removes that character before rebuilding the placeholder. This silently corrupts valid environment
defaults containing escaped closing braces, even though the backend placeholder parser permits
braces within its greedily captured default.
Code

admin/src/components/settings/widgets/EnvPill.tsx[R75-76]

+          const decoded = unescapeFromInput(v);
+          if (decoded !== null) onChange(sanitize(decoded));
Evidence
The raw frontend matcher accepts \u007d because it contains no literal brace, and the new
normalization decodes it to }. The newly added second sanitization then removes it; meanwhile
JsoncNode reconstructs the placeholder from that altered value, despite the backend regex allowing
any characters, including braces, in the default before the final terminator.

admin/src/components/settings/envPill.ts[12-20]
admin/src/components/settings/widgets/EnvPill.tsx[26-31]
admin/src/components/settings/widgets/EnvPill.tsx[69-76]
admin/src/components/settings/JsoncNode.tsx[42-52]
src/node/utils/Settings.ts[1097-1116]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Decoding JSON escapes turns `\u007d` into a closing brace, after which EnvPill removes it from the default. The frontend placeholder matcher must also continue recognizing the reconstructed placeholder when a decoded default contains a brace.
## Fix Focus Areas
- admin/src/components/settings/widgets/EnvPill.tsx[69-76]
- admin/src/components/settings/envPill.ts[12-20]
## Recommended Fix
Stop stripping closing braces from decoded defaults and update the frontend placeholder matcher to capture defaults through the final placeholder terminator, consistent with the backend parser. Add a round-trip test for an environment default containing `\u007d`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Rejected defaults remain visibly unsaved ✓ Resolved 🐞 Bug ≡ Correctness
Description
EnvPill keeps the draft after unescapeFromInput() rejects an invalid or incomplete escape, while
the new callback deliberately suppresses the parent update. When the user blurs the field after
typing a trailing backslash or \q, the displayed value therefore disagrees with the unchanged
settings JSON and can mislead them into saving a value that was never applied.
Code

admin/src/components/settings/widgets/EnvPill.tsx[R75-76]

+          const decoded = unescapeFromInput(v);
+          if (decoded !== null) onChange(sanitize(decoded));
Evidence
The added decoder returns null for invalid escapes and lines 75-76 skip onChange, so the parent
JSON remains unchanged. EnvPill's blur handler only clears its focus ref and its synchronization
effect runs only when initial changes, whereas StringInput explicitly restores its canonical
escaped value on blur.

admin/src/components/settings/stringEscapes.ts[42-60]
admin/src/components/settings/widgets/EnvPill.tsx[35-37]
admin/src/components/settings/widgets/EnvPill.tsx[67-76]
admin/src/components/settings/widgets/StringInput.tsx[38-49]
admin/src/components/settings/FormView.tsx[86-90]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
EnvPill retains invalid or incomplete escaped text after blur even though that text was not propagated to the settings state. The field can therefore display a value different from what will be saved.
## Fix Focus Areas
- admin/src/components/settings/widgets/EnvPill.tsx[67-76]
## Recommended Fix
Track whether the draft decodes successfully, expose rejection through `aria-invalid`, and reset the draft to the latest canonical `initial` value on blur, matching StringInput's behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread admin/src/components/settings/widgets/EnvPill.tsx Outdated
Comment thread admin/src/components/settings/widgets/EnvPill.tsx Outdated
JohnMcLear and others added 2 commits September 17, 2026 13:35
Earlier admin specs rewrite settings.json as minified JSON, so the key is
not at the start of a line in CI. Take the last match instead, which also
skips the template's documentation comment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012kA75NPq8nGRidAwhPXeCi
…d braces

Address review: EnvPill now marks undecodable drafts aria-invalid and
restores the applied value on blur (matching StringInput), and a default
whose decoded form contains `}` is rejected/kept raw instead of having the
brace silently stripped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012kA75NPq8nGRidAwhPXeCi
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.

Admin console does not allow escaped characters in settings values

1 participant