window.confirm() never shows a dialog and auto-accepts: destructive actions run without confirmation

Breadway commented

2026-10-13 19:41:55 +08:00

Owner

How it was found: clicking through the 98be903 build (main, v2 merged) in a headless BOS live VM on 2026-10-13.

What happened:

Users: with a second account added, I clicked Remove. A screenshot 0.4s later showed no dialog on the page, and hyprctl clients listed only the bos-settings window. 3s later getent passwd second was empty and its home directory was gone.
Storage: Empty trash removed ~/.local/share/Trash/files with no prompt.

Confidence: high for the behavior (seen twice, confirmed through the guest's system state). Medium for the cause: WebKitGTK/wry in Tauri 2 does not show native JS script dialogs, so confirm() resolves without asking the user.

Bug: the frontend guards every destructive action with plain confirm(), 14 calls in total. Among them: Users.svelte remove_user, Storage.svelte (empty trash, caches, unused deps), Snapshots.svelte delete, Keybinds.svelte remove layout, Bluetooth.svelte forget, Sharing.svelte enable SSH, ServiceControl.svelte stop critical service, Autostart.svelte core services, Updates.svelte update without snapshot, PowerMenu.svelte sleep/restart/power off, GetAppsHub.svelte AUR, Backup.svelte. None of them actually asks.

Fix: replace confirm() with an in-app confirm sheet (matching the v2 Sheet component), or @tauri-apps/plugin-dialog ask() (dialog:default is already in capabilities). Add a lint/grep check that bans bare confirm(/alert( in frontend/src.

**How it was found:** clicking through the 98be903 build (main, v2 merged) in a headless BOS live VM on 2026-10-13. **What happened:** - Users: with a second account added, I clicked Remove. A screenshot 0.4s later showed no dialog on the page, and `hyprctl clients` listed only the bos-settings window. 3s later `getent passwd second` was empty and its home directory was gone. - Storage: Empty trash removed ~/.local/share/Trash/files with no prompt. **Confidence:** high for the behavior (seen twice, confirmed through the guest's system state). Medium for the cause: WebKitGTK/wry in Tauri 2 does not show native JS script dialogs, so `confirm()` resolves without asking the user. **Bug:** the frontend guards every destructive action with plain `confirm()`, 14 calls in total. Among them: Users.svelte remove_user, Storage.svelte (empty trash, caches, unused deps), Snapshots.svelte delete, Keybinds.svelte remove layout, Bluetooth.svelte forget, Sharing.svelte enable SSH, ServiceControl.svelte stop critical service, Autostart.svelte core services, Updates.svelte update without snapshot, PowerMenu.svelte sleep/restart/power off, GetAppsHub.svelte AUR, Backup.svelte. None of them actually asks. **Fix:** replace `confirm()` with an in-app confirm sheet (matching the v2 Sheet component), or `@tauri-apps/plugin-dialog` `ask()` (dialog:default is already in capabilities). Add a lint/grep check that bans bare `confirm(`/`alert(` in frontend/src.

Breadway self-assigned this

2026-10-13 20:43:08 +08:00

Breadway commented

2026-10-13 21:00:10 +08:00

Author

Owner

PR: #76 (head edfa3b8)

PR: https://git.breadway.dev/Breadway/bos-settings/pulls/76 (head edfa3b8)

Breadway commented

2026-10-14 08:53:55 +08:00

Author

Owner

Review of PR #76 at exact head edfa3b8df1 (base main 14d1315): PASS. Not merged: this reviewer has no authenticated Forgejo write tools.

Criteria:

14 window.confirm sites replaced: PASS. origin/main has 14 bare confirm( in frontend/src (ServiceControl 1, Autostart 2, Backup 1, Bluetooth 1, GetAppsHub 1, Keybinds 1, PowerMenu 1, Sharing 1, Snapshots 1, Storage 2, Updates 1, Users 1). Head has 0 outside the comment in confirm.svelte.ts. Each now awaits confirmAction() and returns before the action when false.
Blocks until confirmed; cancel/Escape/backdrop never run the action: PASS. confirm.svelte.ts resolves true only via answer(true) from the confirm button. ConfirmHost wraps the existing Sheet, whose Escape and backdrop paths call onClose, mapped to answer(false). A superseded request resolves false.
Test bans bare confirm(/alert(/prompt(: PASS. confirm.test.mjs is wired into npm test. Mutation check: putting window.confirm back into Users.svelte made the test fail (exit 1, reports Users.svelte:74); restored afterwards.
No capability regressions: PASS. Autostart save still throws "System service changes were not saved." on cancel before the invoke and before originalSystem is updated. Updates still sets "Update cancelled." and marks step 0 failed. Users, Bluetooth, Snapshots, Keybinds, Sharing, Storage x2, GetApps, PowerMenu and Backup keep their action after the confirm. ServiceControl toggle is now async, with the same guard.
Live VM evidence (Users Remove sheet, Cancel keeps, confirm removes account and home): supplied by the coordinator and the PR body. I did not reproduce it.

Checks run by me on this SHA in a detached scratch worktree:

frontend: npm ci, npm run check (0 errors, 0 warnings), npm test (all pass, includes confirm), npm run build (ok).
src: cargo fmt --check ok, cargo clippy --locked --all-targets -- -D warnings ok, cargo test --locked 144 passed.
Forgejo CI: check / check (push) is success on this exact SHA (run 143, 8m8s). Two older pending statuses for the same context remain on the commit, but the combined status is success. PR is open and mergeable=true, with no reviews or comments on it.

Non-blocking note: Sheet's Escape listener uses stopPropagation, not stopImmediatePropagation, so pressing Escape on a confirm opened from inside another Sheet (Snapshots delete) also closes that underlying sheet. The action still does not run. A cosmetic follow-up is fine.

Next action: coordinator or an authorized user merges with a merge commit (expected head edfa3b8), then verifies on the integrated commit.

Review of PR #76 at exact head edfa3b8df14899c680d4b0c632fe0e68c3bd4e46 (base main 14d1315): PASS. Not merged: this reviewer has no authenticated Forgejo write tools. Criteria: - 14 window.confirm sites replaced: PASS. origin/main has 14 bare confirm( in frontend/src (ServiceControl 1, Autostart 2, Backup 1, Bluetooth 1, GetAppsHub 1, Keybinds 1, PowerMenu 1, Sharing 1, Snapshots 1, Storage 2, Updates 1, Users 1). Head has 0 outside the comment in confirm.svelte.ts. Each now awaits confirmAction() and returns before the action when false. - Blocks until confirmed; cancel/Escape/backdrop never run the action: PASS. confirm.svelte.ts resolves true only via answer(true) from the confirm button. ConfirmHost wraps the existing Sheet, whose Escape and backdrop paths call onClose, mapped to answer(false). A superseded request resolves false. - Test bans bare confirm(/alert(/prompt(: PASS. confirm.test.mjs is wired into npm test. Mutation check: putting window.confirm back into Users.svelte made the test fail (exit 1, reports Users.svelte:74); restored afterwards. - No capability regressions: PASS. Autostart save still throws "System service changes were not saved." on cancel before the invoke and before originalSystem is updated. Updates still sets "Update cancelled." and marks step 0 failed. Users, Bluetooth, Snapshots, Keybinds, Sharing, Storage x2, GetApps, PowerMenu and Backup keep their action after the confirm. ServiceControl toggle is now async, with the same guard. - Live VM evidence (Users Remove sheet, Cancel keeps, confirm removes account and home): supplied by the coordinator and the PR body. I did not reproduce it. Checks run by me on this SHA in a detached scratch worktree: - frontend: npm ci, npm run check (0 errors, 0 warnings), npm test (all pass, includes confirm), npm run build (ok). - src: cargo fmt --check ok, cargo clippy --locked --all-targets -- -D warnings ok, cargo test --locked 144 passed. - Forgejo CI: check / check (push) is success on this exact SHA (run 143, 8m8s). Two older pending statuses for the same context remain on the commit, but the combined status is success. PR is open and mergeable=true, with no reviews or comments on it. Non-blocking note: Sheet's Escape listener uses stopPropagation, not stopImmediatePropagation, so pressing Escape on a confirm opened from inside another Sheet (Snapshots delete) also closes that underlying sheet. The action still does not run. A cosmetic follow-up is fine. Next action: coordinator or an authorized user merges with a merge commit (expected head edfa3b8), then verifies on the integrated commit.

Breadway closed this issue

2026-10-14 12:21:39 +08:00

Breadway commented

2026-10-14 12:30:30 +08:00

Author

Owner

Re-reviewed at bfa068d after the base merge (PR #76 comment 4725) and merged as d432319. Verified on d432319: svelte-check 0/0, npm test including the confirm ban test (no bare confirm/alert/prompt in frontend/src, Memory/Display/Bluetooth included), build, fmt, clippy -D warnings, cargo test 179 passed. Live VM evidence from the original head: Remove shows the in-app sheet with the account intact, Cancel keeps it, confirming removes account and home.

Re-reviewed at bfa068d after the base merge (PR #76 comment 4725) and merged as d432319. Verified on d432319: svelte-check 0/0, npm test including the confirm ban test (no bare confirm/alert/prompt in frontend/src, Memory/Display/Bluetooth included), build, fmt, clippy -D warnings, cargo test 179 passed. Live VM evidence from the original head: Remove shows the in-app sheet with the account intact, Cancel keeps it, confirming removes account and home.

Rows
Columns

window.confirm() never shows a dialog and auto-accepts: destructive actions run without confirmation #70