Skip to content

Commit 960a2ee

Browse files
author
lsc
committed
fix: address non-blocking P2s from yujiawei's approval review
@yujiawei approved but flagged 7 P2 hardening suggestions. Take the three that are mechanical + safe; skip the four that need judgment or backend coordination. - Mirror the presence guard `uploadMcpIconReal` already has: reject `initResp` with missing `presigned_url`/`object_key` in `skillApiReal.uploadIcon` so a malformed 200 response yields a normalized Toast error, not an uncaught TypeError inside `uploadFile` (P2 #6). - Soften the `assertSafeUploadURL` / `assertSafeExternalURL` doc comments so they no longer overstate the protection: they are scheme-level guards only; an `https://` URL pointing at an internal or metadata host still passes. Also spell out why the residual blast radius stays bounded (P2 #2). - Document that `VITE_USE_MOCK` swaps only the 8 CRUD endpoints — the upload / parse / poll / download pipeline is always bound to the real backend because the mock module has no upload surface. Prevents the "why is my mock env hitting the real server" surprise (P2 #7). Skipped: - P2 #1 (icon <img src> host allowlist): needs a marketplace-side allowlist to compare against — not settled here. - P2 #3 (SECRET_KEY_PATTERN gaps: credential/auth/bearer/…): source comment asserts it must stay byte-identical to the backend regex, so a frontend-only tighten would desync. Backend PR paired. - P2 #4 (shell-escape skillId in the install prompt): value is a backend UUID today; hardening path is UUID-validate or shell-quote, either is a judgment call. - P2 #5 (request<T> undefined guard): can't distinguish "delete returned no data" from "misbehaving backend dropped data" at the `request` layer without caller-intent context. Refs: PR Mininglamp-OSS#851 review by @yujiawei (APPROVED verdict, P2 suggestions)
1 parent ceccff4 commit 960a2ee

3 files changed

Lines changed: 31 additions & 8 deletions

File tree

packages/dmworkmcp/src/api/mcpService.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,17 @@ function delay<T>(value: T, ms = MOCK_DELAY_MS): Promise<T> {
6666

6767
/**
6868
* Reject presigned upload / download URLs whose scheme is not http(s), or
69-
* whose http-scheme host is not a loopback (dev proxy). Backend-supplied
70-
* URLs are still trusted for the exact host they name, but we refuse to
71-
* PUT / GET against `javascript:`, `data:`, `file:` etc. — defense in
72-
* depth against a misconfigured or compromised marketplace.
69+
* whose http-scheme host is not a loopback (dev proxy). Blocks the obvious
70+
* bad schemes — `javascript:`, `data:`, `file:` — before an anchor.href /
71+
* axios.put reaches them.
72+
*
73+
* Scope: this is scheme-level defense-in-depth only. An `https://` URL
74+
* pointing at an internal / metadata host (`https://10.x`,
75+
* `https://169.254.169.254`) still passes; that class of concern needs a
76+
* host allowlist against the known storage origin, which the marketplace
77+
* hasn't published yet. Blast radius is bounded either way — the PUT
78+
* carries only the user-selected icon bytes with no app credentials (raw
79+
* axios, no interceptors).
7380
*/
7481
function assertSafeUploadURL(raw: string): void {
7582
let u: URL;

packages/dmworkskillmarket/src/api/skillApi.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@ const processEnv = typeof process === "undefined" ? undefined : process.env;
88
const useMock = env?.VITE_USE_MOCK === "true" || processEnv?.VITE_USE_MOCK === "true";
99
const api = useMock ? mockApi : realApi;
1010

11+
// NOTE: `VITE_USE_MOCK` only swaps the 8 CRUD endpoints below. The upload /
12+
// parse / poll / download pipeline (initUpload / uploadFile / uploadIcon /
13+
// triggerParse / pollParse / initReupload / getDownloadUrl / downloadSkill)
14+
// is always bound to the real backend — the mock module has no upload
15+
// surface. A dev enabling mock mode still hits real network on the upload
16+
// step; use a real dev backend if you need the full flow.
1117
export const getCategories = api.getCategories;
1218
export const getSkills = api.getSkills;
1319
export const getMySkills = api.getMySkills;

packages/dmworkskillmarket/src/api/skillApiReal.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -128,10 +128,14 @@ function wait(ms: number): Promise<void> {
128128

129129
/**
130130
* Reject presigned upload / download URLs whose scheme is not http(s), or
131-
* whose http-scheme host is not loopback. Guards against a compromised or
132-
* misconfigured marketplace returning `javascript:` / `data:` / `file:` /
133-
* arbitrary internal targets that a browser would otherwise honor
134-
* (anchor.href / xhr.open both accept unusual schemes).
131+
* whose http-scheme host is not loopback. Blocks `javascript:` / `data:` /
132+
* `file:` / arbitrary non-web schemes before an anchor.href / xhr.open
133+
* accepts them.
134+
*
135+
* Scope: scheme-level only. `https://10.x` / `https://169.254.169.254`
136+
* still passes — internal-host filtering would need a marketplace-side
137+
* allowlist not shipped here. Blast radius stays bounded because the PUT
138+
* runs with no app credentials (bare XHR / no interceptors).
135139
*/
136140
function assertSafeExternalURL(raw: string): void {
137141
let u: URL;
@@ -358,6 +362,12 @@ export async function uploadIcon(blob: Blob): Promise<string> {
358362
body: JSON.stringify({ file_name: fileName, file_size: blob.size }),
359363
},
360364
);
365+
// Mirror the presence guard `uploadMcpIconReal` already has — a malformed
366+
// response would otherwise dereference `undefined.presigned_url` inside
367+
// `uploadFile` as a bare TypeError instead of a normalized Toast error.
368+
if (!initResp?.presigned_url || !initResp?.object_key) {
369+
throw normalizeError({ code: "invalid_response", message: "上传失败:响应字段缺失" });
370+
}
361371

362372
// Step 2: Upload the file to presigned URL
363373
const file = new File([blob], fileName, { type: "image/png" });

0 commit comments

Comments
 (0)