Skip to content

fix(workspace): strip surrounding quotes from Add Space path input#1750

Closed
nesquena-hermes wants to merge 1 commit intomasterfrom
fix/workspace-path-strip-quotes
Closed

fix(workspace): strip surrounding quotes from Add Space path input#1750
nesquena-hermes wants to merge 1 commit intomasterfrom
fix/workspace-path-strip-quotes

Conversation

@nesquena-hermes
Copy link
Copy Markdown
Collaborator

fix(workspace): strip surrounding quotes from Add Space path input

Why

macOS Finder's "Copy as Pathname" (Cmd+Option+C) wraps paths in single quotes by default — '/Users/x/Documents/foo' — and users routinely paste those quoted strings into the Add Space input expecting them to work. Other shells and OS file managers do similar things with double quotes.

Today _handle_workspace_add reads path_str = body.get("path", "").strip() — only outer whitespace, no quote handling — so the literal quote characters become part of the resolved Path and the validator rejects the result as "not a directory."

Cygnus reported this on Discord (2026-05-01) and self-resolved by manually un-quoting her paths, but explicitly asked for the strip:

"When I take out the quotation marks, it works ... it would be cool if being wrapped in at least single quotes on the outside didn't break it, because that's how paths natively get copied from the context menu (or cmd+option+C) in the Finder."

Change

api/workspace.py (1 new helper, 1 call site update)

def _strip_surrounding_quotes(path: str) -> str:
    """Strip a single pair of surrounding single or double quotes from a path string."""
    s = path.strip()
    if len(s) >= 2 and s[0] == s[-1] and s[0] in ("'", '"'):
        return s[1:-1]
    return s

validate_workspace_to_add() calls it before Path(...).expanduser().resolve() so every code path that registers a workspace benefits, not just the HTTP route.

api/routes.py (1 import + 1 call site update)

_handle_workspace_add() also calls _strip_surrounding_quotes() at the route entry. This means the blocked-system-path check and the duplicate-detection check both see the cleaned form, not the quoted form.

Behavior matrix (14 regression tests in tests/test_workspace_add_quote_strip.py)

