feat(web): add file extension validation to file browser modal - #5524
Conversation
|
|
Is this only for the cover browse button or all? Because all except the cover image one need to accept any extension. |
The validation is strictly applied only to the cover image button. Here is how the logic works:
So all other fields will continue to accept any extension perfectly fine, exactly as they did before. |
# Conflicts: # src_assets/common/assets/web/Apps.vue
There was a problem hiding this comment.
P2 — Add tests for the new cover validation
fileBrowserConfirm() and save() now have new validation branches, but this PR changes no tests. The repository requires tests for modified methods. Please cover rejection of a non-PNG cover, acceptance of a PNG with a mixed-case extension, unrestricted selection for the other browse fields, and rejection of a manually typed non-PNG path before apiFetch().
Here is an illustrative starting point for tests/web/Apps.test.js (I have not run this sketch against the PR checkout):
diff --git a/tests/web/Apps.test.js b/tests/web/Apps.test.js
new file mode 100644
--- /dev/null
+++ b/tests/web/Apps.test.js
@@ -0,0 +1,59 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+vi.mock('../../src_assets/common/assets/web/fetch_utils', () => ({
+ apiFetch: vi.fn(),
+}))
+
+import { apiFetch } from '../../src_assets/common/assets/web/fetch_utils'
+import Apps from '../../src_assets/common/assets/web/Apps.vue'
+
+beforeEach(() => vi.clearAllMocks())
+
+function browserContext(path, acceptedExtensions) {
+ return {
+ fileBrowserSelectedPath: path,
+ fileBrowserTypedPath: '',
+ fileBrowserAcceptedExtensions: acceptedExtensions,
+ fileBrowserType: 'file',
+ fileBrowserError: '',
+ fileBrowserCallback: vi.fn(),
+ fileBrowserClose: vi.fn(),
+ $t: (key) => key,
+ }
+}
+
+describe('application cover validation', () => {
+ it('keeps the browser open for a non-PNG cover', () => {
+ const ctx = browserContext('/covers/cover.jpg', ['.png'])
+ const onSelect = ctx.fileBrowserCallback
+ Apps.methods.fileBrowserConfirm.call(ctx)
+ expect(ctx.fileBrowserError).toBe('file_browser.error_invalid_extension')
+ expect(onSelect).not.toHaveBeenCalled()
+ expect(ctx.fileBrowserClose).not.toHaveBeenCalled()
+ })
+
+ it.each([
+ ['/covers/cover.PNG', ['.png']],
+ ['/output/log.txt', null],
+ ])('accepts %s with extensions %j', (path, extensions) => {
+ const ctx = browserContext(path, extensions)
+ const onSelect = ctx.fileBrowserCallback
+ Apps.methods.fileBrowserConfirm.call(ctx)
+ expect(onSelect).toHaveBeenCalledWith(path)
+ expect(ctx.fileBrowserClose).toHaveBeenCalledOnce()
+ })
+
+ it('blocks a manually typed non-PNG cover before saving', () => {
+ const modalBody = { scrollTop: 100 }
+ const ctx = {
+ editForm: { 'image-path': '/covers/cover.jpg' },
+ editFormError: '',
+ $refs: { editModal: { querySelector: () => modalBody } },
+ $t: (key) => key,
+ }
+ Apps.methods.save.call(ctx)
+ expect(ctx.editFormError).toBe('file_browser.error_invalid_extension')
+ expect(modalBody.scrollTop).toBe(0)
+ expect(apiFetch).not.toHaveBeenCalled()
+ })
+})Please also test that an empty image path and a valid .PNG path still submit the app. That covers the successful save() branch.
eduardomozart
left a comment
eduardomozart
left a comment
There was a problem hiding this comment.
Added tests/web/Apps.test.js covering the new validation branches in fileBrowserConfirm() and save():
fileBrowserConfirm: rejects non-PNG whenacceptedExtensionsis['.png'], accepts mixed-case.PNG, allows any extension whenacceptedExtensionsisnull(output/cmd fields), skips the check entirely fortype: 'directory', handles empty path and typed-path fallbacksave: blocks.jpgand.bmpbeforeapiFetch()is called, scrolls modal to top on error, accepts mixed-case.PnG, submits when image path is empty, verifies double-quote stripping happens before validation
a286800 to
a911fb4
Compare
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
P2 — Make the new Apps.test.js suite load in Vitest
The requested cases are now present, but the new suite fails before collecting any tests. At head a286800, a targeted Vitest run reports Failed to resolve import "/images/logo-sunshine-45.png" from "src_assets/common/assets/web/Navbar.vue" and 0 test. Importing Apps.vue in tests/web/Apps.test.js also imports Navbar.vue, whose root-absolute image URL is not resolved by the test setup. Please stub Navbar before importing Apps, as tests/web/Home.test.js already does:
vi.mock('../../src_assets/common/assets/web/Navbar.vue', () => ({
default: { template: '<div />' },
}))With only that temporary stub added locally, all 11 new tests passed. I removed the stub after verifying it, so the worktree again matches the PR head. The successful-save tests also print jsdom navigation warnings because the apiFetch mock always returns status 200; that noise can be avoided by using a non-200 response for those cases or stubbing the reload.
Workflow status: GitHub reports the PR mergeable against master. The Sunshine docs build passed. The separate Read the Docs pages check timed out waiting for check runs before its job ran, so it does not indicate a code failure; other hosted test results remain unavailable for this head.
Superseded by the amended head a911fb4, which includes the Navbar test stub; all 54 web tests pass locally.
|
Can you rebase onto master? I'm not able to do it through the GitHub ui. |
Done! |
Bundle ReportChanges will increase total bundle size by 937 bytes (0.03%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sunshine-esmAssets Changed:
Files in
Files in
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5524 +/- ##
==========================================
- Coverage 39.98% 39.69% -0.29%
==========================================
Files 122 123 +1
Lines 28361 28825 +464
Branches 12356 12533 +177
==========================================
+ Hits 11339 11443 +104
- Misses 15121 16302 +1181
+ Partials 1901 1080 -821
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 32 files with indirect coverage changes Continue to review full report in Codecov by Harness.
|
Screenshot ComparisonPR #5524 screenshots vs Matrix:
|
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: AppImage-x86_64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: Windows-AMD64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: Windows-ARM64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: macOS-arm64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: macOS-x86_64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
2 participants
Description
This PR adds frontend file extension validation to the shared "Select File" file browser modal in the Sunshine Web UI.
Previously, the file browser would accept any selected file (e.g.
.md) without providing any feedback, even when opening the modal to select an application cover image.This change ensures the file extension is validated directly in the modal right when the user clicks "Select", aligning the frontend behavior with the existing backend enforcement in
validate_app_image_path(inprocess.cpp).Screenshot
Updated
fileBrowserConfirmto validate the selected file against the accepted extensions and display an error message inside the modal if it fails.Gravacao.de.Tela.2026-08-18.as.16.13.48.mov
Updated the
savemethod to validate manually typed image paths, displaying the new error banner and preventing the API request if the extension is invalid.Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage
See our AI usage policy.