[fix] Block option injection in git worktree add + add subprocess deadline (#330)#345
Merged
Merged
Conversation
Shared stdlib-only validator used by both the security layer (A2) and config validation (A4). Accepts the same character set as the existing branchNameRe in gh/pr_meta.go.
A1: Add -- separator to git worktree add to prevent source/path being
parsed as git options (mirrors AddWorktreeFromBranch and Checkout).
A3: Replace duplicated validateBranchName switch in gh/pr_meta.go with
gitref.Validate, keeping PR-number and ghShapeHint decoration.
A4: Validate default_source in config.Validate() via gitref.Validate so
a malicious .rimba/settings.toml is caught before any git call.
Closes part of #330.
…h timeout
B1: Config.CommandTimeout string field (toml command_timeout,omitempty),
EffectiveCommandTimeout() with 120s default, Merge and Validate wired.
B2: ExecRunner.Timeout applies context.WithTimeout per RunInDir invocation;
zero means no timeout (existing callers unaffected until B4 wires it).
B3: gh.Default(timeout) mirrors the same pattern for gh subprocess calls.
Composes with the existing signal context via earliest-deadline-wins.
Closes part of #330.
B4: newRunner(ctx) + newGHRunner(ctx) in helpers.go read timeout from
config.FromContext(ctx); fall back to DefaultCommandTimeout (120s)
when config is absent (bootstrap, completions, update_agent_hint).
B5: All ~30 cmd newRunner() → newRunner(cmd.Context()) call sites.
B6: gh construction sites use newGHRunner(cmd.Context()) uniformly;
duplicate newGHRunner declaration removed from add.go.
B7: Test overrides updated to func(_ context.Context) git/gh.Runner.
MCP path (non-interactive) now gets the same 120s deadline as CLI.
- Add TestAddTaskRejectsUnsafeSource in cmd/add_test.go to cover the --source flag validation added in the previous commit - Promote t.Errorf → t.Fatalf in TestExecRunnerTimeoutExpires so a failed string assertion stops the test immediately - Clarify the gitref.go refNameRe comment to note it is a structural allowlist; git itself enforces trailing-dot / .lock / etc. rules - Update CONTRIBUTING.md to reference the new newRunner(cmd.Context()) and newGHRunner(cmd.Context()) signatures and gh.Default(timeout)
lugassawan
commented
Jun 23, 2026
lugassawan
commented
Jun 23, 2026
… comment - Remove SetAnnotation(flagNoColor, BashCompOneRequiredFlag) from TestAddTaskRejectsUnsafeSource — it was misleading cargo-cult code that marked --no-color as required (harmless only because tests call RunE directly, bypassing Cobra flag validation) - Add inline comment on newRunner(context.Background()) in resolvePostUpdateTipPaths to document the intentional fallback to DefaultCommandTimeout (no cobra cmd in scope at that call site)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--end-of-options separator togit worktree addininternal/git/worktree.go, preventing a user/config-suppliedsourcevalue beginning with-from being parsed by git as a flaginternal/gitrefpackage withValidate()andErrUnsafeRefNamesentinel; validatesdefault_sourcein config at load time and--sourceCLI flag at invocation time, giving a clear error before any subprocess runsExecRunner.Timeoutfield ininternal/git/git.goand mirror it ininternal/gh/runner.go; acontext.WithTimeoutwraps every subprocess call, composing with the existing signal context via earliest-deadline-winscommand_timeoutTOML field (e.g."30s") withEffectiveCommandTimeout()accessor; defaults to 120 s; validated inConfig.Validate(); threaded through allcmd/call sites vianewRunner(cmd.Context())andnewGHRunner(cmd.Context())gh.Default(timeout)now takes an explicit deadline;newRunner/newGHRunnerincmd/helpers.gosource the timeout fromconfig.FromContext(ctx)in one placeTest Plan
make test)make lint)make test-e2e)go test ./... -coverprofile=...→ 97.2%)TestAddWorktreeInsertsDashDash— asserts--appears before path/source in argsTestExecRunnerTimeoutExpires— nanosecond timeout causescontext deadline exceededTestAddTaskRejectsUnsafeSource—--source=-fooreturns "leading dash" errorTestValidate(12 table cases) — all gitref allow/reject paths coveredTestEffectiveCommandTimeout,TestValidateCommandTimeout,TestMergeCommandTimeoutIssue
Closes #330