Skip to content

[security] fix(commands): keep diff local-only by default#253

Merged
tjb-tech merged 2 commits into
HKUDS:mainfrom
Hinotoi-agent:fix/remote-diff-command-security
May 16, 2026
Merged

[security] fix(commands): keep diff local-only by default#253
tjb-tech merged 2 commits into
HKUDS:mainfrom
Hinotoi-agent:fix/remote-diff-command-security

Conversation

@Hinotoi-agent
Copy link
Copy Markdown
Contributor

Summary

This PR hardens the remote slash-command boundary for the repository diff command. /diff full can include the full uncommitted workspace patch, so it should remain a local OpenHarness UI action unless an operator explicitly opts it into the remote-admin command allowlist.

  • Marks /diff as local-only by default with remote_invocable=False.
  • Keeps the existing explicit remote-admin opt-in mechanism available with remote_admin_opt_in=True.
  • Adds registry and gateway regressions proving remote /diff full is denied and does not return a marker value from an uncommitted Git diff.

Security issues covered

Issue Impact Severity
Remote /diff full workspace disclosure An allowed remote channel sender could receive local uncommitted workspace changes through chat Medium

Before this PR

  • /diff inherited the default remote-invocable slash-command behavior.
  • A remote gateway message such as /diff full could execute the command against the active workspace.
  • The command returns git diff HEAD, which may include .env edits, local configuration changes, unpublished patches, prompt/context files, or proprietary source changes.
  • There was no regression test covering the remote gateway behavior for /diff full.

After this PR

  • /diff is local-only by default.
  • Remote /diff full returns the existing local-only denial message.
  • Trusted operators can still explicitly opt the command into remote-admin handling through the existing gateway allowlist controls.
  • Tests now cover both command metadata and the gateway path with a temporary Git repo containing a marker diff.

Why this matters

Remote chat channels are a different trust boundary from the local OpenHarness UI. Even when a sender is allowed to chat with the gateway, returning full local repository diffs can expose sensitive or unpublished workspace contents that the sender should not automatically receive.

How this differs from related issue/PR

This is the same remote slash-command trust-boundary family as prior hardening work, but it covers a distinct residual command:

The fix is intentionally separate because the vulnerable command, handler, and impact are different: this PR prevents returning local workspace patches, not shell execution or configuration mutation.

Attack flow

Allowed remote channel sender sends: /diff full
    -> ohmo gateway resolves the slash command
        -> /diff is allowed remotely by default on vulnerable builds
            -> _diff_handler() runs git diff HEAD in the workspace
                -> full uncommitted patch is returned to the remote chat

Affected code

Issue Files
Remote /diff full workspace disclosure src/openharness/commands/registry.py, ohmo/gateway/runtime.py

Root cause

Remote /diff full workspace disclosure:

  • SlashCommand.remote_invocable defaults to True.
  • /diff was registered without overriding that default.
  • _diff_handler() returns full git diff HEAD output for /diff full, but that local workspace inspection command was reachable from remote gateway messages.

CVSS assessment

Issue CVSS v3.1 Vector
Remote /diff full workspace disclosure 4.3 Medium CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:L/I:N/A:N

Rationale: the issue requires access as an allowed remote channel/gateway sender, has low attack complexity, and primarily affects confidentiality of local workspace contents returned in Git diffs. Severity can be higher in deployments with broad remote channel admission or secret-heavy workspaces.

Safe reproduction steps

  1. Start a vulnerable build where /diff is registered with the default remote_invocable=True behavior.

  2. In the active workspace, create a Git repo with one committed file and then modify it without committing:

    OPENHARNESS_VALUE=LEAKMARK_REMOTE_DIFF_VALUE
    
  3. Send a remote gateway message with /diff full.

  4. Observe that the final response contains the marker value from the uncommitted diff.

Expected vulnerable behavior

On vulnerable builds, the gateway returns a patch containing the uncommitted marker, for example:

-OPENHARNESS_VALUE=old
+OPENHARNESS_VALUE=LEAKMARK_REMOTE_DIFF_VALUE