Input Output
/Users/x/Documents/foo /Users/x/Documents/foo (unchanged)
'/Users/x/Documents/foo' /Users/x/Documents/foo (single-quote pair stripped)
"/Users/x/Documents/foo" /Users/x/Documents/foo (double-quote pair stripped)
'/Users/x/foo' /Users/x/foo (whitespace trimmed first)
'/Users/x/it's-mine/foo' /Users/x/it's-mine/foo (only outermost pair)
'/Users/x/foo '/Users/x/foo (unpaired — preserved)
'/Users/x/foo" '/Users/x/foo" (mismatched — preserved)
'' "" (empty — handled by route's existing "path is required" check)

Only the outermost paired quotes are stripped — paths legitimately containing quote characters mid-string are untouched. Mismatched and unpaired quotes are preserved on the slim chance the path actually contains a literal quote.

Verification

$ pytest tests/ -x --timeout=60 -q
4610 passed, 2 skipped, 3 xpassed, 1 warning, 8 subtests passed in 147.96s

(+14 new tests; 0 regressions; full suite ~2:27.)

Reporter

Cygnus on Discord, May 1 2026.

macOS Finder's 'Copy as Pathname' (Cmd+Option+C) wraps paths in single
quotes by default — '/Users/x/Documents/foo' — and users routinely paste
those quoted strings into the Add Space input expecting them to work.
Other shells and OS file managers do similar things with double quotes.

Today the path is taken via .strip() only, so the literal quote
characters become part of the resolved Path and the validator rejects
the result as 'not a directory'. cygnus reported this on Discord
(2026-05-01) — she had to manually un-quote her paths to register a
new Space.

Fix:
  - New api.workspace._strip_surrounding_quotes() helper. Removes only
    the outermost paired single or double quotes; preserves unpaired or
    mismatched quotes (a path may legitimately contain a literal quote).
  - validate_workspace_to_add() calls it before resolution so every
    code path that registers a workspace benefits, not just the HTTP
    route.
  - _handle_workspace_add() also calls it at the route entry so the
    blocked-system-path check and the duplicate-detection check both
    see the cleaned form.

14 regression tests pin the behavior matrix:
  - Unwrapped path unchanged
  - Single quotes stripped
  - Double quotes stripped
  - Whitespace outside quotes handled (trim-then-strip)
  - Only outermost pair removed (internal quotes preserved)
  - Unpaired / mismatched quotes preserved
  - Empty string + just-a-pair edge cases
  - Validate_workspace_to_add accepts quoted form for existing dir

4610 tests pass (+14 from this PR), 0 regressions, ~2:27 full suite.

Reported by Cygnus on Discord, May 1 2026.
Copy link
Copy Markdown
Owner

@nesquena nesquena left a comment

Choose a reason for hiding this comment

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

Review — end-to-end ✅ (clean approve, behavioural harness confirms quoted-path round-trip)

What this ships

Self-built PR adding _strip_surrounding_quotes() to the Add Space input path so macOS Finder's "Copy as Pathname" output (Cmd+Option+C wraps paths in single quotes by default) and other shell-quoted paste shapes are accepted as workspaces. Reported by Cygnus on Discord (2026-05-01) — she had to manually un-quote her paths and explicitly asked for the strip.

Three changes:

  • api/workspace.py:569-587 — new _strip_surrounding_quotes(path) helper that removes a single pair of matching ' or " quotes (only outermost, only paired). Wired into validate_workspace_to_add() at api/workspace.py:602 so every code path that registers a workspace benefits.
  • api/routes.py:6510_handle_workspace_add() calls the helper at the route entry, so the blocked-system-path check, optional mkdir, and duplicate-detection check all see the cleaned form.
  • tests/test_workspace_add_quote_strip.py — 14 regression tests across two classes (helper unit tests + route-level smoke).

Traced against upstream hermes-agent

Cross-tool: zero. validate_workspace_to_add and _strip_surrounding_quotes are webui-internal. Verified via grep -rn 'validate_workspace_to_add\|_strip_surrounding_quotes' /tmp/hermes-agent-fresh/ — no agent-side references. The CLI never sees this input format. ✓

End-to-end trace

Pre-fix (Cygnus's case):

  1. User does Cmd+Option+C on a Finder folder → clipboard = '/Users/cygnus/Projects/Foo'.
  2. User pastes into Add Space input → POST /api/workspaces/add with body.path = "'/Users/cygnus/Projects/Foo'".
  3. _handle_workspace_add does body.get("path", "").strip() → still has quotes.
  4. Path("'/Users/cygnus/Projects/Foo'").resolve()<cwd>/'/Users/cygnus/Projects/Foo' (literal quote-character path component).
  5. Validator rejects: Path does not exist. ❌

Post-fix:

  1. Same up to step 2.
  2. path_str = _strip_surrounding_quotes(body.get("path", "").strip())/Users/cygnus/Projects/Foo.
  3. Path("/Users/cygnus/Projects/Foo").resolve() → real path.
  4. _is_blocked_system_path(candidate) → not blocked.
  5. Optional mkdir → noop since exists.
  6. validate_workspace_to_add(path_str) → strips again (idempotent), exists check passes, dir check passes, returns Path.
  7. Workspace registered. ✅

Behavioural harness — pre/post fix + edge inputs

Drove the helper against real tempfile.TemporaryDirectory() directories with spaces in the name (Finder's typical case):

=== Pre-fix simulation: validate WITHOUT strip ===
  resolved: /Users/.../hermes-webui/'/var/folders/.../My Workspace'
  exists: False                                                     ← bug shape

=== Post-fix: validate_workspace_to_add with quoted path ===
  resolved: /private/var/folders/.../My Workspace
  exists: True
  matches expected: True                                            ← fix verified

=== Edge inputs through _strip_surrounding_quotes ===
  "'/tmp/foo'"          -> '/tmp/foo'        single-quote pair
  '"/tmp/foo"'          -> '/tmp/foo'        double-quote pair
  "'/tmp/it's/foo'"     -> "/tmp/it's/foo"   internal quote preserved
  '/tmp/foo'            -> '/tmp/foo'        unchanged
  "''"                  -> ''                empty after pair strip
  '""'                  -> ''                empty after pair strip
  ''                    -> ''                empty
  '  /tmp/foo  '        -> '/tmp/foo'        whitespace trimmed
  "  '/tmp/foo'  "      -> '/tmp/foo'        whitespace + quote pair
  "'/tmp/foo"           -> "'/tmp/foo"       unpaired — preserved

All shapes behave as the test matrix specifies.

Pre-fix test verification

Reverted api/workspace.py and api/routes.py to origin/master and ran the new tests:

ImportError: cannot import name '_strip_surrounding_quotes' from 'api.workspace'
ERROR tests/test_workspace_add_quote_strip.py — Interrupted: 1 error during collection

The test file fails at import on master because the helper doesn't exist. That's the right shape of "tests would have caught the missing fix" — these are real regression coverage, not pure shape assertions.

Other audit — things that are correct already

  • Blocklist bypass impossible: I traced whether clever quoting could bypass _is_blocked_system_path. The _handle_workspace_add route does its own block check at api/routes.py:6521 on the resolved candidate, AND validate_workspace_to_add re-runs _is_blocked_workspace_path at api/workspace.py:618 on the fully-resolved path. So even if an attacker passes ''/etc'' (4 quotes total — only outer pair stripped, inner pair survives, resolves to junk path that isn't /etc), the validator's second strip + re-resolve catches it. Defense-in-depth holds.
  • Only paired quotes stripped: s[0] == s[-1] and s[0] in ("'", '"') — guards against single-sided quotes that might be part of a legitimate path. Verified by test_unpaired_leading_quote_preserved, test_unpaired_trailing_quote_preserved, test_mismatched_quote_pair_preserved.
  • Internal quotes preserved: '/Users/x/it's-mine/foo'/Users/x/it's-mine/foo (only outermost pair removed). Apostrophes in directory names survive.
  • Whitespace ordering: s = path.strip() runs FIRST, so '/Users/x/foo' correctly strips outer whitespace, then the quote pair. If we did it in the other order, leading whitespace would block the pair-detection.
  • len(s) >= 2 guard: prevents s[0] == s[-1] matching on a single character (e.g. "'" would otherwise be detected as s[0]=='\''==s[-1] and try to slice to s[1:-1] = empty). Correctly handled — single quote alone returns unchanged.
  • Empty string handled: _strip_surrounding_quotes("") returns "". The route handler's if not path_str: return bad(handler, "path is required") then catches it after strip. So '' (just a quote pair) → strip to "" → "path is required" error message, which is the right UX.
  • Idempotent: calling _strip_surrounding_quotes twice on the same input is safe (first call removes the pair, second call sees a string without surrounding quotes and returns it unchanged). The route handler strips first, then validate_workspace_to_add strips again — no double-strip damage.
  • No path traversal surface: the strip happens before Path.expanduser().resolve() — same path resolution code runs on the cleaned input as on an unquoted user submission. No new attack surface.
  • No eval/exec/shell=True anywhere near this code. Pure string slice + Path() resolution.
  • _handle_workspace_add checks _is_blocked_system_path(candidate) BEFORE the optional mkdir at api/routes.py:6521 — so a quoted blocked-path doesn't create an orphan directory before being rejected. The strip preserves this ordering.

Edge-case trace

Scenario Expected Actual
/Users/x/foo (unquoted) unchanged
'/Users/x/foo' (single quotes — Finder default) strip pair
"/Users/x/foo" (double quotes — bash quoting) strip pair
'/path/foo' (whitespace + quotes) trim then strip pair
'/Users/x/it's-mine/foo' (internal apostrophe) only outer pair removed
'/Users/x/foo (unpaired leading) preserved
/Users/x/foo' (unpaired trailing) preserved
'/Users/x/foo" (mismatched) preserved
'' (just a quote pair) empty after strip
"" (just double-quote pair) empty after strip
''/etc'' (4 quotes — bypass attempt) strip outer, junk path, validator rejects
'/etc' (quoted blocked system root) strip → /etc → blocked by _is_blocked_system_path
Path with literal space '/My Workspace' strips quotes, resolves with space intact
Cross-tool: agent CLI doesn't see this input n/a (webui-only route)
Concurrent quoted+unquoted submissions for same dir duplicate-detection sees same resolved path → "Workspace already in list"

Tests

  • tests/test_workspace_add_quote_strip.py — 14/14 pass (11 unit tests on the helper + 3 integration tests on validate_workspace_to_add with real tmp_path dirs).
  • Pre-fix verification: test file fails at collection on origin/master (ImportError: cannot import name '_strip_surrounding_quotes') — the right shape of regression coverage.
  • Full suite (excluding pre-existing macOS bash 3.2 test_ctl_script.py failures unrelated to this PR): 4551 passed, 57 skipped, 3 xpassed, 0 failed in 40.46s.

Minor observations (non-blocking)

  • Double strip on the route path: _handle_workspace_add strips at the route entry, then validate_workspace_to_add strips again internally. Idempotent and safe, but slightly redundant. The dual strip is intentional per the PR description ("every code path that registers a workspace benefits, not just the HTTP route") — non-route callers of validate_workspace_to_add get the strip even if they didn't pre-clean. Worth a comment but not a code change.
  • Helper named with leading underscore (_strip_surrounding_quotes) suggesting "private to module", but it's imported and re-exported via api.routes.py:1387. Not a real issue — Python doesn't enforce — but consistency-wise, strip_surrounding_quotes (no underscore) would be more honest. Cosmetic.
  • No handling of "smart quotes" ('…' or "…"). If a user pastes from a chat client / word processor / macOS that auto-converted ASCII quotes to Unicode curly quotes, the strip won't fire. macOS Finder's "Copy as Pathname" uses ASCII single quotes (verified empirically), so the targeted bug shape is handled. Smart-quote handling is a follow-up if it gets reported.
  • No handling of triple-quoted shells: """...""" would be detected as s[0]==s[-1]=='"' and the outermost pair stripped, leaving ""...""" → recursion would help, but the test suite intentionally only does one layer (good — keeps the implementation simple). Followers can re-paste if double-stripped is wrong.
  • Cygnus's specific paste case (single quotes from macOS Finder) is the exact shape the helper handles; her quote was reproduced verbatim in the harness above and now resolves correctly.

Recommendation

Approved. Surgical fix targeting the exact shape Cygnus reported. The implementation is the correct lightweight choice (strip outermost paired quotes only, preserve internal/unpaired/mismatched). Defense-in-depth holds: even if an attacker tries to bypass the blocklist via clever quoting, both the route handler and validate_workspace_to_add re-run the block check on the fully-resolved path. Behavioural harness with real tempfile.TemporaryDirectory confirms pre-fix breakage and post-fix success end-to-end.

Cross-tool safe (webui-only route, agent CLI never sees this input). 14/14 PR tests + full suite green. Pre-fix verification confirms the tests catch the missing helper at collection time.

Parked at approval — ready for the release agent's merge/tag pipeline.

@nesquena-hermes
Copy link
Copy Markdown
Collaborator Author

Thanks @nesquena-hermes — this shipped in v0.51.11 (commit 9900248) as part of a 3-PR full-sweep batch release. Stage rebased your branch onto current master, ran the full pre-release gate (4622 pytest, browser tests, Opus advisor verdict SHIP), and merged via release PR #1751.

GitHub didn't auto-close because the merge commit only references the squash-merged stage branch, not your fork's commit directly — closing manually for hygiene.

Live now on https://get-hermes.ai/ and on existing installs after git pull + restart.

Release notes: https://github.com/nesquena/hermes-webui/releases/tag/v0.51.11

pull Bot pushed a commit to soitun/hermes-webui that referenced this pull request May 6, 2026
pull Bot pushed a commit to soitun/hermes-webui that referenced this pull request May 6, 2026
…-color meta, quote-strip) + test-isolation hardening (nesquena#1746 deferred)

Constituent PRs:
- nesquena#1747 (@Michaelyklam) — wait for model catalog before opening picker (closes nesquena#1743)
- nesquena#1748 (@nesquena-hermes) — theme-color meta tag for native chrome bridges (nesquena APPROVED)
- nesquena#1750 (@nesquena-hermes) — strip surrounding quotes from Add Space path (nesquena APPROVED)

Deferred to v0.51.12:
- nesquena#1746 — Opus caught multiprocessing.Queue deadlock pattern (parent
  process.join() before queue drain hangs on output >64KB pipe buffer).
  Deferral comment with two specific fix options posted on PR.

Plus 1 in-stage absorbed test-isolation fix:
- test_issue1426 + test_issue1680: skip on detected prefix pollution
  (prong 2 of test-isolation-flake-recipe). Failure rate ~25% in full
  suite from sys.modules pollution; standalone always passes.

Tests: 4596 → 4622 passing (+26). 0 regressions. Stably green.

Pre-release verification:
- 3 PRs CI-green individually + rebased onto master
- pytest 4622 passed, 0 failed
- node -c clean on static/ui.js + static/boot.js
- 11/11 browser API endpoints PASS
- Opus advisor: SHIP nesquena#1747/nesquena#1748/nesquena#1750, MUST-FIX block on nesquena#1746

Closes nesquena#1743.
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.

2 participants