finos / finos/architecture-as-code

Harden DirectUrlDocumentLoader: shared path/traversal guards, DNS-resolution SSRF check, drop axios debug logging

Open
#3,024 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
399
Forks
138
Avg merge
2d 6h
Merged PRs (30d)
38

Description

## Feature Proposal

### Target Project:
`shared` (`@finos/calm-shared`) — `document-loader/direct-url-document-loader.ts` and `document-loader/calmhub-document-loader.ts`.

### Description of Feature:
Harden the two HTTP document loaders and remove duplicated security checks between them. Raised by @jpgough-ms in the review of #3022; all items pre-date that PR (present at its merge base), so they are tracked here rather than folded into it.

1. **Align the request-path allowlists.** `DirectUrlDocumentLoader.SAFE_PATH_PATTERN` (`/^[a-zA-Z0-9/_.-]+$/`) claims to mirror `CalmHubDocumentLoader.SAFE_PATH_PATTERN` (`/^[a-zA-Z0-9/_\-.]+(\.json)?$/`) but the two differ; a tightening of one will not propagate to the other.
2. **Share the directory-traversal guard.** `documentId.includes('/..')` and its comment are copy-pasted in both loaders. Factor it (and the path allowlist) into a shared helper, following the precedent of `document-loader/response-origin.ts` (`assertResponseOrigin`).
3. **Resolve DNS before connecting (rebinding).** `isPrivateHost` only rejects IP-literal/localhost hostnames; an allowlisted hostname (`calm init-config --allowed-remote-hosts `) that resolves — or later rebinds — to `127.0.0.1` or a cloud metadata address passes the check. In Node, resolve the host first (`dns.lookup`), apply the private-range check to every returned address, and pin the connection to the checked address (axios `lookup` option); in browsers this is inherently the browser's concern, so the Node-only path lives behind the root entry.
4. **Remove the verbose axios debug hooks.** `DirectUrlDocumentLoader.addAxiosDebug()` logs every request/response object whenever `debug` is true; `CalmHubDocumentLoader` already disabled its copy as "far, far too verbose for -v". Remove both or replace with a single-line request log.
5. **Stop loading schemas twice per generation.** `generate()` (`commands/generate/generate-core.ts`) calls `schemaDirectory.loadSchemas()` and `instantiate()` (`components/instantiate.ts`) calls it again, so every generation re-initialises the loader. Keep initialisation in one layer or make `loadSchemas()` idempotent.

### User Stories:
- As a **CLI user with an allowlisted remote host**, I want the SSRF protection to hold even when that host's DNS changes, so a compromised or rebinding host cannot reach my private network through `calm`.
- As a **maintainer**, I want one implementation of each loader security check, so a fix cannot land in one loader and not the other.

### Current Limitations:
Described per item above; all five exist on `main` today.

### Proposed Implementation:
- New `document-loader/request-guards.ts` exporting `assertSafeRequestPath(path, documentId)` and `assertNoTraversal(documentId)` used by both loaders; one `SAFE_PATH_PATTERN`.
- `isPrivateHost` gains a Node-only DNS-resolution step behind the root entry (kept out of `browser.ts`); the browser entry keeps the literal check.
- Delete `addAxiosDebug` from both loaders.
- Remove the duplicate `loadSchemas()` call (or make it idempotent with a loaded flag on `SchemaDirectory`).

### Alternatives Considered:
Leave as-is — rejected: item 3 is a real, if config-gated, SSRF gap; items 1/2/4 are maintenance hazards that already bit once (the "mirrors" comment is wrong today).

### Testing Strategy:
Unit tests for the shared guards; SSRF tests with a stubbed `dns.lookup` returning a private address for an allowlisted hostname; existing loader and generate specs; the browser-entry guard stays green.

### Documentation Requirements:
`shared/AGENTS.md` Document Loader section (shared guards, DNS step).

### Implementation Checklist:
- [ ] Design reviewed and approved
- [ ] Implementation completed
- [ ] Tests written and passing
- [ ] Documentation updated
- [ ] Relevant workflows updated (if needed)
- [ ] Performance impact assessed

### Additional Context:
From the review of #3022 (https://github.com/finos/architecture-as-code/pull/3022#issuecomment-5424057771). Items 1–4 are @jpgough-ms's findings; item 5 was a suppressed Copilot comment on the same PR.

Contributor guide

Open the contributing guide

Research direction

Start with shared/document-loader/direct-url-document-loader.ts, shared/document-loader/calmhub-document-loader.ts, document-loader/response-origin.ts, and the generate and instantiate entry points named in the issue. Review the proposed shared guards, Node DNS-resolution path, existing loader and generate specs, and shared/AGENTS.md. Done means the five listed concerns are addressed, shared guard and SSRF tests pass, duplicate schema loading is removed, debug logging is reduced or removed, and documentation is updated.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
backend-api-design, cli, documentation, security
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.