Fix settings file safety, export filtering, container permissions, and i18n in install flow by Copilot · Pull Request #50 · dvir001/Simple-Org-Chart

Copilot · 2026-03-10T17:59:29Z

Addresses all review feedback on the config directory / import-export PR. Covers concurrent write safety, information leakage in the export endpoint, upload size enforcement, container hardening, a misleading function name, and hardcoded strings in the install flow.

Backend

Shared lock + atomic write (settings.py, email_config.py): Introduced _settings_file_lock (shared threading.Lock) used by both save_settings() and save_email_config(). Each function now holds the lock for the entire read-modify-write cycle and writes atomically via tempfile.mkstemp + os.replace. Validates the loaded JSON is a dict before calling .update() to avoid crashes on malformed files.
Export filtering (app_main.py): export_settings() now restricts load_settings() output to DEFAULT_SETTINGS keys before merging email config, preventing non-default or transient keys from leaking into the export payload.
Import size enforcement (app_main.py): /api/settings/import now enforces MAX_FILE_SIZE (same limit as logo/favicon uploads) and returns 413 when exceeded.
Rename (user_scanner_service.py): get_latest_pypi_version() → get_latest_release_version() to reflect that GitHub Releases is now the primary source.

Container

Dockerfile: chmod 777 → chmod 775 on data/, config/, repositories/.
deploy/entrypoint.sh: Checks the top-level directory owner via stat before running chown -R, skipping the expensive traversal when ownership already matches.

Frontend

i18n (configure.js, en-US.json): Replaced hardcoded "Downloading…", "Installation failed.", "Download failed: ", and "Not installed — …" strings with resolveTranslation() calls. Added notInstalled and downloadFailed locale keys.
No duplicate listeners (configure.js): Replaced the _initUserScannerConfigUI(true) re-call after a successful install with a targeted status-only DOM refresh, preventing checkBtn/applyBtn from accumulating duplicate addEventListener bindings across installs or reloads.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…permissions Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com>

Copilot

Pull request overview

Updates SimpleOrgChart’s configuration import/export and install flow with stronger safeguards (settings persistence, export redaction, upload size enforcement), plus container hardening and i18n cleanup.

Changes:

Added atomic settings/email-config writes guarded by a shared lock, plus JSON shape validation to avoid crashes on malformed config files.
Filtered exported settings to DEFAULT_SETTINGS keys and enforced a max upload size for settings imports.
Hardened container permissions and reduced startup-time chown overhead; replaced install-flow hardcoded strings with i18n keys and avoided re-init that could duplicate listeners.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`simple_org_chart/settings.py`	Adds shared lock + atomic write for settings persistence; exports lock symbol.
`simple_org_chart/email_config.py`	Uses shared lock + atomic write when updating email config within the shared settings file.
`simple_org_chart/app_main.py`	Filters export payload to default keys; enforces max file size for settings import.
`static/configure.js`	Uses i18n for install status strings; refreshes install status without re-running full init.
`static/locales/en-US.json`	Adds new locale keys for install flow messaging (and reformats JSON).
`simple_org_chart/user_scanner_service.py`	Renames latest-version helper to reflect GitHub Releases usage.
`deploy/entrypoint.sh`	Avoids recursive `chown` when top-level ownership already matches.
`Dockerfile`	Tightens directory permissions from `777` to `775`.

Copilot · 2026-03-10T21:06:33Z

