Repository navigation
Conversation
DennisAlund
left a comment
DennisAlund
left a comment
There was a problem hiding this comment.
Inline comments below. One finding sits in a file this PR does not touch:
src/mcp/server.ts:496 (get_link_qr). The MCP tool still builds the QR URL from the caller-supplied base_url and ignores SHORT_ORIGIN, so one deployment now hands out two different QR payloads for the same slug depending on which surface asks. With SHORT_ORIGIN=https://c.example, an MCP client calling get_link_qr with base_url=https://a.com (the host it reaches the Worker on) gets a QR encoding https://a.com/slug?utm_medium=qr, while the admin modal and GET /_/api/links/{id}/qr for the same link encode https://c.example/slug?utm_medium=qr. The tool has this.env; resolve SHORT_ORIGIN first and fall back to base_url, which could then become optional.
| if (!slug) return json({ error: "Slug not found" }, 404); | ||
|
|
||
| const qrUrl = `${url.origin}/${slug.slug}?utm_medium=qr`; | ||
| const qrUrl = `${resolveShortOrigin(env, url)}/${slug.slug}?utm_medium=qr`; |
There was a problem hiding this comment.
This route now encodes SHORT_ORIGIN, but no API response exposes that origin, so API clients that compose short URLs themselves disagree with the QR the same server renders. With SHORT_ORIGIN=https://c.example and the extension configured with baseUrl https://a.com, browser-extensions/src/api.ts:87 shows and copies https://a.com/<slug> while the QR fetched from the same server encodes https://c.example/<slug>?utm_medium=qr. Same for every SDK consumer composing ${baseUrl}/${slug}.
Expose the resolved origin (a short_origin field on link responses, or a config endpoint) so clients can agree. That is an SDK surface change and needs parity across the three SDKs.
| if (!slug) return json({ error: "Slug not found" }, 404); | ||
|
|
||
| const qrUrl = `${url.origin}/${slug.slug}?utm_medium=qr`; | ||
| const qrUrl = `${resolveShortOrigin(env, url)}/${slug.slug}?utm_medium=qr`; |
There was a problem hiding this comment.
The SVG is browser-cached for 24h (Cache-Control: private, max-age=86400 below) under a URL that carries no origin. After SHORT_ORIGIN is set or changed, the modal text names the new origin while the <img> and the SVG/PNG downloads keep serving the old-origin QR: open a link's QR modal (cached with https://a.com), set SHORT_ORIGIN=https://c.example, reopen within a day, and the <p> reads https://c.example/slug while the image and both download buttons (fetch(_qrSrc)) still encode https://a.com/slug.
Include the resolved origin in _qrSrc as a query param, or derive an ETag from the encoded URL.
| function copyUrl(slug) { | ||
| var url = location.origin + '/' + slug; | ||
| var url = shortOrigin() + '/' + slug; | ||
| navigator.clipboard.writeText(url); |
There was a problem hiding this comment.
copyUrl() calls navigator.clipboard.writeText without the !navigator.clipboard guard that copyCodeBlock() has, and this PR adds support for plain-http deployments where navigator.clipboard is undefined.
Admin opened on http://192.168.1.10:8787 (a non-secure context; the PR accepts http SHORT_ORIGIN for this LAN case): clicking any slug chip, the detail-page copy button, or creating a link via quickShorten() throws TypeError: Cannot read properties of undefined (reading 'writeText'), no toast appears, and the quickShorten redirect after copyUrl(primary) never runs. The permission-denied rejection also has no .catch, unlike copyCodeBlock.
| */ | ||
| export function resolveShortOrigin(env: Env, requestUrl: string | URL): string { | ||
| const requestOrigin = new URL(requestUrl).origin; | ||
| const raw = env.SHORT_ORIGIN?.trim(); |
There was a problem hiding this comment.
env.SHORT_ORIGIN?.trim() throws when the var is bound as a non-string (wrangler vars and the dashboard accept JSON values), turning every admin page and both QR routes into 500s, the exact outcome the fail-soft design says a bad value must not cause: TypeError: env.SHORT_ORIGIN.trim is not a function inside getPageData() on every /_/admin/* page and in handleLinkQr.
Guard with typeof env.SHORT_ORIGIN === "string" and treat anything else as unset, with the one-time warning.
| } | ||
|
|
||
| /** Values already reported, so one typo logs once per isolate, not once per request. */ | ||
| const warned = new Set<string>(); |
There was a problem hiding this comment.
resolveShortOrigin() trims the value, normalizeShortOrigin() trims it again, and the configured value is re-parsed through new URL on every page render and QR request even though an isolate's env never changes; this warned Set memoizes only the failure case.
One module-level Map<string, string | null> keyed by the raw env value replaces both the Set and the repeat parse: look up, else normalize once, warn once on null, store. resolveShortOrigin becomes a two-line lookup with the request-origin fallback.
|
|
||
| await chip.click(); | ||
| await expect(toast(page)).toHaveText(`Copied ${SHORT_ORIGIN}/${slug}`); | ||
| expect(await page.evaluate(() => navigator.clipboard.readText())).toBe(`${SHORT_ORIGIN}/${slug}`); |
There was a problem hiding this comment.
This reads the clipboard once after the toast appears, but copyUrl() raises the toast synchronously right after calling writeText() without awaiting it, so the toast proves the call was made, not that the write landed. A slow clipboard IPC in headless Chromium returns the previous clipboard content (empty on a fresh context) and the test fails with no code defect, which the repo's flake policy says must then be chased to mechanism.
Harden with await expect.poll(() => page.evaluate(() => navigator.clipboard.readText())).toBe(...).
| // answers on several domains. A bare host or a full origin ("c.example", | ||
| // "https://c.example"). Unset keeps the origin of the request that | ||
| // rendered the page (see src/short-origin.ts). | ||
| SHORT_ORIGIN?: string; |
There was a problem hiding this comment.
SHORT_ORIGIN is hand-declared as an optional member of Cloudflare.Env, which conflicts with the literal type yarn types generates if a fork sets it the natural way as a wrangler.jsonc vars entry, and the docs (README.md:134, docs/access-control.md:53) do not say so.
Fork adds "vars": { "SHORT_ORIGIN": "c.example" }, runs yarn types, worker-configuration.d.ts declares SHORT_ORIGIN: "c.example", the merge with SHORT_ORIGIN?: string fails typecheck with TS2717 "Subsequent property declarations must have the same type", and CI goes red. State in the docs that the value must be a secret or dashboard var, not a vars entry, or drop the manual declaration and let the generated type carry it.
| // browser-side copy button and QR modal can use it without asking the server | ||
| // again. Every admin page is listed on purpose: the value travels from | ||
| // getPageData() into each route's <Layout> by hand, so a route added or | ||
| // edited without it would silently fall back to the host the admin was |
There was a problem hiding this comment.
The rationale says a route rendered without shortOrigin "would silently fall back", but shortOrigin is a required LayoutProps member, so an omitted prop fails tsc loudly. The comment misdescribes which defect the enumerated-path test guards against: its real value is catching a route that passes a wrong value (for example the request origin computed locally). Say that, or make the prop optional only if this test is meant to be the guard.
A deployment that answers on several domains handed out whichever one the operator opened the admin on: the copy button, the QR modal and the QR code all read the request host. An operator serving a.com and b.com who wants links to live on c.com could not get a consistent URL out of the admin. SHORT_ORIGIN pins the origin. Unset keeps the request origin, so a deployment that never sets it behaves exactly as before. The value reaches the browser as data-short-origin on <html>, beside data-theme, because the client script is a content-hashed immutable asset that cannot carry a per-deployment value. A value that is not a bare http(s) origin is ignored with one warning per isolate rather than failing the admin pages: a typo in a cosmetic setting should not take the dashboard down. The domain itself still has to be attached to the Worker, which the setting cannot do. Review follow-ups, folded in rather than stacked on top: - A binding that is not a string counts as unset. wrangler's vars and the dashboard accept JSON values, and trim() on one answered every admin page and both QR routes with a 500, the one outcome the fail-soft rule forbids. The value is parsed once per isolate into a module-level map now, instead of on every page render and QR request. - copyUrl() and copyRawKey() share a guarded copyText(). navigator.clipboard is undefined outside a secure context, which is the plain-http LAN deployment this commit adds support for, and there the unguarded call threw before the toast and before quickShorten()'s redirect. - The QR request URL carries the resolved origin as a cache key. The endpoint caches for 24h under a URL that would not otherwise change with the setting, so a repointed SHORT_ORIGIN left the modal naming the new origin while the image and both download buttons still encoded the old one. The parameter is a cache key only, and a test pins that the endpoint ignores it: the payload stays the server's decision. - get_link_qr builds its QR from SHORT_ORIGIN first and falls back to the caller's base_url, which is optional now. One deployment was handing out two different QR payloads for the same slug depending on which surface asked. - The e2e clipboard assertion polls instead of reading once. The toast is raised synchronously after writeText(), so it proved the call was made, not that the write landed, and a slow clipboard IPC failed the test with no code defect. The admin page test's comment names the defect it actually guards, a route passing the wrong origin, rather than an omission tsc already catches. - README and docs/access-control.md say to set SHORT_ORIGIN as a secret or a dashboard variable. As a wrangler.jsonc vars entry, yarn types gives the value a literal type that collides with the optional declaration in src/types.ts, and the typecheck stops with TS2430.
A client that composes `${baseUrl}/${slug}` itself could not learn which origin
the deployment hands links out on. On a deployment that answers on several
domains it built URLs on whichever host it was configured with, while the copy
button, the QR code and the redirects all used SHORT_ORIGIN. The browser
extension showed and copied the wrong URL in exactly the case the setting
exists for.
`GET /_/api/config` answers the resolved origin once, and every public link
response gains a required `short_origin` field, so a client that already holds
a link needs no second request. Both report what the admin shell reads off the
data attribute on <html>: SHORT_ORIGIN when set, the origin of the request
otherwise. The admin API is unchanged.
The surface change moves the three SDKs together: each gains a `config`
resource that wraps the endpoint, `short_origin` on its Link model, tests for
both, and a refreshed spec hash in its manifest. Version bumps and CHANGELOG
entries stay with the release commit.
The browser extension still composes `${baseUrl}/${slug}`: it depends on the
published @oddbit/shrtnr, so it adopts the field once this ships.
51f7585 to
04164ec
Compare
2 participants
What
A deployment that answers on several domains handed out whichever one the operator opened the admin on: the copy button, the QR modal and the QR code all read the request host. An operator serving
a.comandb.comwho wants links to live onc.comcould not get a consistent URL out of the admin.SHORT_ORIGINpins the origin. Unset keeps the request origin, so a deployment that never sets it behaves exactly as before. The value reaches the browser asdata-short-originon<html>, besidedata-theme, because the client script is a content-hashed immutable asset that cannot carry a per-deployment value.A value that is not a bare http(s) origin is ignored with one warning per isolate rather than failing the admin pages: a typo in a cosmetic setting should not take the dashboard down. The domain itself still has to be attached to the Worker, which the setting cannot do.
Tests
yarn test: 106 files, 1625 tests pass.yarn e2e: 34 tests pass, including the newe2e/short-origin.spec.ts, which drives the copy button and the QR modal against a configured origin that is deliberately not the admin host. No new font, script or stylesheet, so the page weight budget is unchanged.