Skip to content

Commit b0c7093

Browse files
committed
review round 1: apply council (4 Claude + 4 Codex personas) + CodeRabbit findings
Security/correctness: - sdk: proxy mode (baseUrl) never sends Authorization, even when apiKey is also passed — the documented contract now holds (cross-engine High) - sdk: presigned-URL query strings (bearer-like signatures) redacted from rawFetch error messages; non-https upload_url refused - proxy: reject empty path segments so matching always equals forwarding API semantics (the one breaking-ish change, pre-publish): - proxy: allowedPaths now REPLACES the defaults, exactly like allowedModels — spread DEFAULT_ALLOWED_PATHS to extend, [] to disable the asset routes (consensus P1: two opposite override semantics on one config object; forks could never turn the default uploads off) - proxy: defaults gain GET /v1/assets/:id for rf.assets.get Resilience (perf P1): - sdk: rf.assets.upload retries transient failures (network/timeout/5xx, 250/750ms) on all three legs — parity with the studio path it replaces - sdk: PUT timeout scales with file size (2 Mbit/s floor, min 120s) DX: - sdk: rf.assets.get(id) re-mints expired signed urls; "store the id" documented; RunflowErrorCode union for autocompletable catch blocks - proxy: 403/415 bodies carry actionable messages + machine codes (path_not_allowed, model_not_allowed, origin_not_allowed, json_content_type_required) - studio: uploadFile defaults to the SDK presigned flow through runflowProxy (zero-config); hosts with an explicit urls.upload keep the legacy multipart path — ends the two-contradictory-upload-stories problem; READMEs aligned Maintainability: - studio: useShellConfig fallback warns once instead of silently serving built-ins; = WORKFLOWS default params removed; StudioShellProps documents mount-only source + stable-reference expectations - studio: config memo keys on fields, not object identity — inline copy={{...}}/sentinel={{...}} no longer re-mint the context value - studio: mask controller clamps brush size, warns once on unattached/unsynced use, willReadFrequently on CPU-read canvases; shell lazily inits the controller ref - ci: biome warning-count ratchet (budget 45) CodeRabbit CLI (3 of 8 taken; rest skipped with reasons in the PR): - pin.ts boundary doc corrected (0.66 falls lower/right) - unmount(): theme CSS variables cleared - blob preview URLs revoked on unmount (GeneratePanel + StudioShell) via ref mirror — unmount-only, never revokes live previews Tests: 72 → 84 (auth-mode pair; retry/4xx/https/redaction/assets.get; replacement/spread/opt-out/empty-segment/coded-403 proxy coverage).
1 parent 991b164 commit b0c7093

28 files changed

Lines changed: 697 additions & 145 deletions

.changeset/proxy-allowed-paths.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
"@runflow-io/proxy": minor
33
---
44

5-
Add `allowedPaths` — an extensible, strictly-matched route allow-list on top of the built-ins (dispatch, run polling, health). Defaults now include the asset-upload pair `rf.assets.upload` needs (`POST /v1/asset-uploads`, `POST /v1/asset-uploads/:id/confirmations`); customer rules are additive, support method arrays and `:param` segments, and reject traversal. Org-data reads (run listing, billing) remain strictly opt-in. The handler now also exposes `PUT`/`PATCH`/`DELETE` for framework route exports. `RateLimitResult`'s `void` member is now `undefined` (type-level only).
5+
Add `allowedPaths` — an extensible, strictly-matched route allow-list on top of the built-ins (dispatch, run polling, health). Defaults cover what `rf.assets.upload`/`rf.assets.get` need (`POST /v1/asset-uploads`, `POST /v1/asset-uploads/:id/confirmations`, `GET /v1/assets/:id`). Like `allowedModels`, a custom list **replaces** the defaults — spread the exported `DEFAULT_ALLOWED_PATHS` to extend, or pass `[]` to disable the asset routes. Rules support method arrays and `:param` segments, reject traversal (including percent-encoded) and empty segments. 403/415 bodies now carry actionable messages plus machine-readable `code`s (`path_not_allowed`, `model_not_allowed`, `origin_not_allowed`, `json_content_type_required`). The handler also exposes `PUT`/`PATCH`/`DELETE` for framework route exports. `RateLimitResult`'s `void` member is now `undefined` (type-level only).

.changeset/sdk-assets-pin.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
"@runflow-io/sdk": minor
33
---
44

