Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/agent-server-openapi-weak-schema-allowlist.json
Original file line number Diff line number Diff line change
Expand Up @@ -592,5 +592,11 @@
"kind": "empty-object-schema",
"reason": "This existing endpoint has an opaque or non-JSON response contract and is tracked by the weak-type ratchet.",
"owner": "OpenHands OSS"
},
{
"pointer": "/components/schemas/SecurityAnalysisEvent/properties/details/additionalProperties/additionalProperties",
"kind": "unrestricted-additional-properties",
"reason": "Per-action analyzer detail (probabilities, confidence, rationale) is analyzer-specific by design; the shape belongs to each SecurityAnalyzer implementation.",
"owner": "OpenHands OSS"
}
]
29 changes: 23 additions & 6 deletions openhands-sdk/openhands/sdk/agent/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
CondensationRequest,
)
from openhands.sdk.event.error_classification import AGENT_OUTCOME
from openhands.sdk.event.security import SecurityAnalysisEvent
from openhands.sdk.llm import (
LLM,
ImageContent,
Expand Down Expand Up @@ -1044,7 +1045,10 @@ async def _astep(
)

def _requires_user_confirmation(
self, state: ConversationState, action_events: list[ActionEvent]
self,
state: ConversationState,
action_events: list[ActionEvent],
on_event: ConversationCallbackType | None = None,
) -> bool:
"""
Decide whether user confirmation is needed to proceed.
Expand All @@ -1068,12 +1072,25 @@ def _requires_user_confirmation(
# If a security analyzer is registered, use it to grab the risks of the actions
# involved. If not, we'll set the risks to UNKNOWN.
if state.security_analyzer is not None:
risks = [
risk
for _, risk in state.security_analyzer.analyze_pending_actions(
action_events
analyses = state.security_analyzer.analyze_actions(action_events)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical [P1]: preserve existing batch-analyzer overrides when changing the dispatch API.

This replaces the call to analyze_pending_actions() with analyze_actions(). The compatibility wrapper only preserves callers of the old method, not custom analyzers that override it: the agent now silently ignores their batch verdicts.

I reproduced this in live Haiku-driven conversations on base and head with an otherwise unchanged analyzer:

class LegacyBatchAnalyzer(SecurityAnalyzerBase):
    def security_risk(self, action):
        return SecurityRisk.LOW

    def analyze_pending_actions(self, pending_actions):
        return [(action, SecurityRisk.HIGH) for action in pending_actions]

Under ConfirmRisky(), base stops at waiting_for_confirmation with 0 file writes. This head finishes with 1 actual file write, and the new audit says LOW, even though calling that same analyzer's batch method still returns HIGH. This can bypass confirmation for existing custom integrations that implement batch/context-dependent assessment.

Please preserve the old override contract when bridging to detailed analyses (without analyzing twice), or provide an explicit migration rather than silently selecting a different verdict source. Add a regression exercising a legacy batch override through agent dispatch and asserting both the confirmation outcome and the emitted verdict.

AI-generated feedback from OpenHands on behalf of the requesting user.

risks = [analysis.risk for _, analysis in analyses]
# Record the verdict regardless of what the policy does with it, so
# the log and UI show what the analyzer said even under NeverConfirm.
if on_event is not None:
on_event(
SecurityAnalysisEvent(
analyzer=state.security_analyzer.__class__.__name__,
policy=state.confirmation_policy.__class__.__name__,
risks={
action.id: analysis.risk for action, analysis in analyses
},
details={
action.id: analysis.details
for action, analysis in analyses
if analysis.details is not None
},
Comment on lines +1087 to +1091

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical [P1]: mask registered secrets before emitting analyzer details.

analysis.details is passed directly into an event that the callback chain persists and publishes; the exception fallback now also puts str(e) in these details. There is no general secret-masking step in that callback chain.

Reproduced using only a synthetic value: register QA_SYNTHETIC="qa-fake-secret-5182-not-real", then have an analyzer raise RuntimeError("upstream rejected credential qa-fake-secret-5182-not-real"). On this head, the callback and the event read back after closing/reopening both contain the literal value in details[action_id]["error"]. The registry's own mask_secrets_in_output() correctly returns <secret-hidden> for it; base did not put this error text in the event log. Backend request errors and analyzer rationales can contain credential-bearing URLs/headers, so this creates a new durable/client-visible disclosure path.

Please apply the conversation secret registry's recursive model masking to the completed SecurityAnalysisEvent before on_event, covering both successful details and exception details. Add coverage checking callback payloads and persisted/reopened events, while preserving the HIGH fallback.

AI-generated feedback from OpenHands on behalf of the requesting user.

)
)
]
else:
risks = [risk.SecurityRisk.UNKNOWN] * len(action_events)

Expand Down
5 changes: 3 additions & 2 deletions openhands-sdk/openhands/sdk/agent/response_dispatch.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ def _requires_user_confirmation(
self,
state: ConversationState,
action_events: list[ActionEvent],
on_event: ConversationCallbackType | None = None,
) -> bool: ...

def _maybe_emit_vllm_tokens(
Expand Down Expand Up @@ -183,7 +184,7 @@ def _handle_tool_calls(
continue
action_events.append(action_event)

if self._requires_user_confirmation(state, action_events):
if self._requires_user_confirmation(state, action_events, on_event):
return

if action_events:
Expand Down Expand Up @@ -237,7 +238,7 @@ async def _ahandle_tool_calls(
continue
action_events.append(action_event)

if self._requires_user_confirmation(state, action_events):
if self._requires_user_confirmation(state, action_events, on_event):
return

if action_events:
Expand Down
2 changes: 2 additions & 0 deletions openhands-sdk/openhands/sdk/event/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
RESUME_CONTEXT_MARKER,
render_resume_transcript,
)
from openhands.sdk.event.security import SecurityAnalysisEvent
from openhands.sdk.event.streaming_delta import StreamingDeltaEvent
from openhands.sdk.event.token import TokenEvent
from openhands.sdk.event.types import EventID, ToolCallID
Expand All @@ -46,6 +47,7 @@
"StreamingDeltaEvent",
"Condensation",
"CondensationRequest",
"SecurityAnalysisEvent",
"CondensationSummaryEvent",
"ConversationStateUpdateEvent",
"HookExecutionEvent",
Expand Down
48 changes: 48 additions & 0 deletions openhands-sdk/openhands/sdk/event/security.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
from typing import Any

from pydantic import Field
from rich.text import Text

from openhands.sdk.event.base import Event
from openhands.sdk.event.types import EventID, SourceType
from openhands.sdk.security.risk import SecurityRisk


class SecurityAnalysisEvent(Event):
"""Risk levels the configured security analyzer assigned to pending actions.

Emitted once per batch of actions the agent is about to execute, whenever a
security analyzer is configured on the conversation, regardless of whether
the confirmation policy then asks for confirmation. This makes the
analyzer's verdict visible in the event log and UI even when the policy
(for example ``NeverConfirm``) never consults it.

This event is not shown to the LLM.
"""

source: SourceType = "environment"

analyzer: str = Field(
description="Kind of the security analyzer that produced these risks."
)
policy: str = Field(
description="Kind of the confirmation policy in force when the actions "
"were analyzed."
)
risks: dict[EventID, SecurityRisk] = Field(
description="Risk level per analyzed action, keyed by ActionEvent id."
)
details: dict[EventID, dict[str, Any]] = Field(
default_factory=dict,
description="Optional analyzer-specific detail per action, keyed by "
"ActionEvent id (for example probabilities, confidence, or a rationale).",
)

@property
def visualize(self) -> Text:
text = Text()
text.append("Security Analysis\n", style="bold")
text.append(f"analyzer={self.analyzer} policy={self.policy}\n")
for action_id, risk in self.risks.items():
text.append(f" {action_id}: {risk.value}\n")
return text
3 changes: 2 additions & 1 deletion openhands-sdk/openhands/sdk/security/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from openhands.sdk.security.analyzer import SecurityAnalyzerBase
from openhands.sdk.security.analyzer import SecurityAnalysis, SecurityAnalyzerBase
from openhands.sdk.security.confirmation_policy import (
AlwaysConfirm,
ConfirmationPolicyBase,
Expand Down Expand Up @@ -28,6 +28,7 @@

__all__ = [
"SecurityRisk",
"SecurityAnalysis",
"SecurityAnalyzerBase",
"LLMSecurityAnalyzer",
"ToolShieldLLMSecurityAnalyzer",
Expand Down
70 changes: 54 additions & 16 deletions openhands-sdk/openhands/sdk/security/analyzer.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
from abc import ABC, abstractmethod
from typing import Any

from pydantic import BaseModel, Field

from openhands.sdk.event.base import Event
from openhands.sdk.event.llm_convertible import ActionEvent
Expand All @@ -12,6 +15,17 @@
logger = get_logger(__name__)


class SecurityAnalysis(BaseModel):
"""Result of analyzing one action: a risk level plus optional detail."""

risk: SecurityRisk
details: dict[str, Any] | None = Field(
default=None,
description="Analyzer-specific detail behind the risk (probabilities, "
"confidence, rationale, ...). Surfaced in SecurityAnalysisEvent.",
)


class SecurityAnalyzerBase(DiscriminatedUnionMixin, ABC):
"""Abstract base class for security analyzers.

Expand Down Expand Up @@ -82,30 +96,54 @@ def should_require_confirmation(
# LOW and MEDIUM risk actions don't require confirmation by default
return False

def analyze_action(self, action: ActionEvent) -> SecurityAnalysis:
"""Analyze one action, returning its risk and optional detail.

The default implementation wraps :meth:`security_risk` with no detail.
Analyzers that can explain their verdict (probabilities, confidence, a
rationale) should override this instead of ``security_risk``.
"""
return SecurityAnalysis(risk=self.security_risk(action))

def analyze_actions(
self, pending_actions: list[ActionEvent]
) -> list[tuple[ActionEvent, SecurityAnalysis]]:
"""Analyze pending actions, returning (action, analysis) pairs.

An analyzer error defaults that action to HIGH risk, with the error
recorded in the analysis detail.
"""
analyzed: list[tuple[ActionEvent, SecurityAnalysis]] = []
for action_event in pending_actions:
try:
analysis = self.analyze_action(action_event)
logger.debug(
f"Action {action_event} analyzed with risk level: {analysis.risk}"
)
except Exception as e:
logger.error(f"Error analyzing action {action_event}: {e}")
# Default to HIGH risk on analysis error for safety
analysis = SecurityAnalysis(
risk=SecurityRisk.HIGH, details={"error": str(e)}
)
analyzed.append((action_event, analysis))
return analyzed

def analyze_pending_actions(
self, pending_actions: list[ActionEvent]
) -> list[tuple[ActionEvent, SecurityRisk]]:
"""Analyze all pending actions in a conversation.

This method gets all unmatched actions from the conversation state
and analyzes each one for security risks.
Compatibility form of :meth:`analyze_actions` that returns only the
risk level for each action.

Args:
conversation: The conversation to analyze
pending_actions: The unmatched actions to analyze

Returns:
List of tuples containing (action, risk_level) for each pending action
"""
analyzed_actions = []

for action_event in pending_actions:
try:
risk = self.security_risk(action_event)
analyzed_actions.append((action_event, risk))
logger.debug(f"Action {action_event} analyzed with risk level: {risk}")
except Exception as e:
logger.error(f"Error analyzing action {action_event}: {e}")
# Default to HIGH risk on analysis error for safety
analyzed_actions.append((action_event, SecurityRisk.HIGH))

return analyzed_actions
return [
(action, analysis.risk)
for action, analysis in self.analyze_actions(pending_actions)
]
Loading
Loading