Skip to content

Saving settings replaces config.json wholesale, so a partial payload erases the rest #980

Description

@m4bard

Found while reviewing #880 and #907; out of scope for both. Reproduced by reading, not by
running. Install observation pending.

What happens

POST /api/v1/configuration/startupconfig binds the whole StartupConfig and hands it
straight to the writer. There is no merge with what is already on disk:

  • StartupConfigurationController.SaveStartupConfig normalises ApiVersion and calls
    IConfigurationService.SaveStartupConfigAsync(config).
  • ConfigurationService.SaveStartupConfigAsync checks the auth-enable lockout guard and
    calls startupConfigService.SaveAsync(config).
  • StartupConfigService.SaveAsync assigns _config = config and writes the serialised
    object over the file.
    So every field absent from the posted body is gone from config.json: the API key, the bind
    address, the port, the SSL settings, the log level.
    The SPA reaches that path on every settings save. SettingsView spreads whatever it holds
    in its startupConfig ref into the outgoing body, and that ref starts as null and is
    filled in onMounted from GET /startupconfig. If that GET failed, the ref is still null
    when the user presses Save, original falls back to {}, and the body posted is
    { authenticationRequired: 'false' } and nothing else.

Why it matters more now

Two open changes add fields to StartupConfig that only live in that file:

Options

  1. Merge server-side. SaveAsync reads the current config and overlays only the
    properties present in the request. Needs a way to tell "absent" from "explicitly null",
    so the DTO wants nullable-with-presence semantics (JsonElement, or an explicit patch
    DTO) rather than the domain object bound directly. Changes the endpoint's contract from
    replace to merge, which is a documented behaviour change but the one that matches what
    every caller actually wants. Recommended.
  2. Guard the client only. SettingsView refuses to post a startup config it never
    successfully loaded, and surfaces the load failure. Smallest change, fixes the SPA, and
    leaves the endpoint able to erase a config for anyone using it directly or through a
    Readarr-shaped client.
  3. Both. The client guard is cheap and independently correct; the server merge is what
    makes the endpoint safe for the API key it holds. Doing 2 first and 1 as a follow-up is a
    reasonable split if the contract change wants its own discussion.

Not proposed

Reconstructing a lost API key silently on the next load. StartupConfigService already
generates one when the key is missing, which means a wipe is survivable but hands every
existing client a changed key with no notice.
Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions