feat: add reviews sync Cloudflare Worker#296
Conversation
Standalone D1-backed Worker at workers/reviews/ that receives synced marketplace reviews from user daemons and serves them publicly. Routes: GET / (health), GET /api/reviews (paginated public read with 30s cache), POST /api/reviews/sync (batch upsert from daemon). Security: rate limiting (5 syncs/IP/60s), X-Signet-Sync header gate, full input validation (UUID, timestamps, field lengths, rating bounds), 512KB body cap, idempotent upserts. CORS restricted to marketplace domain + localhost. Deployed to signet-reviews.nicholaivogelfilms.workers.dev on Nicholai's Cloudflare account, D1 database ID d1fcce93-0b8a-4b95-b67d-39e9611dd4b3.
|
Hi @NicholaiVogel - I'm @NicholaiVogel's PR-reviewing agent powered by pr-reviewer. I'm taking a look at the feature work in |
NicholaiVogel
left a comment
NicholaiVogel
left a comment
There was a problem hiding this comment.
Three issues warrant attention before merge: (1) the sync endpoint has no real authentication — X-Signet-Sync:1 is public knowledge and any caller can inject arbitrary reviews; (2) the upsert has no staleness guard, so re-syncing old data silently overwrites newer reviews; (3) db.batch() exceptions are not caught, producing uncorsed 500s on D1 errors. Everything else — parameterized queries, body-size cap, UUID/timestamp validation, CORS logic — is solid.
Confidence: 8.4/10
- Style consistency & maintainability: 9
- Repository conventions adherence: 8
- Merge conflict detection confidence: 9
- Security vulnerability detection confidence: 9
- Injection risk detection confidence: 9
- Attack-surface risk assessment confidence: 8
- Future hardening guidance confidence: 8
- Scope alignment confidence: 9
- Existing functionality awareness: 9
- Existing tooling/pattern leverage: 8
- Functional completeness confidence: 8
- Pattern correctness confidence: 8
- Documentation coverage confidence: 7
{
"style_maintainability": 9,
"repo_convention_adherence": 8,
"merge_conflict_detection": 9,
"security_vulnerability_detection": 9,
"injection_risk_detection": 9,
"attack_surface_risk_assessment": 8,
"future_hardening_guidance": 8,
"scope_alignment": 9,
"duplication_awareness": 9,
"tooling_pattern_leverage": 8,
"functional_completeness": 8,
"pattern_correctness": 8,
"documentation_coverage": 7
}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a4415a293
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Allow requests with no Origin header (server-to-server daemon sync was being 403'd because isOriginAllowed returned false for null origin) - Add staleness guard on upsert (WHERE excluded.updated_at >= reviews.updated_at) to prevent stale re-syncs from overwriting newer data - Wrap db.batch() in try/catch so D1 errors return structured JSON with CORS headers instead of uncorsed 500
|
Hi @NicholaiVogel - quick follow-up pass on |
NicholaiVogel
left a comment
NicholaiVogel
left a comment
There was a problem hiding this comment.
Two of three prior issues are resolved: the staleness guard (WHERE excluded.updated_at >= reviews.updated_at) correctly prevents stale re-syncs from overwriting newer data, and the db.batch() try/catch prevents bare 500s on D1 errors. The original critical issue — unauthenticated write access — remains unaddressed. The isOriginAllowed change (no-Origin → allow) is not a security regression (CORS has never gated non-browser callers), but it makes explicit that the sync endpoint is intentionally open to any server-to-server caller, which sharpens the auth gap. One new correctness issue: when db.batch() throws, the handler still returns HTTP 200 with success: true, making a full DB failure indistinguishable from a stale-batch no-op.
Confidence: 8.5/10
- Style consistency & maintainability: 9
- Repository conventions adherence: 8
- Merge conflict detection confidence: 9
- Security vulnerability detection confidence: 9
- Injection risk detection confidence: 9
- Attack-surface risk assessment confidence: 9
- Future hardening guidance confidence: 8
- Scope alignment confidence: 9
- Existing functionality awareness: 9
- Existing tooling/pattern leverage: 8
- Functional completeness confidence: 8
- Pattern correctness confidence: 8
- Documentation coverage confidence: 7
{
"style_maintainability": 9,
"repo_convention_adherence": 8,
"merge_conflict_detection": 9,
"security_vulnerability_detection": 9,
"injection_risk_detection": 9,
"attack_surface_risk_assessment": 9,
"future_hardening_guidance": 8,
"scope_alignment": 9,
"duplication_awareness": 9,
"tooling_pattern_leverage": 8,
"functional_completeness": 8,
"pattern_correctness": 8,
"documentation_coverage": 7
}
Move db.batch() error handling from upsertReviews into handleSync so storage failures return HTTP 500 with CORS headers instead of 200 with accepted: 0 (which the daemon would interpret as a successful sync where all reviews were stale).
|
Hi @NicholaiVogel - quick follow-up pass on |
NicholaiVogel
left a comment
NicholaiVogel
left a comment
There was a problem hiding this comment.
The prior correctness bug (db.batch() exceptions producing HTTP 200 success:true) is now fixed — the try/catch was correctly moved up to handleSync so D1 failures return a proper 500 with CORS headers and an error body. The staleness guard remains in place. However, the critical unauthenticated write issue flagged in both prior reviews is still unaddressed: any caller who knows the static header (X-Signet-Sync: 1) and payload shape can inject or overwrite arbitrary reviews for any target. Three reviews in, this remains the single blocking issue.
Confidence: 8.5/10
- Style consistency & maintainability: 9
- Repository conventions adherence: 8
- Merge conflict detection confidence: 9
- Security vulnerability detection confidence: 9
- Injection risk detection confidence: 9
- Attack-surface risk assessment confidence: 9
- Future hardening guidance confidence: 8
- Scope alignment confidence: 9
- Existing functionality awareness: 9
- Existing tooling/pattern leverage: 8
- Functional completeness confidence: 7
- Pattern correctness confidence: 8
- Documentation coverage confidence: 8
{
"style_maintainability": 9,
"repo_convention_adherence": 8,
"merge_conflict_detection": 9,
"security_vulnerability_detection": 9,
"injection_risk_detection": 9,
"attack_surface_risk_assessment": 9,
"future_hardening_guidance": 8,
"scope_alignment": 9,
"duplication_awareness": 9,
"tooling_pattern_leverage": 8,
"functional_completeness": 7,
"pattern_correctness": 8,
"documentation_coverage": 8
}
1 participant
Summary
workers/reviews/for aggregating marketplace reviewssignet-reviews.nicholaivogelfilms.workers.devRoutes
GET /GET /api/reviewsPOST /api/reviews/syncSecurity
X-Signet-Sync: 1header required on POSTStructure
Lives at
workers/reviews/, outsidepackages/to avoid polluting the bun workspace and build graph. Independent deployable with its own wrangler.toml.Pair with
Test plan
GET /returns{"ok":true,"service":"signet-reviews"}GET /api/reviewsreturns empty set with summaryPOST /api/reviews/syncaccepts valid daemon payloadX-Signet-Syncheader returns 400