BACK-467 - Add local file preview with syntax highlighting and line numbers#634
BACK-467 - Add local file preview with syntax highlighting and line numbers#634kuwork wants to merge 1 commit into
Conversation
MrLesk
left a comment
There was a problem hiding this comment.
Alex's Agent: Thanks for the PR. I like the underlying idea: being able to preview local files from task references/documentation would be useful in the Web UI. I do not think this implementation is ready to merge yet, though.
Main changes I would ask for:
- Please do not put project file reading/parsing directly in
src/server/index.ts. The new/api/file-contenthandler currently owns path parsing, traversal policy, file IO, line slicing, and response shaping. That should live behind a small, tested core/filesystem API, with the server only adapting HTTP input/output. - The current containment check is unsafe.
targetPath.startsWith(rootDir)lets sibling directories with the same prefix through. I reproduced/api/file-content?path=../project-secret/secret.txtreturning file contents when the project root was/tmp/.../project. Please use a robust containment check, such as normalized relative path semantics and/or realpath where appropriate, and add regression tests for sibling-prefix traversal, absolute paths,.., and valid in-root paths. - The file reader needs safer bounds. Before
file.text(), please consider file type/size limits and clear behavior for binary or very large files. This endpoint can otherwise pull arbitrary project-root files into the browser response. MermaidMarkdownnow intercepts every relative link even when noonFileClickhandler is provided. If the file exists, it prevents default navigation and then does nothing. That affects markdown rendered outside task details and also markdown rendered inside the preview modal. Please only install the preview click behavior when a handler exists, or preserve normal link behavior otherwise.- The markdown autolink sanitizer regex changed in a way that appears unrelated and risky: it now excludes hyphens, which can break links like
<https://example-site.com>. Hash links are also forced intotarget="_blank". Please keep existing markdown/link behavior unchanged unless it is directly needed for the file-preview feature. - Backlog hygiene needs cleanup: the PR title is fine, but the body says
closes BACK-465while the task added here isBACK-467. Current main already has BACK-465 for an unrelated Windows MCP document issue. The added task also has an empty assignee, no final summary/DoD section, and AC #10 mentions a Tailwind upgrade even though this PR does not change package/lock/style files.
CI is green, and the feature direction is good. I would prefer this come back as a smaller implementation where the filesystem access is centralized and regression-tested, then the Web UI consumes that API without changing unrelated markdown behavior.
1. Code structure — Extracted everything into 2. Containment check — directory traversal Replaced const rel = relative(rootDir, targetPath);
const isInside = !rel.startsWith("..") && !isAbsolute(rel);Added regression tests for sibling-prefix traversal, absolute paths, 3. File safety bounds Added a 5MB size limit, a directory check via 4.
5. Markdown autolink sanitizer Restored the original regex ( 6. Backlog hygiene Updated directly in the PR page on GitHub (assignee, removed AC #10, added DoD). |
|
Hi @MrLesk , this PR is ready to merge. Could you please review and approve when you have a moment?By the way, |
BACK-467 - Add local file preview with syntax highlighting and line numbers
Summary
Adds a local file preview feature to the Web UI. When viewing a task, users can click local file paths in References, Documentation, or Markdown content (Description / Plan / Notes / Final Summary) to view file contents directly in a modal.
Key features:
src/server/index.ts:35-39)MDEditor.Markdown../rejected); only files within the project root are accessiblePath semantics: paths are always resolved relative to the project root (the directory containing
backlog/). Use relative paths such assrc/server/index.tsorCLI-INSTRUCTIONS.md. Absolute paths and upward traversal are rejected.Changes since last review
/api/file-contentlogic intocore/filesystem API(FileSystem.readProjectFile)startsWithcontainment check with robustrelative+isAbsolutevalidationMermaidMarkdownonly intercepts relative links when anonFileClickhandler is providedtarget="_blank"Related Tasks
closes BACK-467
Task Checklist
backlog/tasks/Testing
file.ts:10-20) to confirm only the specified range loads and line numbers start at the correct offset../etc/passwdto verify the API returns 403/404 and blocks directory traversal.ts,.md,.css, etc.)