Change default access mode to RESTRICTED and warn on UNRESTRICTED startup

[joergmichno]

Motivation

Following up on #164: the server currently defaults to UNRESTRICTED, which
exposes execute_sql with full write capabilities to the LLM. Because the
sql parameter is passed through without filtering, any prompt-injection
payload in content the agent reads (web pages, Confluence, emails) can reach
the database verbatim — a cross-tool injection path from one poisoned page to
DROP TABLE / DELETE on a production database.

The existing SafeSqlDriver (force_readonly=True) already solves this well;
it just isn't the default.

Changes

• --access-mode now defaults to restricted (module-level default
  aligned in server.py).
• Explicit --access-mode unrestricted prints a startup warning covering
  destructive SQL and the prompt-injection risk, and points to the restricted
  mode.
• README "Access Mode" section updated to document restricted as the
  default (the configuration examples already pass --access-mode=unrestricted
  explicitly, so they keep working unchanged).

Backwards compatibility

This is a deliberate behavior change for users who relied on the implicit
default. Users who start the server with --access-mode=unrestricted (as in
all README examples) see no functional change beyond the new startup warning.

Tests

• pytest tests/unit (Windows, Python 3.12): 167 passed, 24 skipped
  (DB-dependent), 1 xfailed — including all access-mode tests and the
  Python-3.12-only tests/unit/index/test_dta_calc.py (PEP 701).
• Earlier run (Windows, Python 3.11): 134 passed, 24 skipped, 1 xfailed
  (same suite minus the 3.12-only file).
• Existing mode tests pass the flag explicitly and are unaffected.

Out of scope (happy to follow up)

• Recommendation 2 from Security: Default UNRESTRICTED mode allows LLM-driven arbitrary SQL execution #164 (DDL statement allowlist in UNRESTRICTED mode,
  e.g. blocking DROP/ALTER/TRUNCATE unless explicitly enabled) deserves its
  own PR with a proper design discussion.

Static analysis only; no live databases were tested.

[jssmith]

Review by OpenHands agent — concise

Approve. Good security posture improvement.

• Defaulting to RESTRICTED is the right call — the prompt-injection → execute_sql path is real and the existing SafeSqlDriver already mitigates it.
• Module-level default aligned with argparse default — consistent, and safe if current_access_mode is read before main() runs.
• Startup warning for UNRESTRICTED is clear and actionable.
• README update is accurate; existing examples that pass --access-mode=unrestricted explicitly keep working.

Suggestion (non-blocking)