5-
Add `rf.assets.upload(file)` — the browser-safe presigned upload flow (create session → PUT to storage → confirm), returning a model-ready signed HTTPS `url` plus the stable `runflow://assets/{id}` `ref`. Fixes the most common external-fork failure: browser file uploads ending up as `data:` URIs that models reject with a 422.
5+
Add `rf.assets.upload(file)` — the browser-safe presigned upload flow (create session → PUT to storage → confirm) with transient-failure retry and a size-scaled PUT timeout, returning a model-ready signed HTTPS `url` plus the stable `runflow://assets/{id}` `ref`. Fixes the most common external-fork failure: browser file uploads ending up as `data:` URIs that models reject with a 422. Add `rf.assets.get(id)` to re-mint an expired signed url (store the `id`, not the `url`).
66

77
Export `composePinPrompt`, `composeRegionPrompt`, `pinRegion`, and `PinPoint` — the pin→region prompt convention (3×3 grid baked into the edit prompt) that previously existed only as private copies inside the studio bundle.
8+
9+
Hardening: proxy mode (`baseUrl`) now never sends `Authorization`, even when `apiKey` is also passed (the documented contract); presigned-URL query strings are redacted from error messages; non-https `upload_url`s are refused. New `RunflowErrorCode` union for autocompletable `catch` handling.

.changeset/studio-props-mask.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
"@runflow-io/studio": minor
33
---
44

5-
`<StudioShell>` accepts four optional customization props — `tools` (workflow catalogue), `source` (initial asset URL or sample list), `sentinel` (`{ enabled, taskDescription }`), and `copy` (brand/labels) — making vertical forks possible without rebuilding on `./headless`. Zero props renders exactly as before. `mount()` forwards them via the new `props` option.
5+
`<StudioShell>` accepts four optional customization props — `tools` (workflow catalogue), `source` (initial asset URL or sample list, read at mount), `sentinel` (`{ enabled, taskDescription }`), and `copy` (brand/labels) — making vertical forks possible without rebuilding on `./headless`. Zero props renders exactly as before. `mount()` forwards them via the new `props` option.
66

7-
`./headless` now exports `createMaskController` — the framework-free dual-canvas brush engine (stroke interpolation, coverage, full-resolution thresholded mask blob) the shell itself uses, so headless consumers get working mask creation for inpaint workflows without rebuilding it.
7+
`./headless` now exports `createMaskController` — the framework-free dual-canvas brush engine (stroke interpolation, coverage, full-resolution thresholded mask blob, guarded against unattached use and bad brush sizes) the shell itself uses, so headless consumers get working mask creation for inpaint workflows without rebuilding it.
8+
9+
The shell's file uploads now default to the SDK's presigned flow through `runflowProxy` (zero-config — no separate `upload` endpoint needed); hosts that explicitly set `urls.upload` keep the legacy multipart path. `unmount()` now also clears theme CSS variables, and blob preview URLs are revoked on unmount.

.github/workflows/ci.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,21 @@ jobs:
3636
- name: Lint
3737
run: bun run lint
3838

39+
# Warning ratchet: biome warnings (mostly the studio components'
40+
# downgraded a11y/hooks rules) must not grow past the checked-in
41+
# budget. Lower the budget as warnings get fixed; raising it is a
42+
# conscious review decision.
43+
- name: Lint warning ratchet
44+
run: |
45+
BUDGET=45
46+
count=$(bunx biome check . 2>&1 | grep -oE 'Found [0-9]+ warnings' | grep -oE '[0-9]+' | head -1)
47+
count=${count:-0}
48+
echo "biome warnings: $count (budget: $BUDGET)"
49+
if [ "$count" -gt "$BUDGET" ]; then
50+
echo "::error::Warning count $count exceeds the budget of $BUDGET — fix the new warnings or consciously raise the budget in ci.yml."
51+
exit 1
52+
fi
53+
3954
# The live e2e proof (examples/e2e-proof) needs RUNFLOW_API_KEY and
4055
# spends real credits, so it stays a local/manual gate — see
4156
# `bun run proof`.

examples/e2e-proof/run.ts

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import { mkdir, writeFile } from "node:fs/promises";
2424
import { dirname, resolve } from "node:path";
2525
import { fileURLToPath } from "node:url";
2626

27-
import { runflowProxy } from "@runflow-io/proxy";
27+
import { DEFAULT_ALLOWED_PATHS, runflowProxy } from "@runflow-io/proxy";
2828
import { RunFailedError, Runflow, composePinPrompt } from "@runflow-io/sdk";
2929
import { buildSampleMask, fetchBytes } from "./fixtures.js";
3030

