fix(check): no panic on non-JSON response body#298
Merged
Conversation
CheckIsJSONEqual.Check called json.Unmarshal directly without
recovering, so a gateway response that was not valid JSON (HTML
wrapper page, upstream error, empty body) panicked and crashed the
parent sub-test. The panic happened in validateResponse, before the
.../Body and .../Header_Content-Type leaf sub-tests were dispatched,
so consumers could not use Go's -skip to selectively skip a single
body assertion while keeping sibling coverage.
- check.go: return CheckOutput{Success: false} with the parser error
and the offending body in Reason instead of panicking; same shape
for the marshal-error branch
- check.go: unmarshal into any rather than map[string]any so JSON
arrays and scalars are also accepted (matches the IsJSONEqual
constructor)
- check_is_json_equal_test.go: unit tests for match, mismatch, HTML
body, empty body
- validate_test.go: integration test that drives validateResponse
with a synthetic HTML response and asserts the failure surfaces
under testName=="Body" so .../Body is -skip-addressable
Contributor
|
Results against Kubo latest: Summary
|
Contributor
|
Results against Kubo master: Summary
|
lidel
force-pushed
the
fix/checkisjsonequal-panic-non-json-body
branch
from
April 29, 2026 23:22
eb8d53b to
39befd4
Compare
Contributor
v0.13.2Fixed
|
lidel
added a commit
to ipfs/service-worker-gateway
that referenced
this pull request
Apr 29, 2026
Three fixes that surfaced once each CI lane had something to chew on: - Add `file-type` to the production dependency list. The `sniff-raw-content-type` helper imports it directly; verified-fetch pulls it transitively but `aegir dep-check` requires a direct declaration. Pinned to `^22.0.1` to match the verified-fetch resolution. - Bump the gateway-conformance image pin from `v0.13.1` to `v0.13.2`. v0.13.2 ships ipfs/gateway-conformance#298, which makes `CheckIsJSONEqual` fail gracefully instead of panicking on a non-JSON body. With the panic gone, leaf-level `-skip` patterns on `.../Body` and `.../Header_Content-Type` now reach inside the `Validate()` call and skip the asserts that the wrapper invalidates, without losing `/Status_code` coverage. - Stabilise `readWrapperProps` for Firefox: wait for `globalThis.renderMedia` before evaluating. Firefox's `page.goto` occasionally resolves while the wrapper's iframe is still settling, which races `page.evaluate` into "Execution context was destroyed". The wait is also a no-op on Chromium where the timing already works.
lidel
added a commit
to ipfs/service-worker-gateway
that referenced
this pull request
Apr 29, 2026
Three fixes that surfaced once each CI lane had something to chew on: - Add `file-type` to the production dependency list. The `sniff-raw-content-type` helper imports it directly; verified-fetch pulls it transitively but `aegir dep-check` requires a direct declaration. Pinned to `^22.0.1` to match the verified-fetch resolution. - Bump the gateway-conformance image pin from `v0.13.1` to `v0.13.2`. v0.13.2 ships ipfs/gateway-conformance#298, which makes `CheckIsJSONEqual` fail gracefully instead of panicking on a non-JSON body. With the panic gone, leaf-level `-skip` patterns on `.../Body` and `.../Header_Content-Type` now reach inside the `Validate()` call and skip the asserts that the wrapper invalidates, without losing `/Status_code` coverage. - Stabilise `readWrapperProps` for Firefox: wait for `globalThis.renderMedia` before evaluating. Firefox's `page.goto` occasionally resolves while the wrapper's iframe is still settling, which races `page.evaluate` into "Execution context was destroyed". The wait is also a no-op on Chromium where the timing already works.
achingbrain
pushed a commit
to ipfs/service-worker-gateway
that referenced
this pull request
May 8, 2026
Workaround for #574: Chromium saves the SW bootstrap HTML when the user right-clicks "Save As" on inline media served through the service worker. Top-level navigations to image/video/audio/pdf/text /json now return an HTML viewer with an explicit Download button that drives the existing ?download=true flow (Content-Disposition: attachment, honored by all browsers). - src/sw/handlers/content-request-handler.ts: route document navigations through tryRenderMediaViewer; cache key gains request.destination so the wrapped HTML cannot collide with subresource refetches of the same URL. - src/sw/pages/render-media.ts (new): build the viewer HTML. Subresource fetches arrive with destination != 'document' and skip the wrapper, so embeds via src=URL do not recurse. - src/sw/lib/media-viewer-types.ts (new): content-type detection plus filename / displayName derivation. - src/ui/pages/render-media.tsx (new): sticky top bar with filename, content type, size, and the Download link. No download HTML attribute, since Chromium bypasses the SW for those clicks (crbug 40410035). - src/ui/index.tsx: short-circuit App to the viewer when globalThis.renderMedia is set, skipping the global header. - src/sw/pages/page.ts: escape < in JSON props to prevent </script> injection from user-controlled path segments. - src/constants.ts, src/sw/pages/render-entity.ts: extract shared GENERATED_HTML_CACHE_CONTROL constant. * fix: escape title in htmlPage to prevent script injection The media viewer (#1039) is the first call site that passes a user-controlled string into `<title>${title}</title>` (via `displayName`, derived from the IPFS path). A path containing `</title><script>...` would close the title element and execute the injected script. HTML-escape `&`, `<`, `>` at the choke point in `htmlPage()` so any current or future caller is safe. * fix: tolerate malformed x-ipfs-path in media viewer `decodeURI` raises `URIError` on a stray `%` or a truncated escape sequence; the surrounding catch turns that into a 500 error page, which is poor UX for a malformed navigation URL the user typed. Wrap the decode in a try/catch and fall back to the raw header value so the viewer still renders. * fix: include path and length in media viewer etag Two different files under the same CID with the same kind (`/cid/a.png`, `/cid/b.png`) produced identical etags despite different `displayName`, `filename`, and `contentLength` props in the response body. Browsers cache by URL so it's not an immediate collision, but the etag no longer represented the response. Fold the URI-encoded `ipfsPath` and `contentLength` into the tag. * test: cover media-viewer-types helpers Add unit tests for `getMediaTypeInfo` and `deriveViewerNames` next to the existing `update-redirect.spec.ts` pattern. Covers exact and family-fallback content types, charset parameter stripping, case-insensitivity, undefined for non-renderable / empty input, and the deriveViewerNames branches: bare CID, simple file, multi-dot extension, already-extensioned paths (the en-wikipedia .pdf.jpg.webp case), missing extension, trailing slash, and last segment equal to the CID. * test: skip conformance tests rendered moot by media viewer Top-level navigation to a JSON CID now returns the viewer wrapper (text/html) instead of bare `application/json` because of the Chromium "Save As" workaround. Four gateway-conformance tests assert the bare content-type on document-level navigations and have no path forward without breaking the wrapper. The bytes are still served correctly to subresource fetches and to `?download=true` requests; only the document content-type changes. Also fixes 6 lint errors in the media-viewer-types spec (`expect(x).to.be.undefined` is a no-op expression to eslint). * test: update e2e tests for media viewer wrapper The media-viewer wrapper (#574) changes top-level navigation to renderable content (image/video/audio/pdf/text/json) from raw bytes to an HTML viewer page. Tests that asserted the bare content-type or the rendered body text needed updating. Adds a small fixture module (`test-e2e/fixtures/media-viewer.ts`) exposing the intent at the call site: - `loadWithMediaViewer` navigates with the wrapper active (use when asserting on the wrapper UI itself) - `loadBypassingMediaViewer` appends `?download=true` so the SW responds with `Content-Disposition: attachment` and the original `Content-Type` / `Cache-Control` / body bytes are preserved for assertion. `loadWithServiceWorker` already streams attachment downloads through the Playwright download event so `response.text()` keeps working. - `mediaViewerFrame` / `mediaViewerBody` return locators scoped to the wrapper's iframe for assertions on rendered text content. Test-by-test: - smoke / dag-pb / dnslink / hamt-dir / path-routing / website-loading: switch the response-header / response-bytes assertions to `loadBypassingMediaViewer`. - first-hit / subdomain-detection / directory-listing: switch page-rendered text assertions to `mediaViewerFrame` / `mediaViewerBody` since the wrapper embeds text content in an iframe. - video: kick off playback explicitly via `<video>.play()`; the wrapper deliberately does not autoplay. * test: cover media viewer wrapper for image/video/pdf/text/json Add `test-e2e/media-viewer.test.ts` with end-to-end coverage of the wrapper introduced by #574. Each media type is exercised via two URL shapes: 1. `/ipfs/<dir>/<filename>` where verified-fetch derives content type from the filename extension and `displayName` matches it. 2. `/ipfs/<file>` (bare CID) where verified-fetch sniffs the bytes and `displayName` falls back to the CID, with `filename` set to `<cid>.<ext>`. All fixtures are checked-in CAR files generated by `test-e2e/fixtures/data/generate.ts`; no kubo IO at runtime. `generate.ts` now produces both a dag-pb encoding (used for the active tests, where `plugin-handle-unixfs` sniffs content-type from the bytes) and a raw-codec single-block encoding for each fixture. The raw-codec CIDs feed a commented-out test block at the bottom of the spec; today verified-fetch's `plugin-handle-raw` short-circuits to `application/vnd.ipld.raw` even when the bytes carry sniffable magic, so the wrapper does not fire on bare-CID navigations to raw blocks. Re-enable those tests once upstream sniffs content-type per the path-gateway spec. Also splits the inline type import in `test-e2e/fixtures/media-viewer.ts` into a top-level `import type` to match the project's lint rule. * test: skip conformance leaf assertions broken by media viewer Top-level navigation to JSON / text / IPNS-resolved-text content now returns the wrapper page (text/html) instead of the bare bytes (media viewer, #574). Thirteen specific leaf assertions in the conformance suite assert on either the document `Content-Type`, `Content-Disposition`, or body, all of which the wrapper rewrites. Entries are scoped to the failing leaf (`/Body`, `/Header_Content-Type`, `/Check_*/Header_Content-Disposition`) so sibling assertions in the same test (status codes, range headers, Accept-Ranges) keep running. Bytes are still served correctly to subresource fetches and to `?download=true` requests; only the document-level content-type and body change. * fix: sniff content-type for raw-codec responses verified-fetch's `plugin-handle-raw` returns `application/vnd.ipld.raw` for raw-codec single-block CIDs even when the bytes carry sniffable PDF / PNG / MP4 magic, in violation of the path-gateway spec §3.2.4 which requires content-type sniffing when no explicit format hint is given. Modern helia/unixfs content (`rawLeaves: true` + CIDv1, the default for content that fits in one block) is the most affected: bare-CID navigation triggers a forced download to `<cid>.raw` instead of inline rendering. Add a small post-fetch helper that re-runs file-type when: 1. The response Content-Type is `application/vnd.ipld.raw`, and 2. The user did not explicitly ask for raw bytes via `Accept: application/vnd.ipld.raw` or `?format=raw`. The helper runs inside `fetchHandler` before the wrapper decision and before caching, so the corrected content-type drives both the wrapper trigger and the cached entry. Trustless requests (CAR, TAR, dag-json, dag-cbor, raw with explicit accept, etc.) skip the sniff because their effective accept includes the IPLD media type. Reactivate the raw-codec section of `media-viewer.test.ts` (PNG, MP4, PDF bare-CID raw fixtures) now that the wrapper fires on those URLs end-to-end. Local Playwright run: 113 passing. TODOs in `sniff-raw-content-type.ts` flag two follow-ups: - Whether to keep the helper here or push the sniffing into `plugin-handle-raw` upstream where it benefits every consumer of verified-fetch. - Whether to switch from the current full-buffer approach to streaming. Raw blocks are bounded by Helia's 256 KB max block size, so the current code is fine in practice. * fix: green up CI for media viewer PR Three fixes that surfaced once each CI lane had something to chew on: - Add `file-type` to the production dependency list. The `sniff-raw-content-type` helper imports it directly; verified-fetch pulls it transitively but `aegir dep-check` requires a direct declaration. Pinned to `^22.0.1` to match the verified-fetch resolution. - Bump the gateway-conformance image pin from `v0.13.1` to `v0.13.2`. v0.13.2 ships ipfs/gateway-conformance#298, which makes `CheckIsJSONEqual` fail gracefully instead of panicking on a non-JSON body. With the panic gone, leaf-level `-skip` patterns on `.../Body` and `.../Header_Content-Type` now reach inside the `Validate()` call and skip the asserts that the wrapper invalidates, without losing `/Status_code` coverage. - Stabilise `readWrapperProps` for Firefox: wait for `globalThis.renderMedia` before evaluating. Firefox's `page.goto` occasionally resolves while the wrapper's iframe is still settling, which races `page.evaluate` into "Execution context was destroyed". The wait is also a no-op on Chromium where the timing already works.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CheckIsJSONEqual.Check called json.Unmarshal directly without recovering, so a gateway response that was not valid JSON (HTML wrapper page, upstream error, empty body) panicked and crashed the parent sub-test. The panic happened in validateResponse, before the .../Body and .../Header_Content-Type leaf sub-tests were dispatched, so consumers could not use Go's -skip to selectively skip a single body assertion while keeping sibling coverage.
Details