After this PR, the remote gateway path returns:

/diff is only available in the local OpenHarness UI.

Changes in this PR

  • Registers /diff with remote_invocable=False.
  • Registers /diff with remote_admin_opt_in=True so trusted operators can explicitly allow it using the existing remote-admin mechanism if they accept that exposure.
  • Adds registry tests for the new /diff command metadata.
  • Adds a gateway regression that builds a temporary Git repo with a marker diff, sends remote /diff full, and asserts the marker is not leaked.

Files changed

Category Files What changed
Command metadata src/openharness/commands/registry.py Marks /diff local-only by default with explicit remote-admin opt-in support
Registry tests tests/test_commands/test_registry.py Covers /diff remote invocation metadata
Gateway tests tests/test_ohmo/test_gateway.py Covers remote /diff full denial and verifies a marker diff is not returned

Maintainer impact

  • The local /diff behavior remains unchanged.
  • Remote chat users no longer receive workspace patches by default.
  • Self-hosted/trusted deployments can still opt into remote /diff through the existing administrative command allowlist.
  • The patch is narrow and does not change unrelated slash-command handling.

Fix rationale

/diff full reads local workspace state and can return sensitive code or configuration changes. Keeping that command local-only by default matches the existing approach used for other local control-plane commands while preserving an explicit operator-controlled escape hatch for trusted deployments.

Type of change

  • Security fix
  • Tests
  • Documentation update
  • Refactor with no behavior change

Test plan

  • Focused /diff registry and gateway regression tests.
  • Full touched command/gateway test files.
  • Compile check for touched Python files.
  • Whitespace diff check.
  • Ruff lint check for touched files.

Executed with:

uv sync --extra dev
PYTHONPATH=src:. uv run pytest -o addopts='' \
  tests/test_commands/test_registry.py::test_diff_command_is_marked_local_only \
  tests/test_commands/test_registry.py::test_diff_command_supports_explicit_remote_admin_opt_in \
  tests/test_ohmo/test_gateway.py::test_runtime_pool_blocks_registered_diff_full_without_leaking_workspace_changes -q
PYTHONPATH=src:. uv run pytest -o addopts='' tests/test_commands/test_registry.py tests/test_ohmo/test_gateway.py -q
PYTHONPATH=src:. uv run python -m compileall -q src/openharness/commands/registry.py tests/test_commands/test_registry.py tests/test_ohmo/test_gateway.py
git diff --check
uv run ruff check src/openharness/commands/registry.py tests/test_commands/test_registry.py tests/test_ohmo/test_gateway.py

Local results:

  • Focused tests: 3 passed
  • Touched command/gateway files: 114 passed, 7 warnings
  • The warnings are existing datetime.utcnow() deprecation warnings in gateway tests.
  • Compile, whitespace, and Ruff checks passed.

Token usage

  • discovery tokens: partial/unknown
  • validation tokens: partial/unknown
  • duplicate-check tokens: partial/unknown
  • PR/writeup tokens: partial/unknown
  • total tokens: partial/unknown
  • notes: Token accounting was not available from the local validation and PR tooling in this session.

Disclosure notes

@Hinotoi-agent
Copy link
Copy Markdown
Contributor Author

CI note for this run:

  • Python quality passed.
  • Frontend typecheck passed.
  • Python tests (3.11) passed.
  • Python tests (3.10) failed in tests/test_tools/test_integration_flows.py::test_agent_send_message_flow_restarts_completed_agent with agent follow-up output did not become available in time.

That failing test is outside this PR's touched files and appears timing/flakiness-related rather than related to the /diff command metadata or gateway denial path. I cannot rerun the failed job from this forked PR because GitHub reports that rerun requires repository admin rights.

Additional local validation after seeing the failure:

PYTHONPATH=src:. uv run pytest -o addopts='' tests/test_tools/test_integration_flows.py::test_agent_send_message_flow_restarts_completed_agent -q

Local result: 1 passed.

@tjb-tech tjb-tech merged commit 9cff3a0 into HKUDS:main May 16, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants