Skip to content

security: harden JSON parsing in shell scripts#104

Merged
benvinegar merged 4 commits into
mainfrom
bentlegen/harden-shell-json-parsing
Feb 21, 2026
Merged

security: harden JSON parsing in shell scripts#104
benvinegar merged 4 commits into
mainfrom
bentlegen/harden-shell-json-parsing

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

  • add bin/lib/json-common.sh with shared JSON extraction helpers (jq fast-path, python3 fallback)
  • replace brittle grep|sed JSON parsing in critical shell scripts (bin/baudbot, bin/deploy.sh, bin/update-release.sh, bin/rollback-release.sh, bin/security-audit.sh)
  • add regression tests in bin/lib/json-common.test.sh and run them via test/shell-scripts.test.mjs

Validation

  • npm run test:shell
  • bash bin/update-release.test.sh
  • bash bin/rollback-release.test.sh

Notes

  • behavior is backward compatible; this hardens parsing for malformed JSON, missing keys, and whitespace variations

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Feb 21, 2026

Greptile Summary

This PR replaces brittle grep|sed JSON parsing in critical shell scripts with a robust shared library (bin/lib/json-common.sh) that provides jq fast-path with python3 fallback.

Key improvements:

  • Handles malformed JSON, missing keys, and whitespace variations correctly
  • Returns explicit error codes (0=found, 1=missing key, 2=parsing error)
  • Backward compatible behavior with existing scripts
  • Comprehensive test coverage in bin/lib/json-common.test.sh

Files updated:

  • bin/baudbot - version parsing
  • bin/deploy.sh - release metadata reading
  • bin/update-release.sh, bin/rollback-release.sh - deployment verification
  • bin/security-audit.sh - version display

Security context: This hardens parsing in security-critical deployment and audit scripts, reducing risk from malformed metadata files.

Confidence Score: 5/5

  • Safe to merge with minimal risk - hardens JSON parsing across critical infrastructure scripts
  • The changes replace fragile regex-based parsing with proper JSON parsing libraries, include comprehensive tests, maintain backward compatibility, and are purely defensive improvements. The one minor inefficiency in bin/baudbot (4 cat calls) is a style concern, not a correctness issue.
  • No files require special attention - all changes follow consistent patterns and are well-tested

Important Files Changed

Filename Overview
bin/lib/json-common.sh New shared JSON parsing library with jq fast-path and python3 fallback. Clean implementation with proper error codes.
bin/lib/json-common.test.sh Comprehensive test suite covering whitespace handling, missing keys, malformed JSON, and stdin parsing.
bin/baudbot Replaced brittle grep/sed JSON parsing with json-common helpers. One inefficiency: runs cat 4x via sudo instead of once.
bin/deploy.sh Clean migration from grep/sed to json_get_string_or_empty for release metadata parsing.
bin/update-release.sh Replaced complex grep/sed with json_get_string_stdin for deployed SHA verification.
bin/rollback-release.sh Migrated to json_get_string_or_empty and json_get_string_stdin for version file parsing.
bin/security-audit.sh Updated to use json_get_string_or_empty with proper fallback handling for version display.
test/shell-scripts.test.mjs Added test runner integration for json-common.test.sh suite.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Shell Script Needs JSON Value] --> B{jq available?}
    B -->|Yes| C[jq -er with filter]
    B -->|No| D{python3 available?}
    D -->|Yes| E[Python json.load]
    D -->|No| F[Return Error Code 2]
    
    C --> G{Parse Success?}
    E --> G
    
    G -->|Value Found & String| H[Return 0 + Print Value]
    G -->|Key Missing/Wrong Type| I[Return 1]
    G -->|JSON/File Error| F
    
    H --> J[Scripts Use Value]
    I --> K[Scripts Handle Missing Key]
    F --> L[Scripts Handle Parse Error]
    
    style H fill:#90EE90
    style I fill:#FFD700
    style F fill:#FFB6C6
Loading

Last reviewed commit: ebcbf49

Copy link
Copy Markdown

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment thread bin/baudbot Outdated
Comment on lines +304 to +307
short="$(sudo -u "$agent_user" sh -c "cat '$version_file' 2>/dev/null" | json_get_string_stdin "short" 2>/dev/null || true)"
sha="$(sudo -u "$agent_user" sh -c "cat '$version_file' 2>/dev/null" | json_get_string_stdin "sha" 2>/dev/null || true)"
branch="$(sudo -u "$agent_user" sh -c "cat '$version_file' 2>/dev/null" | json_get_string_stdin "branch" 2>/dev/null || true)"
deployed_at="$(sudo -u "$agent_user" sh -c "cat '$version_file' 2>/dev/null" | json_get_string_stdin "deployed_at" 2>/dev/null || true)"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running cat 4 separate times via sudo. Consider reading the file once and piping to 4 separate json_get_string_stdin calls, or creating a temp variable to avoid multiple process spawns.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/baudbot
Line: 304-307

Comment:
Running `cat` 4 separate times via sudo. Consider reading the file once and piping to 4 separate `json_get_string_stdin` calls, or creating a temp variable to avoid multiple process spawns.

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

@benvinegar benvinegar merged commit 47cecc9 into main Feb 21, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant