Commit Graph

57 Commits

  • fix(ci): match quoted write-all + dedupe duplicate checkout violations
    Two round-1 review findings, fixed together because they touch the
    same regex/loop region of `findViolations`:
    
    1. **cubic P0 — quoted write-all bypass**.
       `WRITE_ALL_PATTERN` was `/^\s*permissions:\s*write-all\b/m`, which
       does not match the perfectly valid YAML forms
       `permissions: "write-all"` and `permissions: 'write-all'`. A
       workflow that quoted the shorthand slipped right through the
       persist-credentials gate the previous commit was supposed to close.
    
       Reproduced before this commit:
         $ cat /tmp/q.yml
         name: bad
         on: [push]
         permissions: "write-all"
         jobs:
           do:
             runs-on: ubuntu-latest
             steps:
               - uses: actions/checkout@v4
         $ ECC_WORKFLOWS_DIR=/tmp node scripts/ci/validate-workflow-security.js
         Validated workflow security for 1 workflow files
         exit=0
    
       Fix: tighten the regex to
         /^\s*permissions:\s*["']?write-all["']?\s*$/m
       which accepts the bare, double-quoted, and single-quoted YAML forms
       while still anchoring on the `permissions:` key. The trailing `\s*$`
       prevents accidentally matching keys whose value happens to start
       with `write-all` (e.g. some future literal `write-all-something`).
    
    2. **greptile P2 — duplicate violation when both patterns match**.
       A `ref: refs/pull/${{ github.event.pull_request.head.sha }}/merge`
       value matches both the `pull_request_target` rule's
       `expressionPattern` (the `head.sha` interpolation) and its
       `refPattern` (the `refs/pull/` literal). Each push generates an
       ERROR line with the same description and just a different
       `expression:` echo, so the reviewer sees the same violation twice.
    
       Fix: track `stepFlagged` inside the per-step loop and skip the
       `refPattern` fallback once any `expressionPattern` match has already
       produced a violation for this step. The `refPattern` is a fallback
       for ref-only forms (`refs/pull/123/head`, `${{ env.X }}` whose
       resolved value is a PR ref); when the more specific expression
       already fires, the fallback is redundant by definition.
    
    After both fixes, the round-1 reproductions resolve cleanly:
    
      $ # quoted form now blocks
      $ ECC_WORKFLOWS_DIR=/tmp/q1/.github/workflows node scripts/ci/validate-workflow-security.js
      ERROR: quoted.yml:8 - workflows with write permissions must disable checkout credential persistence
      exit=1
    
      $ # combined head.sha + refs/pull now prints one ERROR, not two
      $ ECC_WORKFLOWS_DIR=/tmp/q2/.github/workflows node scripts/ci/validate-workflow-security.js
      ERROR: dup.yml:10 - pull_request_target must not checkout an untrusted pull_request head ref/repository
        Unsafe expression: ${{ github.event.pull_request.head.sha }}
      exit=1
    
    Test additions land in the next commit.
  • fix(ci): flag refs/pull checkouts under pull_request_target
    The `pull_request_target` rule's `expressionPattern` matches only
    the canonical `github.event.pull_request.head.{ref,sha,repo.full_name}`
    interpolations. It does not match the second canonical form of
    the same exploit — fetching `refs/pull/<N>/{head,merge}` directly:
    
      - uses: actions/checkout@v4
        with:
          ref: refs/pull/${{ github.event.pull_request.number }}/merge
    
    The merge-ref variant is what GitHub's own security guidance calls
    out as the highest-severity privilege-escalation pattern under
    `pull_request_target`: it materialises the PR's merge commit
    (attacker code spliced with base), executes inside a workflow that
    has full repo-scoped tokens, and gives the attacker the chance to
    exfiltrate secrets or push to default branches. `refs/pull/N/head`
    is functionally equivalent — same source, same trust boundary.
    
    Reproduced on `main` before this commit:
    
      $ cat /tmp/bad.yml
      name: bad
      on: { pull_request_target: { types: [opened] } }
      permissions: { contents: read }
      jobs:
        do:
          runs-on: ubuntu-latest
          steps:
            - uses: actions/checkout@v4
              with:
                ref: refs/pull/${{ github.event.pull_request.number }}/merge
                persist-credentials: false
            - run: npm ci --ignore-scripts
    
      $ ECC_WORKFLOWS_DIR=/tmp node scripts/ci/validate-workflow-security.js
      Validated workflow security for 1 workflow files
      $ echo $?
      0
    
    Expected: violation flagging the refs/pull checkout under pull_request_target.
    Actual: passes silently.
    
    Fix: add a `refPattern` to the `pull_request_target` rule:
    
        /^\s*ref:\s*['"]?[^'"\n]*refs\/(?:remotes\/)?pull\/[^'"\n\s]+/m
    
    and apply it per checkout step inside the existing
    event-gated loop. The pattern matches the ref VALUE so it catches
    all interpolation shapes — `refs/pull/123/head`,
    `refs/pull/${{ github.event.pull_request.number }}/merge`,
    `${{ env.FOO }}/refs/pull/N/head` — without enumerating the
    possible interpolations themselves.
    
    Scoping: the rule is already gated on the workflow containing
    `pull_request_target:`, so non-privileged `pull_request` workflows
    that legitimately check out a PR ref are not affected.
    
    After this commit the reproduction above exits 1 with:
    
      ERROR: bad.yml:10 - pull_request_target must not checkout an untrusted pull_request head ref/repository
    
    Three new regression tests in `tests/ci/validate-workflow-security.test.js`:
      - rejects pull_request_target + refs/pull/<N>/merge
      - rejects pull_request_target + hardcoded refs/pull/<N>/head
      - allows pull_request_target with no `with.ref:` (base-ref checkout —
        the safe pattern from GitHub's own guidance)
    
    Test count: 17 → 20 in this file; full `yarn test` still green.
    
    Together with the previous commit, this closes the two
    independent `validate-workflow-security.js` bypasses I found.
  • fix(ci): treat 'permissions: write-all' as a write-permission gate
    `WRITE_PERMISSION_PATTERN` in `validate-workflow-security.js`
    enumerates named GitHub Actions scopes (`contents: write`,
    `issues: write`, etc.) to decide whether a workflow needs to:
      - disable `persist-credentials` on `actions/checkout`
      - pass `--ignore-scripts` to `npm ci`
    
    The pattern misses the top-level shorthand `permissions:
    write-all`, which is the strictly broader form — it grants every
    named scope write access in a single line. As a result, a
    workflow that opts into write-all currently slips both gates.
    
    Reproduced on `main` before this commit:
    
      $ cat /tmp/bad.yml
      name: bad
      on: [push]
      permissions: write-all
      jobs:
        do:
          runs-on: ubuntu-latest
          steps:
            - uses: actions/checkout@v4
            - run: npm ci
    
      $ ECC_WORKFLOWS_DIR=/tmp node scripts/ci/validate-workflow-security.js
      Validated workflow security for 1 workflow files
      $ echo $?
      0
    
    Expected: at least two violations (missing `persist-credentials:
    false`, missing `--ignore-scripts`).
    Actual: passes silently.
    
    Fix: add a sibling pattern `WRITE_ALL_PATTERN` that matches
    `^\s*permissions:\s*write-all\b` and OR it with
    `WRITE_PERMISSION_PATTERN` at the single gate. Both top-level
    and job-level `permissions:` blocks satisfy the `^\s*` prefix.
    
    After this commit the reproduction above exits 1 with:
    
      ERROR: bad.yml:8 - workflows with write permissions must disable checkout credential persistence
      ERROR: bad.yml:9 - workflows with write permissions must install npm dependencies with --ignore-scripts
    
    Three new regression tests in `tests/ci/validate-workflow-security.test.js`:
      - rejects write-all + credential-persisting checkout
      - rejects write-all + `npm ci` without `--ignore-scripts`
      - allows write-all when both gates are satisfied (no over-block)
    
    Test count: 14 → 17 in this file; full `yarn test` still green.
    
    A separate `refs/pull/N/merge` bypass under `pull_request_target`
    exists in the same validator and is fixed in the next commit.
  • feat: add command registry and coverage checks (#1906)
    Salvages the useful parts of #1897 without generated .caliber state or stale counts.
    
    - adds a deterministic command registry generator and drift check
    - commits the current command registry for 75 commands
    - validates the rc.1 README catalog summary against live counts
    - adds a single Ubuntu Node 20 coverage job instead of running coverage in every matrix cell
    
    Co-authored-by: jodunk <jodunk@users.noreply.github.com>
  • docs: add supply-chain incident response playbook
    Add a repo-level supply-chain incident response playbook for npm/GitHub Actions package-registry incidents, anchored on the May 2026 TanStack compromise and prior Shai-Hulud-style npm incidents.
    
    - add `docs/security/supply-chain-incident-response.md` with exposure checks, immediate response steps, workflow rules, publication rules, and escalation triggers
    - link the playbook from `SECURITY.md`
    - reject `pull_request_target` workflows that restore or save shared dependency caches
    - add a regression test for the new `pull_request_target + actions/cache` guardrail
    
    Validation:
    - node tests/ci/validate-workflow-security.test.js (12 passed, 0 failed)
    - node scripts/ci/validate-workflow-security.js (validated 7 workflow files)
    - npx markdownlint-cli 'SECURITY.md' 'docs/security/supply-chain-incident-response.md'
    - npx markdownlint-cli '**/*.md' --ignore node_modules
    - git diff --check
    - node tests/run-all.js (2377 passed, 0 failed)
    - GitHub CI for #1848 green across Ubuntu, Windows, and macOS
    
    No release, tag, npm publish, plugin tag, marketplace submission, or announcement was performed.
  • ci: require npm audit signature checks
    Require npm registry signature verification wherever workflow npm audit checks run.
    
    - add npm audit signatures to CI Security Scan and maintenance security audit jobs
    - teach the workflow security validator to reject npm audit without signature verification
    - keep the repair and Copilot prompt tests portable across Windows path/case and CRLF frontmatter behavior
    
    Validation:
    - node tests/run-all.js (2376 passed, 0 failed)
    - CI current-head matrix green on #1846
  • feat: add GitHub Copilot prompt support
    Adds GitHub Copilot VS Code instruction and prompt files for ECC workflows, with VS Code prompt frontmatter/settings aligned to current docs and tests covering the surface.
    
    Co-authored-by: Girish Kanjiyani <girish.kanjiyani5040@gmail.com>
    Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
  • ci: harden workflow install boundaries
    - run non-test workflow installs with npm ci --ignore-scripts where lifecycle scripts are not needed\n- reject plain npm ci in workflows with write permissions\n- reject actions/cache in id-token: write workflows to reduce OIDC publish cache-poisoning risk
  • docs: salvage focused stale PR contributions
    - add Vite and Redis pattern skills from closed stale PRs
    
    - add frontend-slides support assets
    
    - port skill-comply runner fixes and LLM prompt/provider regressions
    
    - harden agent frontmatter validation and sync catalog counts
  • fix: harden CI validators
    Ports personal-path validator hardening and quoted checkout detection onto current main.
  • fix: sync skill frontmatter and catalog counts
    Adds missing skill frontmatter, normalizes strict YAML metadata, syncs README catalog counts, and extends catalog validation for README/plugin/marketplace count drift.
  • fix(ci): flag SKILL.md frontmatter defects in validate-skills (#1669)
    * fix(ci): flag SKILL.md frontmatter defects in validate-skills
    
    Issue #1663 reported two SKILL.md frontmatter defects (missing `name:`
    on skill-stocktake; literal block-scalar `description: |-` on
    openclaw-persona-forge) that PR #1664 addresses at the data level.
    
    This change is complementary: it extends `scripts/ci/validate-skills.js`
    to catch the same class of defect statically going forward, so the
    frontmatter-vs-renderer problems do not silently reappear as new skills
    land.
    
    ## Checks added
    - Frontmatter must declare a `name:` field.
    - Frontmatter `description:` must not use a literal block scalar
      (`|` / `|-` / `|+`) — these preserve internal newlines and break
      flat-table renderers keyed off `description`. Folded (`>`) and inline
      strings are accepted.
    
    ## Behavior
    - Frontmatter findings default to WARN (exit 0) so this PR does not
      break CI while the two known offenders are still on main. Pass
      `--strict` or set `CI_STRICT_SKILLS=1` to promote them to ERROR
      (exit 1). Structural findings (missing / empty SKILL.md) remain
      errors as before.
    - Today against main, the validator reports exactly two warnings —
      the same two files called out in #1663 — and exits 0. When #1664
      lands, the validator reports zero warnings, at which point strict
      mode can be enabled in CI.
    
    ## Parser notes
    - Bespoke frontmatter parser mirrors the style of `validate-agents.js`
      (tolerant of UTF-8 BOM and CRLF; no new npm dependency).
    - Block-scalar continuation lines are skipped so keys inside a block
      scalar are not mistaken for top-level keys.
    - Hidden directories (`.something/`) under skills/ are now skipped.
    
    ## Tests
    Adds five focused tests to `tests/ci/validators.test.js`:
    - warns when frontmatter is missing `name` (default mode)
    - errors when frontmatter is missing `name` (--strict mode)
    - warns on literal block-scalar description (|-)
    - accepts folded (>) and inline descriptions under --strict
    - skips hidden directories under skills/
    
    ## Docs
    Adds two bullets to the `Skill Checklist` in CONTRIBUTING.md covering
    the two rules now surfaced by the validator.
    
    Refs #1663. Complements (does not compete with) #1664.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * fix(ci): harden SKILL.md frontmatter checks after bot review
    
    Address findings from CodeRabbit, Greptile, and cubic on #1669:
    
    - Guard empty or whitespace-only `name:` values. Previously
      `name:    ` silently passed because the presence check only
      tested key-set membership; now inspectFrontmatter captures
      trimmed values and validate flags an explicit 'name is empty'
      WARN/ERROR.
    - Broaden block-scalar detection to cover YAML 1.2 indent
      indicators (`|2`, `|-2`, `>2-`) and trailing comments
      (`|-  # note`). The old regex required a bare `|`/`>` with
      optional `+`/`-`, which let valid-but-disallowed forms slip
      through.
    - Update CONTRIBUTING.md checklist to list `|+` alongside `|`
      and `|-` for parity with the validator.
    - Extend runSkillsValidator to accept env overrides and add four
      regression tests: empty name, |+ description, |-2 + comment, and
      CI_STRICT_SKILLS=1.
    
    * fix(ci): address round-2 review on validate-skills frontmatter
    
    - Tighten extractFrontmatter closing delimiter to require a newline or
      end-of-file after the closing `---`, so body lines beginning with
      `---text` are not parsed as frontmatter (CodeRabbit).
    - Strip both trailing and comment-only values in inspectFrontmatter, so
      `name: # todo` is surfaced as empty rather than silently passing
      (cubic P2).
    - Extract validateSkillDir helper so the per-directory validation
      block moves out of validateSkills, keeping both functions under the
      50-line guideline (CodeRabbit nit).
    - Hoist runSkillsValidator to module scope in the test harness and
      share the spawnSync import with execFileSync so the helper stops
      re-requiring child_process on every invocation (CodeRabbit nit).
    - Add regression tests: comment-only `name:` values must fail strict
      mode; `---trailing` body lines must not be parsed as frontmatter.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * Update tests/ci/validators.test.js
    
    Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
    
    ---------
    
    Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
  • fix: unblock unicode safety CI lint (#1017)
    * fix: unblock unicode safety CI lint
    
    * fix: unblock shared CI regressions
  • fix(ci): enforce catalog count integrity (#525)
    * fix(ci): enforce catalog count integrity
    
    * test: harden catalog structure parsing
  • feat: architecture improvements — test discovery, hooks schema, catalog, command map, coverage, cross-harness docs
    - AGENTS.md: sync skills count to 65+
    - tests/run-all.js: glob-based test discovery for *.test.js
    - scripts/ci/validate-hooks.js: validate hooks.json with ajv + schemas/hooks.schema.json
    - schemas/hooks.schema.json: hookItem.type enum command|notification
    - scripts/ci/catalog.js: catalog agents, commands, skills (--json | --md)
    - docs/COMMAND-AGENT-MAP.md: command → agent/skill map
    - docs/ARCHITECTURE-IMPROVEMENTS.md: improvement recommendations
    - package.json: ajv, c8 devDeps; npm run coverage
    - CONTRIBUTING.md: Cross-Harness and Translations section
    - .gitignore: coverage/
    
    Made-with: Cursor
  • fix: reject empty/invalid array commands in hooks validator, add 19 tests
    validate-hooks.js: Empty arrays [] and arrays with non-string elements
    (e.g., [123, null]) passed command validation due to JS truthiness of
    empty arrays (![] === false). Added explicit length and element type
    checks.
    
    19 new tests covering: non-array event type values, null/string matcher
    entries, string/number top-level data, empty string/array commands,
    non-string array elements, non-string type field, non-number timeout,
    timeout boundary (0), unwrapped hooks format, legacy format error paths,
    empty agent directory, whitespace-only command files, valid skill refs,
    mixed valid/invalid rules and skills.
  • fix: reject whitespace-only command/field values in CI validators, add 10 tests
    validate-hooks.js: whitespace-only command strings now fail validation
    validate-agents.js: whitespace-only model/tools values now fail validation
  • fix: greedy regex in validate-commands captures all refs per line, add 18 tests
    The command cross-reference regex /^.*`\/(...)`.*$/gm only captured the
    LAST command ref per line due to greedy .* consuming earlier refs.
    Replaced with line-by-line processing using non-anchored regex to
    capture ALL command references.
    
    New tests:
    - 4 validate-commands multi-ref-per-line tests (regression)
    - 8 evaluate-session threshold boundary tests (new file)
    - 6 session-aliases edge case tests (cleanup, rename, path matching)
  • fix: add input validation, date range checks, and security hardening
    - validate-agents.js: reject invalid model names in agent frontmatter
    - package-manager.js: validate script/binary names against shell injection
    - session-manager.js: reject impossible month/day values in filenames
    - utils.js: support options.all for replaceInFile string patterns
    - strategic-compact/SKILL.md: fix hook matcher syntax and script reference
    - install.sh: warn when overwriting existing rule customizations
    - Add 24 new tests covering all validation and edge cases
  • fix: skip code blocks in command cross-reference validation
    The validator was matching example/template content inside fenced code
    blocks as real cross-references, causing false positives for evolve.md
    (example /new-table command and debugger agent).
    
    - Strip ``` blocks before running cross-reference checks
    - Change evolve.md examples to use bold instead of backtick formatting
      for hypothetical outputs
    
    All 261 tests pass.