Skip to content

Commit d259934

Browse files
committed
fix(opencode): fail open on empty token output, guard missing output.headers (review)
Addresses two Copilot review comments on #197: - fetchToken() returned stdout.trim() unconditionally. If `ucode auth-token` ever exits 0 with empty/whitespace-only stdout, the plugin would cache and send Authorization: Bearer <empty>, clobbering a possibly-still-valid bootstrap token for the full TTL window instead of failing open. Empty output now throws, so the hook's catch block fails open as intended. - The chat.headers hook assumed output.headers always exists. If opencode ever invokes the hook without a pre-populated headers object, the assignment would throw -- caught silently by the existing try/catch, but the refresh would never apply. Defensively initialize output.headers before writing to it. Both are proven by new Node-executed runtime tests (matching the class of test that caught the input.provider?.id bug): each new test was verified to fail against the pre-fix code and pass against the fix, not just pass trivially.
1 parent 7fce618 commit d259934

2 files changed

Lines changed: 54 additions & 8 deletions

File tree

src/ucode/agents/opencode.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,14 @@ def render_auth_plugin(workspace: str, profile: str | None, provider_ids: list[s
222222
async function fetchToken() {{
223223
const [cmd, ...args] = UCODE_AUTH_TOKEN_ARGV;
224224
const {{ stdout }} = await execFileAsync(cmd, args);
225-
return stdout.trim();
225+
const token = stdout.trim();
226+
if (!token) {{
227+
// `ucode auth-token` exited 0 but printed nothing usable -- treat as a
228+
// failure so the caller's catch block fails open to the static
229+
// bootstrap token instead of caching/sending an empty bearer.
230+
throw new Error("ucode auth-token returned empty output");
231+
}}
232+
return token;
226233
}}
227234
228235
async function getToken() {{
@@ -259,6 +266,11 @@ def render_auth_plugin(workspace: str, profile: str | None, provider_ids: list[s
259266
}}
260267
try {{
261268
const token = await getToken();
269+
// Defensive: ensure `output.headers` exists even if this opencode
270+
// version ever invokes the hook without a pre-populated headers
271+
// object, so the header still gets set rather than throwing (which
272+
// would be caught below and silently no-op the refresh).
273+
output.headers = output.headers || {{}};
262274
output.headers["Authorization"] = "Bearer " + token;
263275
}} catch (err) {{
264276
console.error("[ucode] failed to refresh Databricks token:", err);

tests/test_agent_opencode.py

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -519,7 +519,13 @@ class TestRenderAuthPluginRuntimeBehavior:
519519
type surface and an isolated end-to-end run against opencode 1.17.10
520520
(#190 follow-up). Skips if `node` isn't on PATH."""
521521

522-
def _run_plugin(self, tmp_path, provider_id: str | None, fake_token: str = "fresh-token-xyz"):
522+
def _run_plugin(
523+
self,
524+
tmp_path,
525+
provider_id: str | None,
526+
fake_token: str | None = "fresh-token-xyz",
527+
headers_preset: bool = True,
528+
):
523529
import shutil
524530
import subprocess
525531

@@ -528,10 +534,11 @@ def _run_plugin(self, tmp_path, provider_id: str | None, fake_token: str = "fres
528534
pytest.skip("`node` is not installed")
529535

530536
fake_ucode = tmp_path / "fake_ucode.py"
531-
fake_ucode.write_text(
532-
"#!/usr/bin/env python3\nimport sys\nprint(sys.argv[-1] if False else "
533-
f"{fake_token!r})\n"
534-
)
537+
# `fake_token=None` simulates `ucode auth-token` exiting 0 but
538+
# printing nothing (or only whitespace) -- a successful subprocess
539+
# with unusable output.
540+
print_stmt = f"print({fake_token!r})" if fake_token is not None else 'print(" ")'
541+
fake_ucode.write_text(f"#!/usr/bin/env python3\n{print_stmt}\n")
535542
fake_ucode.chmod(0o755)
536543

537544
with patch(
@@ -544,12 +551,17 @@ def _run_plugin(self, tmp_path, provider_id: str | None, fake_token: str = "fres
544551
plugin_path.write_text(js)
545552

546553
provider_literal = f"{{ id: {provider_id!r} }}" if provider_id else "undefined"
554+
output_literal = (
555+
'{ headers: { Authorization: "Bearer STALE-BOOTSTRAP-TOKEN" } }'
556+
if headers_preset
557+
else "{}"
558+
)
547559
harness = tmp_path / "harness.mjs"
548560
harness.write_text(f"""
549561
import {{ UcodeDatabricksAuth }} from {str(plugin_path.as_posix())!r};
550562
551563
const hooks = await UcodeDatabricksAuth({{}});
552-
const output = {{ headers: {{ Authorization: "Bearer STALE-BOOTSTRAP-TOKEN" }} }};
564+
const output = {output_literal};
553565
const input = {{
554566
sessionID: "ses_test",
555567
agent: "build",
@@ -558,7 +570,7 @@ def _run_plugin(self, tmp_path, provider_id: str | None, fake_token: str = "fres
558570
message: {{}},
559571
}};
560572
await hooks["chat.headers"](input, output);
561-
console.log(JSON.stringify(output.headers));
573+
console.log(JSON.stringify(output.headers ?? null));
562574
""")
563575

564576
result = subprocess.run(
@@ -589,6 +601,28 @@ def test_missing_provider_leaves_static_header_untouched(self, tmp_path):
589601
headers = json.loads(result.stdout.strip().splitlines()[-1])
590602
assert headers["Authorization"] == "Bearer STALE-BOOTSTRAP-TOKEN"
591603

604+
def test_empty_token_output_fails_open_instead_of_setting_empty_bearer(self, tmp_path):
605+
# `ucode auth-token` exiting 0 with empty/whitespace-only stdout must
606+
# not be treated as a successful fetch -- otherwise the hook would
607+
# cache and send `Authorization: Bearer ` (empty), clobbering a
608+
# possibly-still-valid bootstrap token for the full TTL window.
609+
result = self._run_plugin(tmp_path, provider_id="databricks-anthropic", fake_token=None)
610+
assert result.returncode == 0, result.stderr
611+
headers = json.loads(result.stdout.strip().splitlines()[-1])
612+
assert headers["Authorization"] == "Bearer STALE-BOOTSTRAP-TOKEN"
613+
614+
def test_initializes_missing_output_headers_instead_of_throwing(self, tmp_path):
615+
# If opencode ever invokes the hook with no pre-populated
616+
# `output.headers`, the hook must still set Authorization rather
617+
# than throwing (which the try/catch would otherwise silently
618+
# swallow, no-opping the refresh).
619+
result = self._run_plugin(
620+
tmp_path, provider_id="databricks-anthropic", headers_preset=False
621+
)
622+
assert result.returncode == 0, result.stderr
623+
headers = json.loads(result.stdout.strip().splitlines()[-1])
624+
assert headers["Authorization"] == "Bearer fresh-token-xyz"
625+
592626

593627
class TestWriteAuthPlugin:
594628
def test_writes_plugin_file_containing_hook(self, tmp_path, monkeypatch):

0 commit comments

Comments
 (0)