fix: Forward X-Org-Id during automation auth

[malhotra5]

Summary

• Forward incoming X-Org-Id to OpenHands /api/v1/users/me during automation API authentication.
• Include auth method and requested org scope in the auth cache key so one credential can authenticate separately per org.
• Add tests for API-key/cookie X-Org-Id forwarding, invalid org header handling, and cache separation by org.

Fixes #402

Testing

• uv run pytest tests/test_auth.py -q -k 'not TestAuthIntegration' — passed (50 passed, 4 deselected).
• uv run pre-commit run --files openhands/automation/auth.py tests/test_auth.py --show-diff-on-failure — passed.
• uv run pytest tests/test_auth.py -q — unit tests passed, but Docker-backed TestAuthIntegration setup failed because this environment has no Docker socket.

This PR was created by an AI agent (OpenHands) on behalf of the user.

@malhotra5 can click here to continue refining the PR

[github-actions]

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

[enyst]

Taste Rating: 🟡 Acceptable — clean, focused fix for a real bug; one small taste wart.

Review by OpenHands-DeepSeek-Pro (AI agent), on behalf of the user.

---

[IMPROVEMENT OPPORTUNITIES]

• [openhands/automation/auth.py, line 197] _request_has_header — see inline comment. The try/except TypeError exists only to accommodate the MagicMock test fixture, not any real Request behavior (Starlette Headers.__contains__ never raises TypeError, and get(name, default) already returns the default for a missing header). Production code shouldn't carry a special case only a mock needs. Cleaner path: give the mock_request fixture a real starlette.dataclasses.Headers (or Request), which collapses _extract_x_org_id to a single request.headers.get(X_ORG_ID_HEADER, "").strip() and deletes the _request_has_header helper plus the now-redundant if not header_value guard.

Correct parts (no action needed):

• Keying the cache by (auth_method, normalized_org, credential) is right, and incidentally fixes a latent collision where an API key and a cookie sharing the same token string would map to the same cache entry.
• str(uuid.UUID(...)) normalization is good taste — it canonicalizes urn:uuid:, braced, and hex forms so the same org can't fragment the cache.
• The new tests exercise the real authenticate_request path and assert on forwarded headers and resolved org_id/call_count, not just mock-call counts. The mocked HTTP boundary is the legitimate seam here.

[TESTING GAPS]

• Minor: no assertion that X-Org-Id is absent from outbound headers when the incoming request omits it. The existing test_authenticate_valid_api_key would still pass if someone removed the if x_org_id: guard and unconditionally attached the header. One extra assert "X-Org-Id" not in headers would lock the guard in.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ ✅ LOW

The behavior change is additive (only acts when X-Org-Id is present), the cache-key change is a pure in-memory format change, and the only call site of _credential_cache_key is this file. The one thing a human should confirm is the security boundary: the fix relies on OpenHands /api/v1/users/me rejecting a request when the supplied X-Org-Id disagrees with the credential's bound org (api_key_org_id). That is the property that stops a user from passing another org's UUID to read its automations. The frontend client.ts comment in the OpenHands repo confirms this rejection exists upstream, so I'm treating it as verified rather than blocking — but it is the load-bearing invariant this PR rests on.

VERDICT: ✅ Worth merging.

KEY INSIGHT: The real contribution isn't the header forwarding — it's recognizing that the auth cache key must encode the full authentication scope (method + org), not just the credential, or org scoping silently leaks across requests.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[enyst]

This try/except TypeError exists only so the MagicMock fixture works, not because any real Request raises it. In production request.headers is a Starlette Headers, whose __contains__ returns a real bool and whose get(name, default) already returns the default for a missing header — so the whole helper is mock-specific machinery leaking into production code.

Cleaner: give the mock_request fixture a real starlette.dataclasses.Headers (or a real Request), then collapse _extract_x_org_id to a single request.headers.get(X_ORG_ID_HEADER, "").strip(), drop _request_has_header entirely, and the if not header_value guard becomes redundant. That removes a helper, an exception handler, and a null-check — a net deletion that also makes the tests exercise the same header semantics as prod.