diff --git a/.github/workflows/poem-bot.lock.yml b/.github/workflows/poem-bot.lock.yml index 612c7fc91e..09c655b3ed 100644 --- a/.github/workflows/poem-bot.lock.yml +++ b/.github/workflows/poem-bot.lock.yml @@ -490,7 +490,7 @@ jobs: "name": "add_labels" }, { - "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 2 issue(s) can be updated. Target: *.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 2 issue(s) can be updated. Target: *. Body updates are allowed.", "inputSchema": { "additionalProperties": false, "properties": { diff --git a/.github/workflows/workflow-generator.lock.yml b/.github/workflows/workflow-generator.lock.yml index 8133286705..6b9d16da96 100644 --- a/.github/workflows/workflow-generator.lock.yml +++ b/.github/workflows/workflow-generator.lock.yml @@ -260,7 +260,7 @@ jobs: "name": "assign_to_agent" }, { - "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 1 issue(s) can be updated.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 1 issue(s) can be updated. Body updates are allowed.", "inputSchema": { "additionalProperties": false, "properties": { diff --git a/actions/setup/js/collect_ndjson_output.cjs b/actions/setup/js/collect_ndjson_output.cjs index 94bef1fbeb..fcfca39fb5 100644 --- a/actions/setup/js/collect_ndjson_output.cjs +++ b/actions/setup/js/collect_ndjson_output.cjs @@ -2,7 +2,7 @@ /// const { getErrorMessage } = require("./error_helpers.cjs"); -const { repairJson } = require("./json_repair_helpers.cjs"); +const { repairJson, sanitizePrototypePollution } = require("./json_repair_helpers.cjs"); const { AGENT_OUTPUT_FILENAME, TMP_GH_AW_PATH } = require("./constants.cjs"); async function main() { @@ -128,11 +128,15 @@ async function main() { } function parseJsonWithRepair(jsonStr) { try { - return JSON.parse(jsonStr); + const parsed = JSON.parse(jsonStr); + // Sanitize the parsed object to prevent prototype pollution + return sanitizePrototypePollution(parsed); } catch (originalError) { try { const repairedJson = repairJson(jsonStr); - return JSON.parse(repairedJson); + const parsed = JSON.parse(repairedJson); + // Sanitize the parsed object to prevent prototype pollution + return sanitizePrototypePollution(parsed); } catch (repairError) { core.info(`invalid input json: ${jsonStr}`); const originalMsg = originalError instanceof Error ? originalError.message : String(originalError); diff --git a/actions/setup/js/json_repair_helpers.cjs b/actions/setup/js/json_repair_helpers.cjs index ebf292fd68..3faf543308 100644 --- a/actions/setup/js/json_repair_helpers.cjs +++ b/actions/setup/js/json_repair_helpers.cjs @@ -1,5 +1,101 @@ // @ts-check +/** + * Sanitizes an object to remove dangerous prototype pollution keys using a stack-based algorithm. + * This function removes keys that could be used for prototype pollution attacks: + * - __proto__: JavaScript's prototype chain accessor + * - constructor: Object constructor property + * - prototype: Function prototype property + * + * Uses an iterative approach with a stack to handle deeply nested structures and + * protect against stack overflow from malicious recursive object trees. + * + * @param {any} obj - The object to sanitize (can be any type) + * @returns {any} The sanitized object with dangerous keys removed + * + * @example + * // Removes __proto__ key + * sanitizePrototypePollution({name: "test", __proto__: {isAdmin: true}}) + * // Returns: {name: "test"} + * + * @example + * // Handles nested objects + * sanitizePrototypePollution({outer: {__proto__: {bad: true}, safe: "value"}}) + * // Returns: {outer: {safe: "value"}} + */ +function sanitizePrototypePollution(obj) { + // Handle non-objects (primitives, null, undefined) + if (obj === null || typeof obj !== "object") { + return obj; + } + + // Dangerous keys that can be used for prototype pollution + const dangerousKeys = ["__proto__", "constructor", "prototype"]; + + // Track visited objects to handle circular references + const seen = new WeakMap(); + + // Stack-based traversal to avoid recursion and stack overflow + // Each entry: { source: original object, target: sanitized object, parent: parent target, key: property key } + const stack = []; + const root = Array.isArray(obj) ? [] : {}; + seen.set(obj, root); + stack.push({ source: obj, target: root, parent: null, key: null }); + + while (stack.length > 0) { + const item = stack.pop(); + if (!item) continue; + const { source, target } = item; + + if (Array.isArray(source)) { + // Process array elements + for (let i = 0; i < source.length; i++) { + const value = source[i]; + if (value === null || typeof value !== "object") { + // Primitive value - copy directly + target[i] = value; + } else if (seen.has(value)) { + // Circular reference detected - use existing sanitized object + target[i] = seen.get(value); + } else { + // New object or array - create sanitized version and add to stack + const newTarget = Array.isArray(value) ? [] : {}; + target[i] = newTarget; + seen.set(value, newTarget); + stack.push({ source: value, target: newTarget, parent: target, key: i }); + } + } + } else { + // Process object properties + for (const key in source) { + // Skip dangerous keys + if (dangerousKeys.includes(key)) { + continue; + } + // Only process own properties (not inherited) + if (Object.prototype.hasOwnProperty.call(source, key)) { + const value = source[key]; + if (value === null || typeof value !== "object") { + // Primitive value - copy directly + target[key] = value; + } else if (seen.has(value)) { + // Circular reference detected - use existing sanitized object + target[key] = seen.get(value); + } else { + // New object or array - create sanitized version and add to stack + const newTarget = Array.isArray(value) ? [] : {}; + target[key] = newTarget; + seen.set(value, newTarget); + stack.push({ source: value, target: newTarget, parent: target, key: key }); + } + } + } + } + } + + return root; +} + /** * Attempts to repair malformed JSON strings using various heuristics. * This function applies multiple repair strategies to fix common JSON formatting issues: @@ -76,4 +172,4 @@ function repairJson(jsonStr) { return repaired; } -module.exports = { repairJson }; +module.exports = { repairJson, sanitizePrototypePollution }; diff --git a/actions/setup/js/json_repair_helpers.test.cjs b/actions/setup/js/json_repair_helpers.test.cjs index 6723101fb7..d2332b8cbe 100644 --- a/actions/setup/js/json_repair_helpers.test.cjs +++ b/actions/setup/js/json_repair_helpers.test.cjs @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { repairJson } from "./json_repair_helpers.cjs"; +import { repairJson, sanitizePrototypePollution } from "./json_repair_helpers.cjs"; describe("json_repair_helpers", () => { describe("repairJson", () => { @@ -226,4 +226,402 @@ describe("json_repair_helpers", () => { }); }); }); + + describe("sanitizePrototypePollution", () => { + describe("basic sanitization", () => { + it("should remove __proto__ property", () => { + const obj = { name: "test", __proto__: { isAdmin: true } }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ name: "test" }); + // Verify __proto__ key was removed from own properties + expect(Object.prototype.hasOwnProperty.call(sanitized, "__proto__")).toBe(false); + }); + + it("should remove constructor property", () => { + const obj = { name: "test", constructor: { prototype: { isAdmin: true } } }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ name: "test" }); + // Verify constructor key was removed from own properties + expect(Object.prototype.hasOwnProperty.call(sanitized, "constructor")).toBe(false); + }); + + it("should remove prototype property", () => { + const obj = { name: "test", prototype: { isAdmin: true } }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ name: "test" }); + // Verify prototype key was removed from own properties + expect(Object.prototype.hasOwnProperty.call(sanitized, "prototype")).toBe(false); + }); + + it("should remove all dangerous keys simultaneously", () => { + const obj = { + name: "test", + __proto__: { isAdmin: true }, + constructor: { isAdmin: true }, + prototype: { isAdmin: true }, + }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ name: "test" }); + }); + + it("should preserve safe properties", () => { + const obj = { name: "John", age: 30, city: "NYC", status: "active" }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual(obj); + }); + }); + + describe("nested object sanitization", () => { + it("should sanitize nested __proto__ properties", () => { + const obj = { + user: { + name: "test", + __proto__: { isAdmin: true }, + }, + }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ user: { name: "test" } }); + }); + + it("should sanitize deeply nested dangerous properties", () => { + const obj = { + outer: { + middle: { + inner: { + __proto__: { isAdmin: true }, + constructor: { bad: true }, + safe: "value", + }, + }, + }, + }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ + outer: { + middle: { + inner: { + safe: "value", + }, + }, + }, + }); + }); + + it("should handle mixed safe and dangerous properties at multiple levels", () => { + const obj = { + level1: "safe", + __proto__: { bad: true }, + nested: { + level2: "safe", + constructor: { bad: true }, + deepNested: { + level3: "safe", + prototype: { bad: true }, + }, + }, + }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ + level1: "safe", + nested: { + level2: "safe", + deepNested: { + level3: "safe", + }, + }, + }); + }); + }); + + describe("array sanitization", () => { + it("should sanitize objects within arrays", () => { + const obj = [ + { name: "test1", __proto__: { isAdmin: true } }, + { name: "test2", constructor: { bad: true } }, + ]; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual([{ name: "test1" }, { name: "test2" }]); + }); + + it("should handle nested arrays", () => { + const obj = { + items: [[{ __proto__: { bad: true }, value: 1 }], [{ constructor: { bad: true }, value: 2 }]], + }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ + items: [[{ value: 1 }], [{ value: 2 }]], + }); + }); + + it("should preserve arrays with safe values", () => { + const obj = { items: ["a", "b", "c"], numbers: [1, 2, 3] }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual(obj); + }); + }); + + describe("primitive type handling", () => { + it("should handle null", () => { + const sanitized = sanitizePrototypePollution(null); + expect(sanitized).toBeNull(); + }); + + it("should handle undefined", () => { + const sanitized = sanitizePrototypePollution(undefined); + expect(sanitized).toBeUndefined(); + }); + + it("should handle strings", () => { + const sanitized = sanitizePrototypePollution("test string"); + expect(sanitized).toBe("test string"); + }); + + it("should handle numbers", () => { + const sanitized = sanitizePrototypePollution(42); + expect(sanitized).toBe(42); + }); + + it("should handle booleans", () => { + const sanitized = sanitizePrototypePollution(true); + expect(sanitized).toBe(true); + }); + }); + + describe("edge cases", () => { + it("should handle empty objects", () => { + const sanitized = sanitizePrototypePollution({}); + expect(sanitized).toEqual({}); + }); + + it("should handle empty arrays", () => { + const sanitized = sanitizePrototypePollution([]); + expect(sanitized).toEqual([]); + }); + + it("should handle objects with only dangerous properties", () => { + const obj = { + __proto__: { isAdmin: true }, + constructor: { bad: true }, + prototype: { bad: true }, + }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({}); + }); + + it("should handle objects with null prototype", () => { + const obj = Object.create(null); + obj.name = "test"; + obj.__proto__ = { isAdmin: true }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized).toEqual({ name: "test" }); + }); + + it("should handle circular references in objects", () => { + const obj = { name: "test", safe: "value" }; + obj.circular = obj; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized.name).toBe("test"); + expect(sanitized.safe).toBe("value"); + expect(sanitized.circular).toBe(sanitized); + }); + + it("should handle circular references with dangerous keys", () => { + const obj = { name: "test", __proto__: { bad: true } }; + obj.circular = obj; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized.name).toBe("test"); + expect(sanitized.circular).toBe(sanitized); + expect(Object.prototype.hasOwnProperty.call(sanitized, "__proto__")).toBe(false); + }); + + it("should handle nested circular references", () => { + const obj = { name: "outer", nested: { name: "inner" } }; + obj.nested.parent = obj; + obj.self = obj; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized.name).toBe("outer"); + expect(sanitized.nested.name).toBe("inner"); + expect(sanitized.nested.parent).toBe(sanitized); + expect(sanitized.self).toBe(sanitized); + }); + + it("should handle circular references in arrays", () => { + const arr = [1, 2, { name: "test" }]; + arr.push(arr); + const sanitized = sanitizePrototypePollution(arr); + expect(sanitized[0]).toBe(1); + expect(sanitized[1]).toBe(2); + expect(sanitized[2]).toEqual({ name: "test" }); + expect(sanitized[3]).toBe(sanitized); + }); + + it("should handle very deep nesting (1000 levels)", () => { + let obj = { value: "leaf", __proto__: { bad: true } }; + for (let i = 0; i < 1000; i++) { + obj = { level: i, nested: obj, __proto__: { bad: true } }; + } + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized.level).toBe(999); + expect(Object.prototype.hasOwnProperty.call(sanitized, "__proto__")).toBe(false); + // Verify we can traverse to the leaf + let current = sanitized; + for (let i = 999; i >= 0; i--) { + expect(current.level).toBe(i); + expect(Object.prototype.hasOwnProperty.call(current, "__proto__")).toBe(false); + current = current.nested; + } + expect(current.value).toBe("leaf"); + }); + + it("should handle mixed circular and nested structures", () => { + const root = { name: "root" }; + const child1 = { name: "child1", parent: root }; + const child2 = { name: "child2", parent: root, sibling: child1 }; + root.children = [child1, child2]; + child1.sibling = child2; + const sanitized = sanitizePrototypePollution(root); + expect(sanitized.name).toBe("root"); + expect(sanitized.children[0].name).toBe("child1"); + expect(sanitized.children[1].name).toBe("child2"); + expect(sanitized.children[0].parent).toBe(sanitized); + expect(sanitized.children[1].parent).toBe(sanitized); + expect(sanitized.children[0].sibling).toBe(sanitized.children[1]); + expect(sanitized.children[1].sibling).toBe(sanitized.children[0]); + }); + + it("should handle array with circular object references", () => { + const obj1 = { name: "obj1" }; + const obj2 = { name: "obj2", ref: obj1 }; + obj1.ref = obj2; + const arr = [obj1, obj2, obj1, obj2]; + const sanitized = sanitizePrototypePollution(arr); + expect(sanitized[0].name).toBe("obj1"); + expect(sanitized[1].name).toBe("obj2"); + expect(sanitized[0]).toBe(sanitized[2]); + expect(sanitized[1]).toBe(sanitized[3]); + expect(sanitized[0].ref).toBe(sanitized[1]); + expect(sanitized[1].ref).toBe(sanitized[0]); + }); + + it("should handle objects with repeated references (non-circular)", () => { + const shared = { value: "shared", __proto__: { bad: true } }; + const obj = { + ref1: shared, + ref2: shared, + nested: { + ref3: shared, + }, + }; + const sanitized = sanitizePrototypePollution(obj); + expect(sanitized.ref1.value).toBe("shared"); + expect(sanitized.ref1).toBe(sanitized.ref2); + expect(sanitized.ref1).toBe(sanitized.nested.ref3); + expect(Object.prototype.hasOwnProperty.call(sanitized.ref1, "__proto__")).toBe(false); + }); + + it("should handle malicious deeply nested attack with circularity", () => { + const attack = { __proto__: { exploit: true } }; + for (let i = 0; i < 100; i++) { + attack.nested = { __proto__: { exploit: true }, level: i }; + if (i === 50) { + attack.nested.circular = attack; + } + Object.assign(attack, attack.nested); + } + const sanitized = sanitizePrototypePollution(attack); + expect(Object.prototype.hasOwnProperty.call(sanitized, "__proto__")).toBe(false); + expect({}.exploit).toBeUndefined(); + }); + }); + + describe("real-world attack scenarios", () => { + it("should prevent prototype pollution via __proto__", () => { + const malicious = { type: "create_issue", __proto__: { isAdmin: true } }; + const sanitized = sanitizePrototypePollution(malicious); + expect(sanitized).toEqual({ type: "create_issue" }); + // Verify that the prototype was not polluted + expect({}.isAdmin).toBeUndefined(); + }); + + it("should prevent prototype pollution via constructor", () => { + const malicious = { + type: "update_issue", + constructor: { prototype: { isAdmin: true } }, + }; + const sanitized = sanitizePrototypePollution(malicious); + expect(sanitized).toEqual({ type: "update_issue" }); + }); + + it("should handle agent output with prototype pollution attempt", () => { + const malicious = { + type: "create_issue", + title: "Legitimate Issue", + body: "Description", + __proto__: { isAdmin: true, polluted: true }, + constructor: { prototype: { injected: true } }, + }; + const sanitized = sanitizePrototypePollution(malicious); + expect(sanitized).toEqual({ + type: "create_issue", + title: "Legitimate Issue", + body: "Description", + }); + }); + + it("should handle deeply nested pollution attempts", () => { + const malicious = { + type: "create_issue", + metadata: { + __proto__: { level1: true }, + config: { + constructor: { level2: true }, + settings: { + prototype: { level3: true }, + value: "safe", + }, + }, + }, + }; + const sanitized = sanitizePrototypePollution(malicious); + expect(sanitized).toEqual({ + type: "create_issue", + metadata: { + config: { + settings: { + value: "safe", + }, + }, + }, + }); + }); + }); + + describe("integration with common patterns", () => { + it("should work with Object.assign after sanitization", () => { + const target = { existing: "value" }; + const malicious = { new: "data", __proto__: { isAdmin: true } }; + const sanitized = sanitizePrototypePollution(malicious); + Object.assign(target, sanitized); + expect(target).toEqual({ existing: "value", new: "data" }); + expect({}.isAdmin).toBeUndefined(); + }); + + it("should prevent pollution when pushing to arrays", () => { + const items = []; + const malicious = { type: "item", __proto__: { polluted: true } }; + const sanitized = sanitizePrototypePollution(malicious); + items.push(sanitized); + expect(items).toEqual([{ type: "item" }]); + expect({}.polluted).toBeUndefined(); + }); + + it("should work with spread operator after sanitization", () => { + const malicious = { safe: "data", __proto__: { isAdmin: true } }; + const sanitized = sanitizePrototypePollution(malicious); + const result = { ...sanitized, extra: "value" }; + expect(result).toEqual({ safe: "data", extra: "value" }); + expect({}.isAdmin).toBeUndefined(); + }); + }); + }); }); diff --git a/docs/src/content/docs/agent-factory-status.mdx b/docs/src/content/docs/agent-factory-status.mdx index 8a8a586153..0b913cf48b 100644 --- a/docs/src/content/docs/agent-factory-status.mdx +++ b/docs/src/content/docs/agent-factory-status.mdx @@ -22,7 +22,6 @@ These are experimental agentic workflows used by the GitHub Next team to learn, | [Automated Portfolio Analyst](https://github.com/github/gh-aw/blob/main/.github/workflows/portfolio-analyst.md) | copilot | [![Automated Portfolio Analyst](https://github.com/github/gh-aw/actions/workflows/portfolio-analyst.lock.yml/badge.svg)](https://github.com/github/gh-aw/actions/workflows/portfolio-analyst.lock.yml) | - | - | | [Basic Research Agent](https://github.com/github/gh-aw/blob/main/.github/workflows/research.md) | copilot | [![Basic Research Agent](https://github.com/github/gh-aw/actions/workflows/research.lock.yml/badge.svg)](https://github.com/github/gh-aw/actions/workflows/research.lock.yml) | - | - | | [Blog Auditor](https://github.com/github/gh-aw/blob/main/.github/workflows/blog-auditor.md) | claude | [![Blog Auditor](https://github.com/github/gh-aw/actions/workflows/blog-auditor.lock.yml/badge.svg)](https://github.com/github/gh-aw/actions/workflows/blog-auditor.lock.yml) | - | - | -| [Bot Detection Agent 🔍🤖](https://github.com/github/gh-aw/blob/main/.github/workflows/bot-detection.md) | copilot | [![Bot Detection Agent 🔍🤖](https://github.com/github/gh-aw/actions/workflows/bot-detection.lock.yml/badge.svg)](https://github.com/github/gh-aw/actions/workflows/bot-detection.lock.yml) | - | - | | [Brave Web Search Agent](https://github.com/github/gh-aw/blob/main/.github/workflows/brave.md) | copilot | [![Brave Web Search Agent](https://github.com/github/gh-aw/actions/workflows/brave.lock.yml/badge.svg)](https://github.com/github/gh-aw/actions/workflows/brave.lock.yml) | - | `/brave` | | [Breaking Change Checker](https://github.com/github/gh-aw/blob/main/.github/workflows/breaking-change-checker.md) | copilot | [![Breaking Change Checker](https://github.com/github/gh-aw/actions/workflows/breaking-change-checker.lock.yml/badge.svg)](https://github.com/github/gh-aw/actions/workflows/breaking-change-checker.lock.yml) | - | - | | [Changeset Generator](https://github.com/github/gh-aw/blob/main/.github/workflows/changeset.md) | codex | [![Changeset Generator](https://github.com/github/gh-aw/actions/workflows/changeset.lock.yml/badge.svg)](https://github.com/github/gh-aw/actions/workflows/changeset.lock.yml) | - | - | diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index d50423d981..8d049e52d5 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -2991,7 +2991,8 @@ safe-outputs: # (optional) title: null - # Allow updating issue body - presence of key indicates field can be updated + # Allow updating issue body. Set to true to enable body updates, false to disable. + # For backward compatibility, null (body:) also enables body updates. # (optional) body: null