• Add a test verifying that omitting --access-mode results in RESTRICTED (the existing test_command_line_parsing passes the flag explicitly, so the new default behavior isn't covered). A simple sys.argv without --access-mode asserting current_access_mode == AccessMode.RESTRICTED would lock in the core behavior change.

---

This review was posted by an AI agent (OpenHands) on behalf of the repository maintainers.

[jssmith]

Review — concise

Verdict: Request changes. The security rationale is sound and the default flip is the right direction, but the warning message has an encoding issue that will garble output in some terminals.

Default change — correct

• Flipping current_access_mode from UNRESTRICTED to RESTRICTED at both the module-level global and the argparse default is consistent.
• All existing tests pass the flag explicitly, so no test regressions — good.
• This is a deliberate backwards-incompatible change; the PR body calls it out clearly.

Startup warning — encoding problem

logger.warning(
    "⚠️  UNRESTRICTED mode is active: ..."

The ⚠️ emoji in a logger.warning string will render as garbled bytes (\xe2\x9a\xa0\xef\xb8\x8f) in any environment where the stream encoding is not UTF-8 — Docker containers with LANG=C, Windows consoles without UTF-8, CI logs, journald in some configurations. Since this is a security warning, it must be reliably readable everywhere. Replace the emoji with plain text, e.g. "[WARNING] UNRESTRICTED mode is active: ...".

README update — good

• Reordering to list Restricted first with "(default)" is clear.
• The prompt-injection note in the Unrestricted bullet is a good addition — it explains why the warning exists without being alarmist.

Nits (non-blocking)

1. No test for the warning. The warning is logged via logger.warning, so a simple caplog assertion would verify it fires only in unrestricted mode. Not a blocker, but given the security relevance it would be good to lock in.
2. Help text for --access-mode now says "restricted (read-only with protections, default)" — slightly awkward phrasing. Consider "restricted (default; read-only with protections)".

Summary

The only blocking issue is the emoji in the log message. Fix that and this is ready to merge.

---

This review was created by an AI agent (OpenHands) on behalf of @jssmith.

[jssmith]

Review: Change default access mode to RESTRICTED

Verdict: Approve with discussion point.

Assessment

Changing the default from UNRESTRICTED to RESTRICTED is a good security-by-default decision. The prompt-injection risk described in the PR body (content the agent reads reaching execute_sql unfiltered) is real.

Strengths

1. The startup warning for UNRESTRICTED mode is well-written — concise, names the specific risks (destructive SQL + prompt injection), and points to the fix.
2. README update is clear — documents restricted as default, notes the warning, and existing examples that pass --access-mode=unrestricted still work.
3. Test suite passes — existing mode tests pass the flag explicitly so they're unaffected.

Discussion point

This is a breaking behavior change for users who start the server without --access-mode. Any user currently relying on the implicit unrestricted default will find their agent can no longer write to the database after upgrading. The PR acknowledges this, but consider:

• A migration note in the README or CHANGELOG would help users understand why their agent suddenly can't write.
• The warning text uses an emoji (⚠️) which may render oddly in some log formats. Minor.

This is the right direction — secure defaults matter. The breakage is acceptable given the security improvement.

This review was created by an AI agent (OpenHands) on behalf of @jssmith.

[jssmith]

Review — concise

Verdict: Request changes. Good security-first direction, but this is a breaking change that needs migration considerations.

Strengths

• Changing the default to RESTRICTED follows the principle of least privilege. Good security posture.
• Startup warning for UNRESTRICTED mode is well-written — mentions prompt-injection risk from external content.
• README updated to reflect the new default and warning.
• Help text updated to reflect new default.

Issues to address

1. Breaking change for existing users (blocking)
Every existing deployment that doesn't explicitly set --access-mode=unrestricted will silently switch from full read/write to read-only. Users who rely on write operations (INSERT/UPDATE/DELETE/DDL) will see their agent suddenly unable to modify data, with no error at startup — only tool call failures at runtime. This needs:

• A prominent changelog/release note
• Consider a deprecation cycle: log a warning when --access-mode is not explicitly set and default to UNRESTRICTED with a deprecation notice, then switch the default in the next major version
• Or at minimum, a clear startup log message when running in the new default RESTRICTED mode that says "Running in restricted (read-only) mode. Pass --access-mode=unrestricted for write access."

2. No test updates (blocking)
The test_command_line_parsing test in test_access_mode.py asserts the default mode. Verify it still passes — if it expected UNRESTRICTED as default, it needs updating.

3. Warning message uses emoji (non-blocking)
The ⚠️ emoji in the log warning may not render correctly in all log sinks (syslog, journald, file logs in some encodings). Consider using a text-only prefix like [WARNING] or [SECURITY].

4. AccessMode enum order (non-blocking)
The enum still lists UNRESTRICTED first, RESTRICTED second. Consider reordering to list RESTRICTED first to reflect the new default priority. Not blocking but improves readability.

This review was created by an AI agent (OpenHands) on behalf of @jssmith.

[joergmichno]

@jssmith Thanks for the thorough review — both blocking points are addressed in f20a7cc:

1. Silent behavior change: \main()\ now logs an explicit info line when running in restricted mode (\Running in restricted (read-only) mode. Pass --access-mode=unrestricted for write access.\), and the README gained a prominent breaking-change note under Access Mode.
2. Default coverage: Added \ est_command_line_parsing_default_restricted\ — it runs \main()\ with no --access-mode\ flag and asserts the security default resolves to RESTRICTED.

Also picked up two nits while the branch was open: the warning prefix is now syslog-safe ([SECURITY]\ instead of the emoji), and the \AccessMode\ enum lists RESTRICTED first to match the new default. Full suite green: \pytest tests/unit\ -> 168 passed, 24 skipped, 1 xfailed.

Ready for re-review.

[joergmichno]

@jssmith Thanks for re-reviewing f20a7cc — your summary reads as an approval ("Good security posture improvement"). Since the review is registered as a comment rather than an approval, could you submit it as an official approval? Happy to address anything else that's blocking.