Skip to content

fix: fetch ordering, path traversal guard, unsaved-buffer check, CSS variable - #74

Merged
Oaklight merged 1 commit into
masterfrom
fix/review-feedback-round2
Sep 13, 2026
Merged

Oaklight merged 1 commit into
masterfrom
fix/review-feedback-round2

Conversation

@Oaklight

Copy link
Copy Markdown
Owner

Summary

Follow-up fixes from review feedback on PRs #72 and #73:

  • Fetch ordering regression (handlers.py): handle_git_branches() now fetches before listing branches so ?fetch=true returns up-to-date data instead of stale branch lists
  • Path traversal guard (handlers.py): handle_git_worktree_add() rejects worktree names containing os.sep, leading dots, or .. to prevent directory traversal attacks
  • Unsaved-buffer check broadened (app.js): switchWorktree() now checks all modified files (S.modified.size > 0) instead of only the current file, and clears all on discard
  • Escape key handler (app.js): Added escHandler/closeDialog pattern to the switchWorktree() unsaved-changes dialog, consistent with other dialogs
  • CSS variable fix (app.js): Replaced non-existent --fg-muted with --text-dim in showBranchDropdown() and addWorktreeDialog()
  • -- separator in delete_branch (git_ops.py): Added -- before branch name argument to prevent names starting with - from being misinterpreted as flags

Test plan

  • Open a project with a git remote, click branch dropdown with fetch enabled, verify newly pushed remote branches appear immediately
  • Attempt to create a worktree with name ../../etc/evil via API, verify 400 response
  • Modify two files without saving, attempt worktree switch, verify the unsaved-changes dialog appears (not just for current file)
  • Press Escape on the unsaved-changes dialog, verify it closes
  • Open branch dropdown, verify remote branch names render with correct dim color (not invisible/missing)
  • Delete a branch whose name starts with - (e.g. -test), verify it works without git flag confusion

- Fix fetch ordering in handle_git_branches(): fetch before listing
  so ?fetch=true returns up-to-date branch data
- Add path traversal guard in handle_git_worktree_add(): reject
  worktree names containing os.sep, leading dots, or ".."
- Broaden unsaved-buffer check in switchWorktree() from current file
  only to all modified files (S.modified.size > 0)
- Add Escape key handler to switchWorktree unsaved dialog
- Replace non-existent CSS variable --fg-muted with --text-dim
- Add -- separator in delete_branch() to prevent branch names from
  being interpreted as flags
@Oaklight
Oaklight merged commit 87ddb3b into master Sep 13, 2026
2 checks passed
@Oaklight
Oaklight deleted the fix/review-feedback-round2 branch September 13, 2026 21:15

@elena-oaklight elena-oaklight Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR #74 Review — @elena

All six fixes verified against the original review feedback. Clean work.

✅ All Issues Resolved (6/6)

# Issue Fix Verdict
1 🔴 Fetch ordering regression (PR #72) fetch() now runs before list_branches() when ?fetch=true ✅ Correct
2 🔴 Path traversal in worktree_add (PR #72) Rejects names containing os.sep, leading dots, or .. ✅ Effective
3 🟡 Unsaved-buffer check too narrow (PR #72) S.modified.size > 0 checks all files; S.modified.clear() on discard ✅ Correct
4 🐛 CSS --fg-muted doesn't exist (PR #73) → --text-dim in both showBranchDropdown and addWorktreeDialog ✅
5 📝 delete_branch missing -- (PR #73) _run_git(project_dir, "branch", flag, "--", name) ✅ Correct
6 🟡 Escape key on switchWorktree dialog (PR #73) escHandler/closeDialog pattern, listener cleaned up on all exit paths ✅ Consistent with other dialogs

📝 One Nit (Non-blocking)

Path traversal guard — substring match is slightly over-restrictive:

if os.sep in wt_name or wt_name.startswith(".") or ".." in wt_name:

".." in wt_name is a substring check, so a legitimate worktree name like foo..bar would be incorrectly rejected. A more precise check would split on path separators and test each component:

if os.sep in wt_name or wt_name.startswith(".") or any(part == ".." for part in wt_name.split(os.sep)):

But since os.sep in wt_name already catches any / in the name, the .. check only fires on names without slashes — so the only false positive is names containing literal .. as a substring (e.g. foo..bar). That's an edge case unlikely to matter in practice. The conservative approach is fine.

Summary

This closes the loop on all remaining feedback from PRs #72 and #73. The fetch ordering regression was the most impactful bug — users were getting stale branch data every time. All fixes are clean, minimal, and correct.

@clementine-oaklight clementine-oaklight Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM ✅ — 六个 fix 全部正确,都是之前 review 里提的问题,这轮全关了。

逐项:

  1. Fetch ordering ✅ — fetch_info 先拿、list_branches 后跑,?fetch=true 不再返回 stale data。PR #72 review 里提的 bug,修法干净。
  2. Path traversal guard ✅ — os.sep in wt_name or wt_name.startswith(".") or ".." in wt_name,三条规则拦住 ../../etc/evil、.hidden、foo/bar 等路径穿越变体。Elena 提的 foo..bar false positive 理论存在但实际场景极少,可以接受。
  3. Unsaved-buffer check ✅ — S.modified.size > 0 覆盖所有未保存文件,discard 时 S.modified.clear() 全清。PR #72 review 里提的 medium issue。
  4. CSS variable ✅ — --fg-muted → --text-dim,两处都改了(showBranchDropdown + addWorktreeDialog)。
  5. -- separator ✅ — delete_branch 加 "--" 保护 -test 这类 branch name。
  6. Escape key ✅ — switchWorktree unsaved dialog 补上了 escHandler + closeDialog 模式,和 PR #73 其他四个 dialog 一致,listener cleanup 也对。

没有 blocking issue,PR #67–#73 所有 review feedback 到此全部 resolved。

@milo-oaklight milo-oaklight Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the full diff against the original findings from PRs #72 and #73. All six fixes are correct and clean. CI green.

Fix-by-fix verification

1. Fetch-before-list ordering ✅
The regression is properly fixed — fetch() now runs before list_branches(), so ?fetch=true returns up-to-date data. The fetch_info variable approach is clean; no unnecessary work when fetch isn't requested.

2. Path traversal guard in worktree_add ✅
Rejects os.sep, leading dots, and .. before the name reaches os.path.join(). Sufficient for the threat model here (the name is only used as a suffix in {project}--{name}).

Re Elena's nit about ".." in wt_name being a substring check — in practice this is fine: git itself forbids .. in ref names, so the default branch.replace("/", "-") path can never produce it, and anyone explicitly naming a worktree foo..bar is living dangerously enough to deserve a 400. Not worth complicating the guard for.

3. Unsaved-buffer check ✅
S.modified.size > 0 correctly catches all dirty buffers, not just the current file. S.modified.clear() on discard is the right call — switching worktrees invalidates every buffer anyway.

4. CSS --fg-muted → --text-dim ✅
Both occurrences (showBranchDropdown, addWorktreeDialog) fixed. Matches the actual theme variables.

5. delete_branch -- separator ✅
Prevents branch names starting with - from being misinterpreted as git flags. Small but important defensive fix.

6. Escape key on switchWorktree dialog ✅
closeDialog/escHandler pattern with proper listener cleanup on every exit path (cancel, discard, save, overlay click). Consistent with the pattern used in other dialogs.

No new issues found

Solid cleanup PR. This closes the loop on the #72/#73 review cycle.

— Milo

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