feat: enforce engagement authorization framing#15
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR adds canonical engagement-scope validation, append-only audit logging, fail-closed orchestration and agent checks, authorization-aware prompts, scope propagation through OPSEC APIs and factories, router cache isolation, configuration updates, and comprehensive tests and documentation. ChangesEngagement authorization lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
The static prompt templates asserted the engagement was authorized
("certified experiment", "obtained valid certificates and approval",
"valid permission to perform") regardless of configured scope. Only the
scope-aware preamble in pentestgpt.prompts.authorization should assert
authorization, so neutralize these claims across the base, v1, and v2
templates. This keeps the neutralization consistent if any of these
otherwise-dead templates is ever revived.
Add a regression guard asserting no template field makes an
unconditional authorization claim.
OPSECAgent.create_session hardcoded the "sessions" directory for its scope audit sink, ignoring the orchestrator's configured session_dir on the standalone path. Add a session_dir parameter (default "sessions") and propagate the orchestrator's value so scope audit logs land alongside the rest of the session state.
CodeRabbit exceeds its per-PR file limit on this branch. Temporarily skip the docs/ folder so it can review the code changes. Revert once the review is complete.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
pentestgpt/prompts/prompt_class.py (2)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant sentence in the prompt.
The first two sentences are repetitive ("You're required to work as a cybersecurity penetration tester assistant." vs "You're an assistant for a cybersecurity penetration tester."). Removing the first sentence will make the prompt more concise and save a few tokens.
♻️ Proposed refactor
- input_parsing_init: str = """You're required to work as a cybersecurity penetration tester assistant. -You're an assistant for a cybersecurity penetration tester. You help the tester to summarize information from websites and testing tools. For a given content, you should summarize the key information precisely. In particular, + input_parsing_init: str = """You're an assistant for a cybersecurity penetration tester. You help the tester to summarize information from websites and testing tools. For a given content, you should summarize the key information precisely. In particular,🤖 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 `@pentestgpt/prompts/prompt_class.py` around lines 43 - 44, Remove the redundant opening sentence from the input_parsing_init prompt, keeping “You're an assistant for a cybersecurity penetration tester.” as the sole role statement and preserving the remaining prompt content unchanged.
54-71: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueInclude missing column descriptions in the prompt.
The table lists 15 columns (including
PortandName), but the description section only provides descriptions for 13 of them (missingPortandName). While LLMs can often infer these, providing the complete mapping aligns with the strict 15-element requirement and reduces hallucination risk.♻️ Proposed refactor
Host - The IP or the name of the Host Service - The service name that is tested + Port - The port number where the service is running + Name - The name of the vulnerability Description - a short description about the vulnerability🤖 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 `@pentestgpt/prompts/prompt_class.py` around lines 54 - 71, The report_generator_init prompt lists Port and Name as columns but does not describe them. Add explicit descriptions for both headings in the header-mapping section, defining Port as the tested network port and Name as the vulnerability or finding name, while preserving the required 15-column order and output format.tests/prompts/test_prompt_templates.py (1)
267-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefensively check if the field value is a string.
Calling
.lower()directly on the field value will raise anAttributeErrorif a non-string field is ever added to these dataclasses. It is safer to check the type first, mirroring the logic in the adjacenttest_all_dataclass_fields_non_emptytest.🛡️ Proposed fix
for f in dataclasses.fields(instance): - value = getattr(instance, f.name).lower() - for phrase in UNCONDITIONAL_AUTHORIZATION_PHRASES: - assert phrase not in value, f"{cls.__name__}.{f.name} asserts authorization: {phrase!r}" + value = getattr(instance, f.name) + if isinstance(value, str): + value_lower = value.lower() + for phrase in UNCONDITIONAL_AUTHORIZATION_PHRASES: + assert phrase not in value_lower, f"{cls.__name__}.{f.name} asserts authorization: {phrase!r}"🤖 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 `@tests/prompts/test_prompt_templates.py` around lines 267 - 270, Update the field-value handling in the dataclass authorization test to verify the value is a string before calling lower(), mirroring the defensive type check used by the adjacent test_all_dataclass_fields_non_empty test; only scan lowercase text values for prohibited phrases.tests/api/test_opsec_session_scope.py (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the
monkeypatchfixture.Use
monkeypatch: pytest.MonkeyPatchso this new test remains fully typed.As per coding guidelines, “All new code must be fully typed in strict MyPy mode.”
🤖 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 `@tests/api/test_opsec_session_scope.py` at line 11, Add the pytest.MonkeyPatch type annotation to the monkeypatch parameter of test_authorized_scope_reaches_opsec_session, preserving the test’s existing behavior and satisfying strict MyPy typing.Source: Coding guidelines
pentestgpt/utils/dependency_injection.py (1)
419-423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a narrow protocol for
set_engagement_scope. Inpentestgpt/utils/dependency_injection.py:419-423,hasattrdoes not narrowT, so strict MyPy still flags the method call. Use a smallProtocolor a targetedcastinstead ofAny.🤖 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 `@pentestgpt/utils/dependency_injection.py` around lines 419 - 423, Update wire_engagement_scope to type-narrow agents exposing set_engagement_scope by introducing a narrow Protocol for that method and using an isinstance-compatible check or targeted cast before invocation. Keep the existing _scope_guard condition, arguments, and agent return behavior unchanged, and do not use Any.Source: Coding guidelines
🤖 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 `@pentestgpt/agents/exploit_developer.py`:
- Around line 340-347: Update the outer exploitation workflow handlers around
_generate_exploit_candidate and develop_and_test_exploits to explicitly catch
OutOfScopeError and re-raise it before any generic exception handling. Add the
same propagation clauses at the additional affected layers so scope denial
reaches the public API instead of being converted into an empty result.
In `@pentestgpt/agents/opsec_agent.py`:
- Around line 596-600: Preserve authorization failures in both OPSEC LLM entry
points: in pentestgpt/agents/opsec_agent.py at lines 596-600, update the
target-analysis exception handler to re-raise OutOfScopeError before the generic
fallback handler; apply the same change at lines 830-834 for the recommendations
flow, while retaining fallback handling for other exceptions.
- Around line 534-540: Update the _scope_guard initialization flow to create a
new JsonlAuditSink using the current session_dir and session_id for every OPSEC
session, rather than reusing self._scope_guard.audit_sink when the scope
changes. Ensure the replacement ScopeGuard retains the current _engagement_scope
while writing audit events to that session’s scope_audit.jsonl destination.
In `@pentestgpt/api/stealth_api.py`:
- Around line 316-317: Update the scope handling around
EngagementScope.from_authorized_scope and opsec_agent.create_session so
ValueError from malformed authorization scopes is caught and returned as a
sanitized HTTP 422 client error without exposing exception details. Keep
unexpected exceptions on the existing HTTP 500 path.
In `@pentestgpt/core/engagement.py`:
- Around line 128-138: Update Engagement.__post_init__ to reject any invalid
authorized host or IP-range value instead of silently filtering it out; preserve
normalized values only after every configured value validates. In the runtime
authorization logic using normalize_target, retain invalid targets and classify
each one as offending rather than excluding it from evaluation, so mixed
valid/invalid input cannot pass based only on valid targets.
- Around line 50-52: Update normalize_target() around the urlsplit() call to
catch ValueError raised for malformed bracketed URLs and return an empty string.
Preserve the existing hostname extraction for valid URLs and the current
normalization behavior for non-URL candidates.
In `@pentestgpt/orchestrator.py`:
- Line 604: Update the command-dispatch flow around the scope_guard.check call
to inspect every command in verified_commands before selecting an executor. Use
the canonical command/parser layer to extract each command’s effective targets,
validate them against the active scope, and reject out-of-scope commands while
preserving the existing target_info and discovered-host checks.
In `@pentestgpt/utils/orchestrator_factory.py`:
- Around line 680-688: Ensure one authoritative engagement scope is used for
both prompts and enforcement: in pentestgpt/utils/orchestrator_factory.py lines
680-688, reject a supplied ScopeGuard whose scope differs from engagement_scope
or derive the effective scope from that guard before calling
AgentFactory.set_engagement_scope; apply the same invariant in
pentestgpt/orchestrator.py lines 108-117 for direct constructor callers.
In `@tests/agents/test_opsec_agent.py`:
- Around line 302-313: Extend
test_create_session_audit_uses_configured_session_dir to create a second session
and assert its audit sink path is under the second session ID, while retaining
the first path assertion. Update OPSECAgent.create_session and its
session-scoped audit sink setup so each invocation recreates or rotates the sink
for the new session instead of reusing the existing guard’s sink.
In `@tests/prompts/test_prompt_templates.py`:
- Line 244: Update the global UNCONDITIONAL_AUTHORIZATION_PHRASES declaration
with an explicit strict type annotation for its list elements, using the
appropriate string collection type and preserving the existing values.
---
Nitpick comments:
In `@pentestgpt/prompts/prompt_class.py`:
- Around line 43-44: Remove the redundant opening sentence from the
input_parsing_init prompt, keeping “You're an assistant for a cybersecurity
penetration tester.” as the sole role statement and preserving the remaining
prompt content unchanged.
- Around line 54-71: The report_generator_init prompt lists Port and Name as
columns but does not describe them. Add explicit descriptions for both headings
in the header-mapping section, defining Port as the tested network port and Name
as the vulnerability or finding name, while preserving the required 15-column
order and output format.
In `@pentestgpt/utils/dependency_injection.py`:
- Around line 419-423: Update wire_engagement_scope to type-narrow agents
exposing set_engagement_scope by introducing a narrow Protocol for that method
and using an isinstance-compatible check or targeted cast before invocation.
Keep the existing _scope_guard condition, arguments, and agent return behavior
unchanged, and do not use Any.
In `@tests/api/test_opsec_session_scope.py`:
- Line 11: Add the pytest.MonkeyPatch type annotation to the monkeypatch
parameter of test_authorized_scope_reaches_opsec_session, preserving the test’s
existing behavior and satisfying strict MyPy typing.
In `@tests/prompts/test_prompt_templates.py`:
- Around line 267-270: Update the field-value handling in the dataclass
authorization test to verify the value is a string before calling lower(),
mirroring the defensive type check used by the adjacent
test_all_dataclass_fields_non_empty test; only scan lowercase text values for
prohibited phrases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ede42db-7328-4598-bfad-909616696f7f
📒 Files selected for processing (55)
.coderabbit.yaml.env.exampledocs/CHANGELOG.mddocs/api/README.mddocs/configuration/ENVIRONMENT_VARIABLES.mddocs/getting-started/README.mddocs/guides/INTEGRATION.mddocs/operations/SECURITY.mdpentestgpt/agents/advanced_exploitation_agent.pypentestgpt/agents/agent_abstract.pypentestgpt/agents/command_generator.pypentestgpt/agents/exploit_developer.pypentestgpt/agents/opsec_agent.pypentestgpt/agents/repetition_detector.pypentestgpt/agents/results_verifier.pypentestgpt/agents/stealth_operations_agent.pypentestgpt/agents/strategy_agent.pypentestgpt/api/stealth_api.pypentestgpt/config/settings.pypentestgpt/core/engagement.pypentestgpt/core/exceptions.pypentestgpt/core/scope_guard.pypentestgpt/detection_resistance/evasion_engine.pypentestgpt/effectiveness/success_rate_analytics.pypentestgpt/effectiveness/validation_confirmation_system.pypentestgpt/experimental/_mitre_prototype.pypentestgpt/llms/router.pypentestgpt/monitoring/alert_correlation.pypentestgpt/monitoring/detection_intelligence.pypentestgpt/monitoring/detection_monitor.pypentestgpt/monitoring/security_event_analyzer.pypentestgpt/orchestrator.pypentestgpt/prompts/authorization.pypentestgpt/prompts/mitre_prompts.pypentestgpt/prompts/prompt_class.pypentestgpt/prompts/prompt_class_v1.pypentestgpt/prompts/prompt_class_v2.pypentestgpt/report_generator.pypentestgpt/utils/dependency_injection.pypentestgpt/utils/orchestrator_factory.pypentestgpt/utils/strategy_adaptation_controller.pytests/agents/test_exploit_scope.pytests/agents/test_opsec_agent.pytests/api/test_opsec_session_scope.pytests/core/test_engagement.pytests/core/test_scope_guard.pytests/integration/test_scope_guard_lifecycle.pytests/llms/test_router_cache_isolation.pytests/prompts/test_authorization_preamble.pytests/prompts/test_prompt_templates.pytests/prompts/test_route_task_coverage.pytests/regression/test_critical_fixes.pytests/regression/test_fixes_quick.pytests/stealth/test_stealth_components.pytests/utils/test_orchestrator_factory.py
Comment on lines
+302
to
+313
| def test_create_session_audit_uses_configured_session_dir(self, tmp_path: Any) -> None: | ||
| agent = OPSECAgent( | ||
| model_router=make_mock_router(route_task_return=("Mocked response", "openai", 0.01)), | ||
| config=StealthConfig(), | ||
| session_dir=str(tmp_path), | ||
| ) | ||
| session_id = agent.create_session() | ||
| assert agent._scope_guard is not None | ||
| sink = agent._scope_guard.audit_sink | ||
| assert isinstance(sink, JsonlAuditSink) | ||
| assert sink.path == Path(tmp_path) / session_id / "scope_audit.jsonl" | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Verify audit-sink rotation when creating another session.
This only tests the initial session. The current OPSECAgent.create_session() reuses the existing guard’s sink, so a second session writes audit records beneath the first session ID. Recreate the session-scoped sink each time and assert both paths separately.
🤖 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 `@tests/agents/test_opsec_agent.py` around lines 302 - 313, Extend
test_create_session_audit_uses_configured_session_dir to create a second session
and assert its audit sink path is under the second session ID, while retaining
the first path assertion. Update OPSECAgent.create_session and its
session-scoped audit sink setup so each invocation recreates or rotates the sink
for the new session instead of reusing the existing guard’s sink.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.gitignore:
- Line 214: Update the .gitignore entry for the CI-generated Bandit report from
bandit_results.json to bandit-report.json, matching the filename produced by CI.
In `@pentestgpt/core/engagement.py`:
- Around line 249-305: Update command target extraction and
_extract_command_tool to return an explicit completeness/status indicator
alongside targets, marking unsupported or ambiguous parsing as incomplete so the
executor gate denies configured-scope execution rather than relying on existing
authorized targets. Preserve bare hostname targets such as ping
outside.example.test, and extend wrapper parsing to consume argument-bearing
flags like sudo -u root so the actual tool is identified. Update all callers to
enforce the fail-closed status.
- Around line 235-238: Update the ValueError handling in the command
tokenization flow to avoid adding the raw malformed command to the audit target
set. Return a non-sensitive sentinel target for malformed non-empty commands
that still causes ScopeGuard to fail closed, while preserving the empty-command
behavior and normal shlex.split handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30852441-3840-4cf8-a645-95c51a458414
📒 Files selected for processing (28)
.gitignorebandit_results.jsondocs/api/README.mdpentestgpt/agents/command_generator.pypentestgpt/agents/exploit_developer.pypentestgpt/agents/opsec_agent.pypentestgpt/agents/results_verifier.pypentestgpt/agents/strategy_agent.pypentestgpt/api/stealth_api.pypentestgpt/core/engagement.pypentestgpt/core/scope_guard.pypentestgpt/llms/router.pypentestgpt/orchestrator.pypentestgpt/prompts/authorization.pypentestgpt/prompts/prompt_class.pypentestgpt/utils/dependency_injection.pypentestgpt/utils/orchestrator_factory.pytests/agents/test_exploit_scope.pytests/agents/test_opsec_agent.pytests/agents/test_results_verifier.pytests/api/test_opsec_session_scope.pytests/core/test_engagement.pytests/core/test_scope_guard.pytests/integration/test_scope_guard_lifecycle.pytests/llms/test_router_cache_isolation.pytests/prompts/test_authorization_preamble.pytests/prompts/test_prompt_templates.pytests/utils/test_orchestrator_factory.py
🚧 Files skipped from review as they are similar to previous changes (18)
- pentestgpt/api/stealth_api.py
- tests/prompts/test_prompt_templates.py
- pentestgpt/agents/command_generator.py
- tests/prompts/test_authorization_preamble.py
- tests/utils/test_orchestrator_factory.py
- pentestgpt/prompts/authorization.py
- tests/core/test_scope_guard.py
- tests/agents/test_exploit_scope.py
- tests/agents/test_opsec_agent.py
- pentestgpt/utils/dependency_injection.py
- docs/api/README.md
- pentestgpt/orchestrator.py
- pentestgpt/agents/results_verifier.py
- pentestgpt/agents/opsec_agent.py
- pentestgpt/prompts/prompt_class.py
- pentestgpt/agents/exploit_developer.py
- pentestgpt/agents/strategy_agent.py
- tests/integration/test_scope_guard_lifecycle.py
extract_command_targets only validated the second positional of nxc, netexec, crackmapexec, and cme commands, so extra hostname targets in a multi-target command bypassed scope enforcement (e.g. an out-of-scope host alongside an authorized one executed unchecked). Treat these tools as protocol-then-targets and validate positional[1:], and move curl to the all-positional set since every curl positional is a URL target.
OPSECAgent.create_session rebuilt its ScopeGuard unconditionally, so a guard injected by the orchestrator via set_engagement_scope was replaced by a session-scoped one, splitting the unified audit trail and resetting shared last-allowed-target state. Track whether the guard was injected externally and only build a session-scoped guard when the agent owns it.
Replace the hardcoded `assert calls == 40` with a non-zero floor and swap the line-numbered benchmark allowlist for a whole-file entry, so adding or moving an unrelated route_task call no longer breaks the test. The invariant that production route_task calls carry a preamble-bearing system_prompt stays enforced by the violations check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EngagementScopetarget normalization and validationScopeGuardwith JSONL auditingValidation
Summary by CodeRabbit