fix(cloudflare): honor client targetHost + add target-host suggestion endpoint (LLMO-7275) - #3177
fix(cloudflare): honor client targetHost + add target-host suggestion endpoint (LLMO-7275)#3177adityamisra08 wants to merge 10 commits into
Conversation
… site domain LLMO-7275: the frontend's editable "Target hostname" field was silently discarded on deploy, and the backend had no equivalent to the frontend's own base-URL domain check (e.g. stage.frescopa.com onboarding against a frescopa.com base URL). deployWorker now accepts targetHost from the request body and validates it with hostInSiteDomain — the same rule addRoute already enforces — falling back to server-side derivation when omitted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
LLMO-7275: the frontend prefilled its "Target hostname" field with a raw hostname parse of the site's base URL, with no visibility into the server-side canonical-host derivation (apex-to-www normalization, same-domain redirect follow) that deployWorker uses when targetHost is omitted. That resolution only ever surfaced after a real deploy. Adds GET .../cloudflare/target-host: runs the same site lookup, access control, and resolveCanonicalHost the deploy path uses, but performs no deploy or persistence — a pure preview the wizard can call before the user commits to deploying. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Hey @adityamisra08,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - one API contract gap in the new endpoint's OpenAPI spec.
Complexity: HIGH - medium diff; API surface risk flag.
Changes: Adds optional client-supplied targetHost override to the Cloudflare deploy endpoint with domain validation, and a new read-only target-host preview endpoint (8 files).
Note: Recommend a human read before merge - this change modifies the OpenAPI contract (shared-contract change). The bot review is a complement to, not a replacement for, a human read here.
Must fix before merge
- [Important] Missing
501response in new target-host OpenAPI spec -docs/openapi/llmo-api.yaml:~5975(details inline)
Non-blocking (4): minor issues and suggestions
- nit:
context.datais destructured twice with separate|| {}guards indeployWorker- merge into one statement -src/controllers/llmo/llmo-cloudflare.js:401 - suggestion: Add
format: hostnameor a pattern constraint to thetargetHostfield in the deploy OpenAPI request schema for explicit boundary validation -docs/openapi/llmo-api.yaml:6148 - nit: Test assertions for error responses (400/500) should verify response body messages, not just status codes -
test/controllers/llmo/llmo-cloudflare.test.js:567 - suggestion: Add a whitespace-trimming edge case test for
targetHost(e.g.' www.example.com ') to confirm.trim()fires in practice -test/controllers/llmo/llmo-cloudflare.test.js
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 7m 50s | Cost: $8.99 | Commit: 6702e33261f97b616db7956551d78a9ccd512c3a
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| '404': | ||
| $ref: './responses.yaml#/404' | ||
| '500': | ||
| $ref: './responses.yaml#/500' |
There was a problem hiding this comment.
issue (blocking): The implementation returns 501 for subpath sites (via getSiteAndCheckAccess - confirmed by the test at llmo-cloudflare.test.js:225), but this response is not documented in the OpenAPI spec for the new target-host endpoint. The spec lists 200, 403, 404, and 500 only.
Since this is a new endpoint, the contract should be complete from the start. Other Cloudflare onboarding endpoints may have the same omission, but this PR should not carry it forward.
Fix: Add a 501 response entry:
'501':
description: CDN auto-routing is not supported for subpath sitesPlace it between the 500 and security entries to maintain status-code ordering.
MysticatBot
added
ai-reviewed
Resolves reviewer comment on PR #3177 — getSiteAndCheckAccess returns 501 for subpath sites, but the new endpoint's spec only listed 200/403/404/500.
GitHub Actions
Inactive
| $ref: './llmo-api.yaml#/site-llmo-edge-optimize-routing' | ||
| /sites/{siteId}/llmo/cdn-onboard/cloudflare/config: | ||
| $ref: './llmo-api.yaml#/site-llmo-cloudflare-config' | ||
| /sites/{siteId}/llmo/cdn-onboard/cloudflare/target-host: |
There was a problem hiding this comment.
Can we merge this api with the config api instead of having a separate one just for host?
There was a problem hiding this comment.
Ok I will make the changes.
Per review feedback, GET .../cloudflare/target-host duplicated the config
endpoint's access-control guard for no benefit — both are read-only, need
no Cloudflare token, and share the same 501 subpath-site case. config now
returns { clientId, targetHost } instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ci/it-postgres was red on this branch — the config endpoint merge (1ae5ebb) added targetHost to the response but this pre-existing IT assertion still expected clientId only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
Inactive
… docstring hostInSiteDomain only checks domain-suffix membership, so a value with an embedded newline could still pass it and reach auditLine's log line unescaped, forging a fake [llmo-cf] entry. Reject non-hostname-shaped or overlong targetHost values before the domain check, and collapse CR/LF in auditLine's fmt as defense in depth. Also fixes deployWorker's docstring, which still claimed targetHost is never client-supplied. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e derived getCloudflareConfig bundled a static clientId (OAuth bootstrap) with a live-probed targetHost derivation, so a derivation failure 500'd the whole response even though clientId was already known. Matches the existing best-effort secondary-read pattern (see llmo.js saveStrategy): targetHost is now omitted on failure instead of failing the request, and the OpenAPI schema reflects it as optional. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
3 participants
Summary
deployWorkernow accepts and honors a client-suppliedtargetHost, validated withhostInSiteDomain(the same ruleaddRoutealready uses) —400if it's outside the site's domain, otherwise used verbatim. Falls back to the existing server-sideresolveCanonicalHostderivation only when omitted.GET /sites/:siteId/llmo/cdn-onboard/cloudflare/target-hostendpoint: runs the same site lookup, access control, andresolveCanonicalHostderivationdeployWorkeruses, but performs no deploy or persistence — lets the frontend preview/suggest a target host before the user reaches the deploy step, instead of only finding out after a real deploy.Addresses the first two bullets of LLMO-7275 (Cloudflare Target Hostname Issues). The third bullet (worker-name-conflict flow should point the customer to manual documentation) is a separate, not-yet-implemented change.
Test plan
npm run lintnpx mocha test/controllers/llmo/llmo-cloudflare.test.js(new/updated tests: reject out-of-domain targetHost, honor exact/subdomain targetHost, newgetTargetHostdescribe block)npx mocha test/routes/facs-capabilities.test.js test/routes/required-capabilities.test.js test/routes/capability-constants.test.js test/routes/index.test.jsnpm run docs:lint(0 errors; same pre-existing warning count)npm test— 17518/17518 passingJira: https://jira.corp.adobe.com/browse/LLMO-7275