fix: feed_api_key field doesn't show its "stored" state after masking

[rjonesbsink]

Summary

Fixes the config.js half of #7. 07b9d1d already fixed the actual data-loss bug (data-secret="1" on feed_api_key in settings.php) and covered the 9-field audit + boolean-masking issue — this PR doesn't touch any of that.

What's left is the piece explicitly called out as a separate gap in that thread: applySettingsToForm() has no concept of the <name>_set sentinel the server sends for masked fields, so a correctly-configured feed_api_key still renders as an empty box, and the "external feed is disabled" banner still shows even though a key is in fact set.

Change

loadApiKeys() now reads feed_api_key_set and shows the same "stored — leave blank to keep, type to replace" placeholder loadSlackConfig() already uses for its own secret fields, tracking a data-configured flag so updateFeedKeyBanner() reflects the real state instead of just checking whether the box happens to be non-empty.

This branch previously also carried the data-secret="1" addition to settings.php, but that's now identical to what's already on main via 07b9d1d — dropped it to keep this PR to just the one remaining gap, rather than duplicating your own fix.

Testing

Verified end-to-end on a live install: with a key configured, the field now shows the "stored" placeholder and the banner stays hidden; saving an unrelated field on the same form no longer touches the key (confirmed via direct DB check) and the display now correctly reflects that.

[ejosterberg]

Reviewed this one too — it's good to go from my side. No security surface, no server-side change, ES5-clean, and it mirrors loadSlackConfig()'s existing pattern rather than inventing a new one.

I particularly like that the banner logic keeps the blank-and-configured case hidden, which is what "blank means keep the stored value" should look like to a user. That was the confusing part of #7 and this makes the state visible instead of implied.

It's still marked draft — flip it to ready and I'll merge it.

Thanks for splitting this out from the larger fix rather than bundling it. Reviewing a focused 22-line diff is a very different experience from reviewing it buried in a nine-file change, and I noticed.

[ejosterberg]

Review outcome: no objections. Merge decision is Eric's.

This went through a full adversarial pre-merge security review alongside #10. The verdict on this one is safe as-is — no server-side change, no new network call, no secret reaches the browser, and it mirrors loadSlackConfig() rather than inventing a pattern.

Two things the review confirmed specifically, since a "stored secret" change is worth being precise about:

• The only server data this consumes is settings.feed_api_key_set, a boolean produced by api/config-admin.php ($map[$name.'_set'] = ($row['value'] !== '' && $row['value'] !== null)). No value is transmitted, and data-configured holds '1'/'0' only.
• feedInput.value = '' is set unconditionally, after applySettingsToForm() runs. So even if a future server-side regression started leaking feed_api_key in the settings map, this code would blank it out of the DOM. That is a defensive improvement over the pre-PR behaviour, not just a cosmetic one.

One thing you should know before you spend more time on it

While this was open, the same defect got fixed in the dev tree independently, and in a more general form. Telling you plainly because it affects what happens to your branch, not to take anything away from it — your report in #7 is what drove the fix.

Instead of handling feed_api_key at its call site, applySettingsToForm() now handles every field marked data-secret="1":

// A secret never arrives as a value — the server sends `<key>_set`.
// Reflect that state in the placeholder and leave the box empty, so
// saving without retyping omits the key and the stored value lives.
if (el.getAttribute('data-secret') === '1') {
    markSecretFieldState(el, settings[key + '_set'] === true);
    continue;
}

markSecretFieldState() blanks the value and swaps the placeholder, and the banner reads a _feedKeyStored flag set from feed_api_key_set in loadApiKeys(). Same diagnosis, same contract, applied once for all secret fields rather than per panel.

The rest of what you found in that area landed too:

• The three panels that silently wiped a stored credential on save — aprs_fi_api_key, location_ingest_secret, owntracks_secret — hand-built their pairs object, bypassed collectSettingsFromForm(), and posted ''. An admin toggling "Allow anonymous" in OwnTracks Auth would wipe owntracks_secret, and api/location.php then rejected every device report with X-OwnTracks-Reason: no-auth. Units froze on the dispatch map at their last known position, with no error anywhere in the UI. Fixed in 1923ba5.