simple_org_chart/settings.py

 __all__ = [
    "DEFAULT_SETTINGS",
+    "_settings_file_lock",
    "department_is_ignored",


__all__ is exporting _settings_file_lock, which makes an underscore-prefixed implementation detail part of the module’s public surface. Since email_config.py imports it directly, this export isn’t needed; consider removing it from __all__ (or renaming it without a leading underscore if it’s intended to be public).

Copilot · 2026-03-10T21:06:34Z

static/configure.js

+                            } catch {
+                                // Ignore refresh errors; installation itself succeeded


After a successful install, if the follow-up /api/user-scanner/status refresh fails (network error, non-JSON response, etc.), the code swallows the exception and leaves installBtn.disabled = true while the install row may still be visible. This can strand the UI in a disabled state even though installation succeeded. Consider re-enabling the button (and/or falling back to _initUserScannerConfigUI(true) / showing a generic success message) when the refresh path fails.

Suggested change

} catch {

// Ignore refresh errors; installation itself succeeded

} catch (refreshErr) {

// Installation itself succeeded, but refreshing status failed; log and re-enable the button

console.error('Failed to refresh user scanner status after install:', refreshErr);

installBtn.disabled = false;

Copilot · 2026-03-10T21:06:34Z

static/configure.js

+                                        const ver = latest.version || 'unknown';
+                                        statusEl.innerHTML = '';
+                                        const versionText = document.createElement('span');
+                                        versionText.textContent = `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.style.fontSize = 'inherit';


The newly added installed-state UI strings ('unknown', Installed — v..., and GitHub repo ↗) are hardcoded. Since this flow is being moved to i18n (resolveTranslation) elsewhere in this hunk, these should also be translated via locale keys (and ideally assembled without string literals) to keep the configure page fully localizable.

Copilot · 2026-03-10T21:06:34Z

simple_org_chart/app_main.py

+    # Enforce file size limit consistent with other upload endpoints
+    uploaded.seek(0, 2)
+    file_size = uploaded.tell()
+    uploaded.seek(0)


The file-size check uses uploaded.seek(0, 2) (magic number) whereas other upload handlers in this file use os.SEEK_END. Using os.SEEK_END here improves readability and consistency.

Copilot · 2026-03-10T21:06:35Z

simple_org_chart/settings.py

+        with _settings_file_lock:
+            # Read the current file so we keep all other keys intact
+            existing: Dict[str, Any] = {}
+            if SETTINGS_FILE.exists():
+                try:
+                    with SETTINGS_FILE.open("r", encoding="utf-8") as handle:
+                        loaded = json.load(handle)
+                        if isinstance(loaded, dict):
+                            existing = loaded
+                except Exception:
+                    pass
+
+            existing.update(persisted)
+
+            # Atomic write: write to a temp file then replace
+            tmp_fd, tmp_path = tempfile.mkstemp(
+                dir=SETTINGS_FILE.parent, suffix=".tmp"
+            )
+            try:
+                with os.fdopen(tmp_fd, "w", encoding="utf-8") as tmp_handle:
+                    json.dump(existing, tmp_handle, indent=2)
+                os.replace(tmp_path, SETTINGS_FILE)


_settings_file_lock is a threading.Lock, so it only serializes writes within a single process. In this repo the app runs under Gunicorn with multiple worker processes, so concurrent settings writes from different workers can still race (lost updates) despite the lock + atomic replace. Consider adding an inter-process lock (e.g., fcntl.flock on a dedicated lockfile next to SETTINGS_FILE, similar to the pattern used elsewhere in app_main.py) around the entire read-modify-write cycle.

Copilot · 2026-03-10T21:06:35Z

simple_org_chart/email_config.py

+        with _settings_file_lock:
+            # Read the current file so we keep all other keys intact
+            existing: Dict[str, Any] = {}
+            if SETTINGS_FILE.exists():
+                try:
+                    with SETTINGS_FILE.open("r", encoding="utf-8") as handle:
+                        loaded = json.load(handle)
+                        if isinstance(loaded, dict):
+                            existing = loaded
+                except Exception:
+                    pass
+
+            # Preserve existing lastSent when caller didn't explicitly supply one
+            if 'lastSent' not in config:
+                persisted['lastSent'] = existing.get('lastSent')
+
+            # Write email keys back into the shared config file
+            existing.update(persisted)
+
+            # Atomic write: write to a temp file then replace
+            tmp_fd, tmp_path = tempfile.mkstemp(
+                dir=SETTINGS_FILE.parent, suffix=".tmp"
+            )
+            try:
+                with os.fdopen(tmp_fd, "w", encoding="utf-8") as tmp_handle:
+                    json.dump(existing, tmp_handle, indent=2)
+                os.replace(tmp_path, SETTINGS_FILE)
+            except Exception:


save_email_config() relies on _settings_file_lock (a threading.Lock) to protect updates to SETTINGS_FILE, but this doesn't prevent concurrent writes across Gunicorn worker processes. Since multiple workers can handle config updates, you can still get lost updates even with the atomic os.replace. Use an inter-process lock (e.g., fcntl.flock on a lockfile) for the whole read-modify-write section, consistent with the existing file-lock approach used elsewhere in app_main.py.

* 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>

Initial plan

5099e85

Copilot AI assigned Copilot and dvir001 Mar 10, 2026

Copilot AI mentioned this pull request Mar 10, 2026

Store settings in config/ and add import/export #49

Merged

Copilot started work on behalf of dvir001 March 10, 2026 17:59 View session

Address all PR review comments: locking, i18n, security, naming, and …

15bbfc2

…permissions Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Store settings in config/ and add import/export~~ Fix settings file safety, export filtering, container permissions, and i18n in install flow Mar 10, 2026

Copilot finished work on behalf of dvir001 March 10, 2026 18:08

dvir001 marked this pull request as ready for review March 10, 2026 20:54

Copilot AI review requested due to automatic review settings March 10, 2026 20:54

Copilot started reviewing on behalf of dvir001 March 10, 2026 20:54 View session

dvir001 merged commit eef83aa into 2026-03-10-ReworkConfig Mar 10, 2026
3 checks passed

Copilot AI reviewed Mar 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix settings file safety, export filtering, container permissions, and i18n in install flow#50

Fix settings file safety, export filtering, container permissions, and i18n in install flow#50
dvir001 merged 2 commits into2026-03-10-ReworkConfigfrom
copilot/sub-pr-49

Copilot AI commented Mar 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		} catch {
		// Ignore refresh errors; installation itself succeeded

-                            } catch {
-                                // Ignore refresh errors; installation itself succeeded
+                            } catch (refreshErr) {
+                                // Installation itself succeeded, but refreshing status failed; log and re-enable the button
+                                console.error('Failed to refresh user scanner status after install:', refreshErr);
+                                installBtn.disabled = false;

Conversation

Copilot AI commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backend

Container

Frontend

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Mar 10, 2026 •

edited

Loading