Skip to content

feat(mobile): add session file transfer controls - #529

Open
shenlvkang-collab wants to merge 2 commits into
Ark0N:masterfrom
shenlvkang-collab:feat/mobile-file-transfer
Open

shenlvkang-collab wants to merge 2 commits into
Ark0N:masterfrom
shenlvkang-collab:feat/mobile-file-transfer

Conversation

@shenlvkang-collab

Copy link
Copy Markdown
Contributor

What changed

Mobile users can now manage files from the existing session File Viewer:

  • Choose a file through the browser's native system picker (Android can hand this to ES File Manager or another installed provider).
  • Upload the selected file into the active session's temp/ directory.
  • Copy the resulting relative path (temp/...) to the clipboard so it can be pasted into the active agent prompt.
  • Download remains available through the existing file links.
  • Delete regular workspace files with a confirmation button in the file tree.

Safety

  • Upload and delete are scoped to the authenticated session workspace.
  • Uploaded names are sanitized and written with exclusive, no-follow file creation.
  • Sensitive/blocked paths and .git content cannot be deleted.
  • Remote SSH sessions explicitly reject these local filesystem operations.
  • Existing download limits and multipart limits apply.

Validation

  • TypeScript typecheck passed using the repository's local dependency tree.
  • ESLint passed after the filename sanitization fix.
  • Frontend syntax check passed for all 38 public JavaScript files.
  • git diff --check passed.

The Android wrapper does not need a custom file manager implementation: the hidden <input type=file> delegates selection to Android's system picker, which preserves the user's chosen file provider and returns control to Codeman.

@Ark0N

Ark0N commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Thanks for this, @shenlvkang-collab! This PR adds an upload button that sends a file from the device picker into the session's temp/ folder and copies its path, plus a delete button on file rows in the Files panel. The upload route follows the paste-image hardening closely (refusing a symlinked temp/, O_EXCL|O_NOFOLLOW, the remote refusal), which made it easy to review.

There are two things I need fixed before this can merge, plus tests:

1. The delete button's inline onclick can be escaped by a filename (src/web/public/panels-ui.js:3568 and :3747)

encodeURIComponent does not escape ', so a path containing an apostrophe closes the string literal in decodeURIComponent('...'). I rendered the exact template in jsdom and clicked it. A file named x'-eval(atob('...'))-'.txt ran arbitrary JavaScript in Codeman's origin, and a plain it's.txt throws a SyntaxError, so its delete button does nothing. Workspace filenames come from agents and cloned repos, so this has to be treated as hostile input. Please drop the inline handler and bind the button in the existing querySelectorAll('.file-tree-item') loops instead, reading the already-escaped item.dataset.path:

item.querySelector('.file-tree-delete')?.addEventListener('click', (e) => {
  e.stopPropagation();
  this.deleteFileFromBrowser(ownerSessionId, item.dataset.path);
});

2. Deleting a symlink deletes its target (src/web/routes/file-routes.ts:1735, :1750, :1759)

validateSessionFilePath() returns the realpath, so the lstat and unlink that follow act on the link's target. In a route test with AGENTS.md -> CLAUDE.md, DELETE ?path=AGENTS.md returned 200, removed CLAUDE.md, and left AGENTS.md dangling. The tree lists symlinks as files, so every symlink gets a delete button. Please lstat the unresolved path first. Then either refuse symlinks, or confine the realpath of the parent directory and unlink the link itself.

3. Tests for both routes (src/web/routes/file-routes.ts:1633, :1723)

test/routes/file-write-routes.test.ts is a good template (a real temp workspace via app.inject). Please cover:

  • Upload: the happy path (returns temp/..., file mode 0600), a symlinked temp/ (403), a remote session, a wrong field name and an empty file.
  • Delete: the happy path, the symlink case above, .git/... (403), ../ (404), a directory (400), a remote session, and a foreign session in multi-user mode (404 via the harness authUser option).

Smaller things worth fixing in the same round:

  • file-routes.ts:1682: concurrent first uploads race on mkdir(temp). In my run, 6 parallel uploads returned one 200 and five 500s with EEXIST. paste-image handles this at session-routes.ts:5201-5211 (accept EEXIST after re-checking that the path is a real directory).
  • file-routes.ts:1691-1697: long names fail with a 500 ENAMETOOLONG, and the error includes the absolute server path. Please cap safeName to around 120 bytes, keeping the extension.
  • panels-ui.js:3830: after deleting from search results, loadFileBrowser does not re-render the search view, so the deleted row stays listed. Re-running the active search fixes it.
  • styles.css:10920: the delete button only shows on :hover. On a phone, the first tap opens the preview instead. A @media (hover: none) rule that always shows .file-tree-download and .file-tree-delete fixes this for both buttons.
  • index.html:556 and panels-ui.js:3800: the labels say "current folder/temp", but uploads always go to temp/ at the workspace root.

I'll follow up separately on where uploads should land (a visible temp/ versus a hidden Codeman folder like .claude-images/), so please hold off on that part until then. Once items 1 to 3 are in, I'll re-review, and I'm happy to pick up the CLAUDE.md note about the new write surface myself at merge time.

@JDProfresh

Copy link
Copy Markdown
Contributor

@Ark0N, since you mentioned you'd follow up on where uploads should land: I've written up a proposal for that in #553, together with a prompt-side Attach button and document uploads that touch the same route. @shenlvkang-collab, it suggests a location your Files-panel upload could share, so I wanted you to see it here rather than be surprised by it later.

@Ark0N

Ark0N commented Oct 9, 2026

Copy link
Copy Markdown
Owner

@shenlvkang-collab, following up on where uploads should land, as promised: the decision is in #553. Uploads go to a hidden, self-ignoring <workspace>/.codeman-uploads/ instead of a visible temp/.

For this PR that means:

@Ark0N

Ark0N commented Oct 13, 2026

Copy link
Copy Markdown
Owner

Hi @shenlvkang-collab, a quick check-in on this one. The upload location is settled now: #574 landed <workspace>/.codeman-uploads/ on 10-10, and #606 added the phone Attach button for images and video on top of it.

That means two things for this PR:

  1. Items 1 to 3 from my review never depended on the location, so they can go in right away: bind the delete button through item.dataset.path instead of the inline onclick (a filename can currently run JavaScript), lstat the unresolved path so deleting a symlink no longer deletes its target, and the route tests for upload and delete.
  2. The upload half now overlaps with planned work. In Prompt uploads: where they should land, documents and video, and a visible Attach on phones #553, @JDProfresh said a documents upload comes next on the same paste-image route. Please check with them there before reworking the Files-panel upload, so you don't both build the same thing. If yours stays, it should write through the shared helper into .codeman-uploads/ (the directory checks and the naming), so the 7-day sweep and the delete cleanup cover it. The delete button is unique to this PR either way.

If it's simpler, narrowing this PR to delete only and leaving the upload to the #553 follow-up is completely fine too.

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.

3 participants