Skip to content

fix(projects): separate file browsing from archive exclusions - #380

Open
yunfanye wants to merge 1 commit into
mainfrom
fix/projects-file-policies
Open

yunfanye wants to merge 1 commit into
mainfrom
fix/projects-file-policies

Conversation

@yunfanye

Copy link
Copy Markdown
Contributor

What this PR does

The Projects browser used the same ignore list for browsing, uploads, live updates, and folder downloads. That hid useful files and made visibility changes alter archive contents.

Folders named build, coverage, dist, and node_modules can be browsed, edited, and uploaded. Folder downloads include build outputs and coverage reports, while skipping descendant node_modules entries. An explicitly selected dependency folder remains downloadable.

Top-level folders with these names also resolve as projects, keeping the tree and dashboard consistent. Shared-project views use the same file policies within their authorized project.

Follows #379.

Design & Invariants

  • Browsing, watching, uploads, and archive traversal have separate exclusion lists. Each optional list overrides the existing shared fallback, including an empty list. Search keeps its independent exclusions.
  • Directory listings load incrementally. Live updates retain shallow watches over expanded and selected directories, so displaying dependencies does not recursively watch their contents.
  • Build outputs remain available in downloads because they can be the deliverable. Dependency exclusions apply during archive traversal, rather than hiding files from the browser.
  • Dot-entry filtering and symbolic-link download restrictions remain in place. Entry filters do not define authorization.
  • The durable behavior is in Projects, with the interaction guidance in DESIGN.md. The existing project summary contract was checked for removal and remains applicable.

Test plan

  • pnpm typecheck
  • pnpm test:unit
  • pnpm --filter @rome/core test src/api/routes/projects-files.test.ts src/api/routes/share.test.ts src/lib/file-browser-server.test.ts src/api/routes/memory-files.test.ts — 130 tests, including independent policy overrides and an external dependency symlink.
  • pnpm lint:prose with Vale 3.19.0, matching CI.
  • Biome checks and git diff --check.
  • pnpm dev:all and Rome started in the container log.
  • Browser: all four folders visible, dependency file edited and saved, external file creation reflected without reload, project archive downloaded and inspected.
  • Live multipart upload of a folder containing all four names, plus top-level dist dashboard resolution.

Host checks used a fresh install under Node 24.14.1 and pnpm 11.6.0. Nix is unavailable on this host. The dev stack uses offline local auth and a temporary Traefik landing-page port override because an existing Rome instance owns port 80. Browser folder selection did not attach files through the automation extension, so uploads were verified through the live multipart endpoint and regression tests.

Not in this PR

  • Symlink browsing or traversal: pnpm package links retain the existing tree limitation, and archives omit links and dot entries. A dependency-folder download is not a portable dependency bundle.

@Jessie-QingYu Jessie-QingYu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review: 🛑 REQUEST_CHANGES

The PR cleanly separates per-operation file-browser policies and preserves build artifacts in downloads. However, exposing dependency entries makes a symlink traversal gap reachable in shared-project views.

Verdict: REQUEST_CHANGES — The newly visible dependency tree exposes symbolic-link paths that the read handlers can follow outside an authorized shared project.

1 finding(s) posted as inline comments below.

Severity Category File Title
P1 security packages/core/src/lib/project-file-browser.ts Reject symlink traversal before exposing dependency entries

Automated review by RomeOS Code Review · commit fe05c98

import type { FileBrowserScope } from "./file-browser-server.js";

export const PROJECT_FILE_BROWSER_POLICY = {
treeIgnoredNames: [],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] securityReject symlink traversal before exposing dependency entries

Making node_modules tree-visible exposes package-link names in shared-project trees. createTreeHandler and createFileGetHandler resolve paths lexically and then use statSync/readFileSync, which follow those links; a request such as .../tree?path=projects/node_modules/linked-package (or a file beneath it) can enumerate or read an external target despite the share being scoped to one project. The new test only rejects the download endpoint. Reject symbolic-link components (or canonicalize and require the real path to remain under the scoped root) consistently for tree/read/asset/edit operations, and add a shared-route regression test for an external dependency symlink.

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