Skip to content

Fix shell injection in OS fallback#55

Merged
AkshajSinghal merged 3 commits into
TruFoundation:mainfrom
omobolajiadeyan:fix/os-fallback-shell-injection
Jun 8, 2026
Merged

Fix shell injection in OS fallback#55
AkshajSinghal merged 3 commits into
TruFoundation:mainfrom
omobolajiadeyan:fix/os-fallback-shell-injection

Conversation

@omobolajiadeyan

@omobolajiadeyan omobolajiadeyan commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • parse OS fallback input into an argument vector with shlex.split
  • execute fallback commands with shell=False
  • handle malformed quoting without launching a process
  • add regression tests for quoted arguments and shell metacharacters
  • use shared in-memory SQLite for CRUD tests where connection-lifecycle behavior is not under test
  • restore the CLI help/version entry path exposed by the new CI workflow

Security impact

Shell metacharacters such as ; are now passed as literal arguments instead of being interpreted by a host shell. This aligns the fallback helper with TruShell's documented no-shell-expansion model.

Closes #44.

Maintainer follow-up

  • Rebased onto current main, which now includes .github/workflows/python-ci.yml for Python 3.10-3.12 on every pull request. I did not add a duplicate workflow.
  • Added an in_memory_database fixture and moved the three CRUD-oriented database tests to it. The fresh-connection and initialization-lock tests intentionally retain temporary file databases because separate :memory: connections do not share schema/state.
  • Fixed the current-main argv NameError so both CLI help checks in the workflow can run.

Verification

  • python -m pytest tests/test_cli_argv.py tests/test_database.py tests/test_cli_os_fallback.py -q - 9 passed
  • remaining locally applicable tests - 24 passed, 5 skipped
  • 4 pre-existing Windows path-rendering tests remain excluded locally; the Linux CI matrix is authoritative for them
  • isolated Ruff checks on all touched tests - passed
  • Ruff F821 check on trushell/cli.py - passed
  • python -m trushell --help and trushell --help - passed
  • python -m build --wheel - passed

@AkshajSinghal

Copy link
Copy Markdown
Collaborator

Great work getting the test infrastructure in place. The conftest.py and mocked APIs are exactly what we needed to stop the flaky runs.

Two quick things before I merge:

  1. Switch the DB tests to use in-memory SQLite (:memory:) where possible, it’s much faster and keeps the disk clean.
  2. Can you add a simple GitHub Actions workflow file? We need these tests running automatically on every PR so we don’t accidentally break V1.

Fix those, and it’s good to go!

@omobolajiadeyan omobolajiadeyan force-pushed the fix/os-fallback-shell-injection branch from acced1d to e326402 Compare June 8, 2026 10:34
@omobolajiadeyan

Copy link
Copy Markdown
Contributor Author

Implemented both follow-ups and force-updated the rebased branch:

  1. The CRUD database tests now use a shared :memory: SQLite fixture. I retained temporary files only for the fresh-connection and initialization tests, where independent in-memory connections would invalidate what those tests are meant to prove.
  2. Current main now already contains .github/workflows/python-ci.yml from Fix issues in pytests #60, so rebasing brought in the requested Python 3.10-3.12 PR workflow without adding a duplicate.

The rebase also exposed an argv NameError in current main that made both workflow CLI-help commands fail; I fixed that with the existing argv regression test. Targeted tests, touched-file lint, both help commands, and wheel build pass locally. The new CI run is now the final gate.

@omobolajiadeyan

Copy link
Copy Markdown
Contributor Author

The rebased branch is ready, but the workflow run is currently marked action_required because it comes from a fork. Please approve the run when convenient so the Python 3.9-3.12 matrix can execute: https://github.com/TruFoundation/TruShell/actions/runs/27131855948

@AkshajSinghal

Copy link
Copy Markdown
Collaborator

Excellent!

@AkshajSinghal AkshajSinghal merged commit c731f91 into TruFoundation:main Jun 8, 2026
3 checks passed
@omobolajiadeyan

Copy link
Copy Markdown
Contributor Author

Thank you for the review and merge, @AkshajSinghal. I appreciated the focused feedback on the database tests and CI; it made the final change stronger. Glad the shell-injection fix and regression coverage are now in TruShell.

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.

Shell injection via shell=True in OS fallback

2 participants