diff --git a/.specgit.yaml b/.specgit.yaml index 4f6703d49c..a9a1a27305 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -1,8 +1,12 @@ version: 1 -delivery: dag-inspector-shows +delivery: open-issues-batch context: kind: branch - branch: fix/427-dag-inspector-shows + branch: feat/434-open-issues-batch issues: - - 427 -pr: 428 + - 434 + - 442 + - 443 + - 440 + - 436 +pr: 444 diff --git a/bun.lock b/bun.lock index 2d9f55935d..ffc9b48d01 100644 --- a/bun.lock +++ b/bun.lock @@ -608,6 +608,7 @@ "diff": "catalog:", "drizzle-orm": "catalog:", "effect": "catalog:", + "escape-html": "1.0.3", "fuzzysort": "3.1.0", "gitlab-ai-provider": "6.9.3", "glob": "13.0.5", @@ -653,6 +654,7 @@ "@types/babel__core": "7.20.5", "@types/bun": "catalog:", "@types/cross-spawn": "catalog:", + "@types/escape-html": "1.0.3", "@types/mime-types": "3.0.1", "@types/npm-package-arg": "6.1.4", "@types/semver": "^7.5.8", @@ -2826,6 +2828,8 @@ "@types/deep-eql": ["@types/deep-eql@4.0.2", "", {}, "sha512-c9h9dVVMigMPc4bwTvC5dxqtqJZwQPePsWjPlpSOnojbor6pGqdk541lfA7AqFQr5pB1BRdq0juY9db81BwyFw=="], + "@types/escape-html": ["@types/escape-html@1.0.3", "", {}, "sha512-QbNxKa2IX2y/9eGiy4w8rrwk//ERHXA6zwYVRA3+ayA/D3pkz+/bLL4b5uSLA0L0kPuNX1Jbv9HyPzv9T4zbJQ=="], + "@types/estree": ["@types/estree@1.0.8", "", {}, "sha512-dWHzHa2WqEXI/O1E9OjrocMTKJl2mSrEolh1Iomrv6U+JuNwaHXsXx9bLu5gG7BUWFIN0skIQJQ/L1rIex4X6w=="], "@types/estree-jsx": ["@types/estree-jsx@1.0.5", "", { "dependencies": { "@types/estree": "*" } }, "sha512-52CcUVNFyfb1A2ALocQw/Dd1BQFNmSdkuC3BkZ6iqhdMfQz7JWOFRuJFloOzjk+6WijU56m9oKXFAXc7o3Towg=="], diff --git a/packages/http-recorder/test/record-replay.test.ts b/packages/http-recorder/test/record-replay.test.ts index 93d7bbaded..b7580a9512 100644 --- a/packages/http-recorder/test/record-replay.test.ts +++ b/packages/http-recorder/test/record-replay.test.ts @@ -125,6 +125,10 @@ describe("http-recorder", () => { }) test("detects secret-looking values without returning the secret", () => { + // Assembled fake sample: keeps the AIza prefix + 22 redaction-pattern + // chars so the scanner hits, while never spelling a plausible real key + // (GitHub secret scanning matches literal AIza... blobs in the repo). + const googleKeySample = process.env.TEST_GOOGLE_KEY_SAMPLE ?? "AIza" + "TESTSAMPLE0".repeat(2) + "22" expect( HttpRecorderInternal.secretFindings({ version: 1, @@ -135,7 +139,7 @@ describe("http-recorder", () => { method: "POST", url: "https://example.test/path?key=sk-123456789012345678901234", headers: {}, - body: JSON.stringify({ nested: "AIzaSyDHibiBRvJZLsFnPYPoiTwxY4ztQ55yqCE" }), + body: JSON.stringify({ nested: googleKeySample }), }, response: { status: 200, diff --git a/packages/opencode/package.json b/packages/opencode/package.json index 13283a8d58..b34b677261 100644 --- a/packages/opencode/package.json +++ b/packages/opencode/package.json @@ -42,6 +42,7 @@ "@types/bun": "catalog:", "@types/cross-spawn": "catalog:", "@types/mime-types": "3.0.1", + "@types/escape-html": "1.0.3", "@types/npm-package-arg": "6.1.4", "@types/semver": "^7.5.8", "@types/turndown": "5.0.5", @@ -56,6 +57,7 @@ "dependencies": { "@actions/core": "1.11.1", "@actions/github": "6.0.1", + "escape-html": "1.0.3", "@agentclientprotocol/sdk": "0.21.0", "@ai-sdk/alibaba": "1.0.17", "@ai-sdk/amazon-bedrock": "4.0.112", diff --git a/packages/opencode/src/cli/cmd/github.handler.ts b/packages/opencode/src/cli/cmd/github.handler.ts index 2984ea00f8..c47ce32007 100644 --- a/packages/opencode/src/cli/cmd/github.handler.ts +++ b/packages/opencode/src/cli/cmd/github.handler.ts @@ -33,7 +33,7 @@ import { setTimeout as sleep } from "node:timers/promises" import { Process } from "@/util/process" import { parseGitHubRemote } from "@/util/repository" import { Effect } from "effect" -import { extractResponseText, formatPromptTooLargeError } from "./github.shared" +import { extractResponseText, formatPromptTooLargeError, isAllowedAttachmentUrl } from "./github.shared" type GitHubAuthor = { login: string @@ -787,22 +787,17 @@ export const githubRun = Effect.fn("Cli.github.run")(function* (args: { event?: const start = m.index const filename = path.basename(url) - // SSRF guard: only fetch GitHub user-attachment assets over HTTPS + // SSRF guard (issue #442): only fetch GitHub user-attachment assets + // over HTTPS, and fetch the validated URL object — not the raw + // string it was parsed from — so the token-bearing request can only + // ever target what the guard approved. const attachmentUrl = URL.parse(url) - if ( - !attachmentUrl || - attachmentUrl.protocol !== "https:" || - attachmentUrl.hostname !== "github.com" || - !( - attachmentUrl.pathname.startsWith("/user-attachments/assets/") || - attachmentUrl.pathname.startsWith("/user-attachments/files/") - ) - ) { + if (!isAllowedAttachmentUrl(attachmentUrl)) { continue } // Download image - const res = await fetch(url, { + const res = await fetch(attachmentUrl, { headers: { Authorization: `Bearer ${appToken}`, Accept: "application/vnd.github.v3+json", diff --git a/packages/opencode/src/cli/cmd/github.shared.ts b/packages/opencode/src/cli/cmd/github.shared.ts index 157d0156fb..b92f1e6378 100644 --- a/packages/opencode/src/cli/cmd/github.shared.ts +++ b/packages/opencode/src/cli/cmd/github.shared.ts @@ -28,3 +28,18 @@ export function formatPromptTooLargeError(files: { filename: string; content: st : "" return `PROMPT_TOO_LARGE: The prompt exceeds the model's context limit.${fileDetails}` } + +/** + * SSRF guard for attachment URLs parsed out of issue/PR bodies (issue #442): + * a request that carries the GitHub app token may only ever leave for + * github.com user-attachment paths over https. The handler fetches the + * validated URL object itself, never the raw string it was parsed from. + */ +export function isAllowedAttachmentUrl(url: URL | null): url is URL { + return ( + url !== null && + url.protocol === "https:" && + url.hostname === "github.com" && + (url.pathname.startsWith("/user-attachments/assets/") || url.pathname.startsWith("/user-attachments/files/")) + ) +} diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index eccbb375c6..d5cd383e4f 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -2,7 +2,7 @@ import { Effect } from "effect" import { cmd } from "./cmd" import { effectCmd } from "../effect-cmd" -export { extractResponseText, formatPromptTooLargeError, parseGitHubRemote } from "./github.shared" +export { extractResponseText, formatPromptTooLargeError, parseGitHubRemote, isAllowedAttachmentUrl } from "./github.shared" export const GithubInstallCommand = effectCmd({ command: "install", diff --git a/packages/opencode/src/dag/runtime/loop.ts b/packages/opencode/src/dag/runtime/loop.ts index 0f37a9c25b..f32fa00bf8 100644 --- a/packages/opencode/src/dag/runtime/loop.ts +++ b/packages/opencode/src/dag/runtime/loop.ts @@ -278,7 +278,7 @@ const serviceLayer = Layer.effect( if (nodeConfig?.output_schema) { promptParts.push({ type: "text", - text: `\n\nYou MUST call the submit_result tool with a JSON payload matching this schema before ending your turn:\n${JSON.stringify(nodeConfig.output_schema, null, 2)}\nPut your full summary inside the payload. Do not repeat the payload in your message text. After submit_result succeeds, end your turn without restating the result.`, + text: `\n\n[OUTPUT CONTRACT — this node's success depends on it] You MUST call the submit_result tool with a JSON payload matching this schema before ending your turn:\n${JSON.stringify(nodeConfig.output_schema, null, 2)}\nPut your full summary inside the payload. Do not repeat the payload in your message text. Writing the payload in message text does NOT count as submitting: if you end your turn without a successful submit_result call, this node FAILS and your work is discarded. After submit_result succeeds, end your turn without restating the result.`, }) } diff --git a/packages/opencode/src/dag/runtime/spawn.ts b/packages/opencode/src/dag/runtime/spawn.ts index cf1e4b1575..b1397aef86 100644 --- a/packages/opencode/src/dag/runtime/spawn.ts +++ b/packages/opencode/src/dag/runtime/spawn.ts @@ -490,12 +490,43 @@ export function spawnNode( parts: input.promptParts, }) if (input.outputSchema) { + const readSettlement = Effect.fn("DagRuntime.spawn.readSettlement")(function* () { + const updatedNode = yield* dag.store.getNode(input.dagID, input.nodeID).pipe(Effect.orDie) + const captured = updatedNode?.capturedOutput + return { + neverCalled: captured === undefined || captured === null, + // Single settlement authority shared with crash recovery + // (capture.ts settleCapturedOutput). + settlement: settleCapturedOutput(captured, input.reviewImplementationFingerprint), + } + }) clearCaptureSlot(childSession.id) - const updatedNode = yield* dag.store.getNode(input.dagID, input.nodeID).pipe(Effect.orDie) - // Single settlement decision shared with crash recovery - // (capture.ts settleCapturedOutput) — the review-result contract - // must never be enforced in one path and not the other. - const settlement = settleCapturedOutput(updatedNode?.capturedOutput, input.reviewImplementationFingerprint) + let verdict = yield* readSettlement() + // Issue #436 minimal step: a child that ended its whole turn + // without ever calling submit_result gets exactly one nudge + // turn in the same session — the work is already done, only the + // structured hand-back is missing. A captured-but-invalid payload + // is NOT nudged: that is a deterministic contract violation and + // a retry would only re-bill the same mistake. + if (verdict.settlement.kind === "fail" && verdict.neverCalled) { + registerCaptureSlot(childSession.id, input.outputSchema) + yield* promptSvc.prompt({ + messageID: MessageID.ascending(), + sessionID: childSession.id, + model, + agent: agent.name, + ...(input.variant ? { variant: input.variant } : {}), + parts: [ + { + type: "text", + text: `You ended your turn without calling the submit_result tool, so this node recorded no output and will FAIL. Do not redo the work. Call submit_result NOW with a JSON payload matching the schema from your instructions, containing your full result. If a previous submit_result call failed validation, fix the payload shape and call it again.`, + }, + ], + }) + clearCaptureSlot(childSession.id) + verdict = yield* readSettlement() + } + const settlement = verdict.settlement yield* (settlement.kind === "complete" ? dag.nodeCompleted(input.dagID, input.nodeID, settlement.output) : dag.nodeFailed(input.dagID, input.nodeID, settlement.reason, "verdict_fail") diff --git a/packages/opencode/src/util/html.ts b/packages/opencode/src/util/html.ts index 55028613f7..7864f6eafb 100644 --- a/packages/opencode/src/util/html.ts +++ b/packages/opencode/src/util/html.ts @@ -1,8 +1,9 @@ +import escapeHtmlLib from "escape-html" + +// Delegates to the escape-html package (issue #443): identical output to the +// previous hand-rolled replaceAll chain, but CodeQL models this library as a +// sanitizer, so js/reflected-xss stops flagging the OAuth error pages that +// render through it. export function escapeHtml(value: string) { - return value - .replaceAll("&", "&") - .replaceAll("<", "<") - .replaceAll(">", ">") - .replaceAll('"', """) - .replaceAll("'", "'") + return escapeHtmlLib(value) } diff --git a/packages/opencode/test/cli/github-attachment-guard.test.ts b/packages/opencode/test/cli/github-attachment-guard.test.ts new file mode 100644 index 0000000000..b4d48f8b4a --- /dev/null +++ b/packages/opencode/test/cli/github-attachment-guard.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, test } from "bun:test" +import { isAllowedAttachmentUrl } from "../../src/cli/cmd/github.shared" + +// Issue #442: attachment URLs parsed out of issue/PR markdown are attacker +// controllable. Anything that reaches fetch() carries the GitHub app token, +// so the guard must admit ONLY github.com user-attachment paths over https — +// the handler fetches the validated URL object itself. +describe("isAllowedAttachmentUrl (issue #442)", () => { + test("admits github.com user-attachment assets and files over https", () => { + expect(isAllowedAttachmentUrl(URL.parse("https://github.com/user-attachments/assets/abc123"))).toBe(true) + expect(isAllowedAttachmentUrl(URL.parse("https://github.com/user-attachments/files/21433810/api.json"))).toBe(true) + }) + + test("rejects other hosts, schemes, and paths", () => { + expect(isAllowedAttachmentUrl(URL.parse("https://attacker.com/pixel.png"))).toBe(false) + expect(isAllowedAttachmentUrl(URL.parse("https://github.com.evil.io/user-attachments/assets/x"))).toBe(false) + expect(isAllowedAttachmentUrl(URL.parse("http://github.com/user-attachments/assets/abc123"))).toBe(false) + expect(isAllowedAttachmentUrl(URL.parse("https://github.com/leak/user-attachments/assets/abc123"))).toBe(false) + expect(isAllowedAttachmentUrl(URL.parse("https://api.github.com/user-attachments/assets/abc123"))).toBe(false) + }) + + test("rejects null (unparseable URL)", () => { + expect(isAllowedAttachmentUrl(URL.parse("not a url ::"))).toBe(false) + }) +}) diff --git a/packages/opencode/test/dag/dag-schema-prompt-contract.test.ts b/packages/opencode/test/dag/dag-schema-prompt-contract.test.ts index 24eb8b72b1..6a5acdf83e 100644 --- a/packages/opencode/test/dag/dag-schema-prompt-contract.test.ts +++ b/packages/opencode/test/dag/dag-schema-prompt-contract.test.ts @@ -192,6 +192,10 @@ describe("DAG schema prompt contract (issue #386)", () => { expect(prompt).toContain("Do not repeat the payload in your message text") // A successful submission ends the turn. expect(prompt).toContain("end your turn without restating the result") + // Issue #436: the consequence of skipping submit_result is stated + // up front so the child cannot treat prose as a substitute. + expect(prompt).toContain("does NOT count as submitting") + expect(prompt).toContain("this node FAILS") yield* Deferred.succeed(gate.release, "done") }), ), @@ -238,4 +242,74 @@ describe("DAG schema prompt contract (issue #386)", () => { expect(description).toContain("Do not duplicate the payload") expect(description).toContain("end your turn without restating the result") }) + + // Issue #436 minimal step: a child that ends its turn without ever calling + // submit_result gets exactly one nudge turn in the same session before the + // node is failed. Review-aggregate children were observed (PR #424 era) + // producing the whole review in prose and never handing it back. + describe("submit_result retry nudge (issue #436)", () => { + it("nudges once when the child never submits, then completes from the submitted payload", async () => { + await Effect.runPromise( + runContractTest(({ dag, store, childPrompts }) => + Effect.gen(function* () { + const dagID = yield* dag.create({ + projectID: "project-1", + sessionID: "ses_parent", + title: "Nudge completes", + config: { name: "schema-prompt-contract", nodes: [schemaNode()] }, + }) + // First turn: full analysis in prose, no submit_result call. + const first = yield* Queue.take(childPrompts) + expect(promptText(first.input)).toContain("[OUTPUT CONTRACT") + yield* Deferred.succeed(first.release, "Full review: everything checks out.") + // The nudge arrives as a second prompt to the SAME child session. + const nudge = yield* Queue.take(childPrompts) + expect(nudge.input.sessionID).toBe(first.input.sessionID) + expect(promptText(nudge.input)).toContain("without calling the submit_result tool") + // The child complies on the second chance. + const payload = { summary: "Submitted after the nudge." } + yield* store.setCapturedOutput(nudge.input.sessionID as string, payload) + yield* Deferred.succeed(nudge.release, "Submitted.") + const node = yield* pollWithTimeout( + store.getNode(dagID, "report").pipe( + Effect.map((row) => row?.status === "completed" ? row : undefined), + ), + "node did not complete after the nudge turn", + ) + expect(node.output).toEqual(payload) + }), + ), + ) + }) + + it("fails the node after the nudge still produces no submission", async () => { + await Effect.runPromise( + runContractTest(({ dag, store, childPrompts }) => + Effect.gen(function* () { + const dagID = yield* dag.create({ + projectID: "project-1", + sessionID: "ses_parent", + title: "Nudge exhausted", + config: { name: "schema-prompt-contract", nodes: [schemaNode()] }, + }) + const first = yield* Queue.take(childPrompts) + yield* Deferred.succeed(first.release, "Prose only, no submission.") + const nudge = yield* Queue.take(childPrompts) + yield* Deferred.succeed(nudge.release, "Still nothing.") + // No third prompt comes from spawn: the nudge is one per node + // EXECUTION. (The DagLoop's own NodeFailed handler may respawn + // the node as a fresh attempt afterwards — that retry policy is + // a separate pre-existing layer, not spawn's business.) + const node = yield* pollWithTimeout( + store.getNode(dagID, "report").pipe( + Effect.map((row) => row?.status === "failed" ? row : undefined), + ), + "node was not failed after the exhausted nudge", + ) + expect(node.errorReason).toContain("never successfully called") + }), + ), + ) + }) + }) })