From dfc4e26689e4462e332294d6e696aab178978b7c Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Thu, 6 Aug 2026 00:34:24 +0000 Subject: [PATCH 1/2] fix(security): clear the two security:sast findings blocking pre-push (#729) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mise run security:sast` was red on main, so the pre-push hook failed on every branch regardless of its diff — while SAST is not a required merge check, making the local gate stricter than the merge gate and training contributors to reach for --no-verify. Both findings are false positives, suppressed inline per the AGENTS.md convention rather than by widening config: - orchestration-comment-trigger.ts: `phrase` is never attacker-controlled — the sole caller passes RETRY_PHRASES literals and metachars are escaped one line above. `text` is the untrusted side and is the subject, not the pattern. - linear_epic.py: the target is the LINEAR_URL module constant, not a caller-supplied URL, so there is no SSRF surface. Verified the suppressions are line- and rule-scoped, not blanket: planting a genuinely user-controlled `new RegExp(userInput)` elsewhere in the same file is still reported. Co-Authored-By: Claude --- cdk/src/handlers/shared/orchestration-comment-trigger.ts | 1 + scripts/linear_epic.py | 1 + 2 files changed, 2 insertions(+) diff --git a/cdk/src/handlers/shared/orchestration-comment-trigger.ts b/cdk/src/handlers/shared/orchestration-comment-trigger.ts index d643ecaff..1f1e1dd19 100644 --- a/cdk/src/handlers/shared/orchestration-comment-trigger.ts +++ b/cdk/src/handlers/shared/orchestration-comment-trigger.ts @@ -218,6 +218,7 @@ function hasPhrase(text: string, phrase: string): boolean { // Escape regex metachars (e.g. "re-run"); match on non-word boundaries so // "retry" doesn't fire inside a longer word. const esc = phrase.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + // nosemgrep: javascript.lang.security.audit.detect-non-literal-regexp.detect-non-literal-regexp -- `phrase` is never attacker-controlled: the only caller passes literals from the RETRY_PHRASES const, and metacharacters are escaped on the line above. `text` is the untrusted side and is the subject, not the pattern. return new RegExp(`(^|[^a-z0-9])${esc}([^a-z0-9]|$)`, 'i').test(text); } diff --git a/scripts/linear_epic.py b/scripts/linear_epic.py index ea70bb870..7f789e65f 100644 --- a/scripts/linear_epic.py +++ b/scripts/linear_epic.py @@ -86,6 +86,7 @@ def gql(query, variables=None): headers={"Authorization": pat(), "Content-Type": "application/json"}, ) try: + # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected -- the request target is the LINEAR_URL module constant (https://api.linear.app/graphql), not a caller-supplied or user-supplied URL, so there is no SSRF surface. with urllib.request.urlopen(req, timeout=30) as r: out = json.load(r) except urllib.error.HTTPError as e: From 9a05b6a248fabf3b834e40dd558331a1dbab0386 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Thu, 6 Aug 2026 01:15:02 +0000 Subject: [PATCH 2/2] fix(security): remediate both security:sast findings instead of suppressing them (#729) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior commit on this branch cleared the two `security:sast` findings with inline `nosemgrep` lines. Both are now fixed at the source and the suppressions are gone — `security:sast` passes with zero suppressions in either file. **`detect-non-literal-regexp` (orchestration-comment-trigger.ts)** `hasPhrase` built a regex by interpolating `phrase`, which forced a metacharacter-escaping step to stay *correct* (not merely quiet) and made the phrase list a regex-injection/ReDoS surface the moment it grew a non-literal member. Replaced with a plain `indexOf` scan that checks the flanking chars, so the input cannot be misparsed as a pattern and the escaping step disappears. Proven behaviour-preserving by differential test against the old regex over 2,097,152 (text, phrase) pairs — including case variants, Unicode, and every regex metacharacter: 0 mismatches. Two characterization tests now pin the boundary and match-literally contracts (they pass on both implementations). **`dynamic-urllib-use-detected` (scripts/linear_epic.py)** The real issue was not the URL — it was the opener. `urlopen` uses the default handler set, which includes `file://`, `ftp://` and `data://`, in a script that carries the Linear PAT. That was a live capability, not a phantom: the old path demonstrably reads a local file. Now built via a bare `OpenerDirector` with only `HTTPSHandler` (+ the error handlers, so `except HTTPError` still fires), making HTTPS the only reachable scheme. Note `build_opener(HTTPSHandler)` would have satisfied the scanner while still carrying `FileHandler`/`FTPHandler`/`DataHandler` — a dodge, not a fix. Verified: `security:sast` exit 0 · both findings still detected when a genuine `new RegExp(userInput)` / `urlopen(user_url)` is planted, so the rules remain live · cdk 176/176 suites (3515 tests) · cli 55/55 (695) · eslint clean, no mutations · https 200 + HTTPError paths exercised against a real endpoint · ruff findings on the touched file drop 15 → 13, none new. Also merges current `main` so the stale `mise.toml` on this branch no longer reverts #728's gitleaks scoping. Refs #729, #695, #540/#722, #728 --- .../shared/orchestration-comment-trigger.ts | 27 ++++++++++++++----- .../orchestration-comment-trigger.test.ts | 20 ++++++++++++++ scripts/linear_epic.py | 24 ++++++++++++++--- 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/cdk/src/handlers/shared/orchestration-comment-trigger.ts b/cdk/src/handlers/shared/orchestration-comment-trigger.ts index 1f1e1dd19..2730156ea 100644 --- a/cdk/src/handlers/shared/orchestration-comment-trigger.ts +++ b/cdk/src/handlers/shared/orchestration-comment-trigger.ts @@ -213,13 +213,28 @@ export function buildIntegrationIterationInstruction(trigger: CommentTrigger): s */ const MAX_COMMAND_WORDS = 6; -/** Word/phrase boundary match: the phrase appears as whole words in ``text``. */ +/** True when ``c`` is one of the [a-z0-9] chars that count as "inside a word". */ +function isAlnum(c: string | undefined): boolean { + return c !== undefined && ((c >= 'a' && c <= 'z') || (c >= '0' && c <= '9')); +} + +/** Word/phrase boundary match: the phrase appears as whole words in ``text``. + * + * Plain substring scan rather than a phrase-interpolated ``new RegExp`` — a + * dynamically built pattern would need metachar escaping to stay correct and + * would make the phrase list a regex-injection / ReDoS surface if it ever grew + * a non-literal member. Comparing chars directly cannot misparse its input. + */ function hasPhrase(text: string, phrase: string): boolean { - // Escape regex metachars (e.g. "re-run"); match on non-word boundaries so - // "retry" doesn't fire inside a longer word. - const esc = phrase.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - // nosemgrep: javascript.lang.security.audit.detect-non-literal-regexp.detect-non-literal-regexp -- `phrase` is never attacker-controlled: the only caller passes literals from the RETRY_PHRASES const, and metacharacters are escaped on the line above. `text` is the untrusted side and is the subject, not the pattern. - return new RegExp(`(^|[^a-z0-9])${esc}([^a-z0-9]|$)`, 'i').test(text); + const hay = text.toLowerCase(); + const needle = phrase.toLowerCase(); + if (!needle) return false; + // Accept only occurrences flanked by a non-[a-z0-9] char (or a string edge), + // so "retry" doesn't fire inside a longer word like "retryx" / "abcretry". + for (let i = hay.indexOf(needle); i !== -1; i = hay.indexOf(needle, i + 1)) { + if (!isAlnum(hay[i - 1]) && !isAlnum(hay[i + needle.length])) return true; + } + return false; } /** diff --git a/cdk/test/handlers/shared/orchestration-comment-trigger.test.ts b/cdk/test/handlers/shared/orchestration-comment-trigger.test.ts index f48e64894..5bd939f57 100644 --- a/cdk/test/handlers/shared/orchestration-comment-trigger.test.ts +++ b/cdk/test/handlers/shared/orchestration-comment-trigger.test.ts @@ -204,6 +204,26 @@ describe('parseRetryIntent — recognise a plain "retry" command', () => { expect(parseRetryIntent('**retry**')).toBe(true); }); + test('a retry phrase EMBEDDED in a longer word does not fire', () => { + // The phrase match is whole-word: only a non-[a-z0-9] char (or a string + // edge) may flank it. Guards the boundary contract that the substring scan + // in hasPhrase upholds (it replaced a phrase-interpolated `new RegExp`). + for (const s of ['retryx', 'abcretry', 'xrerun', 'rerunner', 'retry9', '9retry']) { + expect(parseRetryIntent(s)).toBe(false); + } + }); + + test('phrases containing regex metacharacters match literally, not as patterns', () => { + // "re-run" is a RETRY_PHRASES member containing '-'. A phrase must never be + // interpreted as a pattern: "re.run"/"reXrun" must NOT match "re-run", and a + // user typing regex syntax must not have it evaluated. + expect(parseRetryIntent('re-run')).toBe(true); + expect(parseRetryIntent('re.run')).toBe(false); + expect(parseRetryIntent('reXrun')).toBe(false); + expect(parseRetryIntent('.*')).toBe(false); + expect(parseRetryIntent('(retr)y')).toBe(false); + }); + test('KNOWN_EPIC_COMMANDS lists retry (kept in sync with the parser + panel copy)', () => { expect(KNOWN_EPIC_COMMANDS).toContain('retry'); }); diff --git a/scripts/linear_epic.py b/scripts/linear_epic.py index 7f789e65f..906452809 100644 --- a/scripts/linear_epic.py +++ b/scripts/linear_epic.py @@ -32,8 +32,8 @@ import json import os import sys -import urllib.request import urllib.error +import urllib.request LINEAR_URL = "https://api.linear.app/graphql" @@ -79,6 +79,25 @@ def pat(): return p +def https_opener(): + """An opener that can speak ONLY https. + + ``urllib.request.urlopen`` (and ``build_opener``) install the default + handler set, which includes ``file://``, ``ftp://`` and ``data://``. That + turns any future mistake which lets a URL be configured — an env var, a CLI + flag, an API-returned redirect target — into an arbitrary-file read, since + this function carries the Linear PAT. Building the director by hand keeps + HTTPS as the only reachable scheme, so no such mistake can escalate. + ``HTTPErrorProcessor``/``HTTPDefaultErrorHandler`` preserve the usual + raise-on-4xx/5xx behaviour the caller below relies on. + """ + opener = urllib.request.OpenerDirector() + opener.add_handler(urllib.request.HTTPSHandler()) + opener.add_handler(urllib.request.HTTPErrorProcessor()) + opener.add_handler(urllib.request.HTTPDefaultErrorHandler()) + return opener + + def gql(query, variables=None): body = json.dumps({"query": query, "variables": variables or {}}).encode() req = urllib.request.Request( @@ -86,8 +105,7 @@ def gql(query, variables=None): headers={"Authorization": pat(), "Content-Type": "application/json"}, ) try: - # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected -- the request target is the LINEAR_URL module constant (https://api.linear.app/graphql), not a caller-supplied or user-supplied URL, so there is no SSRF surface. - with urllib.request.urlopen(req, timeout=30) as r: + with https_opener().open(req, timeout=30) as r: out = json.load(r) except urllib.error.HTTPError as e: sys.exit(f"HTTP {e.code}: {e.read().decode()[:400]}")