Repository navigation
Port xbmc4lyfe batch 3 (issues #97–#150) - #168
Merged
Merged
Conversation
A job at post-processing level 0 (download only) or 1 (repair only) whose payload is archives ended Failed with "No usable output produced; only archive or PAR2 artifacts remain" and nothing was delivered. That rule assumes the job was meant to be unpacked, which only holds for levels 2 and 3. Follow SABnzbd's semantics: - pp=0: no verify, repair or unpack. The job completes and the raw files are moved to the complete dir, even when some articles were missing (a skipped Verify stage records how many). Hopeless jobs are still failed earlier by the download path. - pp=1: PAR2 verify/repair only, via the new nzb_postproc::run_repair_pipeline, which skips extraction and cleanup. Direct unpack is not started for levels below 2. - The usable-output check runs only for levels 2 and 3. History now records the effective level (job pp override, else the category, else 3) in a new history.post_processing column (migration v14). SAB history slots report it through sab_pp_label, SABnzbd's PP_LOOKUP (0 -> "", 1 -> "R", 2 -> "U", 3 -> "D"), instead of a fixed "D"; rows written before v14 keep "D". Post-processing slots use the live level. Fixes #97 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit a4e9a1d)
POST /api/queue/sort always sorted by remaining percentage and silently ignored any other request: its body had only an `ascending` flag. SAB mode=queue&name=sort had the same limitation and ignored SABnzbd's `sort` and `dir` parameters. - QueueManager::sort_queue(QueueSortField, ascending) sorts by remaining, name (case-insensitive), size, age or priority. sort_by_remaining_percentage now delegates to it. Like the existing sort, it is stable and reorders job_order only, so job status is untouched and active downloads keep running. - The native body accepts optional `sort_by` (default `remaining`) and `direction` (`asc`/`desc`), which takes precedence over the legacy `ascending` flag. An unknown value is a 400 and leaves the queue as it was. - SAB name=sort reads `sort` (avg_age, name, size/bytes, remaining) and `dir`, as SABnzbd's _api_queue_sort does; only `dir=desc` reverses. An unknown field returns status false. Without `sort`, the earlier form (remaining, direction in `value`) still works. Age ascending is youngest first, matching SABnzbd's avg_age. RustNZB has no average post date, so it uses the job's added time. Fixes #99 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 1951094)
POST /api/queue/bulk with action "delete" reported an id that matched
no job as succeeded: remove_job is idempotent and returns Ok for an
unknown id, and the handler counted every Ok as a success. Deleting
[J1, J2, <unknown>] returned succeeded 3, failed 0.
The handler now checks the id with the new QueueManager::contains_job
before deleting, and records a JobNotFound failure for an unknown id.
pause, resume, priority and category already return JobNotFound.
remove_job and the single-job DELETE stay idempotent.
The response gains a `failures` array: one {id, error} entry per id
that failed, in request order. `error` has the same shape as an API
error body (error_kind, human_readable, status). `failed` is its
length.
Fixes #101
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 5e6b59f)
Failed form submits and other mutating requests threw away the response body. For example, adding an RSS feed with a private-address URL returns 400 with "Feed URL rejected: ..." in `human_readable`, but the UI showed at most a generic toast and the add-feed form stayed open with no explanation. Some handlers read `error.message`, which the central API error mapping never sends. Others had no error handler, so history retry/delete/clear, queue bulk actions, the priority change, group mark-all-read and pause-for failed silently. This adds a shared core/http/http-error helper. It takes `human_readable`, falls back to the legacy `error` field, then `message`, then short plain-text bodies, and prefixes the result with what the user was trying to do. Every mutation in settings, RSS, queue, history, groups and the app shell now uses it. Login and the setup wizard use it for their inline messages. The RSS feed and rule forms also show the error inline while the form stays open. Bulk queue actions now report failures once instead of dropping them. 401s are not toasted, because the auth interceptor already refreshes the session or redirects to login. Fixes #104 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 6a2b0c1)
(cherry picked from commit 333b99f) Adapted: kept both our QueueSortField re-export and the new HistoryRetryOutcome, and added the post_processing field our HistoryEntry requires to the new test fixture.
The queue manager already treats 0 as no limit on simultaneous downloads, but neither the config field doc nor config.example.toml said so. Documentation only; no behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit b9d9024)
…nterval RssMonitor::run checked every enabled feed, then slept for the smallest enabled poll interval, or 900 s when there were no feeds. Nothing interrupted that sleep. A feed added after a feed-less boot therefore waited up to 15 minutes for its first check, whatever its own interval. A shortened interval also waited out the old sleep. AppState now owns a Notify (rss_monitor_wake). commit_config signals it after every committed config change, which covers every handler that goes through update_config / update_config_with. Startup builds the state first and hands the signal to the monitor via RssMonitor::with_wake. The loop now tracks when each feed was last checked (FeedSchedule, keyed by name and URL). It checks only the feeds that are due, then sleeps until the earliest due time, the next 900 s maintenance pass, or a config-change wake, whichever comes first. The due time is computed from the feed's current poll_interval_secs on every pass, so an interval edit applies immediately. A new, renamed, re-enabled or re-pointed feed is due at once. A deleted or disabled feed is dropped. Each feed now runs on its own interval instead of the shortest one. Item pruning and expiry still run at least every 900 s. RSS rules live in the database and are re-read on every feed check. They do not affect when a feed is due, so rule edits do not wake the monitor. The config is held as a full Arc, not an arc-swap guard, across the feed-check awaits. No lock is held across an await. Adds tokio test-util as a dev-dependency feature. Bumps nzb-web to 0.4.24 for the additive public API (AppState::rss_monitor_wake, RssMonitor::with_wake). Fixes #107 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 3884543)
Nothing validated poll_interval_secs. A feed saved with 0, through the API, the UI or a hand-edited config.toml, was due again as soon as its check finished. The RSS monitor then polled the indexer in a tight loop, burning CPU and risking a rate-limit ban. - The feed add and update handlers now return 400 for poll_interval_secs below MIN_POLL_INTERVAL_SECS (60), before the URL check and before anything is written. - The monitor clamps lower values that are already in a config to the floor when it computes a feed's due time. FeedSchedule::sync logs a warning once per feed and warns again only if the interval is fixed and later broken again. - The RssFeedConfig doc comment records the floor. config.example.toml does not document feeds, so it is unchanged. Two tests from the parent change used sub-floor intervals (15 s and 30 s) to exercise interval edits. They now use 120 s and 60 s; what they assert is unchanged. MIN_POLL_INTERVAL_SECS is a new public const in nzb-web. It is covered by the unreleased 0.4.24 bump on the parent branch. Fixes #109 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit d894fd5)
The web UI labels the rule's feed field "blank = all feeds" and lists such rules as applying to "all", but the RSS monitor selected rules for a feed with `feed_names.iter().any(..)`, which is false for an empty list. A rule saved with the field left blank therefore never matched any feed and auto-download silently never fired. An empty list now matches every feed; the unknown-feed save warning already stays quiet for it. Fixes #111 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit b83f9c3)
(cherry picked from commit ea5de5e) Adapted: the test harness keeps its Arc clone of the app state, since it returns the state to callers after handing one to build_router.
…categories on save Config handlers wrapped "not found" and "already exists" in anyhow!, which carries no status, so the central mapping from #79 never saw them and clients got 500 internal_error for their own mistakes. Unknown and duplicate entries (BUG-73): - Categories: PUT/DELETE of an unknown name return CategoryNotFound (404); a duplicate POST is 409. - Servers: PUT/DELETE/test of an unknown id return ServerNotFound (404, error_kind server_not_found); POST of an id that already exists is 409 instead of silently adding a second server with the same id. - RSS feeds: PUT/DELETE unknown -> 404, duplicate POST -> 409; downloading an unknown RSS item -> 404. - Groups: get, status, headers/fetch and headers/download of an unknown group id -> 404. - nzb-web gains ApiError::conflict(msg) so a 409 with a message goes through the same type rather than an ad hoc status. Category validation (BUG-74): - POST/PUT category now refuse, with 400, a name that is not a single safe path component and an output_dir that enqueue would reject. Before, these saved with 200 and every later job in the category failed at enqueue with "category output path is unsafe". - The rule lives in one place: nzb_core::path::category_output_root, used by both CategoryConfig::validate and QueueManager::output_dir_for. A relative output_dir must stay below the complete dir; an absolute one is still allowed, but no longer with a ".." component or control characters. An empty output_dir is refused (it already failed at enqueue). - Startup logs a warning for each unsafe category in a hand-edited config.toml instead of failing. The existing contract test asserted 500 for a duplicate category; it now asserts 409. Bumps nzb-core to 0.2.19 and nzb-web to 0.4.24. Fixes #115 Fixes #116 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit ed1f13d)
… download (cherry picked from commit 4311ba5) Adapted: history retry already goes through retry_history_entry, so the missing-NZB-data case maps HistoryRetryOutcome::NoNzbData to 409 instead of re-introducing the inline retry path this commit replaced.
/api/setup/apply replaced the configured categories with the imported ones without the check a category save gets, so an unsafe name or output_dir was persisted and every later job in that category failed at enqueue with "category output path is unsafe". The apply now runs CategoryConfig::validate on every imported category first and, if any fail, returns 400 naming each offending entry and its reason without applying anything. Fixes #119 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit af2157f)
(cherry picked from commit aac8b3e) Adapted: the new PauseReason enum was merged alongside our QueueSortField from the queue-sort fix; neither replaced the other.
UpdateGeneralBody dropped unknown keys silently, so a request like
{"required_completion_pct":150} got {"status":true} and changed nothing.
It also saved values such as cache_size=0 or max_active_downloads=99999.
- Add #[serde(deny_unknown_fields)]. Extractor rejections now map to a
400 ApiError that names the field, instead of axum's plain-text 422.
- Make required_completion_pct (100-200, the range the engine clamps to),
article_timeout_secs (0-3600, 0 = no timeout), max_nested_archive_depth
(0-10), direct_unpack and early_failure_check editable. They are
persisted at once and, like the worker limits, apply on restart.
- Bound cache_size (16 MiB-64 GiB), max_active_downloads (<=100,
0 = unlimited, also on /config/max-active-downloads) and the
post-processing pools (<=64). The existing 0 -> 1 clamp is kept.
- Validate before applying anything, so a rejected request never
half-updates the config or the live queue manager.
log_level and abort_hopeless are deliberately not accepted. The log
filter is built from CLI/env at startup, so a saved value would do
nothing, and abort_hopeless is owned by /config/disk-guards. The web UI
does not call this endpoint, so it is unaffected.
Fixes #124
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 4a0a748)
(cherry picked from commit 277403f) Adapted: the multipart rewrite replaces our next_uploaded_file helper, but a corrupt or over-limit archive still maps to 400 through bad_upload rather than a generic server error.
(cherry picked from commit a2379e6) Adapted: the queue response wraps each job in our QueueJob (for the disk-space pause reason), so the mask is applied to the inner job rather than replacing the wrapper.
The --log-level arg carried a clap default of "info", so the resolved
value was always Some("info") even when neither the flag nor
RUSTNZB_LOG_LEVEL was set, and config.toml's general.log_level was never
consulted. That contradicts the documented CLI > env > TOML > defaults
precedence.
Make the arg Option<String> with no default and resolve the filter via
resolve_log_filter: RUST_LOG > --log-level > RUSTNZB_LOG_LEVEL > TOML >
"info". The config is already loaded before tracing is initialised, so no
reload layer is needed. Invalid values (including bare non-level words,
which EnvFilter would otherwise accept as a target name) fall back to
"info" and a warning is logged once tracing is up. Document the
precedence in config.example.toml.
Fixes #130
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 6101d93)
The job archive password went to the extractors as `-p<pw>`, so any local user could read it from `ps` or /proc/<pid>/cmdline during extraction. rarlab UNRAR 6/7 given a bare `-p`, and 7-Zip on an encrypted archive, read the password as one line from a piped stdin, even when there is a controlling terminal. Checked with UNRAR 6.21 and 7.20 and 7-Zip 25.01. Batch extraction and direct unpack now write the password plus a newline to stdin. Batch extraction then closes stdin, so a wrong password fails at once (or hits EOF on a re-prompt) and can't hang. Support is read from the banner the binary prints with no arguments, and cached. Older unrar, unrar-free, p7zip 16.02 (which reads the terminal through getpass) and passwords that contain a line break still get `-p<pw>`. docs/KNOWN_ISSUES.md documents that, along with plaintext storage. Password failures are now typed consistently: - unrar 6/7 report a missing or wrong password as `Incorrect password for <file>` with exit code 11. Neither matched the old patterns, so those jobs failed as archive_invalid instead of archive_password_required. Reproduced on main with unrar 7.20. - 7-Zip's `Cannot open encrypted archive` spelling is also matched. - The `Enter password` prompt we answer ourselves no longer counts as evidence of a password problem; only a further, unanswered prompt does. - DirectUnpackResult gains `password_required`, set from unrar's password errors, exit code 11 or a repeated prompt, and logged when falling back to normal extraction. nzb-postproc now names the tokio features it uses (io-util, plus time for tests) rather than relying on workspace feature unification. Fixes #128 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 5c47205)
mode=queue hardcoded unpackopts to "3" for every slot, so a job added with pp=0, or in a category with post-processing 2, still claimed full repair+unpack+delete. SABnzbd reports the job's per-job level as a string. Slots now report the effective level (job pp override, else category, else 3) through sab_unpackopts. The value is on the same scale pp is accepted on (sab_pp_override: SAB 0/1 stay, 2|3 become 3) and history reports it (sab_pp_label), so re-adding a job with its reported unpackopts keeps its level. build_queue_response takes a level resolver; the live handler passes QueueManager::post_processing_level. Fixes #134 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 99ab61a)
(cherry picked from commit 94a57a8) Adapted: the form rewrite replaces the earlier upload reader, but the password masking and the 400 mapping for a corrupt or over-limit archive are kept.
On Debian/Ubuntu with both p7zip-full and 7zip installed, `7z` is p7zip 16.02, which reads passwords from the terminal only, so the archive password has to go in argv. `7zz` is modern 7-Zip and takes it on stdin. find_7z checked `7z` first, so the argv path was used even though a stdin-capable binary was installed. Check `7zz`, then `7z`, then `7za`. Depends on #133. Fixes #136 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit dfcad30)
SABnzbd's switch (NzbQueue.switch) takes value2 either as a queue index
or as another job's nzo_id; with an id, the job moves into that job's
position, adopts its priority, and the response reports both. rustnzb
only parsed a number and answered "Missing or invalid target queue
position" for the id form.
Resolve a non-numeric value2 with the same strict job_id_matches rules
as the other per-job commands, adopt its priority, then move into its
position. The numeric form and the {"result":{position,priority}} shape
are unchanged; an unknown value2 still returns status:false.
Fixes #140
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit de5d2ab)
parse_sabnzbd_ini read category and server names only from an inner `name =` key, so an INI with just [[section]] headers imported every category as "Default" and every server with an empty name. SABnzbd keys these sections by the header (and writes the same value to `name`), so use the header when `name` is absent or empty. An empty `displayname` no longer masks a server's `name` either. Fixes #141 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit e468392)
…e names Name sanitizing removed only char::is_control (C0/C1), so bidi overrides, isolates, directional marks, ZWSP and the BOM passed through into job names, output folders and per-file names. A name with U+202E such as "clip<RLO>4pm.exe" displays as "clipexe.mp4" (RTL-override spoofing). sanitize_job_name and sanitize_filename now remove U+202A-202E, U+2066-2069, U+200E, U+200F, U+061C, U+200B and U+FEFF, and safe_component rejects them. Not every Cf character is removed: ZWJ and ZWNJ (U+200D, U+200C) are kept because emoji sequences and scripts such as Persian and the Indic scripts need them. Fixes #144 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 649635c)
(cherry picked from commit b303169) Adapted: retry_history_job delegates to our idempotent retry_history_entry, so a repeat of an entry already queued or running returns the existing job instead of enqueueing a duplicate. The idempotency tests are kept alongside the new retry_all test.
SABnzbd's pp=2 means repair and unpack but keep the archives; only pp=3 adds the delete. The compat layer mapped both onto RustNZB level 3, whose cleanup always removes the extracted archive volumes, so a pp=2 job lost its RAR set and history reported "D". Add NzbJob::delete_archives (serde default; persisted in new nullable queue and history columns via schema migration v15) and PostProcConfig::delete_archives. When it is false, cleanup keeps the archive volumes and the archives found during nested extraction, and still removes PAR2 files, as SABnzbd does after a successful verify. The category cleanup patterns and unwanted extensions still apply. addfile/addurl set the flag from pp (2 -> keep, 3 -> delete, otherwise the default, delete). Queue unpackopts and history pp report an unpacking job that keeps its archives as SABnzbd 2 / "U", so a client re-adding a job with what it reads back gets the same behaviour. RustNZB's native levels are unchanged. Fixes #148 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit c1b8d55)
The SSRF fetch guard built its rejections with ApiError::from(anyhow!),
which carries no status, so a refused private/loopback/metadata/file://
URL on /api/setup/import-sabnzbd-api, /api/queue/add-url and
/api/rss/items/{id}/download came back as 500 internal_error. A rejected
URL is client input.
Add ApiError::url_rejected (400, error_kind "url_rejected") and use it
for every guard rejection in validate_inner, so all callers get the
right status without per-handler mapping. RSS feed add/update now
reports the same error_kind instead of re-wrapping as bad_request.
Add ApiError::bad_gateway (502, error_kind "bad_gateway") for failures
that belong to the remote: DNS resolution failure in the guard, a
connect/timeout error, a non-2xx response, and invalid JSON from a
SABnzbd being imported. An empty add-url body is now 400.
The SABnzbd addurl path is unchanged: guard errors still render as
{"status":false,"error":...} on HTTP 200 with the same message text.
Fixes #150
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 0c53b8e)
Adapted: the contract-test insertion conflicted with our
rss_feed_poll_interval_below_floor_is_rejected_when_saved test; both are
kept, and the closing braces the conflict markers consumed were restored.
…thout delete)" This reverts commit fa62ce8.
This was referenced Oct 9, 2026
SAB queue: unpackopts is always "3" instead of the job's post-processing level
xbmc4lyfe/rustnzb#134
Open
Open
Open
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
Collaborator
Author
There was a problem hiding this comment.
Security review: no security blockers, 1 merge blocker (failing e2e) (head 4f57a74)
All 32 commits were reviewed (28 xbmc4lyfe cherry-picks plus 4 local follow-ups). The upstream content was treated as untrusted. No security blockers and nothing malicious were found. One non-security regression keeps e2e failing on every run, so this head isn't gated yet.
Local checks at this head:
cargo test --workspace: 1034 passed, 0 failed.cargo clippy --workspace --all-targets -D warnings: clean.cargo fmt --check: clean.ci/migration-policy.sh: passes.
CI: the required checks (policy, rust, Analyze (rust), Analyze (javascript-typescript)) are green. e2e and the CodeQL summary are red; see below.
Blocker (not security): e2e first-boot 1.8 fails every run
e2e/tests/first-boot.spec.ts:169 waits for "failed to connect" after entering http://localhost:1 on the welcome page.
- On main:
welcome.component.tsreaderr.error.message/err.error.error. The serialisedApiErrorhas neither field, so the page always showed the fallback "Failed to connect to SABnzbd…". - Now: bedecf3 switched the page to
httpErrorDetail(), which prefershuman_readable. For a loopback URL that text is the fetch-guard rejection: "URL targets a private/reserved address (allow it with …)". 4bc45d7 also made that rejection a 400url_rejected. - Evidence: the first run failed earlier, because the backend wasn't healthy within 15 s. Locally the PR binary is healthy about 2 s after start with the same e2e config, so that part looks like a slow runner. The rerun reached the tests and failed 1.8 (82 passed, 1 failed, 7 skipped).
fresh-config.tomlkeeps private addresses blocked, so this fails every time. Main's CI is green at 1ca5ee7. - Fix: update the test to expect the guard message (e.g.
/private\/reserved|failed to connect/i), or point it at an unreachable public address such as TEST-NET192.0.2.1. Showing the admin the real reason is fine from a security point of view.
CodeQL: 3 "high" rust/cleartext-logging alerts, all false positives
crates/nzb-postproc/src/unpack.rs:1404crates/nzb-core/src/path.rs:481apps/rustnzb/tests/queue_password_mask.rs:93
Each is a test assertion or panic! message printing fixture passwords ("wrong", the split-password table, the mask test). Each sits inside #[cfg(test)] or an integration-test file, and no production code logs a password. I'd recommend dismissing them as "used in tests". They don't block merge, since the CodeQL summary isn't a required check.
Areas checked, no security issues
- Dependencies and pins: no new crates in
Cargo.lockordesktop/src-tauri/Cargo.lock, only internal version bumps (nzb-core 0.2.19, nzb-web 0.4.24). The only manifest changes are extratokiofeatures:io-utilfornzb-postproc, and dev-onlytime/full+test-util. Resolver 3 keeps dev features out of release builds. There are no npm package, CI, build-script or shell-script changes. - Secrets and estate internals: none in the diff, the docs or the commit metadata. The only private IPs are generic SSRF test fixtures (
192.168.1.10,10.0.0.1). Commits use noreply identities and the standard co-author trailer. - Post-processing (cf43930, 3215f6e, 739e5ed, f22c510):
- Archive passwords now reach unrar 6+/7-Zip 21+ on stdin instead of argv, which closes the
ps//procexposure. The fallback to-p<pw>for older tools matches main. - stdin is written once and then closed, and a re-prompt aborts, so a wrong password can't hang the job (batch or direct unpack).
- Archive paths reach argv as full paths, so member names can't become switches.
- Picking
7zzfirst is fine. delete_archives=falseonly narrows cleanup, to.par2files inside the job directory.
- Archive passwords now reach unrar 6+/7-Zip 21+ on stdin instead of argv, which closes the
- Paths:
category_output_rootnow also rejects..and control characters in absolute category roots.- Bidi and zero-width stripping covers spoofing characters and keeps ZWJ/ZWNJ.
- All slicing in
split_job_passwordis char-safe.
- SAB API and web auth:
- A single API key, compared in constant time, still gates every mode except
versionandauth, soretry_allandswitchneed it. The key check is unchanged. - dcf0464 just moves the
/davnesting intobuild_router_with_dav. It is still behinddav_auth, with the same unauthenticated warning. - 73837d9 masks the password in the native queue. The SAB queue still returns it, as on main and as SABnzbd does (now in
KNOWN_ISSUES.md). PUT /config/generalnow rejects unknown fields, andlog_levelcan't be set over the API.- Error bodies echo no secrets.
- The frontend has no
innerHTMLorbypassSecurityTrust; errors appear as plain snackbar text.
- A single API key, compared in constant time, still gates every mode except
- NNTP/TLS: no changes under
nzb-nntpornzb-news. Command tracing logs only the verb, so the newly config-drivenlog_levelcan't exposeAUTHINFO PASS. - Queue, DB and RSS:
- Migrations v14/v15 use bound parameters.
- Queue sort uses a fixed enum and never reaches SQL.
- None of the added code deletes anything outside post-processing.
- RSS fetches still go through the fetch guard with body caps, and polling has a 60 s floor.
- Lock order checked.
- The fixup/revert pair (fa62ce8/4ae7ce8) nets to zero.
Should-fix (non-blocking)
- Native
/api/queue/addzip entries in folders get the wrong name and password.enqueue_nzbrunssplit_job_passwordon the raw entry path. Confirmed:"Season1/Show.S01E01.nzb"→ jobSeason1, passwordShow.S01E01. Every NZB in that folder shares the name, and the bogus password overrides the NZB's own<meta type="password">, so encrypted releases fail to extract. The SAB and watch-folder paths already take the basename first; do the same here. - RSS rules with no feeds now match every feed (ae524c3). That's the intended fix, and the UI already said "blank = all feeds". But enabled blank rules that matched nothing before start auto-downloading after upgrade. Add a release note.
Nits
- The extractor version probe runs the configured binary with no arguments and no timeout.
- The watch-folder file name, including any
{{password}}, still appears in the "Processing NZB" log and theprocessed//failed/file names. That's inherent to the SABnzbd convention. - The disk-guard auto-resume check runs outside
pause_transition. There's a microsecond window against a user "pause all", and it's not exploitable. - 0e94189's body credits PR #149 for
delete_archives, which actually comes from f22c510. - The queue-view style budget is exceeded by 128 bytes (a build warning).
Not merged. Once the e2e expectation is fixed and CI is green, this needs a fresh gate. This review covers only this head SHA.
first-boot 1.8 fills http://localhost:1 on the welcome page and waited
for "failed to connect". The fetch guard refuses private addresses
before any connection, and the page now shows that human_readable
("URL targets a private/reserved address") instead of the generic
fallback, so the spec matches either.
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
Collaborator
Author
There was a problem hiding this comment.
Security review: no blockers (head be60f86)
This is a delta re-review of 4f57a74..be60f86. 4f57a74 is an ancestor of this head, and the base is unchanged (1ca5ee7). The full review at 4f57a74 still applies to everything below that commit.
Delta: be60f86, test-only, e2e/tests/first-boot.spec.ts (+4/−2)
- Spec 1.8 now waits for
/private\/reserved address|failed to connect/iinstead of/failed to connect/i, and the comment explains the fetch guard. No product code, dependencies, config or CI changed. - The worker was right to skip my TEST-NET suggestion.
is_globally_routablerejects documentation ranges (!v4.is_documentation()), so192.0.2.1would also have been refused before any connection. Accepting the guard's reason is the correct fix. - Nit: the following comment, "connecting to :1 will fail quickly", is now stale.
Round-1 blocker resolved: e2e passes at this head.
CI at this head
- Required checks:
policy,rust,Analyze (rust)andAnalyze (javascript-typescript)are green. - Other green checks:
e2e,frontend,desktop,dependency-reviewandrunner-policy. CodeQLsummary: red. It shows the same three test-onlyrust/cleartext-loggingfalse positives from round 1 (#48, #49, #50) and nothing new. I still recommend dismissing them as "used in tests".container-smoke(not required): red in all three attempts.- Cause: Docker Hub was unreachable from both Node B publish runners.
auth.docker.iotimed out at login, then while pullingmoby/buildkit. - Not caused by this PR: the product
Dockerfilecopies onlyapps/,crates/and the Cargo files, nevere2e/. So the image inputs are identical to 4f57a74, wherecontainer-smokepassed onnode-b-gha-public-publish-235 minutes earlier. Worth rerunning once Docker Hub is reachable again.
- Cause: Docker Hub was unreachable from both Node B publish runners.
Still open (non-blocking, from round 1)
- The native
/api/queue/addsplits the inline password on raw zip entry paths (Season1/Show.S01E01.nzb→ jobSeason1, passwordShow.S01E01). - RSS rules with no feeds now match every feed, so this needs a release note.
- Nits as listed in round 1.
Not merged. This verdict covers only this head SHA.
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
Collaborator
Author
There was a problem hiding this comment.
Security review: no blockers (head be60f86): CodeQL triage
Triage of the three high-severity rust/cleartext-logging alerts. Each location is a sink, where CodeQL thinks a password is written. All three sinks are test failure messages printing hard-coded fixtures, so all three are false positives. No production code in this PR logs an archive or job password, and nothing needs fixing.
| Alert | Location | Enclosing code | What reaches the sink | Verdict |
|---|---|---|---|---|
| #50 | crates/nzb-postproc/src/unpack.rs:1404 |
#[cfg(test)] mod, lines 760–1408; #[tokio::test] real_7z_encrypted_archive_round_trip |
The test's own literals "wrong" / None in an assert! message ("{password:?}: {error:#}"), printed only if the assertion fails |
False positive (test-only fixture) |
| #49 | crates/nzb-core/src/path.rs:481 |
#[cfg(test)] mod, lines 280–527; #[test] split_job_password_matches_sabnzbd_scan_password |
The SABnzbd scan_password example table ("secret", "kITTY", …) in a panic! message |
False positive (test-only fixture) |
| #48 | apps/rustnzb/tests/queue_password_mask.rs:93 |
Cargo integration test (apps/rustnzb/tests/, built only by cargo test); #[tokio::test] native_queue_masks_archive_password_but_sab_queue_keeps_it |
A panic! printing a throwaway test server's native queue JSON. That JSON holds the fixture mask-test-password / meta password, already masked as "********", which is what the test asserts. CodeQL itself classifies this instance as test. |
False positive (test-only, value already masked) |
How each was verified:
- Module spans were measured by brace depth, not just by finding the first
#[cfg(test)]in the file. Each sink is inside a test module or an integration-test file, so none is compiled into the shipped binary. - Every value reaching the three sinks is a string literal in that test.
Checking for production leaks CodeQL might have missed:
- I scanned every added production line in
crates/andapps/rustnzb/src/fortrace!/debug!/info!/warn!/error!/println!/eprintln!/panic!calls mentioning passwords, keys or tokens. The only hit is the testpanic!behind #49. - The new extractor log lines (
Failed to write the archive password to unrar,asked for the archive password again,rejected the archive password) use fixed text and never include the value. - NNTP command tracing logs only the command name, so
AUTHINFO PASScan't reach logs even with the new config-drivenlog_level. - The only runtime path where an inline password can appear in a log is the round-1 nit. A watch-folder file named
Name{{pw}}.nzbis logged by path when picked up. That log line already exists on main, and the name is already visible on disk, so this PR adds no exposure.
Recommendation: dismiss #48, #49 and #50 as "Used in tests". I haven't dismissed them; that's oversight's call.
Verdict for be60f86: no blockers
- CI:
container-smokepassed on oversight's rerun, so the earlier failure was the Docker Hub auth flake. All checks are green, including the requiredpolicy,rust,Analyze (rust)andAnalyze (javascript-typescript). The only red check is theCodeQLsummary, which reflects only the three false positives above. - Head: unchanged since my delta review (be60f86, test-only on top of the fully reviewed 4f57a74).
- Still open, non-blocking:
- Native
/api/queue/addzip-entry inline-password split. - RSS blank-feed rules need a release note.
- Round-1 nits.
- Native
Not merged. This verdict covers only this head SHA.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports the third batch of upstream fixes from xbmc4lyfe/rustnzb (issues #97–#150, PRs #98–#151) onto main. Each upstream commit is cherry-picked with
-x, keeping xbmc4lyfe as author.Issue → commit
QueueSortFieldand theirHistoryRetryOutcome; their test gained ourHistoryEntry.post_processingbuild_routernow layers DAV itself, so the test's manual layer was dropped; the harness still callsbuild_router(state.clone())because it uses the state afterwardsretry_history_entrypath;HistoryRetryOutcome::NoNzbDatamaps to 409QueueSortFieldand theirPauseReasonnext_uploaded_file; ourbad_upload()(corrupt/over-limit archives → 400) kept at both extract sitesQueueJob, so masking isQueueJob { pause_reason, job: mask_job_password(job) }log_levelcomment, kept ourcache_sizenoteUploadFormreader superseded #129's; bothmask_job_passwordandbad_uploadkeptretry_history_jobdelegates to our idempotentretry_history_entry(Started/InProgress return the existing nzo_id)Follow-up commits (ours)
delete_archives(added by feat(nzb-web): idempotent queue admission via Idempotency-Key #149) set on theNzbJob/HistoryEntryfixtures inqueue_sort_api,queue_bulk_apiandhistory_retry_idempotent, which otherwise no longer compile.read_uploadwrapped axum'sMultipartErrorinanyhow, discarding the statusFrom<MultipartError>maps, so a truncated multipart body onPOST /api/queue/addanswered 500 instead of the 400 ci: migrate runner selectors off retired node-b/rust labels #120 specifies. Now usesApiError::from.Declined / not in this batch
Verification
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace(onenzb-postproctest,rar_extraction_normalises_owner_only_modes, failed once withText file busyunder parallel load and passed on rerun, alone and in the full crate suite)cargo metadata --lockedindesktop/src-tauri(lockfile still resolves; no refresh needed)npm run build -- --configuration=productionfor the Angular frontend (pre-existing 128-byte style budget warning on the queue view)