fix(clearfolio): require explicit artifact origin trust - #504
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review (2954f1a65491aae49e674bbe9a4c8d243ec54608)
The origin-trust core is sound. Canonical URL.origin comparison, fail-closed allowlist before fetch, and no cross-origin artifactToken transplant into /viewer/... all hold. I did not find an origin-comparison bypass or a default-open redirect.
This remains a Draft stacked on #498. Do not merge #504. Checks on this head were still queued at review time; pending/skipped/author-only evidence is non-passing.
Residual on this head
- Allowlist grammar is looser than the written “origin only” rule.
#, empty userinfo,/foo/.., and:443survive component checks after WHATWG normalization. - Default-deny tests omit protocol-relative, backslash-normalized, and userinfo-as-host shapes.
docs/deploy.md,docs/api.md, and the parent doctoring records still describe the pre-allowlist contract, so an operator cannot configure a reviewed CDN origin from the deploy table.
Landing vehicle
Those residuals are fixed on 0c193aa28e730e5c1a18e7d740637f34efbbd95d (cursor/bc-d8443854-d031-4b84-b085-56f439fcbf05-df45). Prefer that successor over this PR. Do not land both.
Next #489 slice (do not fold in)
Reject private, loopback, link-local, and metadata IP literals in both the allowlist and returned URLs, then constrain the attachment 302 so a trusted origin cannot become an arbitrary in-origin redirector.
I cannot merge from this automation. Independent current-head approval is still required after exact-head checks go green.
Sent by Cursor Automation: Fix Issues
| await resolveArtifact('/signed/file.pdf', undefined), | ||
| 'https://clearfolio.example/signed/file.pdf', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Default-deny currently covers only an absolute https://cdn.example URL and a same-origin relative path. The live URL parser also treats //evil.com/file.pdf, /\evil.com/file.pdf, and https://clearfolio.example@evil.com/x?artifactToken=t as foreign origins. Those shapes already fail here because url.origin is compared, but a later “relative means same-origin” shortcut would reopen the 302 without failing this suite.
Add those three cases next to this test. The successor on cursor/bc-d8443854-d031-4b84-b085-56f439fcbf05-df45 already does.
| 'CLEARFOLIO_ARTIFACT_ORIGINS must contain only comma-separated HTTPS origins.', | ||
| ); | ||
| } | ||
| trustedOrigins.add(url.origin); |
There was a problem hiding this comment.
After WHATWG normalization this accepts https://cdn.example#, https://:@cdn.example, https://cdn.example/foo/.., and https://cdn.example:443 even though the doctoring record says “origin only.” Compare the trimmed entry to url.origin or ${url.origin}/ before adding it to the set, and add empty/whitespace env values to the fail-closed-before-transport list. Platforms that inject an empty CLEARFOLIO_ARTIFACT_ORIGINS already fail closed; the suite should prove it.
|
Superseded after fresh exact-head comparison. PR #538 head |
1 participant
Buyer and security impact
Clearfolio artifact links must never turn an arbitrary cross-origin HTTPS response into a trusted ScopeWeave redirect target. This bounded stacked slice makes artifact authority explicit: the configured Clearfolio origin is trusted by default, and operators may add only exact, reviewed HTTPS artifact origins.
This is a follow-up slice of #489. It is stacked on #498 and does not close #489.
Exact current stack and scope
Current parent:
fix/clearfolio-provider-response-boundary@d4b09cddb61f524efaa6d93e12a006f6a51afd3c(#498).Current child head:
2954f1a65491aae49e674bbe9a4c8d243ec54608.Fresh parent→child comparison is ahead-only, zero-behind, and has the current parent as the exact merge base. The semantic child delta is limited to exactly six artifact-origin-policy files:
CHANGELOG.md(+5/-0 relative to the current parent);docs/doctoring/clearfolio-artifact-origin-trust.md;package.json;server/clearfolio.mjs;tests/unit/clearfolio-artifact-origin.test.mjs;tests/unit/clearfolio-status-signal.test.mjs.No database, auth/session, attachment-refresh engine, dependency lockfile, workflow, scanner suppression, protected Microsoft Project XML behavior, contextual-orchestrator contract, or branch-protection artifact is changed by the final parent→child delta.
Reconciliation discipline
The parent advanced materially while retaining the same branch. This reconciliation therefore did not reuse the stale child tree. Commit
2954f1a6...was built from the exact current parent tree and overlaid only the six child-owned semantic paths. The sharedCHANGELOG.mdwas merged explicitly so the parent’s Clearfolio transport entry, protected XML-import entry, and other parent history remain intact while adding only the artifact-origin policy entry.Fresh comparison proves the current parent as merge base, zero commits behind, and exactly the intended six-file delta. Every later parent move requires the same parent-tree-first reconstruction and semantic shared-file merge before this child may advance.
Implemented contract
CLEARFOLIO_ARTIFACT_ORIGINS, when present, accepts only comma-separated exact HTTPS origins without credentials, path, query, fragment, or empty entries;clearfolio_artifact_origins_invalidbefore provider transport;URL.originvalues;artifactTokenvalues may be translated into the trusted Clearfolio viewer route;Evidence boundary
All workflow/check/review evidence predating
2954f1a65491aae49e674bbe9a4c8d243ec54608is historical and non-authorizing. Fresh exact-head repository and organization workflows are required. Pending, queued, skipped-required, neutral, absent, stale, predecessor, status-only, author-only, or model-only evidence is non-passing.This PR remains Draft until the unchanged current head has terminal-success applicable checks, zero valid unresolved findings, and qualifying independent current-head approval under the live policy.
Remaining #489 work
This slice does not complete the Clearfolio production lifecycle. The dependent stack still owns capability readiness, broader persistence/lifecycle controls, incident and recovery evidence, and protected integration.
Merge gate
Do not integrate this child before #498 and #493. After the parent stack reaches protected
develop, retarget or reconcile this bounded semantic diff against the resulting protected head, rerun all then-applicable repository and organization gates, and require qualifying independent current-head approval. No predecessor-head, author-only, status-only, synthetic-only, or model-only evidence transfers.