Settings aren't saved in admin UI

[Uatschitchun]

Describe the bug
In Admin UI it's possible to edit and save settings.json and also to restart etherpad.
When doing changes and save these, the changes to settings.json are not saved

To Reproduce
Steps to reproduce the behavior:

1. Go to 'Admin Panel (/admin/plugins)'
2. Click on 'settings'
3. Change something in settings.json
4. Click on 'save settings'
5. Click on 'restart etherpad'
6. changes are not saved

Expected behavior
changes to settings.json should be saved when done in Admin UI

Server (please complete the following information):

• Etherpad version: 2.0.3
• Is the server free of plugins: yes

[heldersepu]

testing with Etherpad version is 2.0.3 running a fresh clone of the repo
I'm able to reproduce...

I just add some extra comments via the UI, clicking save shows the animation:

and the success popup

But when the page reloads my change is not there

troubleshooting some more now ...

[heldersepu]

Frontend

etherpad-lite/admin/src/pages/SettingsPage.tsx

Lines 17 to 26 in 9d8f1f6

	<IconButton className="settingsButton" icon={<Save/>}
	title={<Trans i18nKey="admin_settings.current_save.value"/>} onClick={() => {
	if (isJSONClean(settings!)) {
	// JSON is clean so emit it to the server
	settingsSocket!.emit('saveSettings', settings!);
	useStore.getState().setToastState({
	open: true,
	title: "Succesfully saved settings",
	success: true
	})

Backend

etherpad-lite/src/node/hooks/express/adminsettings.ts

Lines 41 to 45 in 9d8f1f6

	socket.on('saveSettings', async (newSettings:string) => {
	console.log('Admin request to save settings through a socket on /admin/settings');
	await fsp.writeFile(settings.settingsFilename, newSettings);
	socket.emit('saveprogress', 'saved');
	});

[SamTV12345]

Frontend

etherpad-lite/admin/src/pages/SettingsPage.tsx

Lines 17 to 26 in 9d8f1f6

	<IconButton className="settingsButton" icon={<Save/>}
	title={<Trans i18nKey="admin_settings.current_save.value"/>} onClick={() => {
	if (isJSONClean(settings!)) {
	// JSON is clean so emit it to the server
	settingsSocket!.emit('saveSettings', settings!);
	useStore.getState().setToastState({
	open: true,
	title: "Succesfully saved settings",
	success: true
	})

Backend

etherpad-lite/src/node/hooks/express/adminsettings.ts

Lines 41 to 45 in 9d8f1f6

	socket.on('saveSettings', async (newSettings:string) => {
	console.log('Admin request to save settings through a socket on /admin/settings');
	await fsp.writeFile(settings.settingsFilename, newSettings);
	socket.emit('saveprogress', 'saved');
	});

Did you find something? The code looks good so far. The call is correct but the function is not called. Weirdly enough the restartServer callback is working

[heldersepu]

@SamTV12345 nothing significant yet ... same as you all points to the function not been called

not sure why there are no logger.info(...); in that adminsettings:
develop...heldersepu:etherpad-lite:develop
Other files do have it is there any reason for that not been here??

[SamTV12345]

@SamTV12345 nothing significant yet ... same as you all points to the function not been called

not sure why there are no logger.info(...); in that adminsettings: develop...heldersepu:etherpad-lite:develop Other files do have it is there any reason for that not been here??

Good idea. console.log is bound to logger.info but it is probably better to use it.

[heldersepu]

I think it has something to do with the size of that settings string ...

Kazam_screencast_00007.mp4

Look there when we remove a few lines from the comments it really does nothing...
but when i try with something smaller:

{
  "title": "Etherpad"
}

we can see my terminal shows:
[INFO] adminSettings - Admin request to save settings through a socket on /admin/settings

[heldersepu]

it feels this is caused by:

etherpad-lite/src/node/utils/Settings.ts

Lines 147 to 157 in 556c3c8

	exports.socketIo = {
	/**
	* Maximum permitted client message size (in bytes).
	*
	* All messages from clients that are larger than this will be rejected. Large values make it
	* possible to paste large amounts of text, and plugins may require a larger value to work
	* properly, but increasing the value increases susceptibility to denial of service attacks
	* (malicious clients can exhaust memory).
	*/
	maxHttpBufferSize: 10000,
	};

[heldersepu]

Yes that is the root cause increasing it to "maxHttpBufferSize": 50000 took care of the problem...

Now what is the "right" thing to do?

• increasing the default to 50000 will solve the issue now
• look for a technical solution to send smaller payload

---

maybe do both:

• increase this now and release fix to not carry this bug any longer.
• then spend some to refactor the saveSettings to send smaller payload, so we don't run into this again when the setting.json grows

[heldersepu]

...thinking about how to send a smaller payload

• an easy approach could be to remove the many many comments from the settings, it could be done before we display it in the admin/settings, and add a link there to the template:
  https://github.com/ether/etherpad-lite/blob/develop/settings.json.template
• another more involved approach, I'm thinking admin users make a small changes and save, what we could do is get a diff and send that to be applied, that should be significantly smaller, this could be helpful:
  https://www.npmjs.com/package/diff-match-patch

[SamTV12345]

Yes that is the root cause increasing it to "maxHttpBufferSize": 50000 took care of the problem...

Now what is the "right" thing to do?

• increasing the default to 50000 will solve the issue now
• look for a technical solution to send smaller payload

maybe do both:

• increase this now and release fix to not carry this bug any longer.
• then spend some to refactor the saveSettings to send smaller payload, so we don't run into this again when the setting.json grows

Oh damn. I added some things in the settings.json, that added extra bytes to the file, so that the default value cannot be used anymore to save the settings. Do you have some spare time for a permanent solution. The problem with the diff approach would be that it wouldn't work if I paste in the complete settings.json. Also it would add itself a payload for returning the diff.

[heldersepu]

Do you have some spare time for a permanent solution

Sure... What did you have in mind as permanent? removing all comments is the easiest way

Not sure how often someone comes around and pastes a complete json, even with that I bet the diff will be small unless they minified the Json or did something radical, at that point we could suggest doing the change directly on the server not via admin...

[heldersepu]

Now let me point ...
@Uatschitchun if you are following the comments and this is a must have for you, your options right now are:

• increasing that maxHttpBufferSize will do it (unfortunately you need to do that on the server)
• remove all the comments from that setting file to make it smaller

[Uatschitchun]

@heldersepu yes, I'm following. It's more of a "if it's there, it should work" than a must have ;)

[heldersepu]

Perfect I see the PR was merged...
I also been working on a function to remove the comments that way we reduce the size of what we save

[SamTV12345]

Perfect I see the PR was merged...

I also been working on a function to remove the comments that way we reduce the size of what we save

Thanks for the help. Really appreciated.

[heldersepu]

Thanks for the help. Really appreciated.

My pleasure! I've been doing a lot of DevOps lately, mostly Terraform; This is a great way to stay sharp while helping a real world project. ... and make some new friends in the process