• The server-side backstop, which is the important half — the "never blank a stored secret" rule lived only in client JS, so it was one hand-written save handler away from being untrue. api/config-admin.php now refuses it server-side regardless of which of the ~40 JS handlers is posting:

  if (is_secret_setting_key($key) && ($value === '' || is_masked_secret_value($value))) {

So: the behaviour you reported is fixed and shipped. Whether to merge this branch as well is Eric's call and I'm not going to pre-empt it.

Why merging it here alone would not have worked anyway

Worth knowing for #10 too, and for anything you send in future — it is not obvious from the outside.

This public repository is a one-way, full-tree-replace snapshot of a private dev tree. A change merged only here is overwritten by the dev tree's version of that file at the next release — or deleted outright if the dev tree has no such file. Git raises no objection and no diff shows it: from this side it just looks like the maintainer committed a tree in which your change is absent. A fix can appear merged, sit for days, and quietly vanish.

That is not hypothetical, which is why there is now a guard. tools/release-divergence-check.php compares three trees per file — the staged snapshot, this repo's main, and the tree the last release published — and fails the release if publishing would overwrite a public-only change or delete a public-only file. It fails closed: if it cannot reach this repo or find a baseline, it refuses rather than reporting clean.

The practical consequence: an accepted contribution has to be applied in the dev tree, not merged here. Merging here is the acknowledgement, not the shipping. Nothing for you to do differently — just so a later git pull that appears to drop your commit doesn't look like it was reverted on purpose.

Thanks for this one. The diagnosis was right, and the reason it is fixed is that you wrote it up.

[ejosterberg]

Closing this one without merging, and I want to be precise about why, because your diagnosis was correct and the credit for finding this is yours.

You identified the real defect: feed_api_key is a data-secret field, the server never sends its value — only a feed_api_key_set boolean — so a blank box on load means "stored, not shown", and code that treats blank as "not configured" tells a correctly-configured install that its feed is disabled. That is exactly right, and on a system where an admin might then go and "fix" a working feature, it matters more than it looks.

While this PR was open, the same defect was fixed in the development tree in a more general form (1923ba5, "Saving a Settings panel wiped the stored secret it never showed you"). Rather than handling feed_api_key specifically, that change routes every data-secret field through one path:

• applySettingsToForm() sends any element carrying data-secret="1" to markSecretFieldState(el, settings[key + '_set'] === true).
• markSecretFieldState() blanks the box unconditionally and swaps the placeholder to •••••••• stored — leave blank to keep, restoring the field's original placeholder when nothing is stored.
• The banner reads a _feedKeyStored flag set from feed_api_key_set on load, rather than reading the input.

That is the same three behaviours your patch introduced — the corrected banner, the unconditional blanking, and the stored/not-set placeholder — applied to every secret field instead of one. I checked each against your diff before closing rather than assuming, including that setFeedApiKey actually carries data-secret="1" in settings.php so it reaches that path at all. It does.

One thing you should know, since it is not visible from here: that fix has not been published yet. This repository is a snapshot of a private development tree, republished whole at each release, so main here still shows the old updateFeedKeyBanner() reading input.value.trim(). The bug you reported is genuinely still live in the published code and will be until the next release carries the general fix over. So if you are running from this repository, you are not imagining it — you are looking at the unfixed version.

The fix went further than the display problem you reported. The same masking gap had a data-loss half: four save handlers hand-built their POST payload and posted the always-empty secret box verbatim, so changing anything else on those panels and clicking Save wrote '' over the stored credential. Blank owntracks_secret makes every OwnTracks device 403 on ingest; blank location_ingest_secret gives 401; blank aprs_fi_api_key stops APRS polling. Silent in every case — responder units simply freeze on the dispatch map at their last known position. No attacker needed; an operator does it to themselves by clicking Save on an unrelated toggle. That is now fixed on the client and backstopped on the server, so it holds regardless of which of the ~40 save handlers does the posting.

Your issue #7 is what started that thread, and your note there that the suffix-based secret classifier deserved an audit is cited in inc/settings-secrets.php where that audit lives.

So: nothing needed from you, nothing wrong with the patch, and it is only being closed because merging it now would conflict with a fix that already covers it. The check failure showing on this PR is a stale run from before a large amount of change on main — it is not a problem with your work.

Ignore the merge status; the bug you found is fixed, and it is fixed more thoroughly than the report asked for. Thank you.