fix(clearfolio): require canonical artifact-origin allowlist entries - #538
Conversation
Reject non-canonical CLEARFOLIO_ARTIFACT_ORIGINS lookalikes before provider transport, cover protocol-relative and userinfo-as-host redirects, and put the operator control in deploy/API docs so a reviewed CDN origin can be configured without guessing. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Landing vehicle for the #504 origin-policy slice
Head 0c193aa28e730e5c1a18e7d740637f34efbbd95d on fix/clearfolio-provider-response-boundary.
This PR keeps #504's fail-closed allowlist and adds the residuals that blocked treating #504 as complete:
- allowlist entries must equal
URL.originor that origin plus/; - protocol-relative, backslash-normalized, and userinfo-as-host redirects are default-deny regressions;
- empty and whitespace
CLEARFOLIO_ARTIFACT_ORIGINSfail before provider transport; - deploy/API/README and parent doctoring records now describe the control.
Prefer this over #504 and #535. Do not land those in parallel. Local Clearfolio unit files passed on this head. Exact-head repository and organization checks were still pending at this comment; pending evidence is non-passing.
Do not fold destination DNS/IP authorization or 302 path-shape limits into this PR. That is the next #489 slice.
I cannot approve or merge this automation-authored PR. Independent current-head approval is required after checks go green.
Sent by Cursor Automation: Fix Issues
…arent Rebuild the artifact-origin child from the exact current #498 parent tree while preserving the child's bounded Clearfolio origin-policy delta. Shared package and changelog paths retain both the protected orchestrator attribution work and the child origin-policy coverage.
…igin parent Advance the Clearfolio capability-readiness child onto the exact current #538 parent without changing its seven-file semantic delta.
opencode-agent
Bot
added
priority: medium
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if ( | ||
| url.origin !== clearfolioUrl.origin | ||
| || url.username | ||
| || url.password | ||
| || url.hash | ||
| Boolean(url.username + url.password) | ||
| || Boolean(url.hash) | ||
| || !trustedArtifactOrigins.has(url.origin) | ||
| ) { | ||
| throw new Error('clearfolio artifact-link response invalid'); | ||
| } |
There was a problem hiding this comment.
📝 Info: Default-case guard stays equivalent after refactor
With no CLEARFOLIO_ARTIFACT_ORIGINS, trustedArtifactOrigins holds only the provider origin, so !has(url.origin) matches the old url.origin !== clearfolioUrl.origin. Boolean(url.username + url.password) is truthy exactly when either credential is non-empty, matching the old url.username || url.password. The refactor at clearfolio.mjs preserves default behavior and only adds allowlist trust.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const token = url.searchParams.get('artifactToken'); | ||
| if (token) { | ||
| if (token && url.origin === clearfolioUrl.origin) { | ||
| return `${configuration.baseUrl}/viewer/${encodeURIComponent(canonicalJobId)}?artifactToken=${encodeURIComponent(token)}`; | ||
| } |
There was a problem hiding this comment.
📝 Info: Token rewrite scoped to provider origin
The viewer rewrite at clearfolio.mjs fires only when url.origin === clearfolioUrl.origin, so an approved cross-origin token stays on its own URL instead of being transplanted into the trusted viewer route.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const abortFromCaller = controller.abort.bind(controller); | ||
| if (callerSignal !== undefined) { | ||
| if (callerSignal.aborted) controller.abort(callerSignal.reason); | ||
| else callerSignal.addEventListener('abort', abortFromCaller, { once: true }); | ||
| } |
There was a problem hiding this comment.
📝 Info: Caller abort reason not forwarded on async cancel
When the caller signal aborts asynchronously, abortFromCaller runs as an event handler, so controller.abort(event) records the DOM Event as the reason instead of callerSignal.reason that AbortSignal.any previously preserved. No caller reads it — all downstream catches collapse to fixed sanitized messages — so behavior is unaffected. The synchronous already-aborted branch still forwards the real reason.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function clearfolioArtifactOrigins(baseUrl) { | ||
| const trustedOrigins = new Set([new URL(baseUrl).origin]); | ||
| const configuredOrigins = process.env.CLEARFOLIO_ARTIFACT_ORIGINS; | ||
| if (configuredOrigins === undefined) return trustedOrigins; |
There was a problem hiding this comment.
📝 Info: Allowlist read per-call, unlike other config
CLEARFOLIO_ARTIFACT_ORIGINS is read from process.env on every artifactUrl call, while CF_URL_INPUT and CF_SECRET are captured once at module load. Functionally fine since server env is fixed before startup, but the mixed timing is inconsistent and can surprise future maintainers.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Buyer and security impact
Clearfolio artifact links must never turn arbitrary cross-origin HTTPS responses into trusted ScopeWeave redirects. This bounded child keeps the fail-closed artifact-origin policy and makes its operator contract explicit: allowlist entries must be canonical
URL.originvalues (or that origin plus/), protocol-relative and userinfo-as-host lookalikes stay rejected, andCLEARFOLIO_ARTIFACT_ORIGINSis documented where operators configure the service.This supersedes the already-closed #504/#535 origin-policy vehicles. Do not land those in parallel. This PR remains a bounded follow-up to #489 and does not close #489.
Exact current stack
fix/clearfolio-provider-response-boundary@c7d0dc75e059d5ca4e9848750b67e2905945331d;bbc9de2d5e39fcf94735325812017e4a25014c1c;developobserved for this reconciliation:df0fa17bd5035af6455c889022c540b4f439e3d6;The prior child head
3d98ed25f40335eeb28c834eed6c868651b48a44was still rooted at the stale #498 snapshot5fa795210b0060f3034466c03f99fb013e838d13, leaving it three parent commits behind and mechanically unmergeable. Commitbbc9de2d...reconstructs the child from the exact current #498 tree, overlays only the bounded artifact-origin semantic paths, and deliberately merges the two shared paths (CHANGELOG.md,package.json) so both the parent contextual-orchestrator attribution changes and child Clearfolio coverage/contract remain present.Fresh parent→child comparison is
ahead, zero behind, has exact live parentc7d0dc75...as merge base, and contains exactly these 12 intended files:ARCHITECTURE.md;CHANGELOG.md;README.md;docs/api.md;docs/deploy.md;docs/doctoring/clearfolio-artifact-origin-trust.md;docs/doctoring/clearfolio-production-configuration.md;docs/doctoring/clearfolio-provider-response-boundary.md;package.json;server/clearfolio.mjs;tests/unit/clearfolio-artifact-origin.test.mjs;tests/unit/clearfolio-status-signal.test.mjs.No parent-only orchestrator source/tests/docs were deleted by the reconstruction.
Contract
CLEARFOLIO_ARTIFACT_ORIGINSentries must equal the parsed origin or that origin plus/, so noncanonical entries such as explicit default ports, empty fragments/userinfo, path normalization tricks, and empty elements fail closed before provider transport;artifactTokenmay be translated to the local viewer, and an approved cross-origin token remains bound to its issuing URL.Still out of scope
Destination DNS/IP authorization for private, loopback, and metadata address families plus constraining redirects to an accepted signed-artifact URL shape remain later #489 slices. Do not widen this child into those boundaries.
Exact-head evidence
Every workflow/check/review result predating
bbc9de2d5e39fcf94735325812017e4a25014c1cis historical and non-authorizing. Fresh exact-head repository workflows were queued immediately after this reconciliation: Dependency Review32317490096, Server Tests32317490198, and OSV Scanner32317490562. Queued/pending/skipped-required/cancelled/absent/neutral/failed/stale/predecessor/status-only/author-only/model-only evidence is non-passing. The only currently enumerated formal review is a predecessor-head Cursor COMMENTED review and is not current independent approval.Merge gate
Remain Draft and do not integrate before #498/#493. After the prerequisite stack reaches protected
develop, reconcile or retarget this exact bounded semantic diff onto the then-live protected head and require all then-applicable exact-head CI/browser/owned coverage/docstring, SAST/security/dependency/supply-chain/package/provenance/review gates, zero valid unresolved findings, and qualifying independent approval under live governance. No predecessor-head evidence transfers.Refs #489