Skip to content

feat: improve inline file viewing#102

Open
arnabnandy7 wants to merge 2 commits into
Vault-Web:mainfrom
arnabnandy7:feature/media-pdf-viewer
Open

feat: improve inline file viewing#102
arnabnandy7 wants to merge 2 commits into
Vault-Web:mainfrom
arnabnandy7:feature/media-pdf-viewer

Conversation

@arnabnandy7

@arnabnandy7 arnabnandy7 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Improves the existing inline file-view endpoint for images, video, audio, and PDFs.
  • Reuses the existing FileService.loadAsResource() flow.
  • Adds inline content disposition, MIME type, ETag, and last-modified headers.
  • Supports HTTP byte-range requests for media seeking and partial PDF loading.
  • Encodes filenames safely in UTF-8, including spaces and non-ASCII characters.
  • Documents viewer integration and existing file-action endpoints.
  • Adds controller coverage for real resource loading, traversal and symlink escapes, MIME fallback, full/open/suffix/unsatisfiable ranges, and encoded filenames.

Linked issue

Part of #98

Frontend authentication integration is tracked in Vault-Web/vault-web#281.

How to test

  1. Run the test suite:

    cd backend
    ./mvnw test
  2. Request a supported file:

    GET /api/files/view?path=video.mp4
    Authorization: Bearer <token>
  3. Verify the response includes the correct Content-Type, Content-Disposition: inline, ETag, and Last-Modified headers.

  4. Send full, open, suffix, and unsatisfiable byte-range requests. Verify valid ranges return 206 Partial Content with Content-Range, while an out-of-bounds range returns 416.

  5. Verify unknown MIME types return application/octet-stream.

  6. Verify traversal and symlink paths outside the authenticated user root are rejected.

Notes / Risk

  • No database migrations or feature flags are required.
  • Existing download, rename, move, delete, and folder-listing APIs are unchanged.
  • The frontend viewer shell and sidebar still need to be implemented in Vault Web.
  • Native media elements cannot attach an arbitrary bearer-token header. The authenticated same-origin route is tracked in Add authenticated same-origin proxy for workspace file previews vault-web#281.

Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>

@DenizAltunkapan DenizAltunkapan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The backend changes here are reasonable: reusing loadAsResource removes duplication and is stricter (it adds a readable check), the ETag and Last-Modified handling is clean, ContentDisposition.inline() is better than manual string building, and the range-response test is a nice end-to-end confirmation. One blocker and a few test gaps before this can go in.

Blocker: this PR sets Closes #98, but #98 is the full workspace file viewer (viewer shell, per-type renderers, actions sidebar, next/previous navigation, unsupported-type fallback UI, loading and error states). This PR is backend only, and the description itself says the frontend viewer shell still needs to be implemented. Merging it would auto-close #98 with almost all of its acceptance criteria still open. Please change Closes #98 to Part of #98 (or point it at a smaller backend-scoped issue) so the viewer work stays tracked.

Separately, the bearer-token point in your notes is worth a dedicated follow-up issue: native

The inline comments cover the test gaps.

}

@Test
void viewFile_nonExistentFile_returns404() throws Exception {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a test that /api/files/view rejects a path outside the authenticated user root (for example a ../../outside.pdf style path, and ideally a symlink pointing out of the root). #98 explicitly requires that files outside the user root are never reachable through the viewer, so this endpoint should have its own path-traversal test rather than relying on the validation being covered elsewhere. This is the one test I would consider close to blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in fc941ec. Added viewFile_pathTraversal_returns400, which runs the real FolderService.validatePath against ../../outside.pdf and verifies FileService.loadAsResource is never reached. Also added viewFile_symlinkOutsideUserRoot_returns400 for an existing symlink whose target is outside the user root; it verifies the same rejection behavior where the platform permits symlink creation. The full suite passes: 172 tests, 0 failures (the symlink test is capability-skipped on this Windows runner because symlink creation is not permitted). Backend CI is green.


Resource resource = new UrlResource(fullPath.toUri());
FileResource result = fileService.loadAsResource(fullPath);
String mimeType = Files.probeContentType(fullPath);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The octet-stream fallback for unknown MIME types has no test. Please add one that requests a file with an unknown extension and asserts Content-Type is application/octet-stream, so the documented download-fallback behavior is locked in. Note this only sets the header; the actual clean download fallback still depends on the (not-yet-built) frontend, which is another reason #98 should not be closed by this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in fc941ec. Added viewFile_unknownMimeType_returnsOctetStream, which creates a real file with an unknown extension, uses the real FileService.loadAsResource implementation, and asserts Content-Type: application/octet-stream. I also changed the PR linkage to Part of #98, so merging this backend work will not close the frontend viewer issue. The same-origin authentication integration is now tracked separately in Vault-Web/vault-web#281.

Path file = Files.writeString(tempDir.resolve("view.txt"), "viewme");
FileResource fileResource =
new FileResource(new UrlResource(file.toUri()), "\"6-123456\"", 123456L);
when(fileService.loadAsResource(any(Path.class))).thenReturn(fileResource);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test writes a real temp file but then mocks loadAsResource, so it does not exercise the real controller-to-FileService load path. Consider one integration-style test that lets the real loadAsResource run against a temp file, to confirm the two actually work together (headers, ETag, and body) rather than only the controller against a stubbed resource.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in fc941ec. viewFile_existingFile_returnsResourceWithContentType now invokes the real FileService.loadAsResource method against the temporary file instead of returning a stubbed FileResource. It verifies the response body, detected content type, inline content disposition, generated size-based ETag, and last-modified header together. The range and MIME fallback tests also use the real resource-loading path. Full suite: 172 tests, 0 failures; Backend CI is green.

when(fileService.loadAsResource(any(Path.class))).thenReturn(fileResource);

mockMvc
.perform(get("/api/files/view").param("path", "video.mp4").header("Range", "bytes=2-5"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The range test covers a single simple range only. For media and PDF seeking it would be worth adding an open range (bytes=2-), a suffix range (bytes=-4), a no-range full-file request, and an unsatisfiable range expecting 416. Not all blocking, but at least the 416 case is cheap and worth having. A filename with spaces or non-ASCII (for example a name like Urlaub Zuerich 2026.pdf) would also be a good Content-Disposition case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in fc941ec. Added coverage for a normal full-file response, bounded range, open range (bytes=6-), suffix range (bytes=-4), and an unsatisfiable range returning 416 with Content-Range: bytes */10. Added a filename case using Urlaub Zürich 2026.pdf; this exposed that the single-argument builder emitted raw non-ASCII text, so the controller now uses Spring's UTF-8 filename overload and the test verifies the RFC-compatible filename* value. Full suite: 172 tests, 0 failures; Backend CI is green.

Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
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