Skip to content

Commit e0d37cd

Browse files
dkulpclaude
andcommitted
fix(settings): stop reporting success when a settings write fails
WriteSettingToFile() opened the settings file with @fopen($f, "c+") and never checked the result. When the web user cannot write the file, the open returns false and PHP 8 throws "TypeError: flock(): Argument #1 must be of type resource, false given" -- but the API response is still HTTP 200, so the browser's success handler runs and jGrowl reports "setting saved" for a value that was never written. The setting then reverts on the next page load, with nothing in the UI to say why. The way in is a stray root write. Apache/PHP run as fpp, and FPP's own writers put ownership back (PutFileContents -> SetFilePerms chowns to fpp; sed -i preserves the owner), but anything running as root that replaces the file with a fresh inode leaves it root:root. Boot re-runs "chown -R fpp:fpp" over the media directory, so the broken state lasts only until the next reboot -- which is exactly long enough to look like a bug in whichever setting was being saved. - WriteSettingToFile() returns bool, checks both the open and the write, and logs which setting was lost. - On an unwritable file it re-asserts fpp ownership via sudo and retries once. Boot already asserts that same ownership, so the correct owner is not a guess, and one stray root write no longer disables saving for the rest of the uptime. - PutSetting() returns 500 with a JSON error instead of {"status":"OK"}, and skips the apply/restart path so the running system cannot diverge from disk. Existing callers ignore the new return value and are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent b72ce98 commit e0d37cd

2 files changed

Lines changed: 69 additions & 2 deletions

File tree

www/api/controllers/settings.php

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,17 @@ function PutSetting()
124124
$value = round(($value - 32) * 5 / 9);
125125
}
126126

127-
WriteSettingToFile($setting, $value);
127+
// A failed write must not fall through to the apply/restart steps below:
128+
// applying a value that isn't on disk leaves the running system and the
129+
// settings file disagreeing, and the UI would show "setting saved" for a
130+
// value that reverts on the next page load.
131+
if (!WriteSettingToFile($setting, $value)) {
132+
http_response_code(500);
133+
return json(array(
134+
"status" => "ERROR",
135+
"message" => "Unable to save the '" . $setting . "' setting. The settings file is not writable - check the FPP logs."
136+
));
137+
}
128138

129139
// Callers can pass ?skipApply=1 to persist the value WITHOUT running the
130140
// (sometimes expensive) side effects that apply it. This is used to write a

www/common.php

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,50 @@ function custom_parse_ini_file($filename)
196196
return $settings;
197197
}
198198

199+
/**
200+
* Re-assert fpp ownership on a config file the web user cannot write.
201+
*
202+
* FPP runs "chown -R fpp:fpp" over the media directory at every boot (see
203+
* setFileOwnership() in src/boot/FPPINIT_Config.cpp), so the correct owner is
204+
* not a guess. Anything running as root that replaces one of these files with
205+
* a fresh inode - an editor, a shell redirect, a restore run by hand - leaves
206+
* it root-owned, and every later write from the web UI fails until the next
207+
* boot. Repair it here so a single stray root write doesn't silently disable
208+
* saving for the rest of the uptime.
209+
*
210+
* Returns true only if the file is writable afterwards.
211+
*/
212+
function RepairConfigFileOwnership($filename)
213+
{
214+
global $SUDO;
215+
216+
// Nothing to repair if the file isn't there (fopen will create it) and
217+
// nothing to repair to on a system with no fpp user, e.g. macOS.
218+
if (!file_exists($filename) || posix_getpwnam('fpp') === false) {
219+
return false;
220+
}
221+
222+
$ids = GetFPPUserIds();
223+
$file = escapeshellarg($filename);
224+
exec($SUDO . " chown " . $ids['uid'] . ":" . $ids['gid'] . " " . $file .
225+
" && " . $SUDO . " chmod 664 " . $file, $output, $return_val);
226+
227+
if ($return_val != 0 || !is_writable($filename)) {
228+
return false;
229+
}
230+
231+
error_log("Repaired ownership of '$filename', which was not writable by the web user.");
232+
return true;
233+
}
234+
235+
/**
236+
* Write a single setting to the settings file, or to a plugin's config file.
237+
*
238+
* Returns true once the value is on disk (including when it was already the
239+
* stored value), false if it could not be written. Callers that tell a user
240+
* the setting was saved MUST check the return - reporting success on a failed
241+
* write shows a "saved" toast for a value that reverts on the next page load.
242+
*/
199243
function WriteSettingToFile($settingName, $new_setting_value, $plugin = "")
200244
{
201245
global $settingsFile;
@@ -207,6 +251,13 @@ function WriteSettingToFile($settingName, $new_setting_value, $plugin = "")
207251
}
208252

209253
$fd = @fopen($filename, "c+");
254+
if ($fd === false) {
255+
$fd = RepairConfigFileOwnership($filename) ? @fopen($filename, "c+") : false;
256+
if ($fd === false) {
257+
error_log("WriteSettingToFile: cannot open '$filename' for writing; '$settingName' was NOT saved.");
258+
return false;
259+
}
260+
}
210261
flock($fd, LOCK_EX);
211262
$tmpSettings = custom_parse_ini_file($filename);
212263
if (!isset($tmpSettings[$settingName]) || $tmpSettings[$settingName] != $new_setting_value) {
@@ -220,13 +271,19 @@ function WriteSettingToFile($settingName, $new_setting_value, $plugin = "")
220271
$RevisedSettingsStr .= $key . " = \"" . $value . "\"\n";
221272
}
222273
}
223-
file_put_contents($filename, $RevisedSettingsStr);
274+
if (@file_put_contents($filename, $RevisedSettingsStr) === false) {
275+
error_log("WriteSettingToFile: write to '$filename' failed; '$settingName' was NOT saved.");
276+
flock($fd, LOCK_UN);
277+
fclose($fd);
278+
return false;
279+
}
224280
}
225281
if ($plugin == "") {
226282
$settings[$settingName] = $new_setting_value;
227283
}
228284
flock($fd, LOCK_UN);
229285
fclose($fd);
286+
return true;
230287
}
231288

232289
/**

0 commit comments

Comments
 (0)