Compare commits

..
Author SHA1 Message Date
ChuckBuilds 780fca6365 fix(logging): give the journal the real severity of each line
Everything this process writes to stdout reaches the journal as PRIORITY=6,
whatever the Python level was, because journald has nothing else to go on.
Measured on a live rig over 24 hours:

    lines containing " - ERROR - "      55
    lines containing " - WARNING - "    13
    journald PRIORITY recorded          6, for every one of them

So `journalctl -p err -u ledmatrix` returns nothing while errors are being
logged, and `-p warning` likewise. Triage falls back to grepping message text,
which is slower and unreliable: during this audit a search for "oom" matched
the radar logging "zoom=9" twenty-four times and briefly looked like the OOM
killer had been firing.

systemd reads a leading "<N>" on each stdout line and takes it as the priority
(sd-daemon(3)), so a formatter that prefixes one costs no dependency. Every
line of a multi-line record is tagged, not just the first -- the journal splits
them, and an untagged continuation reverts to the default, which would leave
the body of a traceback filed as informational while its first line was an
error.

Applied only when JOURNAL_STREAM is set, which systemd sets for services whose
output it captures. Run from a terminal, in the emulator or under pytest the
prefixes would be literal noise, and the file handler keeps the plain
formatter for the same reason.

Mutation-checked three ways: prefixing unconditionally fails the
outside-systemd test, prefixing only the first line fails the multi-line test,
and mapping ERROR to 6 fails the level mapping. 39 tests pass across the
logging suites.
2026-08-20 02:54:18 -04:00
7 changed files with 166 additions and 146 deletions
Binary file not shown.

Before

Width:  |  Height:  |  Size: 467 B

