Two robustness issues in the PUT /skServer/settings handler (src/serverroutes.ts, handler starting ~line 1096):
- Unguarded
settings.options dereference. The handler reads settings.options.mdns, settings.options.ssl, etc. without checking that options exists. A request body lacking an options object throws and returns 500. (The admin UI always sends options, so it does not surface in normal use.)
forIn merges copy arbitrary keys and types. The courseApi and notifications blocks are persisted via forIn(settings.<block>, (value, name) => { ... [name] = value }), which copies every key and value from the request body verbatim. Unknown keys and wrong-typed values (e.g. the string "false", which isManaging() treats as truthy) get written to settings.json. Admin-gated, so impact is low, but it breaks the typed config contract.
Suggested fix: guard the options access, and validate/whitelist known keys with type coercion for both settings blocks (keeping them consistent with each other).
CodeRabbit flagged item 2 for the notifications block on PR #2766; it was deferred there because fixing one of the two mirrored blocks in isolation would make the handler inconsistent — both belong in one focused change here.
Two robustness issues in the
PUT /skServer/settingshandler (src/serverroutes.ts, handler starting ~line 1096):settings.optionsdereference. The handler readssettings.options.mdns,settings.options.ssl, etc. without checking thatoptionsexists. A request body lacking anoptionsobject throws and returns 500. (The admin UI always sendsoptions, so it does not surface in normal use.)forInmerges copy arbitrary keys and types. ThecourseApiandnotificationsblocks are persisted viaforIn(settings.<block>, (value, name) => { ... [name] = value }), which copies every key and value from the request body verbatim. Unknown keys and wrong-typed values (e.g. the string"false", whichisManaging()treats as truthy) get written tosettings.json. Admin-gated, so impact is low, but it breaks the typed config contract.Suggested fix: guard the
optionsaccess, and validate/whitelist known keys with type coercion for both settings blocks (keeping them consistent with each other).CodeRabbit flagged item 2 for the
notificationsblock on PR #2766; it was deferred there because fixing one of the two mirrored blocks in isolation would make the handler inconsistent — both belong in one focused change here.