Skip to content

Fix lock path resolution, i18n gaps, and entrypoint symlink safety #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dvir001 merged 2 commits into from
Mar 11, 2026
Merged

Fix lock path resolution, i18n gaps, and entrypoint symlink safety #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21eab60
Initial plan
Copilot Mar 11, 2026
bf6a33e
Fix lock path at acquire-time, i18n hardcoded strings, entrypoint sym…
Copilot Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion deploy/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ for dir in /app/data /app/config /app/repositories; do
if [ -d "$dir" ]; then
current_owner=$(stat -c '%u:%g' "$dir" 2>/dev/null || echo "")
if [ -z "$current_owner" ] || [ "$current_owner" != "$APP_UID:$APP_GID" ]; then
chown -R app:app "$dir" 2>/dev/null || true
chown -R --no-dereference app:app "$dir" 2>/dev/null || true
fi
fi
done
Expand Down
42 changes: 33 additions & 9 deletions simple_org_chart/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import re
import tempfile
import threading
from typing import Any, Dict, Iterable, Set
from typing import Any, Callable, Dict, Iterable, Set, Union

from .config import SETTINGS_FILE

Expand All @@ -24,8 +24,21 @@ class _InterProcessSettingsFileLock:
falls back gracefully to thread-only locking.
"""

def __init__(self, lock_file_path: str) -> None:
self._lock_file_path = lock_file_path
def __init__(
self,
lock_file_path: Union[str, bytes, "os.PathLike[str]", Callable[[], Union[str, bytes, "os.PathLike[str]"]]],
) -> None:
# Accept either a static path (str/bytes/Path) or a zero-argument
# callable that returns the path at acquire-time. The callable form
# lets callers that re-bind the module-level SETTINGS_FILE (e.g. in
# tests via monkeypatch) have the lock always protect the file that is
# actually being accessed rather than the path that was current at
# import time.
if callable(lock_file_path):
self._get_lock_file_path = lock_file_path
else:
_static = lock_file_path
self._get_lock_file_path = lambda: _static
self._thread_lock = threading.Lock()
self._fd: int | None = None

Expand All @@ -36,7 +49,8 @@ def acquire(self, blocking: bool = True) -> bool:
fd = None
try:
import fcntl
fd = os.open(self._lock_file_path, os.O_CREAT | os.O_RDWR, 0o600)
lock_file_path = self._get_lock_file_path()
fd = os.open(lock_file_path, os.O_CREAT | os.O_RDWR, 0o600)
flags = fcntl.LOCK_EX | (0 if blocking else fcntl.LOCK_NB)
fcntl.flock(fd, flags)
self._fd = fd
Expand Down Expand Up @@ -85,11 +99,21 @@ def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None:
# Both settings.py and email_config.py import this lock so that concurrent
# writes from different code paths (scheduler, HTTP handlers, workers) are
# serialised across all Gunicorn worker processes.
_settings_lock_file = os.path.join(
os.fspath(SETTINGS_FILE.parent),
f"{SETTINGS_FILE.name}.lock",
)
_settings_file_lock = _InterProcessSettingsFileLock(_settings_lock_file)
#
# The factory callable reads SETTINGS_FILE from this module's namespace at
# acquire-time rather than computing the path once at import time. This
# allows test-time monkeypatching of ``simple_org_chart.settings.SETTINGS_FILE``
# to take effect so the lock file is always co-located with the settings file
# actually being accessed.
def _settings_lock_file_factory() -> str:
# Accessing the module-level name SETTINGS_FILE here (rather than via
# globals()) is sufficient: Python resolves global names at call-time via
# LOAD_GLOBAL, so monkeypatching ``simple_org_chart.settings.SETTINGS_FILE``
# in tests will be reflected when this factory is invoked.
return os.path.join(os.fspath(SETTINGS_FILE.parent), f"{SETTINGS_FILE.name}.lock")


_settings_file_lock = _InterProcessSettingsFileLock(_settings_lock_file_factory)

logger = logging.getLogger(__name__)

Expand Down
31 changes: 16 additions & 15 deletions static/configure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2363,18 +2363,18 @@ async function _initUserScannerConfigUI(isEnabled) {
const latest = await statusResp.json();
if (latest.installed) {
if (installRow) installRow.style.display = 'none';
const ver = latest.version || 'unknown';
const ver = latest.version || resolveTranslation('configure.userScanner.status.unknownVersion', 'unknown');
statusEl.innerHTML = '';
const versionText = document.createElement('span');
versionText.textContent = `Installed — v${ver}`;
versionText.textContent = resolveTranslation('configure.userScanner.status.installedPrefix', 'Installed — v') + ver;
statusEl.appendChild(versionText);
const sep = document.createTextNode(' · ');
statusEl.appendChild(sep);
const repoLink = document.createElement('a');
repoLink.href = 'https://github.com/kaifcodec/user-scanner';
repoLink.target = '_blank';
repoLink.rel = 'noopener noreferrer';
repoLink.textContent = 'GitHub repo ↗';
repoLink.textContent = resolveTranslation('configure.userScanner.status.repoLink', 'GitHub repo ↗');
repoLink.style.fontSize = 'inherit';
statusEl.appendChild(repoLink);
if (updateRow) updateRow.style.display = 'flex';
Expand All @@ -2395,18 +2395,18 @@ async function _initUserScannerConfigUI(isEnabled) {
}
} else {
if (installRow) installRow.style.display = 'none';
const ver = data.version || 'unknown';
const ver = data.version || resolveTranslation('configure.userScanner.status.unknownVersion', 'unknown');
statusEl.innerHTML = '';
const versionText = document.createElement('span');
versionText.textContent = `Installed — v${ver}`;
versionText.textContent = resolveTranslation('configure.userScanner.status.installedPrefix', 'Installed — v') + ver;
statusEl.appendChild(versionText);
const sep = document.createTextNode(' · ');
statusEl.appendChild(sep);
const repoLink = document.createElement('a');
repoLink.href = 'https://github.com/kaifcodec/user-scanner';
repoLink.target = '_blank';
repoLink.rel = 'noopener noreferrer';
repoLink.textContent = 'GitHub repo ↗';
repoLink.textContent = resolveTranslation('configure.userScanner.status.repoLink', 'GitHub repo ↗');
repoLink.style.fontSize = 'inherit';
statusEl.appendChild(repoLink);
if (updateRow) updateRow.style.display = 'flex';
Expand All @@ -2417,19 +2417,20 @@ async function _initUserScannerConfigUI(isEnabled) {
if (checkBtn) {
checkBtn.addEventListener('click', async () => {
checkBtn.disabled = true;
if (updateStatusEl) updateStatusEl.textContent = 'Checking…';
if (updateStatusEl) updateStatusEl.textContent = resolveTranslation('configure.userScanner.update.checking', 'Checking…');
if (applyBtn) applyBtn.style.display = 'none';
try {
const resp = await fetch(`${window.location.origin}/api/user-scanner/check-update`, { credentials: 'include' });
const info = await resp.json();
if (info.updateAvailable) {
if (updateStatusEl) updateStatusEl.textContent = `Update available: v${info.currentVersion} → v${info.latestVersion}`;
if (updateStatusEl) updateStatusEl.textContent = resolveTranslation('configure.userScanner.update.availablePrefix', 'Update available: v') + info.currentVersion + resolveTranslation('configure.userScanner.update.availableArrow', ' → v') + info.latestVersion;
if (applyBtn) applyBtn.style.display = '';
} else {
if (updateStatusEl) updateStatusEl.textContent = `Up to date (v${info.currentVersion || info.latestVersion || 'unknown'})`;
const ver = info.currentVersion || info.latestVersion || resolveTranslation('configure.userScanner.update.unknownVersion', 'unknown');
if (updateStatusEl) updateStatusEl.textContent = resolveTranslation('configure.userScanner.update.upToDatePrefix', 'Up to date (v') + ver + resolveTranslation('configure.userScanner.update.upToDateSuffix', ')');
Comment on lines +2426 to +2430
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update status text is assembled by concatenating multiple translation keys (prefix/arrow/suffix) plus version values. This makes localization awkward for languages that need different word order or punctuation. Consider switching to a single i18n key with placeholders (e.g., "Update available: v{current} → v{latest}" / "Up to date (v{version})") and update getTranslation/resolveTranslation to pass params through to window.i18n.t(key, params) (supported by static/i18n.js).

Copilot uses AI. Check for mistakes.
}
} catch (err) {
if (updateStatusEl) updateStatusEl.textContent = 'Failed to check for updates.';
if (updateStatusEl) updateStatusEl.textContent = resolveTranslation('configure.userScanner.update.checkFailed', 'Failed to check for updates.');
} finally {
checkBtn.disabled = false;
}
Expand All @@ -2440,25 +2441,25 @@ async function _initUserScannerConfigUI(isEnabled) {
if (applyBtn) {
applyBtn.addEventListener('click', async () => {
applyBtn.disabled = true;
if (updateStatusEl) updateStatusEl.textContent = 'Updating…';
if (updateStatusEl) updateStatusEl.textContent = resolveTranslation('configure.userScanner.update.updating', 'Updating…');
try {
const resp = await fetch(`${window.location.origin}/api/user-scanner/update`, {
method: 'POST',
credentials: 'include',
});
const result = await resp.json();
if (result.success) {
if (updateStatusEl) updateStatusEl.textContent = `Updated to v${result.version}`;
if (updateStatusEl) updateStatusEl.textContent = resolveTranslation('configure.userScanner.update.updatedPrefix', 'Updated to v') + result.version;
if (statusEl) {
const verSpan = statusEl.querySelector('span');
if (verSpan) verSpan.textContent = `Installed — v${result.version}`;
if (verSpan) verSpan.textContent = resolveTranslation('configure.userScanner.status.installedPrefix', 'Installed — v') + result.version;
}
applyBtn.style.display = 'none';
} else {
if (updateStatusEl) updateStatusEl.textContent = result.error || 'Update failed.';
if (updateStatusEl) updateStatusEl.textContent = result.error || resolveTranslation('configure.userScanner.update.failed', 'Update failed.');
}
} catch (err) {
if (updateStatusEl) updateStatusEl.textContent = 'Update failed: ' + err.message;
if (updateStatusEl) updateStatusEl.textContent = resolveTranslation('configure.userScanner.update.failedPrefix', 'Update failed: ') + err.message;
} finally {
applyBtn.disabled = false;
}
Expand Down
18 changes: 18 additions & 0 deletions static/locales/en-US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,24 @@
"failed": "Installation failed.",
"notInstalled": "Not installed — download to enable the scanner.",
"downloadFailed": "Download failed: "
},
"status": {
"installedPrefix": "Installed — v",
"repoLink": "GitHub repo ↗",
"unknownVersion": "unknown"
},
"update": {
"checking": "Checking…",
"availablePrefix": "Update available: v",
"availableArrow": " → v",
"upToDatePrefix": "Up to date (v",
"upToDateSuffix": ")",
"unknownVersion": "unknown",
"checkFailed": "Failed to check for updates.",
"updating": "Updating…",
"updatedPrefix": "Updated to v",
"failed": "Update failed.",
"failedPrefix": "Update failed: "
Comment on lines +219 to +234
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new translation keys split single messages into multiple fragments (prefix/arrow/suffix) to support string concatenation in JS. That pattern is hard to localize because translators can’t reorder the placeholders. Prefer a single message per status with {} placeholders (supported by window.i18n.t(key, params) in static/i18n.js) so other locales can translate naturally.

Suggested change
"installedPrefix": "Installed — v",
"repoLink": "GitHub repo ↗",
"unknownVersion": "unknown"
},
"update": {
"checking": "Checking…",
"availablePrefix": "Update available: v",
"availableArrow": " → v",
"upToDatePrefix": "Up to date (v",
"upToDateSuffix": ")",
"unknownVersion": "unknown",
"checkFailed": "Failed to check for updates.",
"updating": "Updating…",
"updatedPrefix": "Updated to v",
"failed": "Update failed.",
"failedPrefix": "Update failed: "
"installed": "Installed — v{version}",
"repoLink": "GitHub repo ↗",
"unknownVersion": "unknown"
},
"update": {
"checking": "Checking…",
"available": "Update available: v{currentVersion} → v{latestVersion}",
"upToDate": "Up to date (v{version})",
"unknownVersion": "unknown",
"checkFailed": "Failed to check for updates.",
"updating": "Updating…",
"updated": "Updated to v{version}",
"failed": "Update failed.",
"failedWithReason": "Update failed: {reason}"

Copilot uses AI. Check for mistakes.
}
},
"presence": {
Expand Down
4 changes: 4 additions & 0 deletions tests/test_email_schedule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
should_send_email_now,
)
import simple_org_chart.email_config as _email_config_mod
import simple_org_chart.settings as _settings_mod


# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -252,6 +253,7 @@ class TestSavePreservesLastSent:
def test_last_sent_preserved_on_save(self, tmp_path, monkeypatch):
config_file = tmp_path / "app_settings.json"
monkeypatch.setattr(_email_config_mod, "SETTINGS_FILE", config_file)
monkeypatch.setattr(_settings_mod, "SETTINGS_FILE", config_file)

# Simulate a previous email send
initial = DEFAULT_EMAIL_CONFIG.copy()
Expand All @@ -268,6 +270,7 @@ def test_last_sent_preserved_on_save(self, tmp_path, monkeypatch):
def test_last_sent_overwritten_when_explicitly_provided(self, tmp_path, monkeypatch):
config_file = tmp_path / "app_settings.json"
monkeypatch.setattr(_email_config_mod, "SETTINGS_FILE", config_file)
monkeypatch.setattr(_settings_mod, "SETTINGS_FILE", config_file)

initial = DEFAULT_EMAIL_CONFIG.copy()
initial["lastSent"] = "2026-03-01T12:00:00+00:00"
Expand All @@ -282,6 +285,7 @@ def test_last_sent_overwritten_when_explicitly_provided(self, tmp_path, monkeypa
def test_last_sent_none_when_no_prior_config(self, tmp_path, monkeypatch):
config_file = tmp_path / "app_settings.json"
monkeypatch.setattr(_email_config_mod, "SETTINGS_FILE", config_file)
monkeypatch.setattr(_settings_mod, "SETTINGS_FILE", config_file)

save_email_config({"enabled": True, "frequency": "weekly"})

Expand Down
Loading