Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions admin/src/App.css
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,37 @@ textarea.settings:focus {
border-top: 1px solid #ddd;
}

/* --- env-var banner --- */
/* Shown only when settings.json contains ${VAR} placeholders. The
typical reader is a Docker/K8s operator who has just been surprised
by env-var substitution semantics, so the copy must explain rather
than warn — visual weight matches a note, not an error. */
.settings-envvar-banner {
display: flex;
align-items: flex-start;
gap: 12px;
padding: 12px 16px;
margin-bottom: 16px;
background: #f5f7ff;
border: 1px solid #c7d2fe;
border-radius: 6px;
color: #1e293b;
}
.settings-envvar-banner svg {
flex-shrink: 0;
color: #4f46e5;
margin-top: 2px;
}
.settings-envvar-banner strong {
display: block;
margin-bottom: 4px;
}
.settings-envvar-banner p {
margin: 0;
font-size: 13px;
line-height: 1.5;
}

/* --- mode toggle --- */
.settings-mode-toggle {
display: inline-flex;
Expand Down
25 changes: 24 additions & 1 deletion admin/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,19 @@ export const App = () => {

const settingSocket = connect(`${WS_URL}/settings`, {transports: ['websocket']});
const pluginsSocket = connect(`${WS_URL}/pluginfw/installer`, {transports: ['websocket']})
// When the server explicitly rejects us for not being admin, we must
// NOT reconnect on the subsequent `disconnect` event — otherwise the
// socket cycles forever (connect → admin_auth_error → server
// disconnect → SPA auto-reconnect → …). The flag is reset on each
// successful connect.
let authErrored = false;

pluginsSocket.on('connect', () => {
useStore.getState().setPluginsSocket(pluginsSocket);
});

settingSocket.on('connect', () => {
authErrored = false;
useStore.getState().setSettingsSocket(settingSocket);
useStore.getState().setShowLoading(false)
settingSocket.emit('load');
Expand All @@ -46,7 +53,7 @@ export const App = () => {

settingSocket.on('disconnect', (reason) => {
useStore.getState().setShowLoading(true)
if (reason === 'io server disconnect') settingSocket.connect();
if (reason === 'io server disconnect' && !authErrored) settingSocket.connect();
});

settingSocket.on('settings', (settings: any) => {
Expand Down Expand Up @@ -75,6 +82,22 @@ export const App = () => {
const detail = payload?.message ?? '';
setToastState({open: true, title: t('admin_settings.toast.save_failed') + (detail ? ` (${detail})` : ''), success: false});
}
});

// Backend emits this when the connecting socket does not have an
// admin session. Previously the server just dropped silently, so the
// SPA would sit on a loading screen with no clue. Surface it AND
// suppress the automatic reconnect — without this flag the SPA would
// immediately reconnect to a socket that will reject it again.
settingSocket.on('admin_auth_error', (payload?: {message?: string}) => {
authErrored = true;
const {setToastState} = useStore.getState();
setToastState({
open: true,
title: payload?.message || t('admin_settings.toast.auth_error'),
success: false,
});
useStore.getState().setShowLoading(false);
})

return () => {
Expand Down
21 changes: 19 additions & 2 deletions admin/src/components/settings/ModeToggle.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
import { Trans, useTranslation } from 'react-i18next';

export type Mode = 'form' | 'raw';
export type Mode = 'form' | 'raw' | 'effective';

type Props = {
mode: Mode;
onChange: (mode: Mode) => void;
// When false, the Effective tab is hidden. We hide it for installs that
// aren't using env-var substitution at all — there's no useful difference
// between the raw file and the effective in-memory config for them.
showEffective?: boolean;
};

export const ModeToggle = ({ mode, onChange }: Props) => {
export const ModeToggle = ({ mode, onChange, showEffective = false }: Props) => {
const { t } = useTranslation();
return (
<div className="settings-mode-toggle" role="tablist" aria-label={t('admin_settings.mode.aria_label')}>
Expand All @@ -31,6 +35,19 @@ export const ModeToggle = ({ mode, onChange }: Props) => {
>
<Trans i18nKey="admin_settings.mode.raw" />
</button>
{showEffective && (
<button
type="button"
role="tab"
aria-selected={mode === 'effective'}
data-testid="mode-toggle-effective"
className={mode === 'effective' ? 'active' : ''}
onClick={() => onChange('effective')}
title={t('admin_settings.mode.effective_tooltip')}
>
<Trans i18nKey="admin_settings.mode.effective" />
</button>
)}
</div>
);
};
127 changes: 89 additions & 38 deletions admin/src/pages/SettingsPage.tsx
Original file line number Diff line number Diff line change
@@ -1,22 +1,42 @@
import React, { useState } from 'react';
import React, { useEffect, useMemo, useState } from 'react';
import { useStore } from '../store/store';
import { isJSONClean, cleanComments } from '../utils/utils';
import { Trans, useTranslation } from 'react-i18next';
import { IconButton } from '../components/IconButton';
import { RotateCw, Save, AlignLeft, ShieldCheck } from 'lucide-react';
import { RotateCw, Save, AlignLeft, ShieldCheck, Info } from 'lucide-react';
import { FormView } from '../components/settings/FormView';
import { ModeToggle, type Mode } from '../components/settings/ModeToggle';

const TAB_INDENT = ' ';

// Heuristic: `${VAR}` or `${VAR:default}` in the file means the operator is
// running with env-var substitution (overwhelmingly Docker / Kubernetes).
// We use this to gate the Docker-aware UX (the explanatory banner and the
// Effective-config tab) so non-container installs see the existing UI
// unchanged. Conservative on purpose — false negatives just keep the old
// behaviour.
const ENV_VAR_PATTERN = /\$\{[A-Za-z_][A-Za-z0-9_]*(?::[^}]*)?\}/;

export const SettingsPage = () => {
const { t } = useTranslation();
const settingsSocket = useStore(state => state.settingsSocket);
const settings = useStore(state => state.settings) ?? '';
const resolved = useStore(state => state.resolved);

const usesEnvVars = useMemo(() => ENV_VAR_PATTERN.test(settings), [settings]);

const [mode, setMode] = useState<Mode>('form');
const [exposeExperimental] = useState(false);

// The Effective tab is only meaningful when there is a `resolved`
// payload AND the file uses substitution. Falling back to Raw on
// either condition keeps the toggle honest if the user opens this
// page against an older server.
const canShowEffective = usesEnvVars && resolved != null;
useEffect(() => {
if (mode === 'effective' && !canShowEffective) setMode('raw');
}, [mode, canShowEffective]);

// Tab in textarea inserts two spaces instead of moving focus; rAF restores caret position after React re-renders.
const handleKeyDown = (e: React.KeyboardEvent<HTMLTextAreaElement>) => {
if (e.key !== 'Tab') return;
Comment on lines +12 to 42
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. effective tab lacks feature-flag 📘 Rule violation ⚙ Maintainability

The new env-var banner and Effective settings view are enabled automatically based on ${VAR}
placeholders rather than a dedicated feature flag that is disabled by default. This violates the
requirement to gate new functionality behind an explicit enable/disable mechanism to preserve
default behavior control.
Agent Prompt
## Issue description
New Admin Settings functionality (env-var banner + `Effective` tab/view) is gated by detecting `${VAR}` placeholders in `settings.json`, not by a dedicated feature flag that is disabled by default.

## Issue Context
Compliance requires new features to be behind a feature flag with an explicit enable/disable mechanism and default-off behavior.

## Fix Focus Areas
- admin/src/pages/SettingsPage.tsx[12-116]
- admin/src/components/settings/ModeToggle.tsx[3-50]

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

Expand Down Expand Up @@ -57,49 +77,80 @@ export const SettingsPage = () => {
settingsSocket.emit('saveSettings', settings);
};

const effectiveJson = useMemo(() => {
if (resolved == null) return '';
try { return JSON.stringify(resolved, null, 2); } catch { return ''; }
}, [resolved]);

return (
<div className="settings-page">
<h1><Trans i18nKey="admin_settings.current" /></h1>

<ModeToggle mode={mode} onChange={setMode} />

{mode === 'form'
? <FormView onSwitchToRaw={() => setMode('raw')} />
: (
<textarea
value={settings}
className="settings"
data-testid="settings-raw-textarea"
spellCheck={false}
onKeyDown={handleKeyDown}
onChange={v => useStore.getState().setSettings(v.target.value)}
/>
)
}
{usesEnvVars && (
<div
className="settings-envvar-banner"
role="note"
data-testid="settings-envvar-banner"
>
<Info size={18} aria-hidden="true" />
<div>
<strong><Trans i18nKey="admin_settings.envvar_banner.title" /></strong>
<p><Trans i18nKey="admin_settings.envvar_banner.body" /></p>
</div>
</div>
)}

<div className="settings-button-bar">
<IconButton
className="settingsButton"
data-testid="save-settings-button"
icon={<Save />}
title={<Trans i18nKey="admin_settings.current_save.value" />}
onClick={handleSave}
<ModeToggle mode={mode} onChange={setMode} showEffective={canShowEffective} />

{mode === 'form' && <FormView onSwitchToRaw={() => setMode('raw')} />}
{mode === 'raw' && (
<textarea
value={settings}
className="settings"
data-testid="settings-raw-textarea"
spellCheck={false}
onKeyDown={handleKeyDown}
onChange={v => useStore.getState().setSettings(v.target.value)}
/>
<IconButton
className="settingsButton"
data-testid="test-settings-button"
icon={<ShieldCheck />}
title={<Trans i18nKey="admin_settings.current_test.value" />}
onClick={testJSON}
)}
{mode === 'effective' && (
<textarea
value={effectiveJson}
className="settings"
data-testid="settings-effective-textarea"
spellCheck={false}
readOnly
aria-readonly="true"
/>
{exposeExperimental && (
<IconButton
className="settingsButton"
data-testid="prettify-settings-button"
icon={<AlignLeft />}
title={<Trans i18nKey="admin_settings.current_prettify.value" />}
onClick={prettifyJSON}
/>
)}

<div className="settings-button-bar">
{mode !== 'effective' && (
<>
<IconButton
className="settingsButton"
data-testid="save-settings-button"
icon={<Save />}
title={<Trans i18nKey="admin_settings.current_save.value" />}
onClick={handleSave}
/>
<IconButton
className="settingsButton"
data-testid="test-settings-button"
icon={<ShieldCheck />}
title={<Trans i18nKey="admin_settings.current_test.value" />}
onClick={testJSON}
/>
{exposeExperimental && (
<IconButton
className="settingsButton"
data-testid="prettify-settings-button"
icon={<AlignLeft />}
title={<Trans i18nKey="admin_settings.current_prettify.value" />}
onClick={prettifyJSON}
/>
)}
</>
)}
<IconButton
className="settingsButton"
Expand Down
5 changes: 5 additions & 0 deletions src/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,12 @@
"admin_settings.prettify_confirm": "Prettifying will remove all comments. Continue?",
"admin_settings.mode.form": "Form",
"admin_settings.mode.raw": "Raw",
"admin_settings.mode.effective": "Effective",
"admin_settings.mode.effective_tooltip": "Read-only view of the values Etherpad is actually using right now, after environment-variable substitution. Secrets are redacted.",
"admin_settings.mode.aria_label": "Editor mode",
"admin_settings.envvar_banner.title": "This file is a template, not the live config.",
"admin_settings.envvar_banner.body": "Placeholders like ${VAR:default} are substituted into memory at startup; they are never written back to this file. Edit env vars in your environment (Docker compose, systemd, .env) to change the resolved value, or replace the placeholder here with a literal. Switch to the Effective tab to see what Etherpad is using right now.",
"admin_settings.toast.auth_error": "You are not authenticated as admin. Please log in again.",
"admin_settings.section.general": "General",
"admin_settings.parse_error.title": "Cannot parse settings.json",
"admin_settings.parse_error.cta": "Switch to raw to edit",
Expand Down
12 changes: 11 additions & 1 deletion src/node/hooks/express/adminsettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,17 @@ exports.socketio = (hookName: string, {io}: any) => {
io.of('/settings').on('connection', (socket: any) => {
// @ts-ignore
const {session: {user: {is_admin: isAdmin} = {}} = {}} = socket.conn.request;
if (!isAdmin) return;
if (!isAdmin) {
// Previously this branch silently returned, so a non-admin client
// (e.g. a misrouted Traefik / OIDC session that didn't carry the
// admin cookie) would connect, emit load/save, and get nothing
// back — no error toast, no way to tell the save was ignored. Emit
// a dedicated event the SPA can surface, then drop the socket.
socket.emit('admin_auth_error',
{message: 'Not authenticated as admin. Re-authenticate and retry.'});
socket.disconnect(true);
return;
Comment thread
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
}

socket.on('load', async (query: string): Promise<any> => {
let data;
Expand Down
56 changes: 56 additions & 0 deletions src/tests/backend/specs/admin/adminSettingsAuthError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';

// Regression coverage for #7819 follow-up: non-admin sockets used to be
// silently dropped, which made misconfigured admin auth (e.g. Traefik +
// SSO + cross-origin iframe losing the cookie) look like "save didn't
// work" with no error path. The connection handler now emits a dedicated
// `admin_auth_error` event before disconnecting so the SPA can surface it.

import {strict as assert} from 'assert';

const io = require('socket.io-client');
const common = require('../../common');

const connectAnonymous = (): any =>
io(`${common.baseUrl}/settings`, {
forceNew: true,
// Deliberately omit cookie / auth — this mirrors a socket from a
// session that resolves through express-session but lacks is_admin.
transports: ['websocket'],
});

describe(__filename, function () {
this.timeout(15000);

before(async function () {
await common.init();
});

it('emits admin_auth_error and disconnects when not authenticated as admin',
async function () {
const socket = connectAnonymous();
const result = await new Promise<{event: 'auth_error' | 'disconnect' | 'timeout'; payload?: any}>((res) => {
const timeout = setTimeout(
() => { try { socket.disconnect(); } catch {} res({event: 'timeout'}); },
8000);
socket.once('admin_auth_error', (payload: any) => {
clearTimeout(timeout);
res({event: 'auth_error', payload});
});
socket.once('disconnect', () => {
clearTimeout(timeout);
res({event: 'disconnect'});
});
});
try { socket.disconnect(); } catch {}
// Either order is acceptable (auth_error then disconnect, or just
// disconnect if the event arrives after the close handshake) — but
// a timeout is a regression: the silent-no-op path is back.
assert.notEqual(result.event, 'timeout',
'admin_auth_error or disconnect must arrive within 8s for non-admin socket');
if (result.event === 'auth_error') {
assert.ok(result.payload && typeof result.payload.message === 'string',
`admin_auth_error must carry a message; got ${JSON.stringify(result.payload)}`);
}
});
});
1 change: 1 addition & 0 deletions src/tests/backend/specs/admin/adminSettingsSave.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,4 +217,5 @@ describe(__filename, function () {
assert.ok(onDisk.includes('persisted-marker-7819'),
'comment must survive the write path');
});

});
Loading
Loading