feat: custom meta-tags (social link previews)#231
Conversation
Custom social-preview tags (title/description/image/color) as an optional embedded model; presence = feature enabled. Control chars stripped, https-only non-SVG images, #RRGGBB color.
Flat meta_* fields flow doc→cache so the redirect hot path can branch with zero extra reads. Pre-deploy cache entries without the fields still validate (None defaults, no migration).
Positive allowlist (~65 source-verified tokens) for the meta-tags serving decision — not generic bot detection. Canonical JSON in data/, byte-identical edge copy pinned by tests. In-app-browser tokens (Line/ etc.) are regression-tested as humans.
Positive-allowlist branch before the password gate: preview bots get a self-contained prerendered card (full OG+Twitter+theme-color set, og:url = short URL, noindex, JS fallback for misclassified humans); humans/search/AI keep the 302. Preview serves are never clicks. block_bots links omit the destination from the page.
MetaTagsRequest DTO on create+update, whole-object replace semantics (null clears, clearing never gated). Write gate: verified account + custom_meta_tags flag, 403 with actionable message. Blocklist regexes run over all fields plus a destination re-check on every meta write. Responses expose meta_tags on create/update/list.
The + page renders the owner's custom card next to the real destination — recipients can compare what the sender wants shown vs where the link actually goes.
Log-only: learn the real runtime category strings on this zone before the meta-tags worker branch enforces them.
One entry per link: og_only (write-through at meta-write time; bots served at edge, humans passthrough so origin keeps click tracking) and redirect+og_html (hot links via promotion). Worker classifies via preview_bots.json + verifiedBotCategory (recon-gated). ?bot=1 bypasses the edge. KV puts support no-TTL; lifecycle is event-driven (sync on meta/long_url/status/alias edits, delete on clear/delete). Old workers passthrough og_only and ignore og_html — no lockstep.
Zero new dependencies: SigV4 is ~50 lines of stdlib hmac/hashlib on the shared httpx client (cloudflare_kv house style). put_object raises on failure (user-write path); delete is best-effort. Sniffer covers png/jpeg/webp/gif magic + best-effort dims (imghdr is gone in 3.13, Pillow deliberately not added). R2_* settings, off unless all five fields set.
meta_tags.image also accepts data:image/png|jpeg|webp;base64 — decoded,
magic-byte matched against the declared type, capped at 512KB, stored
content-addressed under og/{owner_id}/ (self-hosted bytes = scannable +
takedown-able, no TOCTOU). Synchronous image_meta records dims → the
page gains og:image:width/height; responses warn above WhatsApp's
300KB reliability ceiling. Self-host without R2: uploads rejected
with a clear error, https URLs unaffected.
External https og:images get validated out-of-band: own stream + meta-image consumer group (reader+claimer, 120s claim idle for slow fetches). fetch_public pins connections to DNS-validated public IPs (rebinding-proof, IPv4-mapped IPv6 unwrapped, per-hop re-checks). Hard failure CAS-clears the image; success CAS-records dims and re-syncs the edge entry with og:image:width/height. Queue gate relaxed: inline-click deployments can run meta-only workers.
CurrentUser.tier carries UserDoc.plan (API-key path from the DB; JWT path reads the future 'plan' claim). Flipping custom_meta_tags from ALLOWLIST to TIER at paid launch becomes a pure data change.
Stdlib HTMLParser, head-only with early stop, property=/name= mixups accepted, first-tag-wins, relative og:image resolved against the final post-redirect URL, https-only images.
The prefill companion to custom meta-tags: fetches the destination through the SSRF-guarded fetcher (512KB cap, html-only), parses via the head-only parser, returns normalized best-picks + raw og/twitter families. Auth-required (anonymous would be a free fetch proxy), 20/min + 500/day limit, Redis-cached 1h with 5m negative caching.
Found in live e2e: github.com's homepage alone exceeds the 512KB cap, hard-failing the metadata endpoint even though the tags sit in the first 50KB. HTML fetches now truncate at the cap and parse the prefix; image fetches keep the hard fail (a truncated image is invalid).
There was a problem hiding this comment.
Sorry @Zingzy, your pull request is larger than the review limit of 150000 diff characters
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds custom URL meta tags, image ingestion and validation, metadata fetching, server-rendered previews, OG HTML edge caching, preview-bot detection, and asynchronous image-validation workers with Redis, R2, MongoDB, and Cloudflare KV integrations. ChangesMeta tags and URL lifecycle
Cloudflare edge preview serving
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
infrastructure/storage/r2.py (1)
69-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd
is_configuredguard toput_objectfor clearer failure mode.
put_objectproceeds with empty credentials and a"/None/key"path when R2 is unconfigured, eventually raisingR2StorageError("Image upload failed")after a pointless HTTP round-trip. An earlyis_configuredcheck would fail fast with a more actionable message.🛡️ Proposed guard
async def put_object(self, key: str, data: bytes, *, content_type: str) -> str: """Upload and return the public URL. Raises R2StorageError on failure.""" + if not self.is_configured: + raise R2StorageError("R2 storage is not configured") path = f"/{self._bucket}/{quote(key, safe='/')}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/storage/r2.py` around lines 69 - 102, Add an early is_configured check in put_object so the method fails fast before building the signed request or calling _http.request when R2 credentials/bucket are missing. Use the existing R2StorageError path in infrastructure/storage/r2.py and keep the current logging/error flow for real upload failures, but return a clearer, actionable failure mode instead of proceeding with empty credentials and a bad path.services/edge_cache/contract.py (1)
35-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd model-level validation to enforce the pinned JSON schema contract.
The Pydantic model doesn't enforce three constraints from
entry.schema.json: (1)urlis required whentype="redirect", (2)og_htmlis required whentype="og_only", and (3)og_htmlhasmaxLength: 32768. Additionally,statusisintbut the schema restricts it toenum: [301, 302]. Current callers always provide valid values, but a future caller could create a contract-violating entry that the edge worker would serve incorrectly.♻️ Proposed refactor to enforce the contract at the model level
from typing import Literal +from pydantic import model_validator class EdgeCacheEntry(BaseModel): ... - status: int = 302 + status: Literal[301, 302] = 302 ... - og_html: str | None = None + og_html: str | None = None # max_length enforced below def to_kv_json(self) -> str: ... + `@model_validator`(mode="after") + def _validate_contract(self) -> "EdgeCacheEntry": + if self.type == "redirect" and self.url is None: + raise ValueError("url is required for type='redirect'") + if self.type == "og_only" and self.og_html is None: + raise ValueError("og_html is required for type='og_only'") + if self.og_html is not None and len(self.og_html) > 32768: + raise ValueError("og_html exceeds maxLength 32768") + return selfAs per the upstream contract in
edge/spoo-edge-cache/contract/entry.schema.json,urlis required fortype=redirect,og_htmlis required fortype=og_only, andog_htmlhasmaxLength: 32768.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/edge_cache/contract.py` around lines 35 - 45, Add model-level validation in the edge cache contract model so it matches entry.schema.json: enforce that `url` is present when `type` is "redirect", `og_html` is present when `type` is "og_only", `status` only allows 301 or 302, and `og_html` does not exceed 32768 characters. Implement this validation on the Pydantic model that defines `type`, `url`, `status`, and `og_html` in the contract module, ideally with a model validator so all callers are checked before `to_kv_json()` serializes the entry.edge/spoo-edge-cache/src/index.ts (1)
76-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding
botCategoryanduato theedge_og_hitlog.The
edge_hitlog (lines 95-107) includesbotCategoryanduafor recon, butedge_og_hitomits both. Since OG hits are specifically from preview bots, logging these fields would help verify correct bot classification during the meta-tags rollout and fillcf_categorieswith observed values.📊 Proposed addition
if (typeof entry.og_html === "string" && wantsPreview(request)) { console.log( - JSON.stringify({ event: "edge_og_hit", key, colo: request.cf?.colo }), + JSON.stringify({ + event: "edge_og_hit", + key, + colo: request.cf?.colo, + botCategory: (request.cf as { verifiedBotCategory?: string } | undefined) + ?.verifiedBotCategory, + ua: request.headers.get("user-agent") ?? "", + }), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@edge/spoo-edge-cache/src/index.ts` around lines 76 - 78, The edge_og_hit log currently only records event, key, and colo, while the nearby edge_hit logging already includes botCategory and ua for recon. Update the edge_og_hit logging in the same area of index.ts to also include request.cf?.botManagement?.botCategory and request.headers.get("user-agent") (or the same ua source used by edge_hit) so OG requests can be classified and verified consistently during rollout.workers/click_worker.py (1)
318-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting a generic subscriber-pair registrar.
_register_meta_imageis structurally identical to_register_group— both create aClaimDeadLetterGuard, define a reader and claimer, and register them as subscribers. A single generic helper parameterized by stream, group, DLQ, tunables, and the consume callable would eliminate this duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/click_worker.py` around lines 318 - 361, _extract the duplicated subscriber-pair setup from _register_meta_image and _register_group into a shared helper that builds the ClaimDeadLetterGuard, defines the reader/claimer callbacks, and registers both StreamSub subscribers; parameterize it with the stream, group name, DLQ stream, max_deliveries, batch/block/claim idle tunables, consumer suffix, and the consume callable so both meta-image and group registrations reuse the same generic registrar._services/meta_tags/validator.py (1)
107-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting
_v2_doc_to_cacheto a shared module.The late import of a private function (
_v2_doc_to_cache) fromservices.url_servicecreates a hidden cross-module coupling. While this avoids a circular import, making the function public or moving it to a shared utility module would clarify the dependency and prevent accidental breakage if the private function is refactored.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/meta_tags/validator.py` around lines 107 - 117, The _invalidate method in Validator is reaching into services.url_service for the private helper _v2_doc_to_cache, which creates fragile cross-module coupling. Move the cache-conversion logic into a shared utility module or make the helper a public API, then update _invalidate to import and use that shared symbol instead of a private function from services.url_service.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infrastructure/safe_fetch.py`:
- Around line 162-163: The content-length check in safe_fetch is assuming
resp.headers.get("content-length") is always numeric, so int(declared) can raise
an uncaught ValueError. Update the logic in the fetch path around the declared
header handling to safely parse malformed Content-Length values and treat them
as a fetch failure that maps to the existing FetchHardError/FetchTransientError
flow rather than escaping. Use the safe_fetch response validation and any helper
around declared/max_bytes to keep the behavior consistent when the header is
missing or invalid.
In `@services/meta_preview.py`:
- Around line 25-37: Guard the destination hostname in the meta preview
response: in `services.meta_preview` where the preview dict is built,
`dest_host` is currently always derived from `url.long_url` and can leak when
`block_bots` is enabled. Use the same `reveal` check already applied to
`long_url` so `dest_host` is only populated when the destination is allowed to
be shown, otherwise return a non-revealing value. Keep the fix localized to the
response-building logic around `extract_hostname` and the `reveal` flag.
In `@services/meta_tags/parse_html.py`:
- Line 18: The import list in parse_html is pulling in dataclasses.field even
though it is never referenced. Remove the unused field import from the module’s
top-level dataclass import so the lint error is cleared, and keep the rest of
the dataclass usage unchanged.
- Around line 81-84: The parsing flow in the HTML collector uses a bare
try/except/pass around collector.feed(html), which triggers SIM105; update this
block to use contextlib.suppress(_StopParsing) for the same control flow. Locate
the change in the parse_html logic near collector.feed and replace the
exception-swallowing pattern with the idiomatic suppress-based form while
keeping the existing _StopParsing behavior unchanged.
In `@services/url_service.py`:
- Around line 223-230: `updated_ip` is being overwritten with null during
meta_tags updates because `_handle_meta_tags` builds `LinkMetaTags` without
preserving the IP field. Fix this in `services.url_service` by updating
`update()` and `_handle_meta_tags` so the last write IP is either accepted and
passed through as `updated_ip` or intentionally omitted from the persisted
payload. Make sure `LinkMetaTags` and its `model_dump()` usage keep the intended
IP semantics consistent with `create()`.
In `@tests/integration/api_v1/test_metadata.py`:
- Around line 37-40: The test setup uses nested with statements that trigger
Ruff SIM117; update the affected blocks in test_metadata.py to use a single with
statement with comma-separated context managers instead of nesting. Apply this
pattern consistently at each occurrence by combining the patch context and
TestClient context in the same with statement, using the existing fetch_public,
TestClient, and _app symbols to locate the three sites.
In `@tests/unit/infrastructure/test_safe_fetch.py`:
- Around line 72-76: The test has nested context managers that trigger the
SIM117 lint failure; update the affected block in the safe fetch test to use a
single combined with statement instead of separate nested with statements. Keep
the same mocked resolver setup in the test around the FetchHardError assertion,
and adjust the surrounding test code only where needed to preserve the existing
behavior.
In `@tests/unit/services/test_meta_image_validator.py`:
- Around line 82-87: The test in test_meta_image_validator uses nested context
managers for patch and pytest.raises, which triggers the SIM117 formatting rule.
Update the validator test to combine these two context managers into a single
with statement while keeping the same AsyncMock side effect and the await
validator.consume(_payload()) assertion behavior.
- Around line 91-96: The test setup in test_malformed_payload_dropped_silently
unpacks a cache value from _validator() but never uses it, triggering RUF059.
Update the local binding to indicate it is intentionally unused by renaming
cache to _cache (or equivalent underscore-prefixed name) in this test while
keeping validator and repo unchanged.
---
Nitpick comments:
In `@edge/spoo-edge-cache/src/index.ts`:
- Around line 76-78: The edge_og_hit log currently only records event, key, and
colo, while the nearby edge_hit logging already includes botCategory and ua for
recon. Update the edge_og_hit logging in the same area of index.ts to also
include request.cf?.botManagement?.botCategory and
request.headers.get("user-agent") (or the same ua source used by edge_hit) so OG
requests can be classified and verified consistently during rollout.
In `@infrastructure/storage/r2.py`:
- Around line 69-102: Add an early is_configured check in put_object so the
method fails fast before building the signed request or calling _http.request
when R2 credentials/bucket are missing. Use the existing R2StorageError path in
infrastructure/storage/r2.py and keep the current logging/error flow for real
upload failures, but return a clearer, actionable failure mode instead of
proceeding with empty credentials and a bad path.
In `@services/edge_cache/contract.py`:
- Around line 35-45: Add model-level validation in the edge cache contract model
so it matches entry.schema.json: enforce that `url` is present when `type` is
"redirect", `og_html` is present when `type` is "og_only", `status` only allows
301 or 302, and `og_html` does not exceed 32768 characters. Implement this
validation on the Pydantic model that defines `type`, `url`, `status`, and
`og_html` in the contract module, ideally with a model validator so all callers
are checked before `to_kv_json()` serializes the entry.
In `@services/meta_tags/validator.py`:
- Around line 107-117: The _invalidate method in Validator is reaching into
services.url_service for the private helper _v2_doc_to_cache, which creates
fragile cross-module coupling. Move the cache-conversion logic into a shared
utility module or make the helper a public API, then update _invalidate to
import and use that shared symbol instead of a private function from
services.url_service.
In `@workers/click_worker.py`:
- Around line 318-361: _extract the duplicated subscriber-pair setup from
_register_meta_image and _register_group into a shared helper that builds the
ClaimDeadLetterGuard, defines the reader/claimer callbacks, and registers both
StreamSub subscribers; parameterize it with the stream, group name, DLQ stream,
max_deliveries, batch/block/claim idle tunables, consumer suffix, and the
consume callable so both meta-image and group registrations reuse the same
generic registrar._
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 649fb762-6bb1-47fb-af41-80c59bd47f95
📒 Files selected for processing (70)
config.pydata/preview_bots.jsondependencies/auth.pydependencies/wiring.pyedge/spoo-edge-cache/contract/entry.schema.jsonedge/spoo-edge-cache/contract/fixtures.jsonedge/spoo-edge-cache/contract/preview_bots.jsonedge/spoo-edge-cache/src/bots.tsedge/spoo-edge-cache/src/index.tsedge/spoo-edge-cache/test/bots.spec.tsedge/spoo-edge-cache/test/index.spec.tserrors.pyinfrastructure/cache/meta_fetch_cache.pyinfrastructure/cache/url_cache.pyinfrastructure/cloudflare_kv.pyinfrastructure/queue_redis.pyinfrastructure/safe_fetch.pyinfrastructure/storage/__init__.pyinfrastructure/storage/r2.pyinfrastructure/storage/sigv4.pymiddleware/rate_limiter.pyrepositories/url_repository.pyroutes/api_v1/__init__.pyroutes/api_v1/_meta_gate.pyroutes/api_v1/management.pyroutes/api_v1/metadata.pyroutes/api_v1/shorten.pyroutes/legacy/url_shortener.pyroutes/redirect_routes.pyschemas/dto/requests/url.pyschemas/dto/responses/metadata.pyschemas/dto/responses/url.pyschemas/models/url.pyservices/click/bot_detection.pyservices/edge_cache/contract.pyservices/edge_cache/og_writethrough.pyservices/edge_cache/promotion.pyservices/edge_cache/render.pyservices/meta_preview.pyservices/meta_tags/__init__.pyservices/meta_tags/events.pyservices/meta_tags/images.pyservices/meta_tags/parse_html.pyservices/meta_tags/sinks.pyservices/meta_tags/validator.pyservices/url_service.pyshared/image_sniff.pytemplates/meta_preview.htmltemplates/preview.htmltests/integration/api_v1/test_meta_tags_api.pytests/integration/api_v1/test_metadata.pytests/integration/test_cache_resilience.pytests/integration/test_legacy_url_routes.pytests/integration/test_meta_preview.pytests/unit/infrastructure/test_cache.pytests/unit/infrastructure/test_safe_fetch.pytests/unit/infrastructure/test_storage.pytests/unit/schemas/models/test_url.pytests/unit/services/test_bot_detection.pytests/unit/services/test_edge_cache.pytests/unit/services/test_edge_contract.pytests/unit/services/test_feature_flag_service.pytests/unit/services/test_meta_image_validator.pytests/unit/services/test_meta_images.pytests/unit/services/test_og_writethrough.pytests/unit/services/test_parse_html.pytests/unit/services/test_url_service.pytests/unit/shared/test_image_sniff.pytests/unit/workers/test_click_worker_app.pyworkers/click_worker.py
Discovered via live tunnel recon: heymeta's fetcher identifies as MetadataScraper, doesn't follow redirects, and reported 'invalid metadata' on the 302. First real-world addition through the recon->add loop. metatags.io's 'Vercel Edge Functions' UA was seen too but deliberately skipped — it's every Vercel function on the internet, including programmatic consumers that want the redirect.
Ambiguous multiplication signs in comments/descriptions, nested with statements collapsed.
- safe_fetch: non-numeric Content-Length no longer raises uncaught ValueError (would crashloop the validator worker) - meta_preview: dest_host withheld from template context for block_bots links — even the hostname must not reach bots - PATCH meta_tags now stamps the writer's IP (updated_ip) via a new client_ip param on UrlService.update — abuse forensics survive edits - test assertions tightened to exact rendered markup (also resolves the two CodeQL URL-substring alerts)
Replaces the one-off _meta_gate helper with the flag-constant + require() pattern from the geo-targeting PR (#230) so both features gate identically; require() is copied verbatim for a clean merge. Also drops the plan-step numbering from added comments.
A 401/403 to our transparent validator UA (WAF / hotlink protection) says nothing about whether preview crawlers — whose UAs are widely allowlisted — can fetch the image. Classify it as FetchDeniedError and keep the user's image (dims just stay unrecorded); 429 now retries as transient instead of hard-failing.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
routes/api_v1/management.py (1)
96-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated meta_tags gate into a shared helper.
This block is near-duplicated in
routes/api_v1/shorten.py(lines 89-92) with intentional divergences (useroptionality and themodel_fields_setcheck). Since this is a security-gating path, independent copies risk drifting — e.g. the flag key, the verified-account check, or the error message getting updated in one place but not the other. A small helper (e.g.require_meta_tags_access(flag_service, user, provided: bool)) would keep both endpoints consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@routes/api_v1/management.py` around lines 96 - 102, The meta_tags access gate is duplicated between the management and shorten endpoints and can drift on security checks or messages. Extract the shared gating logic into a helper such as require_meta_tags_access using flag_service and user, and have both routes call it while preserving their differences (the model_fields_set/provided check in management and the optional user handling in shorten).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@routes/api_v1/management.py`:
- Around line 96-102: The meta_tags access gate is duplicated between the
management and shorten endpoints and can drift on security checks or messages.
Extract the shared gating logic into a helper such as require_meta_tags_access
using flag_service and user, and have both routes call it while preserving their
differences (the model_fields_set/provided check in management and the optional
user handling in shorten).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bae3f046-e002-40a3-a9be-070e9d1b9fca
📒 Files selected for processing (24)
config.pydependencies/wiring.pyinfrastructure/safe_fetch.pyroutes/api_v1/management.pyroutes/api_v1/shorten.pyroutes/redirect_routes.pyschemas/dto/requests/url.pyschemas/dto/responses/url.pyservices/edge_cache/og_writethrough.pyservices/feature_flag_service.pyservices/meta_preview.pyservices/meta_tags/events.pyservices/meta_tags/parse_html.pyservices/meta_tags/validator.pyservices/url_service.pytests/integration/api_v1/test_meta_tags_api.pytests/integration/api_v1/test_metadata.pytests/integration/test_legacy_url_routes.pytests/integration/test_meta_preview.pytests/unit/infrastructure/test_safe_fetch.pytests/unit/services/test_meta_image_validator.pytests/unit/services/test_url_service.pytests/unit/shared/test_image_sniff.pyworkers/click_worker.py
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/integration/test_legacy_url_routes.py
- config.py
- tests/integration/api_v1/test_metadata.py
- services/meta_preview.py
- services/meta_tags/events.py
- services/meta_tags/parse_html.py
- tests/unit/infrastructure/test_safe_fetch.py
- schemas/dto/requests/url.py
- tests/integration/api_v1/test_meta_tags_api.py
- services/meta_tags/validator.py
- tests/unit/services/test_meta_image_validator.py
- tests/unit/shared/test_image_sniff.py
- services/edge_cache/og_writethrough.py
- schemas/dto/responses/url.py
- tests/unit/services/test_url_service.py
- workers/click_worker.py
- routes/redirect_routes.py
- infrastructure/safe_fetch.py
- tests/integration/test_meta_preview.py
- services/url_service.py
META_TAGS_FETCH_USER_AGENT joins the other fetch_* tunables in MetaTagsSettings and threads through the image validator and the /metadata endpoint — self-hosters can brand their own UA.
Pure stdlib signing function with no I/O — shared/'s charter, next to image_sniff. infrastructure/storage keeps only the R2 client.
Dims are already sniffed and stored — surface the documented rendering cliffs in the existing warnings field: <200x200 (FB/WhatsApp may drop), <600px wide (FB demotes to small thumbnail), >4:1 ratio (WhatsApp drops). Warnings only, never rejections.
The DTO was computing domain rules; it now only serializes. The model already owns this species of platform knowledge (SVG rejection, length caps), and the warnings are a pure function of its own image_meta. Also removes the dead WHATSAPP_RELIABLE_BYTES from images.py.
# Conflicts: # dependencies/wiring.py # edge/spoo-edge-cache/contract/entry.schema.json # edge/spoo-edge-cache/contract/fixtures.json # edge/spoo-edge-cache/src/index.ts # edge/spoo-edge-cache/test/index.spec.ts # infrastructure/cache/url_cache.py # routes/api_v1/management.py # routes/api_v1/shorten.py # routes/legacy/url_shortener.py # schemas/dto/requests/url.py # schemas/dto/responses/url.py # services/edge_cache/contract.py # services/edge_cache/promotion.py # services/feature_flag_service.py # services/url_service.py # templates/preview.html # tests/unit/schemas/dto/test_url.py # tests/unit/schemas/models/test_url.py # tests/unit/services/test_edge_cache.py # tests/unit/services/test_edge_contract.py # tests/unit/services/test_url_service.py
The merge commit swept in a types file generated while a leftover .dev.vars was present, which types ORIGIN_OVERRIDE differently and fails CI's strict-vars=false check (the exact trap f8f5b07 documents).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/infrastructure/test_cache.py (1)
139-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting all six meta fields in the roundtrip test.
The decode test thoroughly checks all six meta fields default to
None, but the roundtrip test only verifies three (meta_title,meta_image,meta_color). Addingmeta_description,meta_image_width, andmeta_image_heightto the roundtrip assertion would catch serialization/alias issues for those fields too — particularlymeta_image_width/meta_image_heightwhich are numeric and could have subtle type coercion issues across JSON serialization.♻️ Proposed improvement
data = _url_data( - meta_title="T", meta_image="https://x/i.png", meta_color="`#112233`" + meta_title="T", + meta_description="D", + meta_image="https://x/i.png", + meta_color="`#112233`", + meta_image_width=1200, + meta_image_height=630, ) restored = UrlCacheData.model_validate_json(data.model_dump_json(by_alias=True)) assert restored.meta_title == "T" + assert restored.meta_description == "D" assert restored.meta_image == "https://x/i.png" assert restored.meta_color == "`#112233`" + assert restored.meta_image_width == 1200 + assert restored.meta_image_height == 630🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/infrastructure/test_cache.py` around lines 139 - 148, Update test_meta_fields_roundtrip_json to populate and assert all six metadata fields, adding meta_description, meta_image_width, and meta_image_height to the _url_data fixture input and corresponding restored-value assertions alongside the existing checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/infrastructure/test_cache.py`:
- Around line 139-148: Update test_meta_fields_roundtrip_json to populate and
assert all six metadata fields, adding meta_description, meta_image_width, and
meta_image_height to the _url_data fixture input and corresponding
restored-value assertions alongside the existing checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9897cd6-b6d8-48b2-9888-606f2b0fed7b
📒 Files selected for processing (38)
config.pydependencies/wiring.pyedge/spoo-edge-cache/contract/entry.schema.jsonedge/spoo-edge-cache/contract/fixtures.jsonedge/spoo-edge-cache/src/index.tsedge/spoo-edge-cache/test/index.spec.tsedge/spoo-edge-cache/worker-configuration.d.tsinfrastructure/cache/url_cache.pyinfrastructure/safe_fetch.pyinfrastructure/storage/r2.pyroutes/api_v1/management.pyroutes/api_v1/metadata.pyroutes/api_v1/shorten.pyroutes/legacy/url_shortener.pyroutes/redirect_routes.pyschemas/dto/requests/url.pyschemas/dto/responses/url.pyschemas/models/url.pyservices/edge_cache/contract.pyservices/edge_cache/promotion.pyservices/feature_flag_service.pyservices/meta_tags/images.pyservices/meta_tags/validator.pyservices/url_service.pyshared/sigv4.pytemplates/preview.htmltests/integration/api_v1/test_meta_tags_api.pytests/integration/test_cache_resilience.pytests/integration/test_legacy_url_routes.pytests/unit/infrastructure/test_cache.pytests/unit/infrastructure/test_storage.pytests/unit/schemas/dto/test_url.pytests/unit/schemas/models/test_url.pytests/unit/services/test_edge_cache.pytests/unit/services/test_edge_contract.pytests/unit/services/test_feature_flag_service.pytests/unit/services/test_url_service.pyworkers/click_worker.py
💤 Files with no reviewable changes (2)
- shared/sigv4.py
- services/meta_tags/images.py
✅ Files skipped from review due to trivial changes (3)
- services/feature_flag_service.py
- tests/integration/test_cache_resilience.py
- edge/spoo-edge-cache/worker-configuration.d.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- templates/preview.html
- tests/unit/services/test_edge_contract.py
- edge/spoo-edge-cache/test/index.spec.ts
- infrastructure/cache/url_cache.py
- tests/unit/services/test_edge_cache.py
- tests/integration/test_legacy_url_routes.py
- routes/api_v1/management.py
- routes/api_v1/metadata.py
- edge/spoo-edge-cache/src/index.ts
- routes/redirect_routes.py
- services/meta_tags/validator.py
- tests/unit/schemas/models/test_url.py
- tests/unit/infrastructure/test_storage.py
- services/edge_cache/promotion.py
- routes/legacy/url_shortener.py
- schemas/models/url.py
- config.py
- tests/integration/api_v1/test_meta_tags_api.py
- schemas/dto/requests/url.py
- tests/unit/services/test_url_service.py
- dependencies/wiring.py
- infrastructure/safe_fetch.py
- infrastructure/storage/r2.py
- workers/click_worker.py
- services/url_service.py
Zingzy
left a comment
There was a problem hiding this comment.
Review: custom meta-tags
Strong work — approve with a few should-fixes, nothing blocking. This is a much bigger and riskier surface than geo-targeting (outbound fetching, user-image hosting, a stored-HTML page served from origin, a new edge-serving mode), and the security fundamentals are handled with real care. The house patterns are followed almost everywhere, and the flag-gating now matches geo exactly.
I traced the load-bearing security properties directly:
- SSRF fetcher is solid — per hop it re-requires https, re-resolves the host, rejects if any A/AAAA is private (rebinding + split-horizon), connects to the resolved IP with SNI/Host preserved, and streams under a hard cap. Redirects re-validate from the top.
- Stored-XSS is covered —
meta_preview.htmlrenders user text intotext/htmlserved from the origin with no CSP, so Jinja autoescape is load-bearing. It's on (Starlette forcesautoescape=True),render.pyreuses the same env, no|safe, and the one inline script uses{{ long_url | tojson }}. Worth a defense-in-depth CSP on that response as a follow-up, since it all rests on autoescape staying on.
Should-fix (before merge)
/api/v1/metadataleaks a resolution oracle — the error string differentiates "host does not resolve" vs "resolves to a non-public address" vs content-type/status. An authed user can probe whether arbitrary hosts resolve and whether they're internal. Collapse to one generic message; keep detail in logs. (inline)_v2_doc_to_cacheimported privately from a third place — the deferredfrom services.url_service import _v2_doc_to_cachein the validator. Promote toUrlCacheData.from_v2_doc(). (inline)- Run
validate_meta_tagsblocklist checks off the event loop, like geo now does (d4a18fa). (inline)
Confirm-intended
- HEAD on an og-link now returns 200 text/html instead of 302 — uptime monitors / link scanners that HEAD will see 200-no-Location. No existing link regresses (meta_tags is new). (inline)
- Two image-size caps that can silently disagree — DTO hardcodes 700_000 chars while the real cap is
R2_UPLOAD_MAX_BYTES. (inline)
Product / abuse (roadmap, not this PR)
This is the cloaking primitive squared — stacked with geo, a link can vary destination by country and present a custom card. The mitigations here are the strongest part (positive-allowlist classification so search/Safe Browsing keep seeing the real destination, the + page showing the card next to the real destination, both-path blocklist re-checks, updated_ip forensics, self-hosted scannable image bytes). Two follow-ups:
- Abuse tooling + threat-intel scanning must read
meta_tags(title/description/image) and geo rules, not justlong_url— the card is the payload the victim sees. - spoo now hosts user images on
og.spoo.me— a content-moderation surface. Magic-byte gate +delete_objecttakedown exist; proactive perceptual-hash scanning is worth a roadmap item.
Notably good
The SSRF fetcher's transient/hard/denied taxonomy (esp. FetchDeniedError = indeterminate, don't clear the user's image), CAS-on-image_url concurrency, omit-not-null wire format for old-worker compat, and the self-host degradation matrix (no R2 → uploads rejected cleanly, https URLs still work; no queue Redis → sync-only; no edge → origin serves). Test coverage is thorough — cross-language classification matrix, per-audience edge fixtures, block_bots-omits-destination on both origin and write-through paths.
Settle #1 and #2 before merge; #3–#5 are fine as fast-follows.
Zingzy
left a comment
There was a problem hiding this comment.
Follow-up: correcting my verdict — there is a blocker
I ran two deeper adversarial passes (SSRF/image pipeline; edge worker + bot classification) after my first review and verified the findings against the repo's own .venv. My earlier "nothing blocking" was wrong — there's a real password-bypass. Escalating, plus a few verified SSRF/DoS should-fixes.
🔴 Blocker — password-protected links leak their destination via the preview page
A link with both a password and meta_tags (nothing forbids the combination — they're independent optional fields) hands its destination to anyone sending a preview-crawler UA or a HEAD request, at both origin and edge:
build_preview_contextwithholds the destination only forblock_bots, neverpassword_hash, solong_url+dest_host+ thelocation.replace(...)auto-redirect are in the rendered page for a password link. (inline)- The origin preview branch runs before the password gate; the edge
og_onlywrite-through fires for password links (it checksmeta_title+ ACTIVE only). curl -A 'Twitterbot/1.0' https://spoo.me/<code>(or-I, or any real unfurl) returns the destination + a JS redirect to it; a real browser correctly gets 401.test_password_og_link_serves_page_to_botsasserts onlystatus==200and no click — it never checks the destination is absent, unlike itsblock_botssibling, so the leak passes CI.
One-line fix at the choke point: reveal = not url.block_bots and not url.password_hash in build_preview_context (fixes origin, promotion, and write-through together), plus the missing "password omits destination" assertions on both sides.
SSRF / DoS should-fixes (verified in venv)
- CGNAT
100.64.0.0/10is classified as public —_is_public's flag-union misses it (ipaddress:is_private=False, is_reserved=False, is_global=False). EKS/GKE pod IPs live there.not addr.is_globalcloses it and also covers 6to4/Teredo/NAT64 in one stroke. (inline) - gzip decompression bomb — the fetcher inherits httpx's default
accept-encoding: gzip, deflate(it only overrides Host/User-Agent), soaiter_bytes()decompresses and the byte cap is checked post-decompression — ~60MB can land in one chunk before the cap trips, ×batch_sizein the validator. SendAccept-Encoding: identity. (inline) - No total request deadline — httpx
timeoutis per-operation (read timer resets per chunk), so a slow-drip server holds a validator coroutine / metadata handler open indefinitely under the 1MB cap. Wrap the streamed read in an overallasyncio.timeout().
Edge lifecycle should-fixes
og_onlyentries have no TTL and failed deletes don't self-heal — a failedputis safe (bots fall through to origin), but a faileddeleteon clear/deactivate leaves a persistent entry the edge keeps serving, with no TTL backstop. Giveog_onlya bounded TTL (re-synced on edit) or retry failed deletes. (inline)- Admin-block likely doesn't clear KV (verify) — admin
BLOCKEDstatus isn't set throughurl_service.update(), so I couldn't find anog_writethrough.removeon that path. Combined with the no-TTL persistence, a link blocked for abuse could keep unfurling its card + destination on social platforms while humans get 451. Please confirm the admin-block path invalidates KV. - Promotion vs write-through race — detached
promote()(CF calls stack to ~18s) can land aredirectentry carrying staleog_htmlafter a concurrent meta edit's write-through; no CAS between the two writers. Bounded by the promotion TTL, narrow trigger.
Hardening / nits
- R2 objects are served from
og.spoo.me(a spoo.me subdomain) with onlycontent-typeset — addX-Content-Type-Options: nosniff+Content-Disposition: attachment(to the SigV4-signed set) and prefer a cookieless bucket domain. Magic-byte gate + SVG rejection already make this defense-in-depth, not open XSS. - Upload happens before blocklist validation — a create that fails
validate_meta_tagshas already written the image to R2 (orphan, no GC). Validate text/destination before spending the R2 write. - No CI guard for the bot-classification twin —
contract-guard.ymltriggers only oncontract.py;preview_bots.jsonbyte-identity rests solely on the pytest job (the vitest side deep-equals parsed objects, so whitespace/order drift would pass there). Add the preview_bots paths to the guard. - SigV4 test asserts prefix + determinism, not the published AWS signature — a canonicalization bug that stayed deterministic would pass. Add a real known-answer vector (the construction itself is correct).
Confirmed good (so you can calibrate)
- XSS is safe — verified Starlette forces
autoescapeon.htmlfor both the request path andrender.py's request-free render; no|safe; the one inline script uses{{ long_url | tojson }}(htmlsafe). A CSP on that response is worthwhile defense-in-depth but autoescape is the real control. - SSRF core is genuinely well-built — true IP pinning (connect-to-validated-IP, SNI/cert stay on the real hostname, so
verify=Truecaps the blast radius of the CGNAT gap to liveness probing, not exfil), all-records-public, per-hop redirect re-validation, CAS on every image write. The findings above are the residue, not the core. - Cross-language classifier is functionally equivalent (same JSON, same substring semantics) and byte-identity is unit-tested;
block_botsdestination withholding is thorough (URL + hostname) and tested on both sides.
Net: fix the password-leak blocker before merge; the SSRF/DoS trio and the edge-lifecycle items are the next tier.
Keys are content-addressed, so objects never change — store cache-control: public, max-age=31536000, immutable at upload time so the bucket's custom domain and platform caches hold them forever.
Raw ObjectIds embed the account creation timestamp and let anyone correlate a user's links from public og:image URLs. Keys now use HMAC-SHA256(SECRET_KEY, owner_id)[:16] — takedown sweeps, dedup, and cross-owner isolation all survive; the id no longer leaks.
- meta_preview: withhold destination for password links too (the branch runs before the password gate; a crawler UA bypassed protection) - safe_fetch: reject CGNAT/transitional via is_global; drop gzip (decompression bomb); wall-clock deadline for slow-drip reads - metadata: generic client error, resolve-reason only to logs - og_writethrough: bounded TTL backstop for missed deletes / out-of-band block; configurable via EDGE_CACHE_OG_TTL_SECONDS - validate_meta_tags: blocklist scan off the event loop (geo parity) - UrlCacheData.from_v2_doc classmethod — kills the private cross-module import; projection lives on the model - storage keys already HMAC the owner id (prior commit)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/url_service.py (1)
846-853: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRefresh the OG KV entry on
passwordorblock_botsedits
services/url_service.py:846-853only re-syncs onmeta_tags,long_url,status,alias, ordomain. Changingpasswordorblock_botson an existing OG link updatesupdate_ops, but this gate skipssync(), so the KV entry can keep serving the old preview HTML until the TTL expires. Add both fields to the trigger set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/url_service.py` around lines 846 - 853, Update the relevant-field trigger set in the OG writethrough logic of the URL update method to include both "password" and "block_bots", ensuring _og_writethrough.sync() runs when either field changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@services/url_service.py`:
- Around line 846-853: Update the relevant-field trigger set in the OG
writethrough logic of the URL update method to include both "password" and
"block_bots", ensuring _og_writethrough.sync() runs when either field changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b67cd0d4-6da2-46c4-bf65-75347636640b
📒 Files selected for processing (19)
config.pydependencies/wiring.pyinfrastructure/cache/url_cache.pyinfrastructure/safe_fetch.pyinfrastructure/storage/r2.pyroutes/api_v1/metadata.pyschemas/dto/requests/url.pyservices/edge_cache/og_writethrough.pyservices/meta_preview.pyservices/meta_tags/images.pyservices/meta_tags/validator.pyservices/url_service.pytests/integration/test_cache_resilience.pytests/integration/test_meta_preview.pytests/unit/infrastructure/test_safe_fetch.pytests/unit/infrastructure/test_storage.pytests/unit/services/test_meta_images.pytests/unit/services/test_og_writethrough.pytests/unit/services/test_url_service.py
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/integration/test_cache_resilience.py
- dependencies/wiring.py
- tests/unit/services/test_meta_images.py
- tests/unit/infrastructure/test_storage.py
- infrastructure/cache/url_cache.py
- schemas/dto/requests/url.py
- routes/api_v1/metadata.py
- infrastructure/safe_fetch.py
- config.py
- services/meta_tags/validator.py
- tests/unit/infrastructure/test_safe_fetch.py
- infrastructure/storage/r2.py
- tests/integration/test_meta_preview.py
- tests/unit/services/test_og_writethrough.py
- tests/unit/services/test_url_service.py
asyncio.timeout is 3.11+; the repo supports 3.10. Extract the capped body read into a coroutine and bound it with wait_for instead.
Defence-in-depth for user-supplied bytes served from the R2 bucket: Content-Disposition: inline is real R2 object metadata (stored + served); X-Content-Type-Options: nosniff is added to the signed set as belt to the CF Transform Rule braces (R2 doesn't guarantee echoing it). Both ride the SigV4 SignedHeaders so they can't be stripped in transit.
Per-link custom social previews: link owners set
title,description,image, andcolor(Discord embed accent), and link-preview crawlers (WhatsApp, iMessage, Discord, Slack, Telegram, facebookexternalhit, Twitterbot, …) receive a prerendered HTML page carrying the full OG + Twitter tag set. Humans, search engines, AI crawlers, and generic scrapers keep today's 302 byte-for-byte.Serving architecture (edge-first)
meta_preview.htmlonce and eagerly writes anog_onlyentry to Workers KV. The edge worker answers preview bots straight from KV (~0ms origin); humans always pass through so origin keeps click tracking. Hot og-links getredirectentries with embeddedog_htmlvia the existing hotness promotion.wants_preview()matches ~65 source-verified preview-crawler tokens (data/preview_bots.json, byte-identical copy in the edge contract dir, pinned by tests on both sides) plus HEAD and?bot=1(debug view). Search/AI crawlers simply don't match — no cloaking exposure, destination keeps its SEO/link equity. In-app-browser tokens (Line/, bare Pinterest/Viber/Snapchat) are regression-tested as humans.request.cf.verifiedBotCategoryis wired as an additive edge signal but ships log-only (cf_categories: []) until recon on the real zone records the runtime strings.og_onlytype and ignoreog_htmlon redirect entries — no lockstep deploy needed.API (feature is write-gated)
meta_tagsobject onPOST /api/v1/shortenandPATCH /api/v1/urls/{id}(whole-object replace,nullclears; clearing is never gated). Requires a verified account with thecustom_meta_tagsflag (403 otherwise). Blocklist regexes run over all text fields plus a destination re-check on every meta write.imageaccepts an https URL or adata:image/png|jpeg|webp;base64URI — decoded, magic-byte matched against the declared type, capped at 512KB, stored content-addressed in R2 underog/{owner_id}/(self-hosted bytes are scannable and takedown-able; no TOCTOU). R2 client is ~50 lines of stdlib SigV4 over the existing httpx client — zero new dependencies (imghdris gone in 3.13, so there's a stdlib magic-byte sniffer too).events:meta-image) +meta-imageconsumer group in the click worker, SSRF-guarded fetch (DNS resolved first, private/link-local/IPv4-mapped-IPv6 rejected, connection pinned to the validated IP, per-hop re-checks), CAS-filtered writes so a racing user edit is never clobbered. Success records dims → the served page gainsog:image:width/height.GET /api/v1/metadata?url=— prefill companion: fetches the destination through the same guarded fetcher, parses tags with a head-only stdlib parser, Redis-cached 1h (5m negative), auth-required, 20/min.CurrentUser.tiernow carriesUserDoc.plan, so flipping the flag ALLOWLIST→TIER at paid launch is a pure data change./{alias}+preview page shows the custom card next to the real destination (anti-phishing transparency), andblock_botsog-links never leak their destination into the bot page.Testing
wrangler dev+ explorer-API KV write-through, async image validation against a live remote image, data-URI upload to MinIO via the SigV4 client, click recording through the stream for edge passthroughs (and none for preview serves), SSRF rejection of metadata-endpoint probes.Rollout notes
feature_flags:{name: "custom_meta_tags", enabled: true, rollout_type: "allowlist", allowlist_user_ids: [...]}(values are lowercase).verifiedBotCategoryrecon (strings land incf_categoriesin BOTHpreview_bots.jsoncopies). Empty = UA-regex-only, fully functional.R2_*env vars to enable uploads; without them https image URLs work and uploads reject with a clear error. Self-hosts without CF/queue Redis degrade gracefully (origin serves previews, validation skips).Summary by CodeRabbit
New Features
Bug Fixes