OpenHands / OpenHands/enterprise

Codex credential preflight is bypassed by every integration-triggered conversation start

Open
#352 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Python
Stars
4
Forks
2
Avg merge
1d 22h
Merged PRs (30d)
101

Description

Actual Behavior

_validate_codex_credentials runs only in the two HTTP conversation-start endpoints. It is reachable exclusively through _validate_acp_start:

openhands/app_server/app_conversation/app_conversation_router.py:230   await _validate_codex_credentials(...)
openhands/app_server/app_conversation/app_conversation_router.py:534   await _validate_acp_start(start_request, ...)   # POST /api/v1/app-conversations
openhands/app_server/app_conversation/app_conversation_router.py:1199  await _validate_acp_start(request, ...)         # POST /stream-start

LiveStatusAppConversationService has no equivalent check — grep -rn 'is_valid_codex_auth\|CODEX_AUTH_JSON' over the service returns nothing. But eight integrations call app_conversation_service.start_app_conversation directly, never touching the router:

integrations/{github,gitlab,bitbucket,bitbucket_data_center,jira,jira_dc,slack,azure_devops}/*_view.py

So a user whose saved agent settings select acp_server='codex' without a usable CODEX_AUTH_JSON / OPENAI_API_KEY / CODEX_API_KEY gets:

  • from the UI → a clean 400: "Connect your Codex account or set an API key before starting a Codex conversation."
  • from a GitHub/Jira/Slack/... trigger → a provisioned sandbox, then an opaque failure inside the ACP handshake

The runtime symptom is the ACP server rejecting the session rather than anything naming credentials. Reproduced against the real codex-acp binary with a deliberately invalid auth blob:

RequestError: Authentication required

The sandbox is already running by then, so the cost is paid and the error surfaces far from its cause.

This is the same defect class as the ACP provider allowlist fixed in #336 — a guard placed only in the endpoints is not a gate, because the integrations are a second entry point into the same start path. #336 moved the provider check into the service; this one was left in place deliberately to keep that PR scoped.

Reproduction
# 1. Save Codex agent settings with no Codex credential
curl -sX POST localhost:PORT/api/v1/settings -H 'Content-Type: application/json' \
  -d '{"agent_settings_diff":{"agent_kind":"acp","acp_server":"codex","acp_command":[],"acp_args":[]}}'

# 2. Start via HTTP  -> 400, no sandbox
curl -sX POST localhost:PORT/api/v1/app-conversations -d '{}'

# 3. Start via any integration view (which calls the service directly)
#    -> sandbox provisioned, then an opaque ACP handshake failure
uv run pytest tests/unit/app_server/test_live_status_app_conversation_service.py -k codex

Desired Behavior

The Codex credential preflight runs wherever a conversation starts, not only on the HTTP endpoints — so an integration-triggered start fails fast with the same actionable message and never provisions a sandbox.

The natural home is next to the provider allowlist check added in #336: LiveStatusAppConversationService._start_app_conversation, after _apply_suggested_task and before _wait_for_sandbox_start. It needs a SecretsStore, which the service does not currently hold — that is the design question worth settling here rather than in a rushed patch.

Raising a subclass of OpenHandsError (itself an HTTPException) keeps the endpoints returning the identical 400 while giving integrations a real exception, which is the pattern ACPProviderNotAvailableError already uses.

Acceptance Criteria

  • A Codex conversation started directly through AppConversationService.start_app_conversation with no usable credential is rejected before any sandbox is provisioned
  • The rejection carries the same actionable message as the HTTP path
  • The existing HTTP-endpoint behaviour is unchanged (still a 400 with that message)
  • Regression test drives the service directly with _wait_for_sandbox_start stubbed to raise, asserting it is never called — mirroring TestACPProviderAllowlistAtServiceStart
  • Whatever mechanism supplies the SecretsStore to the service is covered, so the check cannot silently no-op when the store is absent

Related: #336 · OpenHands/software-agent-sdk#4841

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with LiveStatusAppConversationService._start_app_conversation and the provider allowlist test TestACPProviderAllowlistAtServiceStart. Compare the router checks in app_conversation_router.py with direct calls from the eight integration *_view.py files, then run the Codex-focused service tests. Done means direct starts fail before sandbox provisioning with the existing actionable message, while HTTP behavior remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.