fix(searxng): enable JSON output format via settings.yml bind mount, not env vars - #1724
Conversation
…not env vars SearXNG only reads SEARXNG_SETTINGS_PATH and SEARXNG_DISABLE_ETC_SETTINGS — there is no generic SEARXNG_* nested override (the __ pattern is Home Assistant, not SearXNG). The previous env-var approach did nothing. Instead, seed a real settings.yml with use_default_settings:true, search.formats:[html,json], and a per-install generated server.secret_key via a new config_files mechanism in the Docker installer. The file is bind-mounted as ./settings.yml:/etc/searxng/settings.yml:ro so it overrides the default settings.yml inside the named config volume. Fixes jaylfc#969
📝 WalkthroughWalkthroughAdds a ChangesConfig File Generation Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Installer as DockerInstaller.install
participant Writer as _write_config_files
participant FS as App directory / .secret_key
participant Compose as _generate_compose
Installer->>Writer: call with app_id, install_config
Writer->>Writer: validate config_files entries (path, content)
Writer->>FS: read existing .secret_key or generate new one
Writer->>Writer: substitute {secret_key} placeholders in content
Writer->>FS: write rendered files (e.g. settings.yml) to app dir
Installer->>Compose: generate docker-compose referencing bind-mounted files
Related issues: Suggested labels: enhancement, security Suggested reviewers: jaylfc 🐰 A secret key hops into place, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| content = entry["content"] | ||
| if "{secret_key}" in content: | ||
| content = content.replace("{secret_key}", secret_key) | ||
| full_path = app_dir / path |
There was a problem hiding this comment.
CRITICAL: config_files[*].path is written verbatim to app_dir / path with no traversal check, so a manifest with path: "../../../../etc/cron.d/evil" (or any absolute path) escapes the per-app directory and writes anywhere the taOS process can write. Manifests are YAML loaded from the app catalog — if any catalog entry (or a future user-supplied override) is ever sourced externally, this becomes arbitrary file write. Resolve and constrain the path:
| full_path = app_dir / path | |
| full_path = (app_dir / path).resolve() | |
| if not full_path.is_relative_to(app_dir.resolve()): | |
| raise ValueError(f"config_files path escapes app dir: {path!r}") |
| app_dir = self.apps_dir / app_id | ||
| secret_key = secrets.token_hex(32) | ||
| for entry in config_files: | ||
| path = entry["path"] |
There was a problem hiding this comment.
WARNING: No validation of the entry shape. A manifest with config_files: [{path: "x.yml"}] (missing content), or a non-dict entry, will raise a bare KeyError / TypeError deep inside the loop with a stack pointing at the installer rather than at the offending manifest entry. Add an explicit shape check before iterating, e.g. assert isinstance(entry, dict) and "path" in entry and "content" in entry.
| if not config_files: | ||
| return | ||
| app_dir = self.apps_dir / app_id | ||
| secret_key = secrets.token_hex(32) |
There was a problem hiding this comment.
WARNING: secrets.token_hex(32) is regenerated on every call to install(), including the upgrade-re-run path the PR description calls out. Every re-install silently rotates SearXNG's server.secret_key, which invalidates any signed cookies / cached sessions the running container has issued. Either persist the generated key somewhere (e.g. a state.json next to the compose file and only generate when missing), or document this rotation as a known consequence.
| image: searxng/searxng:latest | ||
| volumes: | ||
| - config:/etc/searxng | ||
| - ./settings.yml:/etc/searxng/settings.yml:ro |
There was a problem hiding this comment.
SUGGESTION: The named volume config:/etc/searxng is now created/persisted but only its settings.yml is ever bind-mounted over. All other paths under /etc/searxng/ (e.g. limiter.toml, future engine configs) still live in the named volume and won't be overridden. Worth either dropping the named volume entirely, or adding a comment here that settings.yml is intentionally the only entry it now serves, so future manifest edits don't assume the volume covers other paths.
| # {secret_key} must be replaced with a 64-hex-char random string | ||
| assert "{secret_key}" not in content | ||
| assert content.startswith('secret_key: "') | ||
| key_val = content[len('secret_key: "'):-1] # strip prefix and trailing quote |
There was a problem hiding this comment.
SUGGESTION: key_val = content[len('secret_key: "'):-1] assumes the file ends with a closing " and uses a fixed prefix slice. If _write_config_files ever produces content without the trailing quote (e.g. changed substitution format), this parses as a wrong/empty value but the len == 64 assertion may still pass or fail opaquely. Tighten with a regex or split-based parse so the test pinpoints the real failure mode.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Prior review: The 5 findings from the earlier pass (path traversal, entry-shape validation, Fix these issues in Kilo Cloud Reviewed by hy3-20260706:free · Input: 110.1K · Output: 48.8K · Cached: 352.7K |
…, and clean up manifest CRITICAL: Validate config_files[*].path against traversal — reject absolute paths, '..' components, and symlink escapes outside app_dir. Validate entry shape (dict with path+content) with clear ValueError messages instead of bare KeyError/TypeError. Persist secret_key in .secret_key file so re-installs don't silently rotate SearXNG's secret. SUGGESTION: Remove redundant named volume config:/etc/searxng from manifest since settings.yml is bind-mounted. Fix brittle key_val prefix-slice parse in tests — use split() instead. 6 new tests: missing-path, missing-content, absolute-path, dotdot-path, symlink-escape, secret-key-persistence. All 29 installer+catalog tests pass.
|
Addressed 5 Kilo review findings (5ab57de): CRITICAL — path traversal (docker_installer.py:42):
WARNING — entry validation (docker_installer.py:38): Missing WARNING — secret_key rotation (docker_installer.py:36): SUGGESTION — leftover volume (manifest.yaml:19): Removed the named volume SUGGESTION — brittle parse (tests/test_installers.py:118): Replaced 6 new tests covering all validation paths + persistence. 29 installer+catalog tests pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/installers/docker_installer.py`:
- Around line 68-75: The `.secret_key` persistence in `docker_installer.py`
writes a sensitive signing secret with default file permissions, so update the
`secret_key_path.write_text` flow in the installer to create or immediately
chmod the file to 0600 after writing. Keep the existing persistence logic in the
`secret_key_path` / `app_dir.mkdir` block, but ensure the file is only readable
by the app owner and not by other users on the system.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c6f76b3-d24c-4dea-906e-02c56faf46cb
📒 Files selected for processing (3)
app-catalog/services/searxng/manifest.yamltests/test_installers.pytinyagentos/installers/docker_installer.py
| # Persist secret_key per app so re-installs don't rotate it. | ||
| secret_key_path = app_dir / ".secret_key" | ||
| if secret_key_path.exists(): | ||
| secret_key = secret_key_path.read_text().strip() | ||
| else: | ||
| secret_key = secrets.token_hex(32) | ||
| app_dir.mkdir(parents=True, exist_ok=True) | ||
| secret_key_path.write_text(secret_key) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict .secret_key file permissions to 0600.
The secret key file is written with default permissions (typically 0644 on Linux with umask 022), making it readable by other users on multi-user systems. Since app_dir is created with mkdir using default permissions (typically 0755), other users can traverse into it and read the file. The .secret_key is used for SearXNG session signing — an attacker who reads it could forge session cookies.
🔒️ Proposed fix
else:
secret_key = secrets.token_hex(32)
app_dir.mkdir(parents=True, exist_ok=True)
secret_key_path.write_text(secret_key)
+ secret_key_path.chmod(0o600)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Persist secret_key per app so re-installs don't rotate it. | |
| secret_key_path = app_dir / ".secret_key" | |
| if secret_key_path.exists(): | |
| secret_key = secret_key_path.read_text().strip() | |
| else: | |
| secret_key = secrets.token_hex(32) | |
| app_dir.mkdir(parents=True, exist_ok=True) | |
| secret_key_path.write_text(secret_key) | |
| # Persist secret_key per app so re-installs don't rotate it. | |
| secret_key_path = app_dir / ".secret_key" | |
| if secret_key_path.exists(): | |
| secret_key = secret_key_path.read_text().strip() | |
| else: | |
| secret_key = secrets.token_hex(32) | |
| app_dir.mkdir(parents=True, exist_ok=True) | |
| secret_key_path.write_text(secret_key) | |
| secret_key_path.chmod(0o600) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/installers/docker_installer.py` around lines 68 - 75, The
`.secret_key` persistence in `docker_installer.py` writes a sensitive signing
secret with default file permissions, so update the `secret_key_path.write_text`
flow in the installer to create or immediately chmod the file to 0600 after
writing. Keep the existing persistence logic in the `secret_key_path` /
`app_dir.mkdir` block, but ensure the file is only readable by the app owner and
not by other users on the system.
| # Persist secret_key per app so re-installs don't rotate it. | ||
| secret_key_path = app_dir / ".secret_key" | ||
| if secret_key_path.exists(): | ||
| secret_key = secret_key_path.read_text().strip() |
There was a problem hiding this comment.
SUGGESTION: If .secret_key already exists but is empty or whitespace-only, secret_key becomes "". The {secret_key} placeholder is then substituted with an empty string, producing an invalid/empty server.secret_key in the generated settings.yml. Validate the persisted value (non-empty, ideally 64 hex chars) before trusting it, and regenerate otherwise.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| path = entry["path"] | ||
| content = entry["content"] | ||
| if "{secret_key}" in content: | ||
| content = content.replace("{secret_key}", secret_key) |
There was a problem hiding this comment.
SUGGESTION: content.replace("{secret_key}", secret_key) replaces every occurrence of the token. If manifest content ever legitimately contains the literal {secret_key} for another purpose, it will be overwritten. Consider asserting the token appears exactly once, or use a more distinctive placeholder, to avoid silent mis-substitution.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| content = content.replace("{secret_key}", secret_key) | ||
| full_path = app_dir / path | ||
| full_path.parent.mkdir(parents=True, exist_ok=True) | ||
| full_path.write_text(content) |
There was a problem hiding this comment.
SUGGESTION: Path traversal is validated up front via .resolve(), but the write here uses the unresolved app_dir / path. A symlink swapped into the path between validation and write (TOCTOU) could let the file land outside app_dir. Re-resolve and re-check (or write directly to the already-validated resolved path) at write time for defence-in-depth.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…pp secret (#1734) Fold on #1724 (searxng config_files). The per-app .secret_key signs sessions, so write it owner-only (0600) instead of the default 0644, and regenerate it if a prior write left it empty or not 64 hex chars, so an install never substitutes a blank secret_key into the mounted config.
2 participants
Fixes #969.
SearXNG only reads SEARXNG_SETTINGS_PATH and SEARXNG_DISABLE_ETC_SETTINGS — there is no generic SEARXNG_* nested override (the __ pattern is Home Assistant, not SearXNG). The previous env-var approach did nothing.
What this does
Adds
config_filessupport toDockerInstaller— a declarative way for manifests to seed config files into the app directory before compose generation. Supports{secret_key}templating for per-install random secrets.Seeds a real
settings.ymlwith:use_default_settings: truesearch.formats: [html, json]— JSON format enabled for agent queriesserver.secret_key— per-install generated (64-hex-char random)Bind-mounts
./settings.yml:/etc/searxng/settings.yml:ro— overrides the default settings.yml inside the named config volume.Removes the non-functional
SEARXNG_SEARCH__FORMATS__*env vars from the old approach.Upgrade handling
Existing installs get the fix automatically: re-running the install flow writes the settings.yml and adds the bind mount to compose. The named volume's default settings.yml is overridden by the bind mount.
Tests
pytest tests/test_installers.py tests/routes/test_store_install_v2.py tests/test_routes_store.pytest_write_config_files_creates_files_with_substitution,test_write_config_files_noop_when_no_config_filesSummary by CodeRabbit
New Features
Bug Fixes