+54 -1
View File
@@ -130,7 +130,12 @@ def setup_logging(
# Console handler (always add) # Console handler (always add)
console_handler = logging.StreamHandler(sys.stdout) console_handler = logging.StreamHandler(sys.stdout)
console_handler.setLevel(level) console_handler.setLevel(level)
console_handler.setFormatter(formatter) # Under systemd, tag each line so the journal records the real severity
# rather than filing everything as informational. The file handler below
# keeps the plain formatter: the prefix is meaningful to journald and noise
# anywhere else.
console_handler.setFormatter(
JournalPriorityFormatter(formatter) if _under_systemd() else formatter)
root_logger.addHandler(console_handler) root_logger.addHandler(console_handler)
# File handler (if specified) # File handler (if specified)
@@ -145,6 +150,54 @@ def setup_logging(
sys.stderr.write(f"Warning: Could not set up file logging to {log_file}: {e}\n") sys.stderr.write(f"Warning: Could not set up file logging to {log_file}: {e}\n")
#: syslog priorities, which is what systemd parses from a "<N>" prefix on
#: stdout. Mapped from Python's levels.
_SYSLOG_PRIORITY = {
logging.CRITICAL: 2, # LOG_CRIT
logging.ERROR: 3, # LOG_ERR
logging.WARNING: 4, # LOG_WARNING
logging.INFO: 6, # LOG_INFO
logging.DEBUG: 7, # LOG_DEBUG
}
class JournalPriorityFormatter(logging.Formatter):
"""Wraps a formatter, prefixing each line with its syslog priority.
Under systemd everything this process writes to stdout lands in the journal
as PRIORITY=6, whatever the Python level was. Measured on a live rig: 55
ERROR lines and 13 WARNING lines in a day, every one of them recorded as
informational, so `journalctl -p err -u ledmatrix` returned nothing at all
while errors were being logged. Anyone triaging has to grep the message
text instead, which is both slower and wrong -- a search for "oom" matches
the radar logging "zoom=9".
systemd reads a leading "<N>" on each line and uses it as the priority
(sd-daemon(3)), so this needs no extra dependency. Multi-line records get
the prefix on every line, since the journal splits them and an unprefixed
continuation would fall back to the default.
"""
def __init__(self, inner: logging.Formatter):
super().__init__()
self._inner = inner
def format(self, record: logging.LogRecord) -> str:
text = self._inner.format(record)
prefix = f"<{_SYSLOG_PRIORITY.get(record.levelno, 6)}>"
return "\n".join(prefix + line for line in text.split("\n"))
def _under_systemd() -> bool:
"""True when stdout is the journal.
systemd sets JOURNAL_STREAM for services whose output it captures. Without
this check the "<N>" prefixes would show up as literal noise when the
program is run from a terminal, in the emulator, or in tests.
"""
return bool(os.environ.get("JOURNAL_STREAM"))
class PluginLoggerAdapter(logging.LoggerAdapter): class PluginLoggerAdapter(logging.LoggerAdapter):
"""LoggerAdapter that stamps every record with its plugin_id. """LoggerAdapter that stamps every record with its plugin_id.
+1 -34
View File
@@ -143,12 +143,6 @@ def mask_secret_fields(config: Dict[str, Any], schema_properties: Dict[str, Any]
return result return result
#: What a masked secret looks like on the wire. Named because the write path
#: has to recognise it coming back: a client that renders the mask and posts
#: it unchanged must not store the mask as if it were the secret.
SECRET_MASK = '\u2022' * 8
def mask_all_secret_values(config: Dict[str, Any]) -> Dict[str, Any]: def mask_all_secret_values(config: Dict[str, Any]) -> Dict[str, Any]:
"""Blanket-mask every non-empty value in a secrets config dict. """Blanket-mask every non-empty value in a secrets config dict.
@@ -167,7 +161,7 @@ def mask_all_secret_values(config: Dict[str, Any]) -> Dict[str, Any]:
if isinstance(v, dict): if isinstance(v, dict):
masked[k] = mask_all_secret_values(v) masked[k] = mask_all_secret_values(v)
elif v not in (None, '') and not (isinstance(v, str) and v.startswith('YOUR_')): elif v not in (None, '') and not (isinstance(v, str) and v.startswith('YOUR_')):
masked[k] = SECRET_MASK masked[k] = '••••••••'
else: else:
masked[k] = v masked[k] = v
return masked return masked
@@ -195,30 +189,3 @@ def remove_empty_secrets(secrets: Dict[str, Any]) -> Dict[str, Any]:
elif v is not None and not (isinstance(v, str) and v.strip() == ''): elif v is not None and not (isinstance(v, str) and v.strip() == ''):
result[k] = v result[k] = v
return result return result
def strip_masked_values(secrets: Dict[str, Any]) -> Dict[str, Any]:
"""Remove values a client echoed back rather than changed.
The counterpart to :func:`mask_all_secret_values`. A client that GETs the
masked secrets, edits one field and POSTs the whole object back is sending
``SECRET_MASK`` for every field it did not touch. Storing those would
replace each untouched credential with eight bullet characters.
Drops the mask and, like :func:`remove_empty_secrets`, blank values -- so
the caller can merge the result onto what is already stored and have
"unchanged" mean unchanged. Empty nested dicts are pruned.
"""
result: Dict[str, Any] = {}
for k, v in secrets.items():
if isinstance(v, dict):
nested = strip_masked_values(v)
if nested:
result[k] = nested
elif v is None:
continue
elif isinstance(v, str) and (v.strip() == '' or v == SECRET_MASK):
continue
else:
result[k] = v
return result
+101
View File
@@ -0,0 +1,101 @@
"""Log lines must reach the journal with their real severity.
Everything this process writes to stdout lands in the journal as PRIORITY=6,
whatever the Python level was, because journald has no other signal. Measured
on a live rig over 24 hours: 55 lines containing " - ERROR - " and 13
containing " - WARNING - ", every one of them recorded as informational. So
journalctl -p err -u ledmatrix
returned nothing while errors were being logged, and anyone triaging has to
grep the message text instead. That is slower and it is wrong: a search for
"oom" also matches the radar logging "zoom=9", which is exactly the false
positive it produced during this audit.
systemd reads a leading "<N>" on each stdout line and uses it as the priority
(sd-daemon(3)), so this needs no extra dependency -- and it must only be
applied when systemd is actually reading, or the prefixes become literal noise
in a terminal, the emulator, and test output.
"""
import logging
import os
from unittest.mock import patch
import pytest
from src.logging_config import JournalPriorityFormatter, _SYSLOG_PRIORITY, _under_systemd
class _Plain(logging.Formatter):
def format(self, record):
return record.getMessage()
def _record(level, msg="hello"):
return logging.LogRecord("t", level, "f.py", 1, msg, None, None)
@pytest.mark.parametrize("level,expected", [
(logging.CRITICAL, 2),
(logging.ERROR, 3),
(logging.WARNING, 4),
(logging.INFO, 6),
(logging.DEBUG, 7),
])
def test_each_level_maps_to_its_syslog_priority(level, expected):
out = JournalPriorityFormatter(_Plain()).format(_record(level))
assert out.startswith(f"<{expected}>"), out
assert _SYSLOG_PRIORITY[level] == expected
def test_error_and_info_are_distinguishable():
"""The whole point: journalctl -p err must be able to tell them apart."""
fmt = JournalPriorityFormatter(_Plain())
assert fmt.format(_record(logging.ERROR))[:3] != fmt.format(_record(logging.INFO))[:3]
def test_every_line_of_a_multiline_record_is_tagged():
"""The journal splits them, and an untagged continuation loses its level.
A traceback is the case that matters -- it is the most important thing in
the log and the longest.
"""
out = JournalPriorityFormatter(_Plain()).format(
_record(logging.ERROR, "Traceback:\nline one\nline two"))
lines = out.split("\n")
assert len(lines) == 3
assert all(line.startswith("<3>") for line in lines), lines
def test_the_message_survives_intact():
out = JournalPriorityFormatter(_Plain()).format(_record(logging.WARNING, "disk full"))
assert out == "<4>disk full"
def test_an_unknown_level_falls_back_to_info():
out = JournalPriorityFormatter(_Plain()).format(_record(25))
assert out.startswith("<6>")
def test_prefixing_is_off_outside_systemd():
"""Otherwise a terminal run, the emulator and pytest all show `<6>`."""
with patch.dict(os.environ, {}, clear=True):
assert not _under_systemd()
with patch.dict(os.environ, {"JOURNAL_STREAM": "8:12345"}):
assert _under_systemd()
def test_setup_uses_the_wrapper_only_under_systemd():
from src.logging_config import setup_logging
for env, expect_wrapped in (({}, False), ({"JOURNAL_STREAM": "8:1"}, True)):
with patch.dict(os.environ, env, clear=True):
setup_logging()
handlers = [h for h in logging.getLogger().handlers
if isinstance(h, logging.StreamHandler)]
assert handlers, "no stream handler installed"
wrapped = any(isinstance(h.formatter, JournalPriorityFormatter)
for h in handlers)
assert wrapped is expect_wrapped, (
f"JOURNAL_STREAM={env}: wrapped={wrapped}, expected {expect_wrapped}")
logging.getLogger().handlers.clear()
@@ -1,82 +0,0 @@
"""GET /config/secrets must not hand out credentials, and the client's
read-modify-write cycle must not destroy them.
This interface has no authentication. The endpoint returned the whole
config_secrets.json to anyone who could reach the port; on one rig that was a
40-character GitHub token, a 183-character Home Assistant token and three API
keys. Masking it alone is not enough: the only client fetches every secret,
edits one field and posts all of them back, so the write path has to treat an
echoed mask as "unchanged".
"""
import json
import sys
from pathlib import Path
sys.path.insert(0, str(Path(__file__).parent))
from test_api_v3_secret_roundtrip import env, _on_disk # noqa: F401,E402
from src.web_interface.secret_helpers import SECRET_MASK # noqa: E402
STORED = {
"github": {"api_token": "ghp_" + "x" * 36},
"ledmatrix-weather": {"api_key": "w" * 32},
"incoming-packages": {"ha_token": "h" * 183},
"unset-plugin": {"api_key": ""},
"placeholder-plugin": {"api_key": "YOUR_API_KEY_HERE"},
}
def _seed(env):
env.secrets_file.write_text(json.dumps(STORED))
def _get(env):
r = env.client.get("/api/v3/config/secrets")
assert r.status_code == 200, r.get_data(as_text=True)[:200]
return r.get_json()["data"]
def test_no_credential_leaves_the_process(env):
_seed(env)
body = json.dumps(_get(env))
for secret in ("ghp_" + "x" * 36, "w" * 32, "h" * 183):
assert secret not in body, "endpoint returned a stored credential"
def test_set_and_unset_remain_distinguishable(env):
_seed(env)
data = _get(env)
assert data["github"]["api_token"] == SECRET_MASK
assert data["unset-plugin"]["api_key"] == ""
assert data["placeholder-plugin"]["api_key"] == "YOUR_API_KEY_HERE"
def test_the_clients_read_modify_write_preserves_every_other_secret(env):
"""What the GitHub-token save button actually does."""
_seed(env)
secrets = _get(env) # everything arrives masked
secrets["github"]["api_token"] = "ghp_" + "n" * 36 # user changes one
r = env.client.post("/api/v3/config/raw/secrets", json=secrets)
assert r.status_code == 200, r.get_data(as_text=True)[:200]
on_disk = _on_disk(env.secrets_file)
assert on_disk["github"]["api_token"] == "ghp_" + "n" * 36, "new token not saved"
assert on_disk["ledmatrix-weather"]["api_key"] == "w" * 32
assert on_disk["incoming-packages"]["ha_token"] == "h" * 183
def test_a_mask_echoed_back_is_never_stored(env):
_seed(env)
env.client.post("/api/v3/config/raw/secrets", json=_get(env))
on_disk = _on_disk(env.secrets_file)
assert SECRET_MASK not in json.dumps(on_disk), "the mask was stored as a secret"
assert on_disk["github"]["api_token"] == "ghp_" + "x" * 36
def test_a_brand_new_secret_can_still_be_added(env):
_seed(env)
env.client.post("/api/v3/config/raw/secrets",
json={"new-plugin": {"api_key": "brand-new"}})
on_disk = _on_disk(env.secrets_file)
assert on_disk["new-plugin"]["api_key"] == "brand-new"
assert on_disk["github"]["api_token"] == "ghp_" + "x" * 36
+4 -21
View File
@@ -21,8 +21,7 @@ logger = logging.getLogger(__name__)
# Import new infrastructure # Import new infrastructure
from src.web_interface.api_helpers import success_response, error_response, validate_request_json from src.web_interface.api_helpers import success_response, error_response, validate_request_json
from src.web_interface.errors import ErrorCode from src.web_interface.errors import ErrorCode
from src.web_interface.secret_helpers import (find_secret_fields, mask_all_secret_values, from src.web_interface.secret_helpers import find_secret_fields, separate_secrets
separate_secrets, strip_masked_values)
from src.web_interface.error_handler import describe_exception, redact_text from src.web_interface.error_handler import describe_exception, redact_text
from src.plugin_system.operation_types import OperationType from src.plugin_system.operation_types import OperationType
from src.web_interface.validators import ( from src.web_interface.validators import (
@@ -1334,12 +1333,7 @@ def get_secrets_config():
return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500 return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500
config = api_v3.config_manager.get_raw_file_content('secrets') config = api_v3.config_manager.get_raw_file_content('secrets')
# This interface has no authentication, and this file is nothing but return jsonify({'status': 'success', 'data': config})
# credentials. It was handing all of them to anyone who could reach
# the port. Values are masked; empty and YOUR_* placeholders are left
# alone so a client can still tell "set" from "not set".
return jsonify({'status': 'success',
'data': mask_all_secret_values(config)})
except Exception as e: except Exception as e:
logger.error('Unhandled exception', exc_info=True) logger.error('Unhandled exception', exc_info=True)
return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details', 'details': describe_exception(e)}), 500 return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details', 'details': describe_exception(e)}), 500
@@ -1401,19 +1395,8 @@ def save_raw_secrets_config():
if not data: if not data:
return jsonify({'status': 'error', 'message': 'No data provided'}), 400 return jsonify({'status': 'error', 'message': 'No data provided'}), 400
# The GET above masks what it returns, and this endpoint's only client # Save the secrets config
# reads the whole file, edits one field and posts all of it back. So api_v3.config_manager.save_raw_file_content('secrets', data)
# most of what arrives here is the mask, echoed rather than changed --
# storing it verbatim would replace every untouched credential with
# eight bullets. Strip those, then merge onto what is already stored,
# which makes "unchanged" mean unchanged.
#
# The cost is that a secret can no longer be cleared by blanking it.
# That needs its own affordance; a control that erases credentials as
# a side effect of saving an unrelated one is not it.
current = api_v3.config_manager.get_raw_file_content('secrets') or {}
merged = deep_merge(current, strip_masked_values(data))
api_v3.config_manager.save_raw_file_content('secrets', merged)
# Reload GitHub token in plugin store manager if it exists # Reload GitHub token in plugin store manager if it exists
if api_v3.plugin_store_manager: if api_v3.plugin_store_manager:
+6 -8
View File
@@ -4622,17 +4622,15 @@ window.loadGithubToken = function() {
// Handle empty data (secrets file doesn't exist) - API returns {} in this case // Handle empty data (secrets file doesn't exist) - API returns {} in this case
const secrets = data.data || {}; const secrets = data.data || {};
const token = secrets.github?.api_token || ''; const token = secrets.github?.api_token || '';
const configured = token && token !== 'YOUR_GITHUB_PERSONAL_ACCESS_TOKEN';
if (input) { if (input) {
// The endpoint masks what it returns, so this never holds if (token && token !== 'YOUR_GITHUB_PERSONAL_ACCESS_TOKEN') {
// the real token -- and the field is deliberately left // Token exists and is valid
// empty rather than filled with the mask, which would be input.value = token;
// saved verbatim the next time the user pressed Save. showNotification('GitHub token loaded successfully', 'success');
input.value = '';
if (configured) {
showNotification('A GitHub token is saved. Enter a new one to replace it.', 'success');
} else { } else {
// No token configured or placeholder value
input.value = '';
showNotification('No GitHub token configured. Enter a new token to save.', 'info'); showNotification('No GitHub token configured. Enter a new token to save.', 'info');
} }
} }