@@ -165,8 +165,6 @@ async function main() {
165165
const start = Date.now();
166166
try {
167167
const dispatched = await rf.models.run(mod.model, mod.body);
168-
const enrichedBody = { ...mod.body, client_ref: `e2e-${mod.modality}-${start}` };
169-
void enrichedBody;
170168
const final = await rf.runs.wait(dispatched.id, {
171169
pollIntervalMs: 2_000,
172170
timeoutMs: mod.timeoutMs ?? 3 * 60_000,
@@ -310,7 +308,9 @@ async function main() {
310308
const mockProxy = runflowProxy({
311309
apiKey,
312310
basePath: "/api/runflow",
313-
allowedPaths: [{ method: "GET", path: "/v1/runs" }],
311+
// Replacement semantics (same as allowedModels): spread the
312+
// defaults to extend them with a run-listing read.
313+
allowedPaths: [...DEFAULT_ALLOWED_PATHS, { method: "GET", path: "/v1/runs" }],
314314
fetch: (async (input: RequestInfo | URL, init?: RequestInit) => {
315315
seen.push(new Request(input as RequestInfo, init).url);
316316
return new Response(JSON.stringify({ ok: true }), {
@@ -334,10 +334,32 @@ async function main() {
334334
if (refused.status !== 403) throw new Error(`expected 403 for billing, got ${refused.status}`);
335335
if (seen.length !== 2) throw new Error(`expected exactly 2 upstream calls, saw ${seen.length}`);
336336

337+
// A bare custom list REPLACES the defaults — forks can switch the
338+
// asset routes off.
339+
const optOutProxy = runflowProxy({
340+
apiKey,
341+
basePath: "/api/runflow",
342+
allowedPaths: [],
343+
fetch: (async () =>
344+
new Response(JSON.stringify({ ok: true }), {
345+
headers: { "Content-Type": "application/json" },
346+
})) as typeof fetch,
347+
});
348+
const optedOut = await optOutProxy(
349+
new Request("http://proof.local/api/runflow/v1/asset-uploads", {
350+
method: "POST",
351+
headers: { "Content-Type": "application/json" },
352+
body: "{}",
353+
}),
354+
);
355+
if (optedOut.status !== 403) {
356+
throw new Error(`allowedPaths: [] should disable uploads, got ${optedOut.status}`);
357+
}
358+
337359
const liveListProxy = runflowProxy({
338360
apiKey,
339361
basePath: "/api/runflow",
340-
allowedPaths: [{ method: "GET", path: "/v1/runs" }],
362+
allowedPaths: [...DEFAULT_ALLOWED_PATHS, { method: "GET", path: "/v1/runs" }],
341363
});
342364
const live = await liveListProxy(new Request("http://proof.local/api/runflow/v1/runs?limit=3"));
343365
if (live.status !== 200) {
@@ -427,9 +449,9 @@ async function main() {
427449
await log("");
428450

429451
// ── Mask + reference (runflow/reference-inpaint) ────────────────────
430-
// Mirrors the prototype's mask-ref flow: upload source + mask + ref to
431-
// R2 (using the same Sig V4 path /demos/api/upload uses), then dispatch
432-
// reference-inpaint with three URLs in the body.
452+
// Uploads source + mask + reference as Runflow assets via
453+
// rf.assets.upload (through the proxy's default allow-list), then
454+
// dispatches reference-inpaint with the three signed URLs.
433455
await log("▶ mask + reference — reference-inpaint");
434456
const maskStart = Date.now();
435457
try {

packages/proxy/README.md

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ The proxy accepts these paths out of the box:
104104
| GET | `/v1/health` | Public health. |
105105
| POST | `/v1/asset-uploads` | Create a presigned upload. |
106106
| POST | `/v1/asset-uploads/{id}/confirmations` | Confirm it (`rf.assets.upload`). |
107+
| GET | `/v1/assets/{id}` | Re-sign an asset URL (`rf.assets.get`). |
107108

108109
Everything else returns `403 Not allowed`. Run IDs are validated as
109110
UUIDv4-shape to block path traversal. Dispatch is additionally gated by
@@ -112,21 +113,29 @@ UUIDv4-shape to block path traversal. Dispatch is additionally gated by
112113
### Extending the allow-list: `allowedPaths`
113114

114115
```ts
116+
import { DEFAULT_ALLOWED_PATHS, runflowProxy } from "@runflow-io/proxy";
117+
115118
runflowProxy({
116119
apiKey: process.env.RUNFLOW_API_KEY!,
117120
allowedPaths: [
121+
...DEFAULT_ALLOWED_PATHS, // keep the rf.assets.upload/get routes
118122
{ method: "GET", path: "/v1/runs" }, // run listing
119123
{ method: "GET", path: "/v1/billing/balance" }, // billing read
120124
],
121125
});
122126
```
123127

124-
Rules are additive over the defaults and matched strictly — full path,
125-
segment by segment, no prefixes or wildcards. A `:param` segment matches
126-
exactly one non-empty segment and rejects traversal (`.`, `..`,
128+
Like `allowedModels`, a custom list **replaces** the defaults — spread
129+
`DEFAULT_ALLOWED_PATHS` (exported) to extend them, as above, or pass
130+
`[]` to turn the asset routes off entirely. Matching is strict — full
131+
path, segment by segment, no prefixes or wildcards. A `:param` segment
132+
matches exactly one non-empty segment and rejects traversal (`.`, `..`,
127133
percent-encoded forms). `method` takes a string or an array
128134
(`["GET", "DELETE"]`); the handler also exports `PUT`/`PATCH`/`DELETE`
129-
for framework route files.
135+
for framework route files. Non-GET requests must send
136+
`Content-Type: application/json` (CSRF gate) even when bodyless, and
137+
upstream responses are fully buffered — avoid allowing large or binary
138+
endpoints.
130139

131140
> **Security:** every matched request is forwarded with **your** API
132141
> key, so an allowed `GET /v1/runs` exposes org-wide run data to any

packages/proxy/src/defaults.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,12 @@ export const DEFAULT_ALLOWED_MODELS: ReadonlyArray<string> = [
2626
import type { AllowedPath } from "./types.js";
2727

2828
/**
29-
* Extra upstream routes the proxy forwards out of the box — the
30-
* presigned-upload pair `rf.assets.upload(file)` calls. Customer
31-
* `allowedPaths` entries are additive over these.
29+
* Extra upstream routes the proxy forwards out of the box: the
30+
* presigned-upload pair `rf.assets.upload(file)` calls, plus the
31+
* single-asset read `rf.assets.get(id)` uses to re-sign an expired
32+
* asset URL. Like `allowedModels`, a customer-supplied `allowedPaths`
33+
* REPLACES this list — spread `DEFAULT_ALLOWED_PATHS` to extend it, or
34+
* pass `[]` to turn these routes off entirely.
3235
*
3336
* Deliberately NOT here: `GET /v1/runs` (org-wide run listing),
3437
* billing reads, account info. Those expose org data through the
@@ -37,6 +40,7 @@ import type { AllowedPath } from "./types.js";
3740
export const DEFAULT_ALLOWED_PATHS: ReadonlyArray<AllowedPath> = [
3841
{ method: "POST", path: "/v1/asset-uploads" },
3942
{ method: "POST", path: "/v1/asset-uploads/:id/confirmations" },
43+
{ method: "GET", path: "/v1/assets/:id" },
4044
];
4145

4246
export const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;

packages/proxy/src/handler.ts

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ function normalize(cfg: ProxyConfig): NormalizedConfig {
5151
upstreamTimeoutMs: cfg.upstreamTimeoutMs ?? DEFAULT_UPSTREAM_TIMEOUT_MS,
5252
fetcher: cfg.fetch ?? globalThis.fetch,
5353
allowedModelsFor,
54-
allowedPaths: [...DEFAULT_ALLOWED_PATHS, ...(cfg.allowedPaths ?? [])],
54+
allowedPaths: cfg.allowedPaths ?? DEFAULT_ALLOWED_PATHS,
5555
allowedOrigins: cfg.allowedOrigins ?? "same-origin",
5656
requireJsonContentType: cfg.requireJsonContentType ?? true,
5757
authenticate: cfg.authenticate,
@@ -150,21 +150,41 @@ async function handle(c: NormalizedConfig, req: Request): Promise<Response> {
150150
segments[0] === "v1" &&
151151
segments[1] === "health";
152152

153+
// Empty segments (`//`) would make the matched path differ from the
154+
// forwarded one — refuse to match them at all.
155+
const hasEmptySegments = /\/\//.test(upstreamPath);
153156
const isAllowedPath =
154-
!isDispatch && !runId && !isHealth && matchAllowedPath(req.method, segments, c.allowedPaths);
157+
!isDispatch &&
158+
!runId &&
159+
!isHealth &&
160+
!hasEmptySegments &&
161+
matchAllowedPath(req.method, segments, c.allowedPaths);
155162

156163
if (!isDispatch && !runId && !isHealth && !isAllowedPath) {
157-
return json({ error: "Not allowed" }, 403);
164+
return json(
165+
{
166+
error: `Path not allowed: ${req.method} /${upstreamPath.slice(0, 120)}. The proxy forwards model dispatch, run polling, health, and the asset upload/read routes by default; add other upstream routes via the allowedPaths option.`,
167+
code: "path_not_allowed",
168+
},
169+
403,
170+
);
158171
}
159172

160173
// CSRF gate — must run before any authenticate hook so a malicious
161174
// page can't drain cookie credentials into the customer's API key.
162175
if (req.method !== "GET" && req.method !== "HEAD") {
163176
if (!originAllowed(req, c.allowedOrigins)) {
164-
return json({ error: "Origin not allowed" }, 403);
177+
return json({ error: "Origin not allowed", code: "origin_not_allowed" }, 403);
165178
}
166179
if (c.requireJsonContentType && !jsonContentTypeOK(req)) {
167-
return json({ error: "Content-Type must be application/json" }, 415);
180+
return json(
181+
{
182+
error:
183+
"Content-Type must be application/json (CSRF defense — required on every non-GET request through the proxy, including bodyless DELETEs)",
184+
code: "json_content_type_required",
185+
},
186+
415,
187+
);
168188
}
169189
}
170190

@@ -184,7 +204,10 @@ async function handle(c: NormalizedConfig, req: Request): Promise<Response> {
184204
if (isDispatch && model) {
185205
const allowed = c.allowedModelsFor(auth);
186206
if (!allowed.includes(model)) {
187-
return json({ error: "Model not allowed" }, 403);
207+
return json(
208+
{ error: `Model not allowed: ${model.slice(0, 120)}`, code: "model_not_allowed" },
209+
403,
210+
);
188211
}
189212
}
190213

packages/proxy/src/types.ts

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -83,15 +83,28 @@ export interface ProxyConfig {
8383
allowedModels?: ReadonlyArray<string> | ((auth: AuthResult | null) => ReadonlyArray<string>);
8484

8585
/**
86-
* Extra upstream routes to forward, additive over the defaults
87-
* (`POST /v1/asset-uploads`, `POST /v1/asset-uploads/:id/confirmations`
88-
* — what `rf.assets.upload` needs) and the always-on built-ins.
86+
* Extra upstream routes to forward beyond the always-on built-ins
87+
* (dispatch, run polling, health). Like `allowedModels`, passing a
88+
* list REPLACES the defaults (`DEFAULT_ALLOWED_PATHS`: the asset
89+
* upload pair + `GET /v1/assets/:id`, what `rf.assets.upload`/`get`
90+
* need). Spread the exported defaults to extend them:
91+
*
92+
* ```ts
93+
* allowedPaths: [...DEFAULT_ALLOWED_PATHS, { method: "GET", path: "/v1/runs" }]
94+
* ```
95+
*
96+
* Pass `[]` to disable the asset routes entirely.
8997
*
9098
* SECURITY: every request that matches is forwarded with YOUR API key,
91-
* so an allowed GET exposes that data to any logged-in (or, without an
92-
* `authenticate` hook, any same-origin) browser session. Only allow
93-
* reads like `GET /v1/runs` or `GET /v1/billing/balance` deliberately,
94-
* and pair them with `authenticate` + `rateLimit` in production.
99+
* so an allowed GET exposes that data to any same-origin browser
100+
* session (the default upload routes included — pair the proxy with
101+
* `authenticate` + `rateLimit` in production). Only allow reads like
102+
* `GET /v1/runs` or `GET /v1/billing/balance` deliberately.
103+
*
104+
* Notes: upstream responses are fully buffered (no streaming) — avoid
105+
* allowing large/binary endpoints; non-GET requests must send
106+
* `Content-Type: application/json` (CSRF gate), including bodyless
107+
* DELETE/PATCH/PUT.
95108
*/
96109
allowedPaths?: ReadonlyArray<AllowedPath>;
97110

0 commit comments

Comments
 (0)