Fix lock path resolution, i18n gaps, and entrypoint symlink safety#52
Conversation
…link safety Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a settings lock-file path bug that broke test monkeypatching, fills i18n gaps in the user-scanner UI, and hardens the container entrypoint against symlink-based privilege escalation.
Changes:
- Resolve the settings lock-file path at lock-acquire time (instead of import time) to respect monkeypatched
SETTINGS_FILE. - Update email schedule tests to patch both
email_config.SETTINGS_FILEandsettings.SETTINGS_FILE. - Replace hardcoded user-scanner status/update strings with i18n lookups and add new locale keys; harden entrypoint
chownto not dereference symlinks.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
simple_org_chart/settings.py |
Makes the inter-process lock accept a path factory so lock paths follow SETTINGS_FILE rebinding. |
tests/test_email_schedule.py |
Aligns test monkeypatching so the lock and settings file live together under tmp_path. |
static/configure.js |
Migrates user-scanner status/update UI strings to i18n lookups. |
static/locales/en-US.json |
Adds new translation keys for the user-scanner status/update flow. |
deploy/entrypoint.sh |
Prevents recursive chown from following symlinks (root context). |
| 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', ')'); |
There was a problem hiding this comment.
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).
| "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: " |
There was a problem hiding this comment.
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.
| "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}" |
* Store settings in config/ and add import/export Move application settings into a dedicated config/ directory and unify email settings into the single app_settings.json file. Update simple_org_chart.config to expose CONFIG_DIR/REPO_DIR and ensure directories are created. Modify email_config and settings save/load logic to read/write the unified SETTINGS_FILE while preserving unrelated keys (so email/settings don't overwrite each other). Add API endpoints (/api/settings/export and /api/settings/import) and corresponding UI (configure page, JS, and locale entries) to export/import configuration. Change user-scanner behavior to use GitHub releases (with PyPI fallback) and add UI-driven manual install flow instead of auto-install. Update Dockerfiles and compose files to mount ./config, add gosu and an entrypoint script to fix bind-mount permissions before dropping privileges. Update tests to expect the new config location and adjust other frontend and backend code to handle missing DOM elements and defaults. Overall this enables persistent, portable settings and safer container bind-mount handling. * Fix settings file safety, export filtering, container permissions, and i18n in install flow (#50) * Initial plan * Address all PR review comments: locking, i18n, security, naming, and permissions Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> * Harden settings file concurrency, email key isolation, and import guard (#51) * Initial plan * Fix inter-process locking, email key restriction, dict check, and conditional settings save Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> * Remove committed lock file and add config/*.lock to .gitignore Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> * Fix fd leak in lock acquire, remove locked() race, and extract _filter_email_keys helper Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> * Fix lock path resolution, i18n gaps, and entrypoint symlink safety (#52) * Initial plan * Fix lock path at acquire-time, i18n hardcoded strings, entrypoint symlink safety Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> * Update simple_org_chart/app_main.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update simple_org_chart/settings.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
_settings_file_lockcomputed its lock-file path at import time, causing test monkeypatches ofSETTINGS_FILEto be silently ignored (lock pointed at the repo'sconfig/instead oftmp_path). Several user-scanner UI strings were also hardcoded in English, and the entrypoint's recursivechowncould follow symlinks placed by the unprivilegedappuser.Lock path resolved at acquire-time (
settings.py)_InterProcessSettingsFileLocknow accepts a callable factory evaluated on eachacquire()call. The module-level lock uses_settings_lock_file_factory()which readsSETTINGS_FILEvia normalLOAD_GLOBAL, so any monkeypatch ofsimple_org_chart.settings.SETTINGS_FILEtakes effect:Test fixture alignment (
tests/test_email_schedule.py)TestSavePreservesLastSenttests now patch bothemail_config.SETTINGS_FILEandsettings.SETTINGS_FILEto the sametmp_pathfile, co-locating the lock file with the file under test.i18n for user-scanner UI (
static/configure.js+en-US.json)All hardcoded strings in the user-scanner status/update flow replaced with
resolveTranslation()calls. New keys added underconfigure.userScanner.status.*andconfigure.userScanner.update.*:installedPrefix,repoLink,unknownVersionchecking,availablePrefix,availableArrow,upToDatePrefix/Suffix,unknownVersion,checkFailed,updating,updatedPrefix,failed,failedPrefixEntrypoint symlink hardening (
deploy/entrypoint.sh)Prevents the entrypoint (running as root) from following symlinks an attacker-controlled
appuser could plant in bind-mounted directories before the privilege drop.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.