fix: inject runtime session key into index.html for published binary#795
Conversation
The globally installed agent-canvas binary starts the agent-server with a
persisted session API key (~/.openhands/agent-canvas/session-api-key.txt)
as OH_SESSION_API_KEYS_0, making auth required. However, the pre-built
static frontend in the npm package has a different (or empty)
VITE_SESSION_API_KEY baked in at publish time, so every API request gets
401 Unauthorized.
Fix: static-server.mjs now accepts --session-api-key <key> and injects a
tiny bootstrap <script> before </head> in every index.html response. The
script seeds the key into localStorage['openhands-agent-server-config']
only if no key is already stored there, so explicit user overrides (via
Settings > Agent Server) are always preserved.
dev-with-automation.mjs and dev-static.mjs both pass
--session-api-key ${config.sessionApiKey} when spawning the static server,
so the runtime key is always available regardless of what was baked into
the bundle.
Tests: added 8 new cases to __tests__/scripts/static-server.test.ts
covering parseArgs, injection in direct and SPA-fallback index.html
responses, no injection for non-html assets, cache headers, and null key.
Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…n authenticate The entrypoint computed EFFECTIVE_SESSION_KEY and forwarded it to the agent-server (OH_SESSION_API_KEYS_0) and automation backends, but did not pass it to the static-server. As a result the pre-built index.html served with no session key injected, so every browser API call received 401. Wire --session-api-key "$EFFECTIVE_SESSION_KEY" into the static-server launch command so the runtime key is injected into index.html responses (via the mechanism added in this branch to static-server.mjs). Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste
Elegant fix for published binary auth failures. Runtime key injection preserves user overrides, includes comprehensive test coverage (8 new cases covering arg parsing, direct/SPA injection, cache headers, null handling), and uses proper security practices (JSON.stringify for escaping, try/catch for graceful degradation).
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Bug fix with comprehensive test coverage, backward compatible (only injects if key not already set), no breaking changes. Cache-Control: no-cache prevents stale keys. Injection wrapped in try/catch for safe localStorage access.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26487809286
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review — fix: inject runtime session key into index.html for published binary
Solid, targeted fix for the published binary auth problem. The overall approach is well-executed:
JSON.stringifyfor safe key escaping — no XSS risk from the key value itself- localStorage guard (
if(!_c.sessionApiKey)) correctly preserves explicit user overrides Cache-Control: no-cacheon injected responses prevents browsers from caching the stale (un-injected) pageContent-Lengthusesbuf.length(byte count) rather than character count — correct for UTF-8- Test coverage is thorough and the 8 new cases cover the meaningful paths
A couple of minor points inline.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
@OpenHands, address the all-hands-bot review suggestions |
|
I'm on it! chuckbutkus can track my progress at all-hands.dev |
- static-server.mjs: add comment clarifying replace() targets first </head> only; fall back to inserting before </body> when </head> is absent (avoids prepending before <!DOCTYPE html>) - docker/entrypoint.sh: add comment documenting source of EFFECTIVE_SESSION_KEY before the static-server invocation - static-server.test.ts: add test for </head>-absent fallback path confirming injection lands before </body> Co-authored-by: openhands <openhands@all-hands.dev>
Summary of ChangesAll four ✅ Completeness
✅ ConcisenessThe changes are minimal and surgical — exactly 3 files touched, with only what was needed to satisfy the review feedback. No unrelated changes were made. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste
Elegant fix for the published binary auth issue. The runtime key injection is minimal, secure, and well-tested.
Highlights:
- Solves real production problem (globally installed binaries getting 401s)
- Security best practices:
JSON.stringifyescaping,Cache-Control: no-cache, try/catch graceful degradation - Preserves user overrides (
if(!_c.sessionApiKey)guard) - Comprehensive test coverage (8 new cases covering parseArgs, injection paths, cache headers, edge cases)
- All previous review comments addressed ( fallback, entrypoint comment, replace() clarification)
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Bug fix with comprehensive test coverage, backward compatible (only injects when key provided, preserves user-set overrides), no breaking changes, secure implementation (proper escaping + cache control), all integration points updated.
VERDICT:
✅ Worth merging: Solves a real production bug with a clean, minimal implementation
KEY INSIGHT:
The build-time vs. runtime key mismatch is solved at exactly the right layer — serve-time injection lets the pre-built bundle work with any runtime key without rebuilding.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26491108264
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste
Clean, targeted fix for the published binary auth issue. The review_requested event triggered this review; all prior inline feedback has been addressed by the author.
Summary of changes:
scripts/static-server.mjs: Adds--session-api-key <key>CLI flag.makeConfigInjectionScriptemits a minimal IIFE that seeds the runtime key intolocalStorage['openhands-agent-server-config']only if no key is already present — user overrides via Settings are always preserved.serveInjectedIndexHtmlinjects the script before</head>, falling back to</body>, then a last-resort prepend.Cache-Control: no-cacheis set on injected responses;Content-Lengthis computed from the byte buffer (not character count) — both correct.docker/entrypoint.sh: Passes$EFFECTIVE_SESSION_KEY(defined at line 84 fromsession-api-key.txtor$SESSION_API_KEY) to the static server. An explanatory comment was added (inline shell comments after\continuation are invalid bash syntax, so the comment precedes thenodecall — correct choice).scripts/dev-static.mjs/dev-with-automation.mjs: Both conditionally spread--session-api-keyonly whenconfig.sessionApiKeyis truthy — no behavioral change when the key is absent.- Tests: 8 new cases covering
parseArgsdefaults,--session-api-keyparsing, empty-string coercion to null, direct injection, SPA-fallback injection, non-HTML asset passthrough (no injection),Cache-Control: no-cacheheader, null-key behavior, and the</body>-fallback path for HTML without</head>.
Prior inline comments — all resolved:
| Comment | Resolution |
|---|---|
</head>-absent fallback prepended before <!DOCTYPE html> |
Fixed: intermediate </body> fallback added; test added |
String.replace() replaces first occurrence — add comment |
Fixed: clarifying comment added above the replace() call |
EFFECTIVE_SESSION_KEY definition not visible in diff context |
Fixed: comment added; variable confirmed defined at line 84 |
Test for </head>-absent path missing |
Fixed: test added with <html><body>no-head</body></html> input |
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Backward compatible bug fix — no injection occurs unless--session-api-keyis explicitly passed, and the localStorage guard prevents overwriting user-set keys.JSON.stringifyescaping is correct (the key is system-generated, alphanumeric). Comprehensive test coverage.
VERDICT:
✅ Worth merging: Solves a real production 401 problem with a clean, minimal implementation. All prior review feedback addressed.
KEY INSIGHT:
Serve-time key injection is the correct layer to fix the build-time vs. runtime key mismatch — the pre-built bundle remains static; the runtime key is woven in at request time without rebuilding.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste
Clean, targeted fix for the published binary auth issue. The review_requested event triggered this review; all prior inline feedback has been addressed by the author.
Summary of changes:
scripts/static-server.mjs: Adds--session-api-key <key>CLI flag.makeConfigInjectionScriptemits a minimal IIFE that seeds the runtime key intolocalStorage['openhands-agent-server-config']only if no key is already present — user overrides via Settings are always preserved.serveInjectedIndexHtmlinjects the script before</head>, falling back to</body>, then a last-resort prepend.Cache-Control: no-cacheis set on injected responses;Content-Lengthis computed from the byte buffer (not character count) — both correct.docker/entrypoint.sh: Passes$EFFECTIVE_SESSION_KEY(defined at line 84 fromsession-api-key.txtor$SESSION_API_KEY) to the static server. An explanatory comment was added (inline shell comments after backslash continuation are invalid bash syntax, so the comment precedes thenodecall — correct choice).scripts/dev-static.mjs/dev-with-automation.mjs: Both conditionally spread--session-api-keyonly whenconfig.sessionApiKeyis truthy — no behavioral change when the key is absent.- Tests: 8 new cases covering
parseArgsdefaults,--session-api-keyparsing, empty-string coercion to null, direct injection, SPA-fallback injection, non-HTML asset passthrough (no injection),Cache-Control: no-cacheheader, null-key behavior, and the</body>-fallback path for HTML without</head>.
Prior inline comments — all resolved:
| Comment | Resolution |
|---|---|
</head>-absent fallback prepended before <!DOCTYPE html> |
Fixed: intermediate </body> fallback added; test added |
String.replace() replaces first occurrence — add clarifying comment |
Fixed: comment added above the replace() call |
EFFECTIVE_SESSION_KEY definition not visible in diff context |
Fixed: comment added; variable confirmed defined at line 84 |
Test for </head>-absent path missing |
Fixed: test added with <html><body>no-head</body></html> input |
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Backward compatible bug fix — no injection occurs unless--session-api-keyis explicitly passed, and the localStorage guard prevents overwriting user-set keys.JSON.stringifyescaping is correct (the key is system-generated, alphanumeric). Comprehensive test coverage.
VERDICT:
✅ Worth merging: Solves a real production 401 problem with a clean, minimal implementation. All prior review feedback addressed.
KEY INSIGHT:
Serve-time key injection is the correct layer to fix the build-time vs. runtime key mismatch — the pre-built bundle remains static; the runtime key is woven in at request time without rebuilding.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
malhotra5
left a comment
malhotra5
left a comment
There was a problem hiding this comment.
LGTM
I'm guessing this worked for some people because their localstorage already had session api key stored from prior sessions?
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
|
@OpenHands can you figure out why the app was working for some people and not for others? understand the fix made here in order to determine this |
|
I'm on it! malhotra5 can track my progress at all-hands.dev |
Why It Worked for Some People and Not Others(This comment was generated by an AI agent, OpenHands, on behalf of the user.) The bug came down to a single asymmetry: The two code paths
|
| Scenario | Why it worked |
|---|---|
User had previously run npm run dev |
The correct key was already stored in localStorage from a prior dev session. The static binary re-used the same persisted session-api-key.txt, so the agent-server key matched what was in the browser's storage. |
| User had manually configured the key in Settings > Agent Server | The UI's settings form writes directly to localStorage['openhands-agent-server-config'].sessionApiKey, which takes priority over the baked-in empty value. |
| Agent-server was started with no auth requirement | Only if OH_SESSION_API_KEYS_0 was somehow not set (never happens in practice because the launcher always generates one). |
What the fix does
static-server.mjs now accepts --session-api-key <key> and injects a tiny bootstrap <script> before </head> in every index.html response:
(function(){
var _k = 'openhands-agent-server-config',
_c = JSON.parse(localStorage.getItem(_k) || '{}');
if (!_c.sessionApiKey) { // ← only seed if not already set
_c.sessionApiKey = "<runtime-key>";
localStorage.setItem(_k, JSON.stringify(_c));
}
}());This closes the gap: on the very first page load (empty localStorage), the correct runtime key is seeded into storage before any app code runs. Subsequent requests use that key and authenticate successfully. The if (!_c.sessionApiKey) guard means any key the user has explicitly configured through the UI is never overwritten.
The Docker entrypoint and both static-mode launcher scripts (dev-with-automation.mjs, dev-static.mjs) were updated to pass --session-api-key ${effectiveSessionKey} to the static server, completing the chain.
SummaryThis was a question to answer, not a code change request. Here's what I did: Task: Explain why the app worked for some users and not others, based on understanding the fix in PR #795. Answer delivered (posted as a comment on PR #795): The root cause was a build-time vs. runtime key mismatch with a single asymmetry at its heart:
Who escaped the bug without the fix:
The fix closes the gap by having |
4 participants
The globally installed agent-canvas binary starts the agent-server with a persisted session API key (~/.openhands/agent-canvas/session-api-key.txt) as OH_SESSION_API_KEYS_0, making auth required. However, the pre-built static frontend in the npm package has a different (or empty) VITE_SESSION_API_KEY baked in at publish time, so every API request gets 401 Unauthorized.
Fix: static-server.mjs now accepts --session-api-key and injects a tiny bootstrap <script> before in every index.html response. The script seeds the key into localStorage['openhands-agent-server-config'] only if no key is already stored there, so explicit user overrides (via Settings > Agent Server) are always preserved.
dev-with-automation.mjs and dev-static.mjs both pass --session-api-key ${config.sessionApiKey} when spawning the static server, so the runtime key is always available regardless of what was baked into the bundle.
Tests: added 8 new cases to tests/scripts/static-server.test.ts covering parseArgs, injection in direct and SPA-fallback index.html responses, no injection for non-html assets, cache headers, and null key.
Issue Number
#796
How to Test
-p 8000:8000
-v ~/.openhands:/home/openhands/.openhands
-v ${PROJECTS_PATH}:/projects
ghcr.io/openhands/agent-canvas:1.0.0-alpha.6
You should see 401s during the onboarding screens.
-p 8000:8000
-v ~/.openhands:/home/openhands/.openhands
-v ${PROJECTS_PATH}:/projects
ghcr.io/openhands/agent-canvas:sha-a44a808
Now it should startup without any issues.
Video/Screenshots
Type
Notes
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.23.1-pythonopenhands-automation==1.0.0a50190f14670341e5350ce6f6672f23e590b73ce8aPull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-0190f14Run
All tags pushed for this build
About Multi-Architecture Support
sha-0190f14) is a multi-arch manifest supporting both amd64 and arm64sha-0190f14-amd64) are also available if needed