From 977d66d3163dd0cabdd7ba56dada37f238ea88e5 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 14:32:33 -0500 Subject: [PATCH 1/3] A removed key's direct-build refusal makes no claim that no environment variable set it (BACKLOG #3350) security show builds [security] straight from the file's table, so for require_sign_in or handles_real_patient_data in the file it printed the direct-build refusal: "The key was passed straight to the section, so no environment variable set it." The service's environment may still set the same key. The refusal now only says to remove the key from the arguments or the file table. Set in the environment, security show already printed the loader's refusal, which names the MEFOR_ variable. Tests run security show with each [security] removed key in the file and the environment (no "environment" in the output), with an env-only control. Proposed PR title: security show no longer says no environment variable set a removed key (BACKLOG #3350) Proposed vault banner: CLOSED by engine PR . The direct-build refusal security show prints for a removed [security] key in the file no longer claims that no environment variable set it; tests run security show with the key in the file and the environment. --- changelog.d/3350.fixed.md | 7 ++++ messagefoundry/config/settings.py | 10 +++--- tests/test_open_mode_is_no_service.py | 4 ++- tests/test_security_cli.py | 49 +++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 7 deletions(-) create mode 100644 changelog.d/3350.fixed.md diff --git a/changelog.d/3350.fixed.md b/changelog.d/3350.fixed.md new file mode 100644 index 0000000000..3ea5e46395 --- /dev/null +++ b/changelog.d/3350.fixed.md @@ -0,0 +1,7 @@ +- **A removed key's refusal no longer says that no environment variable set it.** `security show` + refuses a `[security]` table that still holds `require_sign_in` or `handles_real_patient_data`. + Its message said the key was passed straight to the section, so no environment variable set it. + That held for the file `security show` read, not for the service, whose environment may set the + same key and would still refuse at start. The refusal now only says to remove the key from the + file table or the arguments. The loader's refusal still names the `MEFOR_` variable to unset. + (vault BACKLOG #3350) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index bcaecf6b11..c19b0d2faa 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -7943,13 +7943,11 @@ def _removed_key_message( The removal step depends on where the key came from (vault BACKLOG #3216). The loader reads a file and the environment, so it names both. A section built directly reads neither: code passed the key (the loader's own ``cli`` overrides count as code), or ``security show`` - read it from the file's table. Naming an environment - variable there would send the reader to a place the key cannot be.""" + read it from the file's table. That build read no variable, so its refusal names none, and it + makes no claim about the environment either (vault BACKLOG #3350): ``security show`` prints it + for a key in the file while the service's environment may set the same key too.""" if built_directly: - fix = ( - f"The key was passed straight to the section, so no environment variable set it. " - f"Remove `{key}` from the arguments or the file table the section was built from" - ) + fix = f"Remove `{key}` from the arguments or the file table the section was built from" if hint := _REMOVED_KEY_BUILT_DIRECTLY_HINT.get((section, key)): fix = f"{fix}. {hint}" else: diff --git a/tests/test_open_mode_is_no_service.py b/tests/test_open_mode_is_no_service.py index 774ee45454..26ad9e4bb4 100644 --- a/tests/test_open_mode_is_no_service.py +++ b/tests/test_open_mode_is_no_service.py @@ -239,7 +239,9 @@ def test_each_refusal_names_only_the_places_the_key_could_be(section: str, key: with pytest.raises(ValueError) as built: _section_models()[section].model_validate({key: False}) assert variable not in str(built.value) - assert "no environment variable set it" in str(built.value) + # Nor does it say that no variable set the key (vault BACKLOG #3350): the service's + # environment may set it beside the file `security show` read. + assert "environment" not in str(built.value) assert f"Remove `{key}`" in str(built.value) # The loader's own refusal, from the environment: it names the variable that set the key. with pytest.raises(ValueError, match=variable): diff --git a/tests/test_security_cli.py b/tests/test_security_cli.py index a5de642b6a..0d701a4313 100644 --- a/tests/test_security_cli.py +++ b/tests/test_security_cli.py @@ -14,6 +14,7 @@ import pytest from messagefoundry.__main__ import main +from messagefoundry.config.settings import _ENV_PREFIX, _REMOVED_KEYS def _show(path: Path, capsys: pytest.CaptureFixture[str]) -> dict: @@ -368,6 +369,54 @@ def test_show_names_the_stray_variable_that_made_the_report_partial( assert "SYNTHETIC-VALUE-9f3c" not in json.dumps(out_data) +# --- a removed key the environment may still set (vault BACKLOG #3350) ------------------------ + + +#: Only ``security show``'s own table is built straight from the file, so only these keys reach the +#: direct-build refusal from it. Every other removed key reaches it through the loader's refusal. +_SECURITY_REMOVED_KEYS = sorted(key for section, key in _REMOVED_KEYS if section == "security") + + +@pytest.mark.parametrize("key", _SECURITY_REMOVED_KEYS) +def test_show_never_says_no_variable_set_a_removed_key( + key: str, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The key is in the file AND the environment, as a service environment may still set it. + + The refusal comes from the section built from the file's table, which read no variable. It + used to say so as "no environment variable set it", which sent an operator away from the + variable the service would still refuse at start.""" + toml = tmp_path / "mf.toml" + toml.write_text(f"[security]\n{key} = false\n", encoding="utf-8") + monkeypatch.setenv(f"{_ENV_PREFIX}SECURITY_{key.upper()}", "false") + rc = main(["security", "show", "--service-config", str(toml), "--json"]) + printed = "".join(capsys.readouterr()) + # Control: the direct-build refusal, the one that carried the claim, reached the output. + assert rc == 1 and "invalid [security]" in printed and f"Remove `{key}`" in printed + assert "environment" not in printed + + +@pytest.mark.parametrize("key", _SECURITY_REMOVED_KEYS) +def test_show_names_the_variable_that_set_a_removed_key( + key: str, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The control: with the key in the environment only, the file builds and the partial reason + is the loader's refusal, which names the variable to unset.""" + toml = tmp_path / "mf.toml" + toml.write_text("[auth]\nlockout_minutes = 15\n", encoding="utf-8") + variable = f"{_ENV_PREFIX}SECURITY_{key.upper()}" + monkeypatch.setenv(variable, "false") + data = _show(toml, capsys) + assert data["loosenings_partial"] is True + assert variable in data["loosenings_partial_reason"] + + # --- operator JSON that nests past the decoder (BACKLOG #1855) -------------------------------- # # `json.loads` guards its own decode depth and raises `RecursionError`, which is a `RuntimeError` -- From 70c1e9d807eeb63c8e815112d4d54d751629e371 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 14:41:32 -0500 Subject: [PATCH 2/3] Pin the removal step's position and the file-only equality in the removed-key refusal tests (BACKLOG #3350) Code review round one. The tests now assert that the removal step follows the reason directly, so a reworded provenance claim fails too, and that security show prints the same refusal with and without the variable set. The docstring and changelog name security set beside security show, as a floor. The earlier commit message's "Set in the environment, security show already printed the loader's refusal" holds only when the environment is the sole source: with the key in the file too, security show prints only the direct-build refusal. Proposed PR title: security show no longer says no environment variable set a removed key (BACKLOG #3350) Proposed vault banner: CLOSED by engine PR . The direct-build refusal for a removed key, which security show and security set print for a [security] key in the file, no longer claims that no environment variable set it; tests run security show with the key in the file, with and without the variable, and with the variable alone. --- changelog.d/3350.fixed.md | 16 ++++++------ messagefoundry/config/settings.py | 9 ++++--- tests/test_open_mode_is_no_service.py | 7 +++--- tests/test_security_cli.py | 35 ++++++++++++++++++--------- 4 files changed, 41 insertions(+), 26 deletions(-) diff --git a/changelog.d/3350.fixed.md b/changelog.d/3350.fixed.md index 3ea5e46395..4e7b12bd3d 100644 --- a/changelog.d/3350.fixed.md +++ b/changelog.d/3350.fixed.md @@ -1,7 +1,9 @@ -- **A removed key's refusal no longer says that no environment variable set it.** `security show` - refuses a `[security]` table that still holds `require_sign_in` or `handles_real_patient_data`. - Its message said the key was passed straight to the section, so no environment variable set it. - That held for the file `security show` read, not for the service, whose environment may set the - same key and would still refuse at start. The refusal now only says to remove the key from the - file table or the arguments. The loader's refusal still names the `MEFOR_` variable to unset. - (vault BACKLOG #3350) +- **A removed key's refusal no longer says that no environment variable set it.** The refusal a + section built directly gives for a removed key said the key was passed straight to the section, + so no environment variable set it. That section is built in code, and also from the file's + `[security]` table by at least `security show` and `security set`, so an operator whose file + still held `require_sign_in` or `handles_real_patient_data` read that claim. It held for the + file the command read, not for the service, whose environment may set the same key and would + still refuse at start. The refusal now gives the removal step with no claim about where the key + came from. The loader's refusal still names the `MEFOR_` variable to unset. (vault BACKLOG + #3350) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index c19b0d2faa..ee53747db7 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -7942,10 +7942,11 @@ def _removed_key_message( The removal step depends on where the key came from (vault BACKLOG #3216). The loader reads a file and the environment, so it names both. A section built directly reads neither: code - passed the key (the loader's own ``cli`` overrides count as code), or ``security show`` - read it from the file's table. That build read no variable, so its refusal names none, and it - makes no claim about the environment either (vault BACKLOG #3350): ``security show`` prints it - for a key in the file while the service's environment may set the same key too.""" + passed the key (the loader's own ``cli`` overrides count as code), or ``security show`` or + ``security set`` read it from the file's table, at least. That build read no variable, so its + refusal names none, and it makes no claim about the environment either (vault BACKLOG + #3350): ``security show`` prints it for a key in the file while the service's environment + may set the same key too.""" if built_directly: fix = f"Remove `{key}` from the arguments or the file table the section was built from" if hint := _REMOVED_KEY_BUILT_DIRECTLY_HINT.get((section, key)): diff --git a/tests/test_open_mode_is_no_service.py b/tests/test_open_mode_is_no_service.py index 26ad9e4bb4..65436ca64e 100644 --- a/tests/test_open_mode_is_no_service.py +++ b/tests/test_open_mode_is_no_service.py @@ -240,9 +240,10 @@ def test_each_refusal_names_only_the_places_the_key_could_be(section: str, key: _section_models()[section].model_validate({key: False}) assert variable not in str(built.value) # Nor does it say that no variable set the key (vault BACKLOG #3350): the service's - # environment may set it beside the file `security show` read. - assert "environment" not in str(built.value) - assert f"Remove `{key}`" in str(built.value) + # environment may set it beside the file `security show` read. The removal step follows the + # decision directly, so no claim about where the key came from sits between them. + assert f"{reason}. Remove `{key}` from" in str(built.value) + assert "no environment variable" not in str(built.value) # The loader's own refusal, from the environment: it names the variable that set the key. with pytest.raises(ValueError, match=variable): load_settings(environ={variable: "false"}, default_file=False) diff --git a/tests/test_security_cli.py b/tests/test_security_cli.py index 0d701a4313..d157dd69eb 100644 --- a/tests/test_security_cli.py +++ b/tests/test_security_cli.py @@ -375,28 +375,39 @@ def test_show_names_the_stray_variable_that_made_the_report_partial( #: Only ``security show``'s own table is built straight from the file, so only these keys reach the #: direct-build refusal from it. Every other removed key reaches it through the loader's refusal. _SECURITY_REMOVED_KEYS = sorted(key for section, key in _REMOVED_KEYS if section == "security") +# An empty list would turn both tests below into a silent skip. +assert _SECURITY_REMOVED_KEYS, "no removed [security] key left to test" + + +def _show_refusal(path: Path, capsys: pytest.CaptureFixture[str]) -> str: + assert main(["security", "show", "--service-config", str(path), "--json"]) == 1 + error = json.loads(capsys.readouterr().out)["error"] + assert isinstance(error, str) + return error @pytest.mark.parametrize("key", _SECURITY_REMOVED_KEYS) -def test_show_never_says_no_variable_set_a_removed_key( +def test_show_makes_no_claim_about_where_a_removed_key_was_not_set( key: str, tmp_path: Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch, ) -> None: - """The key is in the file AND the environment, as a service environment may still set it. + """The key is in the file, and the run is repeated with the environment setting it too. - The refusal comes from the section built from the file's table, which read no variable. It - used to say so as "no environment variable set it", which sent an operator away from the - variable the service would still refuse at start.""" + The refusal comes from the section built from the file's table, so it is the same text in both + runs: it cannot know what the environment holds. It used to say "no environment variable set + it", which sent an operator away from a variable the service would still refuse at start. The + removal step now follows the decision directly, with no claim about where the key came from.""" toml = tmp_path / "mf.toml" toml.write_text(f"[security]\n{key} = false\n", encoding="utf-8") + file_only = _show_refusal(toml, capsys) monkeypatch.setenv(f"{_ENV_PREFIX}SECURITY_{key.upper()}", "false") - rc = main(["security", "show", "--service-config", str(toml), "--json"]) - printed = "".join(capsys.readouterr()) - # Control: the direct-build refusal, the one that carried the claim, reached the output. - assert rc == 1 and "invalid [security]" in printed and f"Remove `{key}`" in printed - assert "environment" not in printed + file_and_env = _show_refusal(toml, capsys) + assert file_and_env == file_only + assert file_only.startswith("invalid [security]") + assert f"{_REMOVED_KEYS['security', key]}. Remove `{key}` from" in file_only + assert "no environment variable" not in file_only @pytest.mark.parametrize("key", _SECURITY_REMOVED_KEYS) @@ -406,8 +417,8 @@ def test_show_names_the_variable_that_set_a_removed_key( capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch, ) -> None: - """The control: with the key in the environment only, the file builds and the partial reason - is the loader's refusal, which names the variable to unset.""" + """With the key in the environment only, the file builds and the partial reason is the + loader's refusal, which names the variable to unset.""" toml = tmp_path / "mf.toml" toml.write_text("[auth]\nlockout_minutes = 15\n", encoding="utf-8") variable = f"{_ENV_PREFIX}SECURITY_{key.upper()}" From cf962a3b9a1ce0e2a2cd1178caa67b894a9547b1 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 14:48:48 -0500 Subject: [PATCH 3/3] Pin the whole removal step and fix the removed-key refusal wording claims (BACKLOG #3350) Code review round two. The security show test pins the full removal step, so a provenance claim before, inside or after it fails, and both needles are case-insensitive. The docstring no longer says a direct build reads neither source while listing the file table as one. The changelog uses the conditional, and the test comments drop two false premises. Shipped open: security show and security set could render the loader's conditional wording (or the partial reason they already compute) instead of the direct-build refusal; that is a __main__.py change this item's brief steered away from. No test runs security set with a removed key. Proposed PR title: security show no longer says no environment variable set a removed key (BACKLOG #3350) Proposed vault banner: CLOSED by engine PR . The direct-build refusal for a removed key, which security show and security set print for a [security] key in the file, no longer claims that no environment variable set it; tests run security show with the key in the file, with and without the variable, and with the variable alone. --- changelog.d/3350.fixed.md | 10 +++++----- messagefoundry/config/settings.py | 11 +++++------ tests/test_open_mode_is_no_service.py | 2 +- tests/test_security_cli.py | 20 ++++++++++++-------- 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/changelog.d/3350.fixed.md b/changelog.d/3350.fixed.md index 4e7b12bd3d..a342d7321b 100644 --- a/changelog.d/3350.fixed.md +++ b/changelog.d/3350.fixed.md @@ -2,8 +2,8 @@ section built directly gives for a removed key said the key was passed straight to the section, so no environment variable set it. That section is built in code, and also from the file's `[security]` table by at least `security show` and `security set`, so an operator whose file - still held `require_sign_in` or `handles_real_patient_data` read that claim. It held for the - file the command read, not for the service, whose environment may set the same key and would - still refuse at start. The refusal now gives the removal step with no claim about where the key - came from. The loader's refusal still names the `MEFOR_` variable to unset. (vault BACKLOG - #3350) + still held `require_sign_in` or `handles_real_patient_data` would read that claim. It held for + the file the command read, not for the service, whose environment may set the same key and + would still refuse at start. The refusal now says only to remove the key from the arguments or + the file table, and no longer claims that no environment variable set it. The loader's refusal + still names the `MEFOR_` variable to unset. (vault BACKLOG #3350) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index ee53747db7..597f80564c 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -7941,12 +7941,11 @@ def _removed_key_message( """The refusal for a removed key: the decision (``reason``), then how to remove the key. The removal step depends on where the key came from (vault BACKLOG #3216). The loader reads a - file and the environment, so it names both. A section built directly reads neither: code - passed the key (the loader's own ``cli`` overrides count as code), or ``security show`` or - ``security set`` read it from the file's table, at least. That build read no variable, so its - refusal names none, and it makes no claim about the environment either (vault BACKLOG - #3350): ``security show`` prints it for a key in the file while the service's environment - may set the same key too.""" + file and the environment, so it names both. A section built directly reads no environment: + code passed the key (the loader's own ``cli`` overrides count as code), or at least + ``security show`` and ``security set`` passed it from the file's table. Its refusal names no + variable, and makes no claim that none set the key (vault BACKLOG #3350): ``security show`` + prints it for a key in the file while the service's environment may set the same key too.""" if built_directly: fix = f"Remove `{key}` from the arguments or the file table the section was built from" if hint := _REMOVED_KEY_BUILT_DIRECTLY_HINT.get((section, key)): diff --git a/tests/test_open_mode_is_no_service.py b/tests/test_open_mode_is_no_service.py index 65436ca64e..eeec0a0ced 100644 --- a/tests/test_open_mode_is_no_service.py +++ b/tests/test_open_mode_is_no_service.py @@ -243,7 +243,7 @@ def test_each_refusal_names_only_the_places_the_key_could_be(section: str, key: # environment may set it beside the file `security show` read. The removal step follows the # decision directly, so no claim about where the key came from sits between them. assert f"{reason}. Remove `{key}` from" in str(built.value) - assert "no environment variable" not in str(built.value) + assert "environment variable" not in str(built.value).lower() # The loader's own refusal, from the environment: it names the variable that set the key. with pytest.raises(ValueError, match=variable): load_settings(environ={variable: "false"}, default_file=False) diff --git a/tests/test_security_cli.py b/tests/test_security_cli.py index d157dd69eb..7a82465e34 100644 --- a/tests/test_security_cli.py +++ b/tests/test_security_cli.py @@ -372,8 +372,9 @@ def test_show_names_the_stray_variable_that_made_the_report_partial( # --- a removed key the environment may still set (vault BACKLOG #3350) ------------------------ -#: Only ``security show``'s own table is built straight from the file, so only these keys reach the -#: direct-build refusal from it. Every other removed key reaches it through the loader's refusal. +#: ``security show`` builds only ``[security]`` straight from the file's table, so only these keys +#: reach the direct-build refusal through it. It reports every other removed key through the +#: loader's refusal. _SECURITY_REMOVED_KEYS = sorted(key for section, key in _REMOVED_KEYS if section == "security") # An empty list would turn both tests below into a silent skip. assert _SECURITY_REMOVED_KEYS, "no removed [security] key left to test" @@ -395,10 +396,10 @@ def test_show_makes_no_claim_about_where_a_removed_key_was_not_set( ) -> None: """The key is in the file, and the run is repeated with the environment setting it too. - The refusal comes from the section built from the file's table, so it is the same text in both - runs: it cannot know what the environment holds. It used to say "no environment variable set - it", which sent an operator away from a variable the service would still refuse at start. The - removal step now follows the decision directly, with no claim about where the key came from.""" + The refusal is built from the file's table alone, so it is the same text in both runs. It used + to add "no environment variable set it", which would send an operator away from a variable the + service would still refuse at start. The removal step is pinned whole, so a provenance claim + placed before it, inside it or after it fails.""" toml = tmp_path / "mf.toml" toml.write_text(f"[security]\n{key} = false\n", encoding="utf-8") file_only = _show_refusal(toml, capsys) @@ -406,8 +407,11 @@ def test_show_makes_no_claim_about_where_a_removed_key_was_not_set( file_and_env = _show_refusal(toml, capsys) assert file_and_env == file_only assert file_only.startswith("invalid [security]") - assert f"{_REMOVED_KEYS['security', key]}. Remove `{key}` from" in file_only - assert "no environment variable" not in file_only + assert ( + f"{_REMOVED_KEYS['security', key]}. Remove `{key}` from the arguments or the file table " + "the section was built from (see docs/CONFIGURATION.md)." + ) in file_only + assert "environment variable" not in file_only.lower() @pytest.mark.parametrize("key", _SECURITY_REMOVED_KEYS)