Harden settings file concurrency, email key isolation, and import guard#51
Conversation
…ditional settings save Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com>
Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com>
…r_email_keys helper Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Hardens concurrent access to the shared app_settings.json by introducing an inter-process file lock, prevents email config writes from overwriting unrelated settings keys, and avoids resetting settings to defaults during “email-only” imports.
Changes:
- Replace in-process-only lock with a combined thread +
flockinter-process lock shared by settings and email config. - Filter persisted email config to known keys and safely handle non-dict JSON during load.
- Skip
save_settings()during import when no valid settings keys are present.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| simple_org_chart/settings.py | Introduces shared inter-process lock implementation and lock-file path derivation. |
| simple_org_chart/email_config.py | Adds helper for email-key isolation and hardens email-config loading. |
| simple_org_chart/app_main.py | Prevents settings reset on email-only imports by conditionally saving settings. |
| .gitignore | Ignores runtime lock files to keep repo clean. |
| import fcntl | ||
| fd = os.open(self._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) |
There was a problem hiding this comment.
os.open(self._lock_file_path, ...) will raise FileNotFoundError if the parent directory doesn’t exist (e.g., first run / before any code path calls SETTINGS_FILE.parent.mkdir(...)). Since acquiring the lock can happen during read paths, this can crash callers. Create the lock file’s parent directory inside acquire() (or during initialization) before calling os.open.
| self._thread_lock.release() | ||
|
|
||
| def __enter__(self) -> "_InterProcessSettingsFileLock": | ||
| self.acquire() |
There was a problem hiding this comment.
__enter__ ignores the return value of acquire(). While with lock: currently calls the blocking path, this makes the class easier to misuse (e.g., if acquire(blocking=False) behavior is later introduced into context-manager usage). Consider raising an exception if acquire() returns False, or changing acquire() to always raise on failure so the context manager can’t enter without owning the lock.
| self.acquire() | |
| acquired = self.acquire() | |
| if not acquired: | |
| raise RuntimeError("Failed to acquire settings file lock") |
| if not isinstance(stored, dict): | ||
| logger.warning("app_settings.json does not contain a JSON object; using email defaults") | ||
| return DEFAULT_EMAIL_CONFIG.copy() |
There was a problem hiding this comment.
The warning hard-codes app_settings.json. If SETTINGS_FILE is ever renamed or configured differently, this message becomes misleading. Prefer including the actual SETTINGS_FILE (path or name) in the log message.
* 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>
app_settings.jsonis shared by both settings and email config writers, but was only protected by an in-processthreading.Lock— insufficient for Gunicorn's multi-worker model. Arbitrary caller keys insave_email_configcould also overwrite unrelated settings, andimport_settingsunconditionally calledsave_settingseven when the import contained only email keys (resetting all settings to defaults).Inter-process lock (
settings.py)Replaced
threading.Lockwith_InterProcessSettingsFileLock— a combinedthreading.Lock+fcntl.flockadvisory lock stored inconfig/app_settings.json.lock. Bothsettings.pyandemail_config.pyshare the same lock instance. Falls back to thread-only locking on Windows.Email key isolation (
email_config.py)_filter_email_keys(source)helper — returns only keys in_EMAIL_KEYS; used in bothload_email_configandsave_email_configto eliminate the duplicate filtering loop and guarantee callers can't bleed arbitrary keys into the shared file.load_email_confignow guards against non-dict JSON with anisinstancecheck and falls back to defaults instead of raising.Conditional settings save (
app_main.py)import_settingsskipssave_settings()when the uploaded JSON contains no recognized settings keys, preventing a silent reset-to-defaults on an email-only import.Housekeeping
Added
config/*.lockto.gitignore.🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.