Skip to content

Add secure SSH profiles and remote asset fetching - #46

Draft
matchaboar wants to merge 1 commit into
nonatofabio:mainfrom
matchaboar:feat/ssh-asset-fetching
Draft

matchaboar wants to merge 1 commit into
nonatofabio:mainfrom
matchaboar:feat/ssh-asset-fetching

Conversation

@matchaboar

Copy link
Copy Markdown

Summary

Focused SSH, remote document, and local/remote asset split from #43, rebased onto current main.

  • Load disabled-by-default SSH profiles from Application Support and open a favorite configured root.
  • List and fetch remote documents without copying the full workspace.
  • Mirror remote cache paths and migrate legacy document/annotation caches.
  • Resolve local image paths without breaking literal or percent-encoded ?/# filenames.
  • Fetch referenced remote images under explicit security boundaries.

Review fixes

  • Hoist encodeImagePath into callable scope; a bad image is isolated to a placeholder instead of aborting applyAll() before annotations, search, or Mermaid setup.
  • Allow only avif, gif, jpeg, jpg, png, svg, and webp assets.
  • Decode and normalize references, reject absolute/scheme/backslash/control-character paths, and confine lexical paths to the configured profile root.
  • Resolve remote canonical paths with realpath and reject symlink escapes outside the root.
  • Cap fetches at 32 unique image assets per document and report skipped/failing images.
  • Fail clearly when the configured remote root is missing; there is no $HOME fallback or whole-home scan.
  • Ship the generated profile template commented out; there is no active hostname: test default.
  • Preserve README’s “Never escapes upward” file-browser promise.

Red/green evidence

Against the original #43 implementation, the focused regression harness exposed five SSH/asset security failures and the reader harness reproduced the encodeImagePath ReferenceError. The final implementation passes:

  • SSHTarget: 22 checks
  • SSHProfile: 14 checks
  • RemoteMarkdownAssets: 16 checks
  • SSHTransport: 43 checks
  • Reader image pipeline: path encoding and continuation through post-image passes

Manual image pass

A WKWebView fixture rendered a relative sample.svg through mindle-file:// at 120×80 and verified the reader pipeline continued. No real SSH host was available, so remote listing, root/symlink confinement, fetch limits, and failure handling are automated rather than manually exercised. Hosts without realpath fail remote image fetching loudly rather than weakening confinement.

Validation

  • ./run-tests.sh — passes Swift and reader/WKWebView harnesses
  • ./build.sh — passes

@nonatofabio

Copy link
Copy Markdown
Owner

Please keep this in draft for now. The image root check still has a gap: SSHTransport.swift validates the path with realpath, then scp opens it in a separate operation. Someone who can change the remote tree can swap the file or a parent directory for a symlink between those steps.

A local test using the production fetchReferencedImages path reproduced this with an injected runner: it ran the actual resolver command, swapped the validated image for a symlink, then simulated the later read. The cache ended up with bytes from outside the root, with one successful fetch and no reported failures. No real SSH host was involved. Please make confinement apply to the file actually opened and transferred, rather than relying on an earlier path check, and add a regression test for that swap. We also need limits on bytes and transfer time; the 32-image cap doesn't cover either.

There are two other fixes needed. In openRemote, image fetching adds an await after the duplicate-tab check, so two opens of the same document can both get through and create tabs. Recheck after the last await, immediately before finishOpen, or share the in-flight open, and test with delayed image fetching.

RemoteMarkdownAssets also decodes percent escapes repeatedly while reader.js decodes once. For example, image%2523.png gets fetched as image#.png, but the reader looks for image%23.png. Please use the same decoding rules throughout and add a test that fetches and renders a filename containing a literal percent escape.

The 95 supplied logic checks pass locally, but they miss these cases. Once they're covered, please rebase onto current main, preserve the v3.2 reader changes, and get the build and reader tests passing on the rebased commit. A real-host SSH check would help too, since the current evidence uses local fixtures and fake runners.

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