Skip to content

fix(proxy): pick JSON vs HTML errors by intent, not User-Agent - #3565

Closed
mishushakov wants to merge 1 commit into
mainfrom
ottawa
Closed

mishushakov wants to merge 1 commit into
mainfrom
ottawa

Conversation

@mishushakov

Copy link
Copy Markdown
Member

Extracted from #3389, which bundled this with the CORS fix. This half stands alone.

Problem

The proxy's synthesized error responses (packages/shared/pkg/proxy/template/) chose the HTML error page over JSON by sniffing the User-Agent. A fetch() from page scripts carries the browser's own User-Agent and cannot override it — UA is a forbidden header name — so an SDK call from a browser got the HTML error page where it expects JSON. Accept was ignored entirely:

no UA                                -> 502 application/json
chrome UA                            -> 502 text/html
chrome UA + Accept: application/json -> 502 text/html   <- Accept ignored

Change

Intent decides first, in wantsHtml:

  • Accept prefers JSON (application/json present, text/html absent) → JSON.
  • Sec-Fetch-Mode is cors / no-cors / same-origin, or X-Requested-With is set → JSON. Both are set by the browser, not by the caller; a genuine top-level navigation sends Sec-Fetch-Mode: navigate instead.
  • Otherwise fall back to the existing User-Agent sniff, which now only catches those top-level navigations — which is what the HTML pages are for.

This is one choke point: TemplatedError.HandleError covers all eight error templates.

Tests

packages/shared/pkg/proxy/template/template_test.go — a table over the negotiation matrix (bare browser UA, navigation, Accept: application/json, cross-origin fetch, same-origin fetch, legacy XHR, no UA). Checked to have teeth by reverting wantsHtml back to isBrowser and watching 4 cases fail.

Verified with go test -race ./packages/shared/pkg/proxy/..., make fmt, make lint. The two remaining lint failures (envd/internal/services/process/dup3_other.go, orchestrator/cmd/create-build/proxyport_test.go) are pre-existing on main and platform-related.

🤖 Generated with Claude Code

The proxy's error templates chose the HTML page over JSON by sniffing the
User-Agent. A fetch() from page scripts carries the browser's own
User-Agent and cannot override it — UA is a forbidden header name — so an
SDK call from a browser got the HTML error page where it expects JSON,
and Accept was ignored entirely:

    no UA                                -> 502 application/json
    chrome UA                            -> 502 text/html
    chrome UA + Accept: application/json -> 502 text/html   <- ignored

Intent now decides first: JSON when Accept prefers it, or when
Sec-Fetch-Mode / X-Requested-With mark the request as script-initiated.
UA sniffing stays as the fallback, where it only catches genuine
top-level navigations — which is what the HTML pages are for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

If your organization is eligible for promotional free reviews, this run could not use one — if free runs remain, retrying may succeed without raising the limit.

Once credits are available — or to retry now — reopen this pull request to trigger a review.

@cursor

cursor Bot commented Aug 13, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes how all proxy templated errors are serialized for browser clients; wrong header handling could still return the wrong format for edge-case clients.

Overview
Proxy synthesized errors used User-Agent sniffing to return HTML, so browser fetch() and XHR (which cannot change UA) got HTML instead of JSON even when Accept: application/json was set.

HandleError now uses wantsHtml, which returns JSON when Accept prefers JSON or when Sec-Fetch-Mode / X-Requested-With indicate script-initiated requests, and only falls back to UA sniffing for top-level navigations. Tests cover the negotiation matrix.

Reviewed by Cursor Bugbot for commit a4e309b. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Aug 13, 2026 •

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
4031 1 4030 10
View the top 1 failed test(s) by shortest run time
github.com/e2b-dev/infra/packages/api/internal/sandbox/storage/redis::TestSubscriptionManager_PubSubEndToEnd
Stack Traces | 8.64s run time
=== RUN   TestSubscriptionManager_PubSubEndToEnd
=== PAUSE TestSubscriptionManager_PubSubEndToEnd
=== CONT  TestSubscriptionManager_PubSubEndToEnd
    subscription_manager_test.go:245: 
        	Error Trace:	.../storage/redis/subscription_manager_test.go:245
        	Error:      	did not receive PubSub notification
        	Test:       	TestSubscriptionManager_PubSubEndToEnd
--- FAIL: TestSubscriptionManager_PubSubEndToEnd (8.64s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant