mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 06:15:09 +00:00
fix(config): load config_secrets.json on hosts without os.geteuid (#590)
ensure_shared_group_ownership() - the chgrp self-heal ConfigManager runs before reading config_secrets.json (#416) - looked up os.geteuid unguarded. That name does not exist on Windows, and the AttributeError is not an OSError, so it escaped the helper's best-effort handling and every except clause in load_config(). Any Windows checkout with a config/config_secrets.json got a ConfigError from every config load and could not import web_interface.app. That is what made test_update_all_plugins.py error at setup: its client fixture imports web_interface.app. It was not state leaked between test files - the trigger is whether the checkout has a secrets file. Return early when os.geteuid or os.chown is missing. No change on POSIX. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -333,3 +333,27 @@ class TestArraySecretStripAndMerge:
|
||||
|
||||
manager._deep_merge(stripped, secrets)
|
||||
assert stripped == full # round trip restores the items
|
||||
|
||||
|
||||
class TestLoadWithoutPosixOwnershipApis:
|
||||
"""A secrets file must load on platforms that have no uid/gid at all.
|
||||
|
||||
load_config() chgrp's the secrets file to the shared group before reading
|
||||
it, to self-heal a root-written file the non-root web user can't read.
|
||||
That helper is documented as best-effort, but it looked up os.geteuid
|
||||
unguarded -- absent on Windows -- and the resulting AttributeError is not
|
||||
an OSError, so it escaped every except clause on the way out. The symptom
|
||||
was a ConfigError from load_config on any Windows checkout carrying a
|
||||
config/config_secrets.json, which took `import web_interface.app` with it.
|
||||
"""
|
||||
|
||||
def test_secrets_still_merge_without_geteuid(self, tmp_path, monkeypatch):
|
||||
monkeypatch.delattr(os, "geteuid", raising=False)
|
||||
monkeypatch.delattr(os, "chown", raising=False)
|
||||
manager = make_manager(
|
||||
tmp_path,
|
||||
config={"weather": {"city": "Austin"}},
|
||||
secrets={"weather": {"api_key": "s3cret"}},
|
||||
)
|
||||
|
||||
assert manager.load_config()["weather"] == {"city": "Austin", "api_key": "s3cret"}
|
||||
|
||||
@@ -1,16 +1,25 @@
|
||||
"""
|
||||
Tests for src.common.permission_utils's URL-credential redaction.
|
||||
Tests for src.common.permission_utils.
|
||||
|
||||
Covers the fix for a CodeQL clear-text-logging-of-secrets alert:
|
||||
install_requirements_file() must never let a private index URL's embedded
|
||||
user:pass@ credentials reach logs or its returned CompletedProcess, since
|
||||
pip can echo that URL back verbatim in its own stderr/stdout on failure.
|
||||
Covers two things:
|
||||
|
||||
* URL-credential redaction -- the fix for a CodeQL clear-text-logging-of-secrets
|
||||
alert: install_requirements_file() must never let a private index URL's
|
||||
embedded user:pass@ credentials reach logs or its returned CompletedProcess,
|
||||
since pip can echo that URL back verbatim in its own stderr/stdout on failure.
|
||||
* ensure_shared_group_ownership() staying a silent no-op on platforms without
|
||||
the POSIX ownership APIs.
|
||||
"""
|
||||
|
||||
import os
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from src.common.permission_utils import _redact_url_credentials, install_requirements_file
|
||||
from src.common.permission_utils import (
|
||||
_redact_url_credentials,
|
||||
ensure_shared_group_ownership,
|
||||
install_requirements_file,
|
||||
)
|
||||
|
||||
|
||||
class TestRedactUrlCredentials:
|
||||
@@ -81,3 +90,47 @@ class TestInstallRequirementsFileRedaction:
|
||||
assert "swordfish" not in result.stderr
|
||||
assert "https://***:***@pypi.internal" in result.stdout
|
||||
assert "https://***:***@pypi.internal" in result.stderr
|
||||
|
||||
|
||||
class TestEnsureSharedGroupOwnership:
|
||||
"""The chgrp self-heal must stay a no-op wherever it cannot apply.
|
||||
|
||||
``ConfigManager.load_config()`` calls this on every load that finds a
|
||||
secrets file, and its callers only ever catch ``OSError``. An
|
||||
``AttributeError`` from looking up a POSIX-only name on Windows therefore
|
||||
escaped all the way out of ``load_config``, and took the import of
|
||||
``web_interface.app`` with it on any Windows checkout that had a
|
||||
``config/config_secrets.json``.
|
||||
"""
|
||||
|
||||
def test_no_geteuid_is_a_silent_no_op(self, monkeypatch, tmp_path):
|
||||
secrets = tmp_path / "config_secrets.json"
|
||||
secrets.write_text("{}", encoding='utf-8')
|
||||
monkeypatch.delattr(os, "geteuid", raising=False)
|
||||
monkeypatch.delattr(os, "chown", raising=False)
|
||||
|
||||
ensure_shared_group_ownership(secrets) # must not raise
|
||||
|
||||
def test_non_root_never_chowns(self, monkeypatch, tmp_path):
|
||||
chown = MagicMock()
|
||||
monkeypatch.setattr(os, "geteuid", lambda: 1000, raising=False)
|
||||
monkeypatch.setattr(os, "chown", chown, raising=False)
|
||||
|
||||
ensure_shared_group_ownership(tmp_path / "config_secrets.json")
|
||||
|
||||
chown.assert_not_called()
|
||||
|
||||
def test_root_chowns_a_file_whose_group_is_wrong(self, monkeypatch, tmp_path):
|
||||
secrets = tmp_path / "config_secrets.json"
|
||||
secrets.write_text("{}", encoding='utf-8')
|
||||
# Any gid the file does not already have, so the chgrp is due.
|
||||
wanted = secrets.stat().st_gid + 1
|
||||
chown = MagicMock()
|
||||
monkeypatch.setattr(os, "geteuid", lambda: 0, raising=False)
|
||||
monkeypatch.setattr(os, "chown", chown, raising=False)
|
||||
monkeypatch.setattr('src.common.permission_utils.get_shared_group_gid',
|
||||
lambda: wanted)
|
||||
|
||||
ensure_shared_group_ownership(secrets)
|
||||
|
||||
chown.assert_called_once_with(secrets, -1, wanted)
|
||||
|
||||
Reference in New Issue
Block a user