fix(security)!: temporarily disable /help_docs command (#2445)

[naorpeled]

Summary

Immediate mitigation for #2445.

/help_docs accepts an untrusted runtime override of the clone target (--pr_help_docs.repo_url=...) and, because the clone-URL host validation only checks substring containment, the git provider token can be embedded into a clone URL pointing at an attacker-controlled host (e.g. github.com.attacker.tld) — leaking GITHUB_TOKEN.

This PR is a stopgap: it unregisters the /help_docs command so it can no longer be invoked, to be merged ahead of the full fix. An incoming /help_docs now resolves to an unknown command and is rejected.

The complete hardening (exact host validation across all providers + blocking the runtime override) is in #2450; this command should be re-enabled as part of that work.

Changes

• Remove the help_docs entry (and its import) from command2class in pr_agent/agent/pr_agent.py, with a comment pointing at PR-Agent /help_docs allows untrusted commenters to override git clone target and exfiltrate GITHUB_TOKEN via weak hostname validation #2445 / re-enable instructions.
• Add tests/unittest/test_help_docs_disabled.py asserting the command is unregistered and not routed.

Re-enable

Restore "help_docs": PRHelpDocs and its import, and delete the stopgap test, once #2450 (or equivalent) lands.

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

Context used

✅ Tickets: 🎫 PR-Agent /help_docs allows untrusted commenters to override git clone target and exfiltrate GITHUB_TOKEN via weak hostname validation

1. Test depends on global settings 📘 Rule violation ☼ Reliability

Description

test_help_docs_command_is_not_routed calls PRAgent()._handle_request() without controlling
get_settings().config.response_language, which can vary by environment and make the test flaky or
behave differently under CI/dev settings.

Code

tests/unittest/test_help_docs_disabled.py[R14-24]

+@pytest.mark.asyncio
+async def test_help_docs_command_is_not_routed(monkeypatch):
+    """An incoming /help_docs command resolves to an unknown command and is rejected."""
+    monkeypatch.setattr(pr_agent_module, "apply_repo_settings", lambda pr_url: None)
+    monkeypatch.setattr(pr_agent_module.CliArgs, "validate_user_args", lambda args: (True, ""))
+    monkeypatch.setattr(pr_agent_module, "update_settings_from_args", lambda args: args)
+
+    handled = await PRAgent()._handle_request(
+        "https://github.com/owner/repo/pull/1",
+        "/help_docs \"what docs exist\" --pr_help_docs.repo_url=https://github.com/x/y",
+    )

Evidence

The checklist requires tests to explicitly control external/global state. The new test invokes
_handle_request() (which reads response_language from global settings and may execute additional
logic when it is not en-us) without setting/patching that setting, making test behavior
environment-dependent.

tests/unittest/test_help_docs_disabled.py[14-24]
pr_agent/agent/pr_agent.py[79-104]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new unit test calls `PRAgent()._handle_request()` but does not isolate global configuration state. `_handle_request()` branches on `get_settings().config['response_language']`, so an environment/CI setting can change execution and make the test flaky.

## Issue Context
This PR adds `tests/unittest/test_help_docs_disabled.py` to verify `/help_docs` is rejected. The test should be deterministic regardless of ambient Dynaconf/env configuration.

## Fix Focus Areas
- tests/unittest/test_help_docs_disabled.py[14-24]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Help still advertises /help_docs 🐞 Bug ⚙ Maintainability

Description

/help_docs is unregistered in command2class, but multiple help/usage outputs still list
/help_docs, so users who follow those instructions will always hit the unknown-command path and be
rejected. This creates confusing behavior and noisy warnings (Unknown command: help_docs) until
the command is re-enabled.

Code

pr_agent/agent/pr_agent.py[R42-45]

+    # SECURITY: "/help_docs" is temporarily disabled while the clone-target validation
+    # fix is reviewed (see issue #2445). Re-enable by restoring `"help_docs": PRHelpDocs`
+    # and its import once the hardening PR is merged.
}

Evidence

The router rejects any command not present in command2class, while help/usage text still
explicitly instructs users to run /help_docs, making those instructions invalid after this PR.

pr_agent/agent/pr_agent.py[23-105]
pr_agent/tools/pr_help_message.py[197-242]
pr_agent/servers/help.py[1-14]
pr_agent/cli.py[14-68]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`/help_docs` has been removed from the command router, but the help/usage text still includes it. As a result, users are guided to invoke a command that is guaranteed to be rejected as unknown.

## Issue Context
The command routing checks `action not in command2class` and returns `False` (logging a warning). Help and CLI usage strings are currently hard-coded to include `/help_docs`, so they are now out of sync with the router.

## Fix Focus Areas
- pr_agent/tools/pr_help_message.py[197-242]
- pr_agent/servers/help.py[1-14]
- pr_agent/cli.py[14-46]

## Implementation notes
- Remove `/help_docs` from these help/usage lists, or render it conditionally based on whether `"help_docs" in pr_agent.agent.pr_agent.command2class` (or `commands`).
- Keep the security comment in `command2class` as-is; just make the user-facing help consistent with the disabled state.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Temporarily disable /help_docs command to mitigate token exfiltration risk
🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Walkthroughs

Description

• Unregister /help_docs to prevent invocation while clone-target validation is hardened.
• Add regression tests ensuring /help_docs is unknown and not routed.
• Document re-enable steps inline, referencing issue #2445.

Diagram

graph TD
  U["PR commenter"] --> H["PRAgent._handle_request()"] --> M["command2class (no help_docs)"] --> X["Unknown command"] --> R["Reject request (False)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Feature-flag gate /help_docs

• ➕ Allows emergency disable without code changes/deploys
• ➕ Can be toggled per environment/repo
• ➖ Requires config plumbing and secure defaults
• ➖ More moving parts than a hard unregister for a stopgap

2. Ship minimal host validation hotfix instead of disabling

• ➕ Preserves /help_docs functionality
• ➕ Targets the root cause sooner
• ➖ Higher risk to rush (parsing/URL edge cases across providers)
• ➖ Conflicts with the stated plan to land the full hardening in fix(security): validate clone host exactly across providers and block /help_docs repo overrides (#2445) #2450

Recommendation: For an immediate mitigation, fully unregistering /help_docs is the safest and lowest-risk stopgap because it eliminates the exploit path entirely. If operational flexibility is needed before #2450 lands, consider a feature flag next; otherwise keep this change minimal and re-enable only after the comprehensive hardening is merged.

File Changes

Bug fix (1)

pr_agent.py Unregister /help_docs command from PRAgent dispatch table +3/-2

Unregister /help_docs command from PRAgent dispatch table

• Removes the 'PRHelpDocs' import and the 'help_docs' entry from 'command2class', preventing routing of '/help_docs'. Adds an inline security comment referencing issue #2445 and explicit re-enable instructions.

pr_agent/agent/pr_agent.py

---

Tests (1)

test_help_docs_disabled.py Add tests asserting /help_docs is disabled and rejected +25/-0

Add tests asserting /help_docs is disabled and rejected

• Introduces unit tests to ensure 'help_docs' is absent from both 'command2class' and the exported 'commands' list. Adds an async routing test validating that an incoming '/help_docs' request is treated as unknown and returns 'False'.

tests/unittest/test_help_docs_disabled.py

---