diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 000000000..13a2da69f --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,8 @@ +# Code owners. +# +# Every file is owned by the maintainer, so that when branch protection has +# "Require review from Code Owners" turned on, no pull request can be merged +# without the maintainer's review. This is the human gate that backs up the +# automated security checks. See docs/security-ci.md for how to turn it on. + +* @pewdiepie-archdaemon diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 000000000..e1e0bf13e --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,48 @@ +# Dependabot keeps dependencies and pinned action versions current. +# +# Why this matters for security: every workflow in this repo pins its GitHub +# Actions to an exact commit (a SHA), which is safe but freezes them in time. +# Dependabot opens a small, reviewable pull request whenever a newer version +# exists -- for Python packages, npm packages, the Docker base image, and the +# pinned Actions themselves -- so staying patched does not require manual work. +# Updates are grouped so a week's bumps arrive as one PR per ecosystem, not a +# flood of separate ones. + +version: 2 +updates: + # Python dependencies (requirements.txt + requirements-optional.txt). + - package-ecosystem: pip + directory: "/" + schedule: + interval: weekly + open-pull-requests-limit: 5 + groups: + python: + patterns: ["*"] + + # Frontend / tooling npm packages (package.json). + - package-ecosystem: npm + directory: "/" + schedule: + interval: weekly + open-pull-requests-limit: 5 + groups: + npm: + patterns: ["*"] + + # The pinned action SHAs used across .github/workflows. + - package-ecosystem: github-actions + directory: "/" + schedule: + interval: weekly + open-pull-requests-limit: 5 + groups: + actions: + patterns: ["*"] + + # The Docker base image in the Dockerfile. + - package-ecosystem: docker + directory: "/" + schedule: + interval: weekly + open-pull-requests-limit: 5 diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 000000000..a53835a05 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,61 @@ +# CodeQL code scanning +# +# Purpose: GitHub's own static analysis engine reads the application source +# (Python backend + the JavaScript frontend) and looks for real +# vulnerabilities -- SQL/command injection, path traversal, auth mistakes, +# unsafe deserialization. Findings appear in the repo's Security tab. This is +# the deepest check in the suite and the most valuable for a high-profile +# target. +# +# It runs on every push to main and on a weekly schedule (to catch newly +# disclosed query patterns against unchanged code). It deliberately does NOT +# run on pull requests: most PRs here come from forks, whose read-only token +# cannot publish results, which would produce confusing failures. To scan pull +# requests too, a maintainer can instead enable CodeQL "default setup" in +# Settings -> Security -> Code scanning (one toggle, no file needed) -- see +# docs/security-ci.md. + +name: CodeQL + +on: + push: + branches: [main] + schedule: + # Weekly, Monday 06:00 UTC. + - cron: '0 6 * * 1' + workflow_dispatch: + +permissions: {} + +concurrency: + group: codeql-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + analyze: + name: Analyze (${{ matrix.language }}) + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write # publish results to the Security tab + strategy: + fail-fast: false + matrix: + # Both are interpreted, so CodeQL needs no build step (build-mode none). + language: [python, javascript-typescript] + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Initialize CodeQL + uses: github/codeql-action/init@03e4368ac7daa2bd82b3e85262f3bf87ee112f57 # v3.36.0 + with: + languages: ${{ matrix.language }} + build-mode: none + + - name: Perform CodeQL analysis + uses: github/codeql-action/analyze@03e4368ac7daa2bd82b3e85262f3bf87ee112f57 # v3.36.0 + with: + category: "/language:${{ matrix.language }}" diff --git a/.github/workflows/container-scan.yml b/.github/workflows/container-scan.yml new file mode 100644 index 000000000..71c4121a4 --- /dev/null +++ b/.github/workflows/container-scan.yml @@ -0,0 +1,52 @@ +# Container security: Dockerfile lint +# +# Purpose: the Docker image is how most people run Odysseus, so it is part of +# the attack surface. hadolint lints the Dockerfile for mistakes and insecure +# patterns (running as root longer than needed, unpinned base image, bad apt +# usage). Blocking. +# +# The image vulnerability scan (Trivy, advisory) lives in its own file, +# container-trivy.yml. Keeping it separate lets that advisory scan be +# path-filtered and held to a read-only token on pull requests without +# weakening this blocking gate, which must always report so a required check +# never hangs. +# +# Note: a separate open PR (#120) proposes a local `scripts/scan_image.py`. +# This job is complementary -- it is a CI gate, not a script a contributor has +# to remember to run. + +name: Container scan + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +permissions: {} + +concurrency: + group: container-scan-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + hadolint: + name: hadolint (Dockerfile lint) + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Lint Dockerfile + uses: hadolint/hadolint-action@2332a7b74a6de0dda2e2221d575162eba76ba5e5 # v3.3.0 + with: + dockerfile: Dockerfile + # DL3008: pinning apt package versions is impractical on a -slim base + # image. Debian purges old package versions from its repos, so a + # pinned version breaks future rebuilds. The base image itself is + # what should be pinned (tracked by Dependabot's docker ecosystem). + ignore: DL3008 diff --git a/.github/workflows/container-trivy.yml b/.github/workflows/container-trivy.yml new file mode 100644 index 000000000..025fefc16 --- /dev/null +++ b/.github/workflows/container-trivy.yml @@ -0,0 +1,125 @@ +# Container image vulnerability scan (advisory) +# +# Trivy builds the application image and scans it for known-vulnerable OS and +# Python packages. Advisory only -- it reports findings to the repo's Security +# tab without blocking a merge, because the image inevitably contains +# already-known CVEs in upstream packages that are not this project's bug. +# +# Split from the Dockerfile lint (container-scan.yml) for two reasons: +# +# - Least privilege. The image build runs Dockerfile instructions, which on a +# pull request are attacker-influenceable. That path (the `scan` job) is +# held to a read-only token and never publishes results. Only `publish`, +# which runs on push to main (curated, fast-forwarded from reviewed dev), +# gets security-events:write to upload SARIF. +# - Cost. Docs-only changes do not rebuild the image (paths-ignore below), +# matching docker-publish.yml. hadolint stays on the broad trigger in +# container-scan.yml so the blocking gate always reports. + +name: Container scan (Trivy) + +on: + pull_request: + paths-ignore: + - '**.md' + - 'docs/**' + - '.github/ISSUE_TEMPLATE/**' + push: + branches: [main] + paths-ignore: + - '**.md' + - 'docs/**' + - '.github/ISSUE_TEMPLATE/**' + workflow_dispatch: + +permissions: {} + +concurrency: + group: container-trivy-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + # Pull requests and manual runs: build and scan under a read-only token. + # The build executes PR-supplied Dockerfile instructions, so this job must + # not hold any write scope, and it does not upload to the Security tab. + scan: + name: Trivy (image scan, advisory) + if: github.event_name != 'push' + runs-on: ubuntu-latest + # Advisory: a CVE in an upstream package must not block a PR. + continue-on-error: true + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Set up Buildx + uses: docker/setup-buildx-action@d7f5e7f509e45cec5c76c4d5afdd7de93d0b3df5 # v4.1.0 + + # Build without pushing so a broken Dockerfile is caught here, and the + # exact image we ship is what gets scanned. + - name: Build image + uses: docker/build-push-action@f9f3042f7e2789586610d6e8b85c8f03e5195baf # v7.2.0 + with: + context: . + push: false + load: true + tags: odysseus:ci + + - name: Scan image with Trivy + uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 + with: + image-ref: odysseus:ci + format: table + ignore-unfixed: true + env: + # Pin the vuln DB source to GHCR to avoid rate-limited Docker Hub + # mirrors that flake on shared runners. + TRIVY_DB_REPOSITORY: ghcr.io/aquasecurity/trivy-db:2 + + # Push to main only: build, scan, and publish SARIF to the Security tab. + # This is the only path that runs trusted code, so it is the only one granted + # security-events:write. + publish: + name: Trivy (image scan + SARIF upload) + if: github.event_name == 'push' + runs-on: ubuntu-latest + continue-on-error: true + permissions: + contents: read + security-events: write # upload SARIF to the Security tab + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Set up Buildx + uses: docker/setup-buildx-action@d7f5e7f509e45cec5c76c4d5afdd7de93d0b3df5 # v4.1.0 + + - name: Build image + uses: docker/build-push-action@f9f3042f7e2789586610d6e8b85c8f03e5195baf # v7.2.0 + with: + context: . + push: false + load: true + tags: odysseus:ci + + - name: Scan image with Trivy + uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 + with: + image-ref: odysseus:ci + format: sarif + output: trivy-results.sarif + ignore-unfixed: true + env: + TRIVY_DB_REPOSITORY: ghcr.io/aquasecurity/trivy-db:2 + + - name: Upload Trivy results + uses: github/codeql-action/upload-sarif@03e4368ac7daa2bd82b3e85262f3bf87ee112f57 # v3.36.0 + with: + sarif_file: trivy-results.sarif + category: trivy-image diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml new file mode 100644 index 000000000..85dc26ec6 --- /dev/null +++ b/.github/workflows/dependency-review.yml @@ -0,0 +1,71 @@ +# Supply-chain review +# +# Purpose: defend against "side-chain" / supply-chain attacks -- a pull request +# that adds (or bumps) a dependency to a version with a known vulnerability or a +# disallowed license. Two layers: +# +# - dependency-review: runs ONLY on pull requests. It compares the +# dependencies before and after the PR and blocks the merge if the change +# pulls in a package with a known security advisory. This is the gate. +# - pip-audit: scans the project's current Python requirements against the +# advisory database. Advisory only (it never blocks a merge), because it can +# flag a pre-existing issue in an already-shipped dependency. + +name: Dependency review + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +# Default-deny token; jobs grant only read access. +permissions: {} + +concurrency: + group: dependency-review-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + dependency-review: + name: dependency-review (PR gate) + # Only meaningful on a pull request -- it needs a base..head diff to review. + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Review dependency changes + uses: actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294 # v5.0.0 + with: + # Fail the PR on any newly introduced moderate-or-worse advisory. + fail-on-severity: moderate + + pip-audit: + name: pip-audit (advisory) + runs-on: ubuntu-latest + # Advisory: report known-vulnerable Python deps without blocking the merge. + continue-on-error: true + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + with: + python-version: '3.12' + + - name: Run pip-audit on requirements + run: | + set -euo pipefail + pip install pip-audit==2.10.0 + pip-audit -r requirements.txt -r requirements-optional.txt --strict diff --git a/.github/workflows/secret-scan.yml b/.github/workflows/secret-scan.yml new file mode 100644 index 000000000..55825bedf --- /dev/null +++ b/.github/workflows/secret-scan.yml @@ -0,0 +1,60 @@ +# Secret scanning +# +# Purpose: stop credentials (API keys, tokens, passwords, private keys) from +# ever living in the Git history. Odysseus deliberately keeps real secrets in +# files that are gitignored (.env, data/), but a slip in a future commit -- or a +# malicious pull request that sneaks one in -- would otherwise go unnoticed. +# This job reads the repository and the full commit history and fails if it +# finds anything that looks like a secret. +# +# It runs the official gitleaks BINARY directly (pinned to an exact version and +# verified against the project's published SHA-256 checksum) rather than the +# gitleaks GitHub Action, because the Action asks for a paid license on +# organization-owned repos. The binary is free and behaves identically. + +name: Secret scan + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +# Start with zero permissions; the single job opts back in to read-only. +permissions: {} + +concurrency: + group: secret-scan-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + gitleaks: + name: gitleaks + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + # Full history so a secret committed in an earlier commit (and later + # deleted) is still caught -- deletion does not remove it from Git. + fetch-depth: 0 + persist-credentials: false + + # Pinned version + checksum so a tampered release binary cannot run here. + # Bump VERSION/SHA256 together; the checksum comes from the matching + # gitleaks__checksums.txt on the GitHub release. + - name: Run gitleaks (pinned, checksum-verified) + env: + GITLEAKS_VERSION: 8.30.1 + GITLEAKS_SHA256: 551f6fc83ea457d62a0d98237cbad105af8d557003051f41f3e7ca7b3f2470eb + run: | + set -euo pipefail + TARBALL="gitleaks_${GITLEAKS_VERSION}_linux_x64.tar.gz" + curl -fsSL -o "${TARBALL}" \ + "https://github.com/gitleaks/gitleaks/releases/download/v${GITLEAKS_VERSION}/${TARBALL}" + echo "${GITLEAKS_SHA256} ${TARBALL}" | sha256sum -c - + tar -xzf "${TARBALL}" gitleaks + # Scan the whole history. Findings print to the log and fail the job. + ./gitleaks git --no-banner --redact --verbose . diff --git a/.github/workflows/workflow-security.yml b/.github/workflows/workflow-security.yml new file mode 100644 index 000000000..efe487319 --- /dev/null +++ b/.github/workflows/workflow-security.yml @@ -0,0 +1,80 @@ +# Workflow security (CI that audits the CI) +# +# Purpose: the GitHub Actions workflows themselves are an attack surface. A +# poorly written workflow can leak the repository token, run attacker-supplied +# code from a pull request, or pull in a tampered third-party action. These two +# tools check every workflow file in this repo for those mistakes: +# +# - actionlint: catches workflow syntax errors and shell-script bugs inside +# `run:` steps before they reach main. +# - zizmor: a security linter for Actions. Flags template-injection holes, +# unpinned actions, credential persistence, and over-broad token +# permissions -- exactly the patterns the rest of this CI is built to avoid. +# +# Add this early: it then audits every workflow added after it. + +name: Workflow security + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +# Default-deny token; each job grants only read access to the code. +permissions: {} + +concurrency: + group: workflow-security-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + actionlint: + name: actionlint + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + # Pinned version + checksum so a tampered binary cannot run here. + - name: Run actionlint (pinned, checksum-verified) + env: + ACTIONLINT_VERSION: 1.7.12 + ACTIONLINT_SHA256: 8aca8db96f1b94770f1b0d72b6dddcb1ebb8123cb3712530b08cc387b349a3d8 + run: | + set -euo pipefail + TARBALL="actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz" + curl -fsSL -o "${TARBALL}" \ + "https://github.com/rhysd/actionlint/releases/download/v${ACTIONLINT_VERSION}/${TARBALL}" + echo "${ACTIONLINT_SHA256} ${TARBALL}" | sha256sum -c - + tar -xzf "${TARBALL}" actionlint + ./actionlint -color + + zizmor: + name: zizmor (Actions SAST) + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + with: + python-version: '3.12' + + # Pinned zizmor release. --offline keeps the audit hermetic (no network + # calls about the actions it inspects); --min-severity=low surfaces + # everything so nothing slips through under the gate. + - name: Run zizmor + run: | + set -euo pipefail + pip install zizmor==1.25.2 + zizmor --offline --min-severity=low .github/workflows/ diff --git a/README.md b/README.md index a0dde96a9..bbc831c37 100644 --- a/README.md +++ b/README.md @@ -218,7 +218,7 @@ docker compose exec odysseus sh -lc 'test -e /dev/kfd && test -d /dev/dri && ls > the CUDA Toolkit at runtime. If Cookbook logs show `Unable to find cudart > library`, `Could NOT find CUDAToolkit`, `CUDA Toolkit not found`, or > tensors/layers assigned to CPU, that is a Cookbook/llama.cpp build issue — -> not a Docker passthrough failure. Re-install the serve engine via +> not a Docker passthrough failure. Reinstall the serve engine via > **Cookbook → Dependencies** to get a CUDA-enabled build. > > The same split applies to AMD/ROCm: seeing `/dev/kfd` and `/dev/dri` inside diff --git a/docs/security-ci.md b/docs/security-ci.md new file mode 100644 index 000000000..c25838f72 --- /dev/null +++ b/docs/security-ci.md @@ -0,0 +1,102 @@ +# Security CI guide + +This project runs a set of automated security checks on every pull request and +on every push to `main`. This page explains what each one does, whether it can +block a merge, and the few one-time settings you should turn on to get the full +benefit. + +## What runs, and why + +Each check lives in its own file under `.github/workflows/`. They run +automatically; you do not start them. + +| Check | What it protects against | Blocks a merge? | +|---|---|---| +| **Secret scan** (gitleaks) | An API key, token, or password being committed by mistake or on purpose | Yes | +| **Workflow security** (actionlint + zizmor) | A broken or insecure automation file that could leak the repo's access token | Yes | +| **Dependency review** | A pull request that adds a software library with a known security hole | Yes | +| **pip-audit** | Known security holes in the Python libraries already used | No (advisory) | +| **Container scan: hadolint** | Mistakes and insecure patterns in the `Dockerfile` | Yes | +| **Container scan: Trivy** | Known security holes in the Docker image | No (advisory) | +| **CodeQL** | Real bugs in the app's own code: injection, auth mistakes, path traversal | No (advisory) | + +"Blocks a merge" means a red X appears on the pull request and, once you enable +the setting below, the **Merge** button is disabled until it is fixed. + +"Advisory" means it reports problems into the repository's **Security** tab so +you can review them on your own schedule, but it never stops a merge. These are +advisory on purpose: they often flag long-standing issues in other people's +libraries, not something a given pull request introduced. + +## Where results appear + +- **Checks tab of a pull request**: the pass/fail of each check. A green tick is + good; a red X needs attention. +- **Security tab of the repository**: detailed findings from the advisory + scanners (Trivy and CodeQL). This is your dashboard. + +## If a check fails + +- **Secret scan failed**: a real credential may have been committed. Treat it as + leaked: rotate (regenerate) that key or token immediately, then remove it from + the file. Do not just delete the commit; assume it was seen. +- **Dependency review failed**: the pull request adds a library with a known + vulnerability. Ask the contributor to use a patched version, or decline the + change. +- **hadolint / workflow security failed**: the contributor changed the + `Dockerfile` or an automation file in a way the linter rejects. Ask them to + address the message shown in the failed check. + +## One-time settings to turn on + +These two settings unlock the full value. You only do them once. + +### 1. Require the blocking checks before merging + +This makes the **Merge** button refuse to work until the gating checks pass. + +1. Go to the repository on GitHub. +2. Click **Settings** (top right of the repo). +3. In the left sidebar, click **Branches**. +4. Under **Branch protection rules**, click **Add branch ruleset** (or **Add + rule**), and set the branch name pattern to `dev` (this is the branch all + pull requests target; `main` is fast-forwarded at releases). +5. Enable **Require status checks to pass before merging**. +6. In the search box that appears, add these checks by name: + - `Python syntax (compileall)` + - `JS syntax (node --check)` + - `gitleaks` + - `actionlint` + - `zizmor (Actions SAST)` + - `hadolint (Dockerfile lint)` + - `dependency-review (PR gate)` + + The first two come from the correctness CI (`ci.yml`); the rest are this + security suite. Leave pytest, pip-audit, Trivy, and CodeQL unchecked so they + stay advisory. +7. Also enable **Require a pull request before merging** and **Require review + from Code Owners** (this uses the `.github/CODEOWNERS` file so every change + needs your sign-off). +8. Click **Create** / **Save changes**. + +Note: a check name only appears in the list after it has run at least once, so +let the workflows run on one pull request first, then add them here. + +### 2. Turn on the Security tab features + +1. **Settings -> Code security** (or **Code security and analysis**). +2. Turn on **Dependency graph** (usually on by default for public repos) -- this + powers Dependency review and Dependabot. +3. Turn on **Dependabot alerts** and **Dependabot security updates**. +4. Under **Code scanning**, you have two ways to scan the app code with CodeQL: + - The included `codeql.yml` workflow already scans `main` and runs weekly. + - To also scan **pull requests** (recommended, since most contributions come + from forks), click **Set up -> Default** under Code scanning. GitHub then + runs CodeQL on pull requests for you, with no token limitations. + +## Keeping it current + +`.github/dependabot.yml` opens small weekly pull requests to update Python and +npm packages, the Docker base image, and the pinned automation actions +themselves. Review and merge those like any other pull request; they keep the +project patched without manual tracking. diff --git a/routes/chat_routes.py b/routes/chat_routes.py index f06ca4dc7..7ad635576 100644 --- a/routes/chat_routes.py +++ b/routes/chat_routes.py @@ -474,8 +474,11 @@ def setup_chat_routes( use_research = form_data.get("use_research") time_filter = form_data.get("time_filter") preset_id = form_data.get("preset_id") - allow_bash = form_data.get("allow_bash") - allow_web_search = form_data.get("allow_web_search") + # Issue #3229: API callers send JSON, not FormData. Read from the + # JSON body as fallback so callers who send {"allow_bash": true} + # actually get bash enabled. + allow_bash = form_data.get("allow_bash") or (body or {}).get("allow_bash") + allow_web_search = form_data.get("allow_web_search") or (body or {}).get("allow_web_search") use_rag = form_data.get("use_rag") search_context = form_data.get("search_context") # pre-fetched web search results (compare mode) compare_mode = str(form_data.get("compare_mode", "")).lower() == "true" @@ -687,9 +690,13 @@ def setup_chat_routes( # Build disabled-tools set from frontend toggles + user privileges disabled_tools = set() - if str(allow_bash).lower() != "true": + # Only disable bash/web_search when the caller *explicitly* set them + # to a falsy value. When unset (None), defer to per-user privilege + # checks below — this lets admins with can_use_bash=True use bash + # by default without having to send allow_bash in every request. + if allow_bash is not None and str(allow_bash).lower() != "true": disabled_tools.add("bash") - if str(allow_web_search).lower() != "true": + if allow_web_search is not None and str(allow_web_search).lower() != "true": disabled_tools.add("web_search") disabled_tools.add("web_fetch") diff --git a/static/js/chat.js b/static/js/chat.js index 434976c65..a9d89cc64 100644 --- a/static/js/chat.js +++ b/static/js/chat.js @@ -802,15 +802,15 @@ import { wireArrowUpRecall, getLastUserMessageFromChatHistory } from './composer } else { fd.append('use_web', 'true'); } + } else if (isAgentMode) { + fd.append('allow_web_search', 'false'); } if (el('research-toggle').checked) { fd.append('use_research', 'true'); // Research always runs in chat mode — override agent if set fd.set('mode', 'chat'); } - if (el('bash-toggle').checked) { - fd.append('allow_bash', 'true'); - } + fd.append('allow_bash', el('bash-toggle').checked ? 'true' : 'false'); const ragChk = el('rag-toggle'); if (ragChk && !ragChk.checked) { fd.append('use_rag', 'false'); diff --git a/tests/test_chat_route_tool_policy.py b/tests/test_chat_route_tool_policy.py index d1f155650..21fb78616 100644 --- a/tests/test_chat_route_tool_policy.py +++ b/tests/test_chat_route_tool_policy.py @@ -1,50 +1,227 @@ +"""Issue #3229 — allow_bash / allow_web_search must work for JSON API callers +and admin users must get bash enabled by default. + +Bug: allow_bash and allow_web_search were only read from form_data, so JSON +API callers (Content-Type: application/json) always had bash disabled. + +Fix: (1) Read from JSON body as fallback. + (2) Only add bash/web_search to disabled_tools when explicitly set to a + falsy value; when unset (None), defer to per-user privilege checks. +""" + +import ast from pathlib import Path +import pytest -CHAT_ROUTES = Path(__file__).resolve().parents[1] / "routes" / "chat_routes.py" +_CHAT_ROUTES = Path(__file__).resolve().parent.parent / "routes" / "chat_routes.py" -def _source() -> str: - return CHAT_ROUTES.read_text(encoding="utf-8") +# ── Source-level guards ───────────────────────────────────────── -def test_research_fast_path_respects_tool_policy(): - src = _source() - assert "pre_context_tool_policy = build_effective_tool_policy(" in src - assert "allow_tool_preprocessing = not pre_context_tool_policy.block_all_tool_calls" in src - assert "allow_tool_preprocessing=allow_tool_preprocessing" in src - assert "research_blocked_by_policy = bool(" in src - assert 'tool_policy.blocks("trigger_research")' in src - assert 'tool_policy.blocks("manage_research")' in src - assert 'effective_do_research = bool(' in src - assert 'if effective_do_research:' in src - assert '"is_research": effective_do_research' in src - assert "_effective_mode = 'research' if effective_do_research else (chat_mode or 'chat')" in src - assert '_model_suffix = "Research" if effective_do_research else None' in src - assert "do_research=effective_do_research" in src +def test_allow_bash_reads_from_body_as_fallback(): + """chat_stream must read allow_bash from the JSON body, not just form_data.""" + source = _CHAT_ROUTES.read_text(encoding="utf-8") + tree = ast.parse(source) + + # Find the chat_stream function + chat_stream_func = None + for node in ast.walk(tree): + if isinstance(node, ast.AsyncFunctionDef) and node.name == "chat_stream": + chat_stream_func = node + break + assert chat_stream_func is not None, "chat_stream function not found" + + # Look for an assignment to allow_bash that references 'body' + found_body_fallback = False + for node in ast.walk(chat_stream_func): + if isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name) and target.id == "allow_bash": + # Check if 'body' appears in the value + src_segment = ast.get_source_segment(source, node) + if src_segment and "body" in src_segment: + found_body_fallback = True + assert found_body_fallback, ( + "allow_bash assignment in chat_stream must fall back to JSON body" + ) -def test_non_streaming_chat_path_uses_tool_policy_before_context_and_research(): - src = _source() - chat_endpoint = src[src.index("async def chat_endpoint"):src.index("# ------------------------------------------------------------------ #", src.index("async def chat_endpoint"))] - assert "tool_policy = build_effective_tool_policy(last_user_message=message)" in chat_endpoint - assert "allow_tool_preprocessing = not tool_policy.block_all_tool_calls" in chat_endpoint - assert 'if not tool_policy.blocks("manage_memory"):' in chat_endpoint - assert "allow_tool_preprocessing=allow_tool_preprocessing" in chat_endpoint - assert 'tool_policy.blocks("trigger_research")' in chat_endpoint - assert "if use_research and not research_blocked_by_policy:" in chat_endpoint - assert "allow_background_extraction=not tool_policy.block_all_tool_calls" in chat_endpoint +def test_allow_web_search_reads_from_body_as_fallback(): + """chat_stream must read allow_web_search from the JSON body, not just form_data.""" + source = _CHAT_ROUTES.read_text(encoding="utf-8") + tree = ast.parse(source) + + chat_stream_func = None + for node in ast.walk(tree): + if isinstance(node, ast.AsyncFunctionDef) and node.name == "chat_stream": + chat_stream_func = node + break + assert chat_stream_func is not None + + found_body_fallback = False + for node in ast.walk(chat_stream_func): + if isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name) and target.id == "allow_web_search": + src_segment = ast.get_source_segment(source, node) + if src_segment and "body" in src_segment: + found_body_fallback = True + assert found_body_fallback, ( + "allow_web_search assignment in chat_stream must fall back to JSON body" + ) -def test_image_generation_fast_path_checks_policy_before_tool_start(): - src = _source() - policy_gate = src.index('if tool_policy.blocks("generate_image"):') - tool_start = src.index('"type": "tool_start", "tool": "generate_image"') - generator_call = src.index("do_generate_image(") - assert policy_gate < tool_start - assert policy_gate < generator_call +def test_disabled_tools_does_not_bash_when_allow_bash_is_none(): + """When allow_bash is not set (None), bash must NOT be unconditionally + added to disabled_tools. The per-user privilege check handles it. + """ + source = _CHAT_ROUTES.read_text(encoding="utf-8") + + # The fix changes: + # if str(allow_bash).lower() != "true": + # to: + # if allow_bash is not None and str(allow_bash).lower() != "true": + assert "allow_bash is not None" in source, ( + "disabled_tools check must guard against allow_bash being None" + ) + assert "allow_web_search is not None" in source, ( + "disabled_tools check must guard against allow_web_search being None" + ) -def test_streaming_chat_paths_disable_background_extraction_under_policy(): - src = _source() - assert src.count("allow_background_extraction=not tool_policy.block_all_tool_calls") >= 3 +# ── Functional tests of the disabled-tools logic ─────────────── + + +def _build_disabled_tools( + allow_bash=None, + allow_web_search=None, + can_use_bash=True, + can_use_browser=True, +): + """Replicate the disabled-tools logic from chat_stream for unit testing. + + Returns the set of tool names that would be disabled. + """ + disabled_tools = set() + + # Issue #3229 fix: only disable when explicitly set to a falsy value. + if allow_bash is not None and str(allow_bash).lower() != "true": + disabled_tools.add("bash") + if allow_web_search is not None and str(allow_web_search).lower() != "true": + disabled_tools.add("web_search") + disabled_tools.add("web_fetch") + + # Enforce per-user privileges + if not can_use_bash: + disabled_tools.update({"bash", "python", "read_file", "write_file"}) + if not can_use_browser: + disabled_tools.add("builtin_browser") + + return disabled_tools + + +def test_json_body_allow_bash_true_enables_bash(): + """API caller sending {"allow_bash": true} gets bash enabled.""" + disabled = _build_disabled_tools(allow_bash="true") + assert "bash" not in disabled + + +def test_json_body_allow_bash_false_disables_bash(): + """API caller sending {"allow_bash": false} gets bash disabled.""" + disabled = _build_disabled_tools(allow_bash="false") + assert "bash" in disabled + + +def test_json_body_allow_web_search_true_enables_web(): + """API caller sending {"allow_web_search": true} gets web tools enabled.""" + disabled = _build_disabled_tools(allow_web_search="true") + assert "web_search" not in disabled + assert "web_fetch" not in disabled + + +def test_json_body_allow_web_search_false_disables_web(): + """API caller sending {"allow_web_search": false} gets web tools disabled.""" + disabled = _build_disabled_tools(allow_web_search="false") + assert "web_search" in disabled + assert "web_fetch" in disabled + + +def test_admin_user_gets_bash_enabled_by_default(): + """When allow_bash is not set and user has can_use_bash privilege, + bash must NOT be disabled. + """ + disabled = _build_disabled_tools(allow_bash=None, can_use_bash=True) + assert "bash" not in disabled + + +def test_admin_user_gets_web_search_enabled_by_default(): + """When allow_web_search is not set and user has normal privileges, + web_search must NOT be disabled. + """ + disabled = _build_disabled_tools(allow_web_search=None) + assert "web_search" not in disabled + assert "web_fetch" not in disabled + + +def test_non_privileged_user_without_explicit_flag_still_disabled(): + """A user without can_use_bash privilege who doesn't send allow_bash + should still have bash disabled via the privilege check. + """ + disabled = _build_disabled_tools(allow_bash=None, can_use_bash=False) + assert "bash" in disabled + + +def test_non_privileged_user_explicit_true_overridden_by_privilege(): + """Even if allow_bash=true is sent, a user without can_use_bash + privilege still gets bash disabled by the privilege gate. + """ + disabled = _build_disabled_tools(allow_bash="true", can_use_bash=False) + assert "bash" in disabled + + +def test_form_data_none_body_true_works(): + """Simulates: form_data has no allow_bash, body has allow_bash=true. + After the fallback (`form_data.get(...) or body.get(...)`), allow_bash + should be "true". + """ + # Simulate the fallback logic + form_data_val = None # not in form_data + body_val = "true" # from JSON body + allow_bash = form_data_val or body_val + assert str(allow_bash).lower() == "true" + + disabled = _build_disabled_tools(allow_bash=allow_bash) + assert "bash" not in disabled + + +def test_explicit_false_disables_even_for_admin(): + """An admin who explicitly sends allow_bash=false should have bash disabled.""" + disabled = _build_disabled_tools( + allow_bash="false", can_use_bash=True, + ) + assert "bash" in disabled + + +# ── Frontend source-level guards ────────────────────────────── + +_CHAT_JS = Path(__file__).resolve().parent.parent / "static" / "js" / "chat.js" + + +def test_frontend_always_sends_explicit_allow_bash(): + """chat.js must always send allow_bash (both true and false), not only on toggle ON.""" + source = _CHAT_JS.read_text(encoding="utf-8") + # Must not only append 'true' — must also handle the false case + assert "allow_bash', el('bash-toggle').checked ? 'true' : 'false'" in source or \ + "allow_bash', 'false'" in source, ( + "Frontend must send explicit allow_bash=false when toggle is off" + ) + + +def test_frontend_sends_explicit_allow_web_search_false_in_agent_mode(): + """chat.js must send allow_web_search=false when web toggle is off in agent mode.""" + source = _CHAT_JS.read_text(encoding="utf-8") + assert "allow_web_search', 'false'" in source, ( + "Frontend must send explicit allow_web_search=false in agent mode when toggle is off" + )