Conversation
|
@copilot add more tests, fuzz tests |
…ix spec-enforcer validation Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ec9c665d-7ab1-42fc-a7c9-e64ba729ace7 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…compound expressions Agent-Logs-Url: https://github.com/github/gh-aw/sessions/176a13d4-89c8-425e-ada5-3dac2bdf2132 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added in commit 92b0b65: New JS fuzz harness (
Go fuzz tests updated ( Go unit tests updated ( |
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent — all new tests enforce behavioral contracts.
Test Classification DetailsView all test groups (11 groups across 4 files)
Flagged Tests — Requires ReviewNo tests were flagged for behavioral quality issues. One structural note:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All new tests verify behavioral contracts of isSafeExpression across standalone literals, comparisons, AND/OR compounds, ternary chains, and exhaustive security invariants.
There was a problem hiding this comment.
Pull request overview
This PR updates the JavaScript runtime-import expression validator to align with the Go-side validator so that spec-enforcer.md can safely use ternary-style &&/|| patterns without failing activation-time validation.
Changes:
- Extend
isSafeExpression(JS) to accept standalone literals,&&expressions, and simple comparisons. - Expand Go and JS tests (unit + fuzz seeds) to cover the new compound-expression patterns.
- Add a JS fuzz harness + property-based tests to exercise security invariants for
isSafeExpression.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/runtime_import.cjs |
Extends JS isSafeExpression to allow literals / && / comparisons, and fixes OR/AND fall-through behavior. |
actions/setup/js/runtime_import.test.cjs |
Adds JS unit tests for newly supported expression forms. |
actions/setup/js/fuzz_is_safe_expression_harness.cjs |
Adds a stdin-driven harness to call isSafeExpression for fuzzing/property tests. |
actions/setup/js/fuzz_is_safe_expression_harness.test.cjs |
Adds property-based tests validating the new expression forms and security boundaries. |
pkg/parser/runtime_import_test.go |
Adds Go table tests to exercise JS runtime validator for compound expressions. |
pkg/parser/runtime_import_fuzz_test.go |
Expands fuzz seed corpus and adds a new invariant around unsafe leading namespaces. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 5
| name: "comparison expression with safe property", | ||
| expression: "github.event.inputs.enforce_all == 'true'", | ||
| expectSafe: true, | ||
| description: "Comparison with safe LHS property is allowed", | ||
| }, |
| const { safe, error } = testIsSafeExpression(`${left} ${op} 'value'`); | ||
| expect(error).toBeNull(); | ||
| expect(safe).toBe(true); | ||
| }); | ||
| } |
| const isStringLiteralStandalone = /^(['"`]).*\1$/.test(trimmed); | ||
| if (isStringLiteralStandalone) { | ||
| const contentMatch = trimmed.match(/^(['"`])(.+)\1$/); | ||
| if (contentMatch) { | ||
| const content = contentMatch[2]; | ||
| // Reject nested expressions | ||
| if (content.includes("${{") || content.includes("}}")) { | ||
| return false; | ||
| } | ||
| // Reject escape sequences that could hide keywords | ||
| if (/\\[xu][\da-fA-F]/.test(content) || /\\[0-7]{1,3}/.test(content)) { | ||
| return false; | ||
| } | ||
| // Reject zero-width characters | ||
| if (/[\u200B-\u200D\uFEFF]/.test(content)) { | ||
| return false; | ||
| } | ||
| } |
| // Check for simple comparison expressions (e.g., "github.event.inputs.enforce_all == 'true'"). | ||
| // This check only runs for expressions that have no top-level || or && operators (since those | ||
| // cases are fully handled above), preventing a partially-validated compound expression from | ||
| // sneaking through via the comparison path. | ||
| // Extract each property access on the left side of a comparison operator and verify it is in | ||
| // the allowed list. This mirrors the Go comparisonExtractionRegex logic. | ||
| const compExtractRegex = /([a-zA-Z_][a-zA-Z0-9_.]*)\s*(?:==|!=|<=?|>=?)\s*/g; | ||
| const comparisonProps = []; | ||
| let compMatch; | ||
| while ((compMatch = compExtractRegex.exec(trimmed)) !== null) { | ||
| comparisonProps.push(compMatch[1].trim()); | ||
| } | ||
| if (comparisonProps.length > 0 && comparisonProps.every(prop => isSafeExpression(prop))) { | ||
| return true; | ||
| } |
| it("should allow comparison expressions with safe properties", () => { | ||
| expect(isSafeExpression("github.event.inputs.enforce_all == 'true'")).toBe(!0); | ||
| expect(isSafeExpression("github.actor == 'octocat'")).toBe(!0); | ||
| expect(isSafeExpression("inputs.mode != 'dry-run'")).toBe(!0); | ||
| }); |
|
@copilot this expression links incomplete ${{ github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin' }} Refuse constant literal string subexpressioj in a disjunction/conjunction |
… regex; fix spec-enforcer expression Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d281a45c-1fea-437c-96af-a7570162b2cf Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… invariant fuzz tests Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d281a45c-1fea-437c-96af-a7570162b2cf Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 5e438d6 (with follow-up cleanup in 8f9d307):
Tests updated across Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
spec-enforcer.mdruntime-imports itself during the activation job. The JS runtime validator (isSafeExpression) rejected compound expressions, causing immediate activation failure:Changes
isSafeExpression&&) compound expressions — both sides must be safe non-literal property expressions; a bare literal on either side of&&is refused (a literal in a conjunction is semantically incomplete and may hide injection vectors)||refused —'literal' || expris rejected (always-truthy left side makes the right side dead code); the established OR-fallback patternexpr || 'literal'remains valid==,!=,<,>,<=,>=) — LHS property is validated against the allowlist; comparison check only runs for leaf expressions with no top-level||/&&/^(['"\]).\1$/to/^'(?:[^'\\]|\.)'$|.../, preventing compound expressions like'a' || secrets.TOKEN || 'b'` from being misclassified as a single string literalgithub.actor == 'value' || secrets.TOKENwas incorrectly allowed)spec-enforcer.mdupdated — the ternary-style expression${{ condition && 'full-sweep' || 'round-robin' }}(which uses a literal in&&) is replaced with the valid OR-fallback form:${{ github.event.inputs.enforce_all || 'round-robin' }}Tests
fuzz_is_safe_expression_harness.cjs— new Go-driveable fuzz harness forisSafeExpressioncallable via stdin JSONfuzz_is_safe_expression_harness.test.cjs— 295 property-based fuzz tests covering:expr || 'literal'(allowed)cond && 'val' || 'default'— all refused (literal in AND operand)secrets.*,vars.*, orrunner.*safepkg/parser/runtime_import_fuzz_test.go) — expanded seed corpus with compound expression patterns including refused forms; strengthened invariant checkspkg/parser/runtime_import_test.go) — table-driven cases covering standalone literals, comparisons, AND without literals, refused AND-with-literal, refused ternary, OR-fallback allowed, literal-on-OR-left refused, and unsafe compound variants