Breadway/bos-settings

Fork 0

Canonicalize wallpaper preview path before the asset scope #88

Merged

Breadway merged 2 commits from fix/bset-85-wallpaper-canonicalize into main

2026-10-20 17:35:50 +08:00

Breadway commented

2026-10-20 15:31:43 +08:00

Owner

How it was found

BSET-85 (medium confidence, found in review of PR #80 / BSET-70). Confirmed from source: breadpaper.rs allow_preview passed the raw current-wallpaper path to asset_protocol_scope().allow_file() and checked the extension and existence on that raw path. A symlink named *.png could point the scope at a non-image target, because the checks never saw the file the scope actually grants. allow_preview had no unit test.

What changed

preview_target(path) -> Option<PathBuf>: pure function. Rejects relative paths, runs std::fs::canonicalize, then requires the resolved path to have an image extension and to be a regular file. Returns the resolved path.
allow_preview now calls preview_target and grants the scope only the resolved file. Behaviour is otherwise unchanged.
Six unit tests in commands::breadpaper::tests, using pid-scoped tempdirs cleaned up on drop: normal image allowed; missing file refused; symlink named .png to a non-image refused; symlink named .png to a real image resolves to the target; relative path refused; directory named like an image refused.

Tests and gates (run locally)

cd src && cargo test --lib: 200 passed, 0 failed (includes the 6 new breadpaper tests, all passing).
cargo test --lib breadpaper: 6 passed, 0 failed.
cargo clippy --lib --tests -- -D warnings: exit 0, no warnings.
rustfmt --check src/commands/breadpaper.rs: exit 0 (this file is clean).
Not run: cargo fmt --check for the whole crate. It fails on src/commands/power_settings.rs, which already fails on origin/main and is not touched here. Recorded as a finding, not fixed in this PR.
Build note: frontend/build was missing. Created a real empty directory at /tmp/opencode/bset85-frontend-build and symlinked frontend/build to it, per the card instruction. The path is gitignored (frontend/.gitignore: /build) and is not in the commit.
The Tauri scope itself was not exercised with a running app. The tests cover the decision logic; the allow_file call is a one-line wrapper and was not run against a live webview.

Refs BSET-85

## How it was found BSET-85 (medium confidence, found in review of PR #80 / BSET-70). Confirmed from source: `breadpaper.rs` `allow_preview` passed the raw current-wallpaper path to `asset_protocol_scope().allow_file()` and checked the extension and existence on that raw path. A symlink named `*.png` could point the scope at a non-image target, because the checks never saw the file the scope actually grants. `allow_preview` had no unit test. ## What changed - `preview_target(path) -> Option<PathBuf>`: pure function. Rejects relative paths, runs `std::fs::canonicalize`, then requires the resolved path to have an image extension and to be a regular file. Returns the resolved path. - `allow_preview` now calls `preview_target` and grants the scope only the resolved file. Behaviour is otherwise unchanged. - Six unit tests in `commands::breadpaper::tests`, using pid-scoped tempdirs cleaned up on drop: normal image allowed; missing file refused; symlink named `.png` to a non-image refused; symlink named `.png` to a real image resolves to the target; relative path refused; directory named like an image refused. ## Tests and gates (run locally) - `cd src && cargo test --lib`: **200 passed, 0 failed** (includes the 6 new breadpaper tests, all passing). - `cargo test --lib breadpaper`: 6 passed, 0 failed. - `cargo clippy --lib --tests -- -D warnings`: exit 0, no warnings. - `rustfmt --check src/commands/breadpaper.rs`: exit 0 (this file is clean). - Not run: `cargo fmt --check` for the whole crate. It fails on `src/commands/power_settings.rs`, which already fails on origin/main and is not touched here. Recorded as a finding, not fixed in this PR. - Build note: `frontend/build` was missing. Created a real empty directory at `/tmp/opencode/bset85-frontend-build` and symlinked `frontend/build` to it, per the card instruction. The path is gitignored (`frontend/.gitignore`: `/build`) and is not in the commit. - The Tauri scope itself was not exercised with a running app. The tests cover the decision logic; the `allow_file` call is a one-line wrapper and was not run against a live webview. Refs BSET-85

Breadway added 1 commit

2026-10-20 15:31:43 +08:00

Canonicalize wallpaper preview path before the asset scope

check / check (push) Failing after 14s

Details

7a712a9a2f

allow_preview passed the raw current-wallpaper path to the asset protocol
scope and checked its extension there. A symlink named *.png could point
the scope at a non-image target, since the checks never saw the resolved
file.

Resolve the path with canonicalize first and run every check (absolute,
image extension, regular file) on the resolved path. The scope is granted
that resolved file.

Add unit tests with tempdirs: a normal image is allowed, a missing file
is refused, a symlink named *.png to a non-image is refused, a symlink to
a real image resolves to its target, a relative path and a directory are
refused.

Refs BSET-85

Breadway referenced this pull request

2026-10-20 15:31:44 +08:00

Wallpaper preview: canonicalize the path before allowing it in the asset scope #86

Breadway commented

2026-10-20 16:07:27 +08:00

Author

Owner

Review of PR #88 (BSET-85), exact head 7a712a9a2f (re-read from Forgejo before review: matches). Base on the PR is fe03b457 but merge-base is 2ba912b2.

VERDICT: BLOCKED (code is acceptable, no code changes requested; the required CI check is red on this head)

Required check on this exact SHA: "check / check (push)" = failure ("Failing after 14s", run 187). It is the only status on the SHA. Not waived.

Cause of the red check (verified independently): on the head, cargo fmt --all -- --check reports exactly 3 diffs, all in src/src/commands/power_settings.rs (lines 415, 422, 431). The PR touches only src/src/commands/breadpaper.rs, so those come from the stale base 2ba912b2, fixed on main by #89 (fe03b457). I did not read the CI job log itself; the fmt reproduction is the evidence.

Merge of head into current main (fe03b457), trial merge in a scratch clone, clean, no conflicts, diff vs main still only breadpaper.rs (+108/-3):

cargo fmt --all -- --check: exit 0
cargo clippy --lib --tests -- -D warnings: exit 0
cargo test --lib: 200 passed, 0 failed (includes the 6 new breadpaper tests)
Note: CI runs cargo clippy --all-targets and cargo test (not --lib); I ran the commands you specified, not the full CI set.

Code review (src/src/commands/breadpaper.rs):

Relative path: refused before any filesystem call. OK.
canonicalize failure (missing file, dangling symlink, loop, permission): None. OK.
Symlink to non-image: extension and is_file checked on the RESOLVED path, so refused. OK (test symlink_to_a_non_image_is_refused).
Symlink to image: resolved target returned and granted, not the link name. OK. This is also functionally better: tauri 2.11.5 is_allowed resolves symlinks on the requested path before matching, so granting the link path would not have matched anyway.
Directory named *.png: resolved.is_file() false, refused. OK.
Case-insensitive extension: is_wallpaper_file uses eq_ignore_ascii_case, applied to the resolved path. OK. Not covered by a test (see nit).
Scope grant: allow_file uses escaped_pattern, so glob metacharacters in the filename are not expanded; only the single resolved file is granted.
TOCTOU: yes, there is a window between canonicalize/is_file and the allow_file call (and the webview read later), during which the file at the resolved path could be swapped. Severity: low. The grant is to the fully resolved path, not through a symlink, so swapping an intermediate symlink afterwards does not redirect the grant; an attacker would need write access to the resolved file's location, i.e. already the same user or a directory the user trusts. The webview read also re-resolves symlinks in is_allowed. Not worth blocking; it cannot be eliminated by path-based checks anyway.
Callers: allow_preview is used by get_current_wallpaper and the outputs listing; signature unchanged, behaviour otherwise unchanged. Scope is additive and never revoked (pre-existing, unchanged).

Tests: 6 tests exercise normal, missing, symlink to non-image, symlink to image, relative, directory-named-image. Assertions check the returned value (including that the symlink case returns the target path), not just is_some. Scratch dirs are pid-scoped under temp_dir, removed up front and on Drop (RAII, runs on assertion panic too); I confirmed no bos-settings-breadpaper-test-* directories remain in /tmp after the run. Unix-only symlink usage is fine for this Linux-only app. The PR body's claim that the tests pass and that the Tauri scope itself was not run against a live webview is accurate; I also did not run a live webview.

Nits, non-blocking: no test for an uppercase extension (e.g. WALL.PNG) or a dangling symlink; adding them would pin the case-insensitive and canonicalize-failure paths.

Scope and conventions: diff is one file, 108 additions, 3 deletions, matching the PR body. No CHANGELOG file exists in the repo and neither CONTRIBUTING.md nor AGENTS.md mandates one, so none is required. No em or en dashes in the diff. Branch name follows fix/x.

Required action: bring the branch up to date with main (merge or rebase onto fe03b457) so the new head passes the fmt gate, wait for check to go green on that new SHA, then request a fresh review of the new head (the SHA will change; the code diff should be identical). No code changes needed from this review.

Review of PR #88 (BSET-85), exact head 7a712a9a2fcce2157d33c89b7b9cdf7f764fc02f (re-read from Forgejo before review: matches). Base on the PR is fe03b457 but merge-base is 2ba912b2. VERDICT: BLOCKED (code is acceptable, no code changes requested; the required CI check is red on this head) Required check on this exact SHA: "check / check (push)" = failure ("Failing after 14s", run 187). It is the only status on the SHA. Not waived. Cause of the red check (verified independently): on the head, `cargo fmt --all -- --check` reports exactly 3 diffs, all in src/src/commands/power_settings.rs (lines 415, 422, 431). The PR touches only src/src/commands/breadpaper.rs, so those come from the stale base 2ba912b2, fixed on main by #89 (fe03b457). I did not read the CI job log itself; the fmt reproduction is the evidence. Merge of head into current main (fe03b457), trial merge in a scratch clone, clean, no conflicts, diff vs main still only breadpaper.rs (+108/-3): - cargo fmt --all -- --check: exit 0 - cargo clippy --lib --tests -- -D warnings: exit 0 - cargo test --lib: 200 passed, 0 failed (includes the 6 new breadpaper tests) Note: CI runs `cargo clippy --all-targets` and `cargo test` (not --lib); I ran the commands you specified, not the full CI set. Code review (src/src/commands/breadpaper.rs): - Relative path: refused before any filesystem call. OK. - canonicalize failure (missing file, dangling symlink, loop, permission): None. OK. - Symlink to non-image: extension and is_file checked on the RESOLVED path, so refused. OK (test symlink_to_a_non_image_is_refused). - Symlink to image: resolved target returned and granted, not the link name. OK. This is also functionally better: tauri 2.11.5 is_allowed resolves symlinks on the requested path before matching, so granting the link path would not have matched anyway. - Directory named *.png: resolved.is_file() false, refused. OK. - Case-insensitive extension: is_wallpaper_file uses eq_ignore_ascii_case, applied to the resolved path. OK. Not covered by a test (see nit). - Scope grant: allow_file uses escaped_pattern, so glob metacharacters in the filename are not expanded; only the single resolved file is granted. - TOCTOU: yes, there is a window between canonicalize/is_file and the allow_file call (and the webview read later), during which the file at the resolved path could be swapped. Severity: low. The grant is to the fully resolved path, not through a symlink, so swapping an intermediate symlink afterwards does not redirect the grant; an attacker would need write access to the resolved file's location, i.e. already the same user or a directory the user trusts. The webview read also re-resolves symlinks in is_allowed. Not worth blocking; it cannot be eliminated by path-based checks anyway. - Callers: allow_preview is used by get_current_wallpaper and the outputs listing; signature unchanged, behaviour otherwise unchanged. Scope is additive and never revoked (pre-existing, unchanged). Tests: 6 tests exercise normal, missing, symlink to non-image, symlink to image, relative, directory-named-image. Assertions check the returned value (including that the symlink case returns the target path), not just is_some. Scratch dirs are pid-scoped under temp_dir, removed up front and on Drop (RAII, runs on assertion panic too); I confirmed no bos-settings-breadpaper-test-* directories remain in /tmp after the run. Unix-only symlink usage is fine for this Linux-only app. The PR body's claim that the tests pass and that the Tauri scope itself was not run against a live webview is accurate; I also did not run a live webview. Nits, non-blocking: no test for an uppercase extension (e.g. WALL.PNG) or a dangling symlink; adding them would pin the case-insensitive and canonicalize-failure paths. Scope and conventions: diff is one file, 108 additions, 3 deletions, matching the PR body. No CHANGELOG file exists in the repo and neither CONTRIBUTING.md nor AGENTS.md mandates one, so none is required. No em or en dashes in the diff. Branch name follows fix/x. Required action: bring the branch up to date with main (merge or rebase onto fe03b457) so the new head passes the fmt gate, wait for check to go green on that new SHA, then request a fresh review of the new head (the SHA will change; the code diff should be identical). No code changes needed from this review.

Breadway added 1 commit

2026-10-20 16:08:09 +08:00

Merge remote-tracking branch 'origin/main' into fix/bset-85-wallpaper-canonicalize

check / check (push) Successful in 5m45s

Details

bd0e090a81

Breadway commented

2026-10-20 17:35:12 +08:00

Author

Owner

Review of PR #88 (BSET-85), exact head bd0e090a81 (re-read from Forgejo before review and again before posting: matches). Base main fe03b457, merge-base fe03b457.

VERDICT: PASS (head bd0e090a81)

Independent verification, not relying on the earlier review (comment 5668, old head 7a712a9a):

Scope and integration

git diff origin/main..bd0e090a is exactly one file: src/src/commands/breadpaper.rs, +108/-3.
bd0e090a's tree is 6bab71ea37, identical to the tree of a trial merge of old head 7a712a9a into main fe03b457. The only difference from 7a712a9a is the power_settings.rs rustfmt change that came in from main (#89). No force-push, no extra changes.

Checks, in a scratch worktree with an empty frontend/build stub (gitignored), via the cargo wrapper, same commands as .forgejo/workflows/check.yml

cd src/src && cargo fmt --all -- --check: exit 0
cd src && cargo clippy --all-targets -- -D warnings: exit 0, no warnings
cd src && cargo test: exit 0, 200 passed, 0 failed (includes the 6 new breadpaper tests)
CI: check / check (push) on bd0e090a is success (Successful in 5m45s, run 192). It is the only status and the only required check I could observe. No outstanding reviews or change requests on the PR (reviews API empty; the only comment is 5668).

preview_target / allow_preview judgement

Relative path: rejected up front (is_absolute). Tested.
Canonicalize: std::fs::canonicalize runs before any other check; a missing file or dangling symlink returns None via ok()?.
Extension and regular-file checks run on the resolved path, so a *.png symlink to a non-image is refused (tested), and a non-image-named link to a real image resolves to the image target.
Only the resolved file is granted: allow_file receives the resolved PathBuf, not the raw input. Tested for the symlink-to-image case.
Directory named like an image: refused by is_file on the resolved path (tested).
Uppercase extension: is_wallpaper_file uses eq_ignore_ascii_case, so .PNG is allowed.
Extra probes I ran in a temporary test (not committed, reverted, tempdir removed): WALL.PNG allowed; dangling png symlink refused; png -> png -> txt symlink chain refused; extensionless symlink to a png resolves to the png target only; path with sub/../ resolves to the canonical target; a FIFO named .png refused (is_file false). All behaved as intended.
Behaviour otherwise unchanged: callers (get_current_wallpaper, get_wallpapers_by_output) still return the original path string to the frontend; only the scope grant changes.
TOCTOU: there is a window between canonicalize/is_file and allow_file in which the resolved path could be swapped (for example a regular file replaced by a symlink). Severity: low. The scope is a path grant, not an opened handle, and the swap requires write access to the wallpaper path as the same user, who can already read those files. The PR removes the practical bug (checks on a different path than the one granted), and the residual race cannot be closed with the allow_file API. Non-blocking, no change requested.
Tests: they assert returned values (Some(canonical path) / None) rather than mirroring the implementation, use pid-scoped tempdirs removed on Drop, and I confirmed no leftover dirs in /tmp after the run. The Tauri allow_file call itself is a one-line wrapper and is not exercised by a live webview; that is acceptable.

Nits from the earlier review

No uppercase-extension test and no dangling-symlink test: I agree these are non-blocking. Both cases are covered by my probe above and behave correctly. A follow-up test is optional.

Not merged, card not moved. Awaiting merge decision by the coordinator.

Review of PR #88 (BSET-85), exact head bd0e090a81a8ce1cf2d5f7e8a35d2d8da81191da (re-read from Forgejo before review and again before posting: matches). Base main fe03b457, merge-base fe03b457. VERDICT: PASS (head bd0e090a81a8ce1cf2d5f7e8a35d2d8da81191da) Independent verification, not relying on the earlier review (comment 5668, old head 7a712a9a): 1. Scope and integration - git diff origin/main..bd0e090a is exactly one file: src/src/commands/breadpaper.rs, +108/-3. - bd0e090a's tree is 6bab71ea37fdad9baf96d180a87b86b4da5f2c4e, identical to the tree of a trial merge of old head 7a712a9a into main fe03b457. The only difference from 7a712a9a is the power_settings.rs rustfmt change that came in from main (#89). No force-push, no extra changes. 2. Checks, in a scratch worktree with an empty frontend/build stub (gitignored), via the cargo wrapper, same commands as .forgejo/workflows/check.yml - cd src/src && cargo fmt --all -- --check: exit 0 - cd src && cargo clippy --all-targets -- -D warnings: exit 0, no warnings - cd src && cargo test: exit 0, 200 passed, 0 failed (includes the 6 new breadpaper tests) - CI: check / check (push) on bd0e090a is success (Successful in 5m45s, run 192). It is the only status and the only required check I could observe. No outstanding reviews or change requests on the PR (reviews API empty; the only comment is 5668). 3. preview_target / allow_preview judgement - Relative path: rejected up front (is_absolute). Tested. - Canonicalize: std::fs::canonicalize runs before any other check; a missing file or dangling symlink returns None via ok()?. - Extension and regular-file checks run on the resolved path, so a *.png symlink to a non-image is refused (tested), and a non-image-named link to a real image resolves to the image target. - Only the resolved file is granted: allow_file receives the resolved PathBuf, not the raw input. Tested for the symlink-to-image case. - Directory named like an image: refused by is_file on the resolved path (tested). - Uppercase extension: is_wallpaper_file uses eq_ignore_ascii_case, so .PNG is allowed. - Extra probes I ran in a temporary test (not committed, reverted, tempdir removed): WALL.PNG allowed; dangling png symlink refused; png -> png -> txt symlink chain refused; extensionless symlink to a png resolves to the png target only; path with sub/../ resolves to the canonical target; a FIFO named .png refused (is_file false). All behaved as intended. - Behaviour otherwise unchanged: callers (get_current_wallpaper, get_wallpapers_by_output) still return the original path string to the frontend; only the scope grant changes. - TOCTOU: there is a window between canonicalize/is_file and allow_file in which the resolved path could be swapped (for example a regular file replaced by a symlink). Severity: low. The scope is a path grant, not an opened handle, and the swap requires write access to the wallpaper path as the same user, who can already read those files. The PR removes the practical bug (checks on a different path than the one granted), and the residual race cannot be closed with the allow_file API. Non-blocking, no change requested. - Tests: they assert returned values (Some(canonical path) / None) rather than mirroring the implementation, use pid-scoped tempdirs removed on Drop, and I confirmed no leftover dirs in /tmp after the run. The Tauri allow_file call itself is a one-line wrapper and is not exercised by a live webview; that is acceptable. 4. Nits from the earlier review - No uppercase-extension test and no dangling-symlink test: I agree these are non-blocking. Both cases are covered by my probe above and behave correctly. A follow-up test is optional. Not merged, card not moved. Awaiting merge decision by the coordinator.

Breadway merged commit 31f7dab5a8 into main

2026-10-20 17:35:50 +08:00

Breadway referenced this pull request from a commit

2026-10-20 17:35:52 +08:00

Merge pull request 'Canonicalize wallpaper preview path before the asset scope' (#88) from fix/bset-85-wallpaper-canonicalize into main