Repository navigation
fix(web): close story OG card layout and image boundary gaps - #192
Merged
Merged
Conversation
Follow-up to #175. Four confirmed defects in the merged story OG card, each reproduced against the real satori/resvg render pipeline and each covered by a regression test that fails on the old behaviour. 1. Bounded long-title layout. The headline set `-webkit-box` + `WebkitLineClamp: 4` but omitted `textOverflow: "ellipsis"`. Satori's `processTextOverflow` only honours the clamp when the ellipsis is present, so the box grew unbounded: a 353-character title rendered 11 lines, painting over the `AI;DR` wordmark, over the footer metadata, and off the bottom of the card. Add the ellipsis plus an explicit `maxHeight` backstop, and add the same ellipsis to the footer host row so a 90-character host degrades instead of being hard-clipped. 2. Pixel-dimension ceiling. Only the 1 MB byte cap and magic bytes were checked, so a 33-byte file claiming 30000x30000 reached the renderer. Parse real dimensions from PNG IHDR, JPEG SOF, GIF LSD, and WebP VP8/VP8L/VP8X, and reject anything past 4096 per side or 4,000,000 pixels total. The byte ceiling is unchanged. 3. Truncated and garbage-tailed payloads. A header-valid file with a broken tail passed and rendered as an empty gray photo panel. Require complete container structure (PNG IHDR + IEND, JPEG EOI, GIF trailer, exact WebP RIFF size) so those fall back to the branded panel. 4. Trailing-dot hostnames. `new URL("https://localhost./x").hostname` is `localhost.`, which slipped past the inline `host === "localhost"` and `.internal` suffix checks. Normalize trailing dots before the blocklist. `isSafeStoryImageUrl` was only safe because the shared `sanitizeImageUrl` happened to catch it; the boundary now stands alone. Also: export the card's title-band geometry, add focused timeout-clamping and `api/og/$id` route-wiring tests (locale, `Content-Language`, cache and id-prefix behaviour), and regenerate the committed previews for the English, Vietnamese, and fallback cases from a deterministic in-repo synthetic source so no network is involved. Preserved: the detailed-story redesign, EN/VI copy and category labels, attribution metadata, the byte/time/redirect/credential/referrer/MIME protections, and the zero-source-fetch contract.
Adds a reproducible 353-character headline render so the clamp fix is visible in review rather than only asserted in a test.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThis follow-up hardens the story OG card across rendering, image ingestion, and Worker routing: it clamps long text, validates raster dimensions and structural completeness before inlining, closes trailing-dot SSRF gaps, clamps fetch timeouts, expands route regression coverage, and adds deterministic preview artifacts. Sequence diagram for bounded story OG image renderingsequenceDiagram
participant Client
participant Route as api/og/$id
participant Fetcher as fetchStoryOgImage
participant Validator as readRasterContainer
participant Renderer as ImageResponse
Client->>Route: GET /api/og/$id.png?lang=en|vi
Route->>Fetcher: fetchStoryOgImage(image_url)
Fetcher->>Validator: readRasterContainer(bytes)
alt valid bounded complete raster
Validator-->>Fetcher: RasterContainer
Fetcher-->>Route: StoryOgImage data URI
else unsafe, oversized, truncated, or invalid
Validator-->>Fetcher: null
Fetcher-->>Route: null
end
Route->>Renderer: storyOgCard(story, image, language)
Renderer-->>Client: PNG card with photo or branded fallback
Flow diagram for story OG image safety boundaryflowchart TD
A[Remote image URL] --> B{Safe normalized hostname?}
B -- No --> F[Branded fallback]
B -- Yes --> C[Bounded fetch with clamped timeout]
C --> D{Within 1 MB byte limit?}
D -- No --> F
D -- Yes --> E[readRasterContainer]
E --> G{Complete supported raster and valid dimensions?}
G -- No --> F
G -- Yes --> H[Inline as data URI]
H --> I[Render story OG card]
F --> I
Flow diagram for bounded long-title card layoutflowchart TD
A[Story headline] --> B[Title column]
B --> C[Four-line WebkitLineClamp]
C --> D[Ellipsis via textOverflow]
D --> E[maxHeight structural backstop]
E --> F[Title band]
F --> G[Footer and bottom rule remain unobstructed]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The VP8L branch of `readWebp` read `width - 1` as a plain 24-bit little-endian value and mis-shifted `height - 1`. VP8L packs both into a single 28-bit little-endian run after the 0x2f signature byte: 14 bits of `width - 1` at bits 0-13, 14 bits of `height - 1` at bits 14-27, then `alpha_is_used` and the 3-bit version. The two 14-bit fields straddle the second and third header bytes. Reading 24 bits folded the top of height into width, so a real 1200x630 lossless file decoded as 1918128x7171 and was then rejected by the pixel ceiling. Every simple-VP8L thumbnail silently fell back to the branded panel. The height expression also bound `+ 1` tighter than `|`, summing three terms instead of OR-ing them. Now reads the two fields from the correct bit positions and keeps the existing bounded validation on top: 4096 per side, 4,000,000 pixels total, 1 MB of bytes. Also proves the VP8L header actually lies inside the declared first sub-chunk, and applies the same minimum-payload check to the VP8 and VP8X branches. Fixtures are real lossless WebP files from libwebp (`lossless: true` through sharp), not hand-rolled: nine 38-byte simple-VP8L files covering 1x1, 16x16, 300x200, 1200x630, 1024x13, 255x257, 4096x1 (at the side ceiling), 4097x1 (one past it, must be rejected), and an alpha variant. Each was decoded back through libwebp before being committed, and `webpLosslessBytes()` reproduces all nine byte for byte, asserted by a test, so the in-code encoder cannot drift from real output. The generator is committed and re-runs byte-stable; `sharp` reaches the workspace via the existing `miniflare` devDependency, so nothing new is required and no test touches the network. Reverting only the bit layout fails 4 of the new tests, including the real-file acceptance case. PNG, JPEG, GIF, VP8, VP8X, truncation, pixel/byte/time, SSRF, and title-clamp behaviour are unchanged.
workerd accepts only "follow" and "manual" for `redirect`. Given "error"
it throws a TypeError before any request leaves the edge:
Invalid redirect value, must be one of "follow" or "manual" ("error"
won't be implemented since it does not make sense at the edge; use
"manual" and check the response status code)
The boundary caught that and returned null, so every deployed story OG
thumbnail silently fell back to the branded panel while the Node unit
tests passed: undici accepts "error", so the divergence was invisible
until a real workerd run.
Switch to `redirect: "manual"`, matching `worker/clerk-proxy.ts` and
`worker/enrich.ts`, which already use it. A 3xx is never followed, and
because a redirect response is never `ok` the existing non-OK check turns
it into a clean miss without ever reading Location. The byte, pixel,
timeout, credential, referrer, MIME, and SSRF boundaries are untouched.
Tests: the assertion that pinned `redirect: "error"` now pins
`"manual"`, plus three regressions covering 301/302/303/307/308 resolving
to null, the redirect target and Location never being read or leaked into
the rendered fallback, and a guard that the mode sent is one workerd
accepts.
New `scripts/story-og-workerd-smoke.ts` drives the real boundary inside
Miniflare so this class of Node-versus-workerd divergence fails before
deploy. It asserts workerd rejects "error", a valid image resolves in
workerd, a 3xx is a clean miss that is not followed (the Location points
at the good URL, so a follow would show up in the outbound log), and
blocked hosts never reach the network. Reintroducing "error" makes it
fail with `threw-TypeError` and zero outbound requests, which is exactly
the production symptom. Run with
`pnpm --filter @aidr/web test:workerd:story-og`.
VP8L/VP8/VP8X, title clamp, pixel/byte/time, SSRF, locale, and
determinism behaviour is unchanged; the four preview artifacts and the
nine libwebp fixtures still regenerate byte-identically.
duyet
marked this pull request as ready for review
September 25, 2026 17:28
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This was referenced Sep 25, 2026
duyet
added a commit
that referenced
this pull request
Sep 30, 2026
#297) The homepage uses og-home.jpg since #192 and story links carry ?lang=, so drive homepage failed on a healthy site. Claude-Session: https://claude.ai/code/session_01LUvnDNBM63UC4pk8VMMz72 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.
Round 3:
redirect: "error"is invalid in workerd (production blocker)Review caught that the boundary passed a
redirectmode workerd rejects, so every deployed story OG fetch returned the branded fallback. Fixed, and now covered by a workerd-level smoke so the Node-vs-workerd class of divergence fails before deploy.Root cause. workerd accepts only
"follow"and"manual". Given"error"it throws before any request leaves the edge:The boundary caught that and returned
null, so every deployed thumbnail silently fell back while the Node unit tests passed — undici accepts"error", so nothing caught it. Measured in real workerd:redirectmode"error"TypeError, 0 outbound requests"manual""follow"That is the exact production symptom:
{"ok":false,"reason":"threw-TypeError"}with an empty outbound request log.Fix —
redirect: "manual", matching the repo's existing convention:worker/clerk-proxy.ts(3 call sites)manualworker/enrich.tsmanualworker/__tests__/fetch-boundary.test.tsmanuallib/story-og.tsx(was the only"error")manualA 3xx is never followed, and because a redirect response is never
ok, the existing non-OK check turns it into a clean miss without ever readingLocation. The byte, pixel, timeout, credential, referrer, MIME, and SSRF boundaries are untouched.Tests
redirect: "error"now pins"manual"301/302/303/307/308all miss, each with exactly one request to the original URLLocationtarget, the header itself, and the source URL never reach the rendered fallback"error"can never be reintroducedNew workerd smoke
apps/web/scripts/story-og-workerd-smoke.tsdrives the real boundary inside Miniflare, modelled on the existingclerk-proxy-workerd-smoke.ts(outbound fetcher, no network). 8 checks, all passing:The 3xx upstream's
Locationdeliberately points at the good image URL, so a followed redirect would showphoto.pngtwice in the outbound log — it appears once.Proof it catches the blocker. Reintroducing
redirect: "error"makes the smoke exit non-zero with the production symptom:Run with
pnpm --filter @aidr/web test:workerd:story-og. The existingtest:workerd:clerk-proxystill passes.Round 2: VP8L lossless WebP regression
Review caught that my own
readWebpmis-decoded VP8L — the lossless WebP variant — so every simple-VP8L thumbnail was silently rejected.Root cause. VP8L packs both dimensions into a single 28-bit little-endian run after the
0x2fsignature byte: 14 bits ofwidth - 1at bits 0–13, then 14 bits ofheight - 1at bits 14–27, thenalpha_is_usedand a 3-bit version. The two fields straddle the second and third header bytes. My code read width as a plain 24-bit value and mis-shifted height — with two faults: the 24-bit read folds the top of height into width, and the height expression bound+ 1tighter than|, summing three terms instead of OR-ing them.00 00 00 000f c0 03 002b c1 31 00af 44 9d 00fe 00 40 00Every value blew past the ceiling — a 100 % rejection rate for lossless WebP. Fixed to read the two fields from the correct bit positions, keeping the bounded validation (4096/side, 4,000,000 px, 1 MB). The header is now also proven to lie inside the declared first sub-chunk.
Fixtures are real, not hand-rolled. Nine committed 38-byte simple-VP8L files from libwebp, each decoded back through libwebp before committing:
1x1·16x16·300x200·1200x630·1024x13·255x257·4096x1(at ceiling) ·4097x1(past ceiling, must be rejected) ·300x200-alpha(alpha_is_usedat bit 28).webpLosslessBytes()reproduces all nine byte for byte, asserted by a test, so the encoder cannot drift from real output. The generator is committed and byte-stable.sharpreaches the workspace via the existingminiflaredevDependency — no new dependency, and no test touches the network.Reverting only the bit layout fails 4 of the new tests, including the real-file acceptance case.
Round 1 summary
Five confirmed defects, each reproduced against the real satori/resvg render pipeline and the real Worker boundary, each covered by a test that fails on the pre-fix behaviour.
api/og/$idroute wiring — focused tests addedThe detailed-story redesign, EN/VI copy and category labels, attribution metadata, the byte/time/redirect/credential/referrer/MIME protections, and the zero-source-fetch contract are all preserved.
1. Bounded long-title layout (satori)
The headline set
display: "-webkit-box"+WebkitLineClamp: 4but omittedtextOverflow: "ellipsis". Satori'sprocessTextOverflow(src/text/processor.ts) only honours the clamp when the ellipsis is present; without it,return [Infinity].Measured, same 353-character title, real satori/resvg render:
Title band is y 119..522 (403 px). The old card painted from y=55 — inside the
AI;DRwordmark — down to y=619, through the footer metadata and into the bottom rule. The new card uses 219 px of the band and terminates the 4th line with….Fix. Add
textOverflow: "ellipsis"alongside the clamp, plus an explicitmaxHeight: 233pxstructural backstop. The same ellipsis was added to the footer host row, where a 90-character host was previously hard-clipped under the engagement counters.Proof. Reverting just the
textOverflow/maxHeightlines fails:The test drives the actual
ImageResponsepipeline, decodes the PNG, and diffs against a blanked-title baseline to locate the ink — markup assertions could not catch this.2. Pixel-dimension ceiling
Only the 1 MB byte cap and leading magic bytes were checked, so a 33-byte file declaring
30000x30000was accepted.apps/web/src/lib/story-og-image.tsnow parses real dimensions from PNG IHDR, JPEG SOF0–SOF3/5–7/9–11/13–15, GIF LSD, and WebP VP8 / VP8L / VP8X, rejecting anything pastMAX_STORY_OG_IMAGE_SIDE = 4096per side orMAX_STORY_OG_IMAGE_PIXELS = 4_000_000total. The byte ceiling is unchanged.3. Truncated / undecodable payloads
Magic bytes alone accepted header-valid files with a broken tail, which rendered as an empty gray photo panel. Now requires structurally complete containers: PNG
IHDR+ terminalIEND, JPEG terminalEOIand an SOF segment, GIF trailer0x3B, and an exact WebP RIFF size. Validated against real repository images. No decoder and no memory-limit increase.4. Trailing-dot hostnames
new URL("https://localhost./x").hostnameis"localhost.", which slipped pasthost === "localhost"and the suffix checks. The blocklist was only safe by accident — the sharedsanitizeImageUrlhappened to catch it.normalizeHostname()now strips all trailing dots before any comparison. Tests coverlocalhost.,LOCALHOST.,metadata.google.internal.,cdn.internal.,foo.local.,db.localhost.,printer.local.,localhost.., and public hosts that must still be allowed.5. Timeout clamping and route wiring
timeoutMs: 1and the 5 s ceiling fortimeoutMs: 60_000. Reverting fails:expected null to be 'pending'.api/og/$id— 14 tests covering?lang=vi→ Vietnamese card andContent-Language: vi,?lang=en→ English, unknown/repeated/empty locales defaulting to English without smuggling a second locale,Cache-Controlstability per locale,.pngsuffix stripping, suffix-style slugs, 404/500 paths, exactly one bounded image fetch, and the card never containing the source URL, query string, or signature.Previews
All four committed artifacts regenerate from a deterministic, licence-clean in-repo synthetic source, so no network is involved and the bytes are stable across runs:
og-story-preview.png(EN + photo)9f56ed690812707a029f4d340e6d6ae3f6f5a4c660894a1135109150aa577296og-story-preview-vi.pnga096350fd5ccd090c3cdf9d5ecea514264646b7bfc0f9663fd472a33f87ef9feog-story-preview-fallback.png60d32e6d63886f4efe88081be92d97780896be600763bae5b7a85d728357b141og-story-preview-long-title.pngdd89ed435c5dd48856fa79f04480ea8a809c8dd4638b3688f6a0233c82322e91Re-verified byte-identical after the VP8L and redirect-mode fixes.
Happy path:
Vietnamese:
Branded fallback:
353-character headline, clamped to 4 lines:
Verification
Exact head:
dbe57f8d7603ffe22d9ae1adef046230d7e9d253Lint, test, typesondbe57f8pnpm run testpnpm run lintpnpm run check-typespnpm run build(client + SSR)pnpm --filter @aidr/web test:workerd:story-ogpnpm --filter @aidr/web test:workerd:clerk-proxygit diff --check origin/master...HEADgit merge-base --is-ancestor origin/master HEADglobalThis.fetch, asserts blocked hosts issue no request and that only the allowed host reaches fetch, then renders a card and asserts no further requestsOG coverage: 78 focused tests — 31 boundary/copy, 26 container parser, 7 render-level, 14 route wiring.
Remaining live verification
Not verifiable locally; needs a deployed environment:
/api/og/{id}.png(the boundary is exercised against synthetic, in-repo, and libwebp-generated images here)Content-Language/ cache-header behaviour through the real Cloudflare edgeRefs #175