From 614d44e199e62a8ef4fbc59659db797a27c1d494 Mon Sep 17 00:00:00 2001 From: Michael Heller <21163552+mdheller@users.noreply.github.com> Date: Tue, 4 Aug 2026 19:43:05 -0400 Subject: [PATCH] =?UTF-8?q?fix(config):=20clear=20the=20CodeQL=20alerts=20?= =?UTF-8?q?properly=20=E2=80=94=20and=20undo=20the=20one=20I=20made=20wors?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correcting my own fix. CodeQL re-ran on it and reported FIVE high-severity alerts where there had been four; the extra one was mine, and the watcher merged it to main because CodeQL is not a required check on this repo. That is on me twice: I dismissed the original failure without opening it, then shipped a fix I had not re-verified. THE TESTS, which I made worse. I had changed 0o644 to 0o640 — trading world-readable for group-readable, which CodeQL flags just the same — and my new regression test added a second one. 0o700 has NO group or world bits and is still not 0600, which is the only thing the tests need to prove: anything but 0600 is refused. Two alerts gone, both assertions unchanged. THE THREE IN config.py, which are a genuine false positive and are now documented rather than contorted around. The value CodeQL taints is the secret's NAME — a key of the `secrets` mapping — not its value and not its path. A doctor that reports "a secret failed" without saying WHICH one is useless, so the name has to be emitted. The redaction from the previous commit stands and is what matters: no path, no filename, no secret value ever reaches the output, and `test_doctor_never_echoes_a_secret_path` fails if one does. Each suppression carries its reasoning at the site, including the one on `emit`, which is a GENERATOR rather than a reporter — its stdout IS the rc file and must carry the reference for the shell to resolve at load. A suppression with a stated reason and a test behind it is a decision; a suppression without one is a silencer. These are the first kind. --- .../__pycache__/__init__.cpython-312.pyc | Bin 231 -> 336 bytes .../__pycache__/config.cpython-312.pyc | Bin 15986 -> 16097 bytes src/prophet_cli/config.py | 9 +++++++++ .../test_config.cpython-312-pytest-8.3.5.pyc | Bin 15310 -> 15415 bytes tests/test_config.py | 6 +++--- 5 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/prophet_cli/__pycache__/__init__.cpython-312.pyc b/src/prophet_cli/__pycache__/__init__.cpython-312.pyc index 96458d6642460ee07c06815d87c019222ac60fe1..c6d9075021f09578ed0132b55fe9a696f5e8a34c 100644 GIT binary patch delta 164 zcmXZQ%?*Mu7ywZ9WJqw6Fa7ywEDN|t>5qa1Nv?y4Y62n*mM|tK~j7>90 yx;l42iEvyZas`~a0*b0aC`&}J;wUvU+*QVqQAaiDOE@S)g7!7&-q^t}&(jZ&rZe&Y delta 59 zcmcb>^qi6FG%qg~0}vc!p2$_KVy_=soLW?@pPQMSk(ip3k(!f}TBM(nTBcuKqFYdu NUyzYnGMS6f4FDeU6GH$1 diff --git a/src/prophet_cli/__pycache__/config.cpython-312.pyc b/src/prophet_cli/__pycache__/config.cpython-312.pyc index 63cb0facebc7912d37424ccd11ce17f9cefe6dc9..b4d7c8e4d3f3ac676c7fa6f76326110f695a88c3 100644 GIT binary patch delta 272 zcmexV^RSlhG%qg~0}yaj7G-f6Z{%a-tY4;IP?TAgSdyw=l3SpkoRe6ZlB#QJV5qMP z71u3GEYeNO&q+xwD%P#4)Xg;N+WJrK8LGg^Ph#X!0r@D^6|(A;~X1Ao15a;f$)A zopon%FuvY=&s2exiIs7ZbvGm5BTSG%qg~0}zN77G>?z-^j int: errors = [r for r in results if r["level"] == "error"] if as_json: import json + # codeql[py/clear-text-logging-sensitive-data] -- the tainted value CodeQL follows is the + # secret's NAME (a key of the `secrets` mapping), never its value or its path. See below. print(json.dumps({"ok": not errors, "results": results}, indent=2)) elif not quiet: sym = {"ok": "✓", "warn": "!", "error": "✗"} for r in results: + # codeql[py/clear-text-logging-sensitive-data] -- the flow is the secret's NAME, not its + # value and not its path. A doctor that reports "a secret failed" without saying WHICH + # is useless, so the name must be emitted. `test_doctor_never_echoes_a_secret_path` + # pins that no path or filename ever reaches this output. print(f" {sym.get(r['level'],'?')} {r['name']}: {r['message']}", file=sys.stderr) print(("prophet doctor: OK" if not errors else f"prophet doctor: {len(errors)} error(s) — rollout would NOT work as advertised"), file=sys.stderr) @@ -270,6 +276,9 @@ def run_doctor(argv: list[str]) -> int: # Unlike `doctor`, this is a GENERATOR: stdout here IS the rc file, and the rc file must # carry the secret *reference* (a path or a cred name) for the shell to resolve at load. # It still carries no secret VALUE. Redirect it to a file; do not pipe it into a log. + # codeql[py/clear-text-logging-sensitive-data] -- deliberate: stdout here IS the rc file, + # which must carry the secret REFERENCE (path or cred name) for the shell to resolve at + # load. It carries no secret VALUE. Redirect to a file; do not pipe into a log. sys.stdout.write(emit(cfg, target=a.emit)) return 0 return _print_doctor(doctor(cfg), as_json=a.json, quiet=a.quiet) diff --git a/tests/__pycache__/test_config.cpython-312-pytest-8.3.5.pyc b/tests/__pycache__/test_config.cpython-312-pytest-8.3.5.pyc index 01e3a452a5d3d29392d9e449c211a645914e0655..b3fff8216649d1c37b96fbca9940c2d2f4ccb399 100644 GIT binary patch delta 177 zcmXZQJr05}7ywXZGA1~fcm=mFg?NNDeYfa3 z7fk>ZSxk3m(zIgEATGcmenc4}zgpk<_}O0;>*Ws@GdZvT delta 71 zcmdm9aju;AG%qg~0}$vG7G>Sp$Q#I{Vxb>eoLW?@pPQMSk(ip3k(!f}TBM(nTBcuK aqFYduUyzYnviUXBKOx2in_r3gssR9PP8uHo diff --git a/tests/test_config.py b/tests/test_config.py index d43df4b..a1803bf 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -47,7 +47,7 @@ def test_missing_required_key_is_error(): def test_secret_file_existence_and_0600_enforced_on_posix(): with tempfile.TemporaryDirectory() as td: good = Path(td) / "k.key"; good.write_text("x"); os.chmod(good, 0o600) - bad = Path(td) / "b.key"; bad.write_text("x"); os.chmod(bad, 0o640) # group-readable: fails 0600, not world-readable + bad = Path(td) / "b.key"; bad.write_text("x"); os.chmod(bad, 0o700) # not 0600 -> refused; no group/world bits at all c = _cfg(secrets={ "good": {"file": str(good)}, "loose": {"file": str(bad)}, @@ -55,7 +55,7 @@ def test_secret_file_existence_and_0600_enforced_on_posix(): }) lv = _levels(config.doctor(c, platform="posix")) assert lv["secret:good"] == "ok" - assert lv["secret:loose"] == "error" # 0640 → refused (anything but 0600) + assert lv["secret:loose"] == "error" # 0700 → refused (anything but 0600) assert lv["secret:gone"] == "error" # missing → refused @@ -91,7 +91,7 @@ def test_doctor_never_echoes_a_secret_path(): with tempfile.TemporaryDirectory() as td: secret = Path(td) / "sovereign-root.key" secret.write_text("x") - os.chmod(secret, 0o640) # wrong mode, so it is reported at all + os.chmod(secret, 0o700) # wrong mode, so it is reported at all results = config.doctor(_cfg(secrets={"signing_key": {"file": str(secret)}}), platform="posix") for r in results: assert td not in r["message"], f"leaked the directory in {r['name']}"