fix(web): stop an unrelated config edit from erasing a plugin's secret - #478
fix(web): stop an unrelated config edit from erasing a plugin's secret#478ChuckBuilds wants to merge 1 commit into
Conversation
Saving any field on a plugin's config form destroyed that plugin's stored
credential. On a rig with a weather API key, changing the city silently
emptied the key, and the plugin stopped working at the next fetch with no
indication why.
The path had no guard at any step. The config partial masks secrets before
rendering (pages_v3.py:740), so the browser posts them back blank; _parse_value
deliberately preserves "" for optional string fields; separate_secrets routes
that "" into secrets_config, which is a truthy dict; deep_merge writes it over
the stored value; save_raw_file_content persists it.
The blank does not even need the round-trip. merge_with_defaults injects the
schema's api_key default ("") into every save, so a client that never sends
the field at all still erases it. test_secret_count_message_counts_top_level_keys
was counting exactly that injected blank as a saved secret field -- the visible
edge of the bug, pinned as expected behaviour.
remove_empty_secrets() already existed for this, with seven unit tests and a
docstring describing this precise scenario ("clients will send those empty
strings back ... so that existing stored secrets are not overwritten with
blanks"). It was never wired into a call site. This wires it into both save
paths that merge into the secrets file.
A blank now means "unchanged" rather than "delete", which is the same contract
the helper's tests already describe. The cost is that a secret can no longer be
cleared by emptying the field; clearing needs its own affordance, since a
control that erases credentials as a side effect of ordinary edits is not one.
Verified by reverting the guard: the new round-trip test then fails with the
stored key read back as ''. 262 web tests pass with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
|
Warning Review limit reached
Next review available in: 15 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
|
Superseded by #485, which combines the seven api_v3.py PRs so they do not conflict with each other. Every change from this PR is verified present on that branch; the branch here is untouched if you want to compare. |
1 participant
Saving any plugin setting erases that plugin's API key
Change the city on the weather plugin, and the weather API key is gone. The
plugin keeps rendering until its next fetch, then fails with no indication that
a credential was destroyed — and the field it was typed into still looks the
same afterwards, because it renders masked either way.
Reproduced end to end through the real endpoint, not the helpers:
Why nothing stopped it
Every step is individually reasonable, which is why this survived:
pages_v3.py:740hx-post, so htmx submits every field, including the blanked one_parse_valueseparate_secrets{'api_key': ''}intosecrets_config, a truthy dictdeep_mergeThe blank does not even require the round-trip.
merge_with_defaultsinjectsthe schema's
api_keydefault ("") into every save, so a client that neversends the field still erases it.
The fix already existed
remove_empty_secrets()is insecret_helpers.pywith seven unit tests and adocstring describing this exact scenario:
It was called from no production code. Its counterpart
mask_secret_fieldswas wired in; the half that protects the write path was not. This wires it into
both save paths that merge into the secrets file (
save_plugin_configand theplugin branch of
save_main_config).What changes for users
A blank now means "unchanged" rather than "delete" — the contract the helper's
own tests already describe. The cost is that a secret can no longer be cleared
by emptying the field. That needs its own affordance, and it should: a control
that destroys credentials as a side effect of ordinary edits is not a way to
clear them.
Verification
test_an_unrelated_edit_does_not_erase_a_stored_secretdrives the realFlask endpoint; reverting the guard makes it fail with
''read back.test_a_secret_can_still_be_changedguards the over-correction — a real newvalue must still save.
test_secret_count_message_counts_top_level_keyschanged 2 → 1. It wascounting the injected blank as a "saved secret field", pinning the bug's
visible edge as expected behaviour; the comment now records that.
Related, not fixed here
save_main_configlogs the entire POSTed body at ERROR on every save(
logging.error(f"DEBUG: save_main_config received data: {data}"), plus fullrequest headers). That puts credentials in the journal and is debug output
shipped at ERROR level. Separate change.