Repository navigation
fix: bring in xbmc4lyfe batch 2 fixes - #167
Conversation
mode=addurl&name=<URL> passed name both as the URL and as the job name, so the job was named after the whole URL, which output_dir_for rejects as not a single safe path component: every successful addurl fetch failed at enqueue. The nzbname override was also ignored on addfile and addurl over GET, form and multipart. Read nzbname from the query string, a urlencoded body or a multipart field. For addurl the job name is nzbname, else the Content-Disposition filename, else the URL's last path segment (percent-decoded, query excluded), each without .nzb; never the URL itself. addfile prefers nzbname over the uploaded file name. Unit tests reach the loopback fixture server through a cfg(test)-only task-local seam in fetch_guard (ALLOW_LOOPBACK_FOR_TESTS), scoped to the test's own future; the SSRF guard is unchanged in every other build. Fixes #69 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 9ed6598) Adapted: tests admit the loopback fixture through an explicit fetch_allowed_hosts entry on the test state instead of the upstream cfg(test) ALLOW_LOOPBACK_FOR_TESTS seam, which our fetch guard (private-LAN allowlist plus explicit hosts) does not have.
Every /api/config/* handler cloned the current config, mutated the copy and stored it with an unguarded ArcSwap::store. Two concurrent writers started from the same snapshot, and the later store silently dropped the earlier writer's change, in memory and in config.toml. A burst of 24 concurrent category/feed adds lost entries on every run. Add AppState::update_config_with(|cfg| ...), which runs the closure on the latest config and saves the TOML and publishes the result while holding a config write mutex. An error from the closure writes nothing. update_config takes the same lock and is documented as a blind replacement. All 17 mutating handlers (SAB API key rotation, servers, categories, history retention, max active downloads, speed limit, disk guards, RSS feeds, general settings, SABnzbd import apply, DAV) now go through it. Server handlers push the latest committed server list to the queue manager, so concurrent writers converge on the same state. Bumps nzb-web to 0.4.23. Fixes #71 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit aec138d) Adapted: update_config_with keeps our partial server merge, password masking and RSS feed URL validation inside the new write lock; started_at stays alongside config_write.
The watch folder only matched .nzb and .nzb.gz, so a .zip of NZBs (or a .nzb.bz2) dropped there was skipped without a log line. The upload API already unpacked .gz and .zip, but through extract_nzbs in the binary crate, which the watcher in nzb-web cannot reach. Move extract_nzbs (unchanged, with its size bounds and tests) into nzb_web::nzb_archive and have both the upload handler and the watcher call it. The helper also learns .bz2, using the bzip2 crate that was already in the dependency tree via zip. The watcher now enqueues each .nzb inside a zip, names jobs after the NZB without any archive directory, and moves the archive to processed/ once at least one NZB was enqueued. zip and flate2 drop out of the binary crate, which no longer uses them directly. Fixes #73 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 67a7070)
StatPipeline::execute set the connection Busy for each batch and then propagated send, flush and read failures with a bare `?`, leaving the connection Busy. After such a failure the stream is misaligned with the outstanding STATs, so the connection must not be reused. All three now set ConnectionState::Error first, matching fetch_article and the ARTICLE pipeline. Fixes #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit e20357d)
SABnzbd's pp parameter selects post-processing for a single job, but the compat layer dropped it, so every job used its category's level. Add NzbJob::pp_override (serde default; persisted in a new nullable queue.pp_override column via schema migration v13, which tolerates partial schemas without a queue table) and QueueManager::post_processing_level, which prefers the override over the category setting where post-processing chooses its level. addfile/addurl read pp from the query string, a urlencoded body or a multipart field, mapping SABnzbd's cumulative scale onto RustNZB's (2 and 3 -> 3; -1/absent/out of range -> category default). script is accepted and ignored, as before (unknown parameters are not rejected). Fixes #77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit a3c7cd7) Adapted: kept both the earlier nzbname plumbing and the pp override in the shared form/multipart parsers and in handle_addurl; existing SAB config/priority/diskspace tests kept alongside the new pp tests.
…nput
Client errors on the native API surfaced as 500 internal_error because
From<NzbError> for ApiError always used 500, and several handlers wrapped
"not found" and validation failures in anyhow!, which carries no status.
Central mapping (nzb-web error.rs):
- NzbError::JobNotFound/ServerNotFound/CategoryNotFound -> 404,
ParseError/InvalidNzb -> 400, AdmissionConflict -> 409, rest 500.
- error_kind recognises the wrapped NzbError variants and otherwise
classifies by status (bad_request / not_found / conflict /
internal_error) instead of labelling every non-typed error
internal_error.
Handlers:
- history retry and history logs: unknown id -> 404 (logs previously
returned 200 with an empty list).
- queue priority: out-of-range value -> 400; unknown job -> 404 via the
central mapping, as is queue move.
- GET /api/articles/{id}: NNTP 430 (ArticleNotFound) -> 404.
- Group routes extract ids with IdPath, which rejects a non-numeric id
with a JSON 400 "Invalid id in request path" instead of axum's parser
text that leaked Rust type names.
- RSS rules: invalid or overlong regex -> 400; PUT of an unknown rule
-> 404 (DELETE stays idempotent); a rule naming feeds that are not
configured is still saved, logged as a warning, and the response
carries a "warnings" array.
Bumps nzb-web to 0.4.23.
Fixes #79
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 1e09a67)
Adapted: the new contract tests authenticate with the existing login() helper (this tree's first-run setup is already covered elsewhere) and sit alongside the existing contract tests.
…to settle The watch folder could import one NZB twice, retry a bad one forever, or read one mid-copy: - A parse failure only logged a warning and left the file in place, so every restart retried it. Unreadable, unparseable or unenqueueable files now move to failed/. - A fixed 500 ms sleep was the only guard against reading a file still being written. The watcher now waits until the size and modification time are unchanged across two checks 500 ms apart (capped at 10 minutes), for both new events and files present at startup. - The move to processed/ after add_job was best-effort, so if it failed the next restart enqueued a duplicate. The watcher now renames the file to <name>.processing before enqueueing. Claimed files are never imported again, and the claimed file then moves to processed/ or failed/. If that move fails, the file stays claimed, and startup logs a warning for leftover claimed files instead of re-importing them. Fixes #81 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 5b8c138) Adapted: the claim-before-enqueue and write-settle logic wraps our archive-aware watcher (zip/bz2/gz), so enqueue_claimed unpacks every NZB in the claimed file and any failure sends the whole file to failed/; both sets of watcher tests are kept.
SAB addfile/addurl passed compressed NZBs straight to the XML parser, which rejects them, while the native add endpoint already unpacked .gz and .zip. Move that extraction (and its decompression-bomb limits) from the app crate into a shared nzb_web::nzb_archive module, used by both the native handlers and the SAB layer, and add .bz2 support there (bzip2 0.6 was already in the dependency tree via zip's default features; no new crates are locked). addfile unpacks the upload; a multi-NZB zip enqueues each NZB as its own job and reports every nzo_id. addurl unpacks the fetched body, naming the job after the inner NZB, and hands multi-NZB zips to addfile. The addurl test reaches its loopback fixture through the same cfg(test)-only fetch_guard seam (ALLOW_LOOPBACK_FOR_TESTS) introduced on the nzbname branch; the hunk is identical. Fixes #83 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 066b77f)
mode=server_stats was aliased to fullstatus, so clients got the
{"status": {...}} dashboard document instead of SABnzbd's server_stats
shape. Add a dedicated handler that remaps the native per-server
statistics (QueueManager::server_stats_get_all, as served by
GET /api/config/servers/stats) into SABnzbd's layout: overall
total/month/week/day bytes and a per-server map keyed by server name
with total/month/week/day, daily, articles_tried and articles_success.
Fixes #85
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 205e70d)
mode=warnings is a real SABnzbd mode that clients poll, but RustNZB
rejected it as unknown. Answer with SABnzbd's {"warnings": [...]}
shape. RustNZB keeps no SABnzbd-style warnings list (fullstatus
already reports warnings: [] and have_warnings "0"), so the list is
empty and consistent with fullstatus; name=clear returns
{"status": true}.
Fixes #87
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 83938f6)
(cherry picked from commit faebca6) Adapted: our pipeline error path already caps per-article outage retries (server-wide vs article-outage vs fail-over). Only the circuit-breaker accounting (record_failure / report_provider_outage) is gated by counts_against_circuit_breaker; the outage-try cap is unchanged. Connect-time failures still always count — they are provider-level.
acquire_download loaded the limiter once and awaited it for the whole article in burst-sized chunks. At a very low limit (20 B/s) one ~750 KB article parks a worker for hours, and set_download_bps only swapped the ArcSwap: parked acquires kept waiting on the old limiter, so raising the limit or clearing it to 0 had no effect. Because every worker connection stayed parked, new jobs stalled too, and only a restart recovered. The limiter now signals a Notify on every reconfiguration. Each chunk wait races the limiter against that notification, which is registered before the limiter is loaded so a change cannot be missed. On a change the acquire reloads the current limiter and continues with the bytes still owed. With no limit configured it returns immediately. No lock is held across an await. Tests: unit tests in bandwidth.rs (20 B/s to unlimited and to 10 MB/s release a parked acquire within 2 s; unlimited never waits), plus harness_speed_limit.rs, which drives a real QueueManager against the mock NNTP server: a limit set at startup, a limit set before a job is added, and throttle/unthrottle mid-download. Fixes #91 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 1050af7)
Two jobs with the same name derived the same output folder at add time and both completed into it, so the second release overwrote or merged into the first and both history rows reported the same storage path. Reserve the complete folder when a job first writes output (direct unpack, post-processing, or the final move) rather than at add time, so queued jobs never hold a name. The reservation uses create_dir and retries with `<name>.1`, `<name>.2`, ... on AlreadyExists, as SABnzbd does, so two jobs finishing together cannot pick the same folder. The chosen folder is recorded on the job and therefore in history, and in the job checkpoint so an interrupted post-processing run resumes into its own folder instead of reserving another. A failed job releases an empty reservation, and a rename or category change drops it. Fixes #93 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 2ef6207) Adapted: kept our post_processing_level() helper (job override wins over the category default) and our sanitize_job_name rename path; the output dir claim and release calls from upstream are applied on top of both.
(cherry picked from commit 60eeb1e) Adapted: the new incomplete-dir sweep helpers were merged alongside our existing claim_unique_output_dir / restore_claimed_output_dir helpers; neither replaced the other.
The batch 2 port bumped nzb-web to 0.4.23 and added bzip2 and zip to its dependency list, which left desktop/src-tauri/Cargo.lock stale under `cargo build --locked`. Only the desktop lockfile changes; no dependency versions were upgraded.
thedancingdeveloper
left a comment
There was a problem hiding this comment.
Security review: 3 blockers (head 1b6009e)
I reviewed all 16 commits line by line. The upstream content was treated as untrusted. No malicious code was found. Each blocker below was confirmed with a local PoC test (not committed).
Local checks at this head:
cargo test --workspace: 925 passed, 0 failed.cargo clippy --workspace --all-targets -D warnings: clean.cargo fmt --check: clean.ci/migration-policy.sh: passes.- CI is green.
Blockers
1. The zip size cap can be bypassed by under-declaring entry sizes (crates/nzb-web/src/nzb_archive.rs:45-60)
The running total sums each entry's declared entry.size(). The actual read is capped at 100 MiB per entry. zip 8.6.0 does not check decompressed output against the declared size: Deflate is a plain DeflateDecoder, and only the CRC is verified. Overlapping central-directory entries are also accepted unless has_overlapping_files() is called.
- PoC: a 180 KB zip with three entries declaring
uncompressed_size = 1(CRCs left correct) returned 3 entries totalling 180 MiB against the 100 MiB cap. - Impact: at about 1000:1 deflate, a 100 MB
addurlbody (MAX_ADDURL_BODY_BYTES) can decompress to roughly 100 GB held in memory, so the process is OOM-killed. - New exposure: this logic was already in the native upload handler on main. This PR newly exposes it to SAB
addurl(remote, indexer-controlled content), SABaddfileand the watch folder. - Fix: budget on bytes actually read, e.g.
entry.take(MAX - total + 1)thentotal += buf.len(), and bail whentotal > MAX. Optionally cap the entry count, or reject archives wherehas_overlapping_files()is true.
2. The incomplete-dir reclaim fails open on DB errors and deletes active downloads (crates/nzb-web/src/queue_manager.rs:4217-4260, f8dac41)
referenced_work_dirs only logs a warning when db.queue_list() or db.history_list() fails, then carries on with whatever it collected. A per-row history_get_retry_data error is also just skipped (:4245). In those cases sweep_orphaned_work_dirs (run at startup) and remove_unreferenced_work_dir call remove_dir_all on directories that are still in use.
- PoC: a queued job with a UUID work dir. With a healthy DB the sweep keeps the dir. After one column of that row is made undecodable (
UPDATE queue SET total_bytes='x'),queue_list()returnsErrand the same sweep deletes the dir. - Why it matters:
queue_listcollects into aResult, so one bad row, orSQLITE_BUSYfrom a backup tool or sqlite shell, wipes every partial download.restore_from_dbthen fails on the samequeue_list()?, whichstartup.rsonly logs. On main, such a failure was recoverable. With this PR the data is gone. - Fix: have
referenced_work_dirsreturnResult, and skip all removal if any listing or retry-data read fails (fail closed).
3. clean_nzb_name panics on non-ASCII names (crates/nzb-web/src/sabnzbd_compat.rs:378, 30523ab)
base[base.len() - 4..] slices by bytes. This is the same class as the #158 parse_rar_volume bug.
- PoC:
"Amélie"and"Pokémon"panic, because bytelen-4falls inside theé. - Reach:
nzbnameon SABaddfileandaddurl(Sonarr and Radarr send release titles here, and accented titles are common), the remote server's Content-Disposition filename, and zip entry names. - Impact: the handler task panics and the request is dropped. The process survives because nothing sets
panic = "abort", but those adds fail every time. - Fix:
base.get(base.len() - 4..).is_some_and(|s| s.eq_ignore_ascii_case(".nzb")), or useto_ascii_lowercase. Add a test withAmélie.
Areas checked, no issues
- Dependencies:
zipmoved from the app crate tonzb-webat the same 8.6.0, withdefault-features = falseanddeflateonly.bzip20.6.1 uses the pure-Rustlibbz2-rs-sysbackend, so there is no C build.- Both were already in the lockfile through
nzb-postproc. No new crates, no checksum changes, and no build.rs, CI or script changes.
- gz/bz2: output is bounded by
take(MAX+1). The.gz/.bz2suffix slicing is char-safe, because no non-ASCII character lowercases to.,g,z,bor2. - Path traversal:
- Zip entry names,
nzbnameand Content-Disposition names all pass throughparse_nzb→sanitize_job_name. add_jobre-checkssafe_componentand that the output dir is under the complete root.- SAB paths use
output_dir_for. The watcher'scomplete_dir().join(name)is unchanged from main and is still guarded byadd_job.
- Zip entry names,
- Watch folder:
- Each file is claimed by rename before enqueueing.
- Symlinks are refused after the claim, via
symlink_metadata. - Reads are capped at 100 MiB, with a settle wait capped at 600 s.
- The watcher starts only after
restore_from_db.
- addurl: the SSRF guard and pinned client are unchanged, and the body is capped before decompression.
- Unique output folders (b1536a2): reservation is atomic via
create_dir. Release isremove_dir, which only removes an empty directory. A checkpointed folder is accepted only if it is a real (non-symlink)<base>or<base>.<n>folder next to the target. - Reclaim confinement (apart from blocker 2): removal is limited to direct, non-symlink children of the canonical incomplete root. The sweep only touches UUID-named directories.
- Config read-modify-write lock (102a3ab): every mutating handler goes through
update_config_with, and no blindupdate_configcallers remain. The lock order is config, then queue manager, andQueueManagernever takes the config lock, so there is no deadlock. Password masking and feed URL validation stay inside the lock. - Native API validation (8a639b2):
- Status mapping only.
- Regex length is bounded, and Rust
regexis linear-time. - Priority must be 0–3.
IdPathreturns a JSON 400 for bad path ids.
- NNTP (6d05f7a, 098bf57): a STAT pipeline I/O failure now sets the connection to
Error, so the misaligned stream is never reused. The circuit-breaker gating is consistent with #157's per-article outage cap. - Bandwidth (03a1f85): correct
Notifypattern: the notification is enabled before the limiter reload. - SAB modes:
ppmaps to {0, 1, 3}.warningsalways returns an empty list.server_statsreturns only names and counters. - Migration v13: does not collide with existing migrations, and uses bound parameters only.
Should-fix (non-blocking)
Timeoutno longer counts against the circuit breaker, so a server that black-holes reads is only limited by the per-article outage cap.- A multi-NZB
addurlzip is decompressed twice: once inaddurl, then again inaddfileon the raw bytes. - The general-settings closure applies queue-manager runtime changes before the TOML save. If the save fails, runtime and disk diverge.
Nits
- 1b6009e's message says "no dependency versions were upgraded". It also re-points existing lock edges down:
windows-sys0.61.2 → 0.59.0/0.45.0 for dirs-sys, errno, rustix, tempfile, quinn-udp, nu-ansi-term, os_pipe and winapi-util, andtoml1.1.2 → 0.9.12 for tauri-utils. All of those versions were already in the lock, and desktop CI is green. Considercargo update -p nzb-webto keep the diff minimal. - Attribution:
- xbmc4lyfe authorship and
cherry picked fromlines are kept. - Unlike batch 1, there are no
Upstream-PR:/Fixes-upstream-issue:trailers. - The bare
Fixes #69…#93lines point at unrelated items in this repo. All are already closed, so nothing auto-closes, but the references are misleading.
- xbmc4lyfe authorship and
This verdict covers only this head SHA. A new head needs a fresh review.
- Budget zip extraction on bytes actually read instead of the declared entry size, cap the number of NZB entries, and reject archives whose central-directory entries overlap. - Make the incomplete-dir reclaim fail closed: an error from queue_list, history_list or a per-row history_get_retry_data skips every removal in both the startup sweep and single-directory removal. - Slice the ".nzb" suffix in clean_nzb_name on a char boundary, so non-ASCII names no longer panic. Each has a regression test. Timeouts now also count against the circuit breaker: a server that accepts the connection and then black-holes the read is as unhealthy as one that refuses it.
thedancingdeveloper
left a comment
There was a problem hiding this comment.
Security review: no blockers (head 54e49e0)
The follow-up commit 54e49e0 sits directly on 1b6009e, so the 16 commits reviewed in round 1 are unchanged. 54e49e0 touches only the three blocker sites, their tests, and the circuit-breaker gating. It adds no dependencies, and there are no lockfile, build, CI or script changes.
Round-1 blockers: all fixed
I reran my original PoCs unchanged against this head.
- Zip size cap (
nzb_archive.rs)- Fix: each entry's read is limited to the remaining budget + 1 byte, and the running total counts bytes actually read. Archives whose central-directory entries overlap (
has_overlapping_files()) are rejected, and NZB entries are capped at 10,000. - PoC: three 60 MiB entries declaring 1 byte each are now rejected ("exceeds the 100 MB limit"); round 1 accepted 180 MiB. The byte budget alone bounds overlapping entries, so the overlap check is defence in depth.
- Fix: each entry's read is limited to the remaining budget + 1 byte, and the running total counts bytes actually read. Archives whose central-directory entries overlap (
- Reclaim fails closed (
queue_manager.rs)- Fix:
referenced_work_dirsreturnsResult.queue_list,history_listand per-rowhistory_get_retry_dataerrors are all propagated, and both the startup sweep andremove_unreferenced_work_dirskip all removal on error. - PoC: with an undecodable queue row, the queued job's work dir now survives both the sweep and single removal; round 1 deleted it.
- Fix:
clean_nzb_namepanic (sabnzbd_compat.rs)- Fix: the suffix is checked with
base.get(len-4..), so&base[..len-4]is only taken on a char boundary. - PoC:
Amélie→Amélie,Pokémon→Pokémon,Amélie.nzb→Amélie,é.NZB→é, anda€→a€. None panic.
- Fix: the suffix is checked with
Each fix has a regression test, and all three pass.
Also in 54e49e0
NntpError::Timeoutnow counts against the circuit breaker. This resolves my round-1 should-fix about servers that accept the connection and then black-hole reads. Article-level answers still don't count, and #157's per-article outage cap is unchanged.
Checks at this head
cargo test --workspace: 928 passed, 0 failed.cargo clippy --workspace --all-targets -- -D warnings: clean.cargo fmt --check: clean.ci/migration-policy.sh: passes.- All CI checks are green at this head.
Remaining non-blocking items
- Should-fix:
- A multi-NZB
addurlzip is decompressed twice. - The general-settings closure applies runtime changes before the TOML save.
- A multi-NZB
- Nits:
startup_sweep_keeps_everything_when_a_queue_row_is_undecodableshells out to an externalsqlite3binary. It passes on CI, but will fail whereversqlite3isn't installed.extract_nzbs_rejects_zip_entries_that_underdeclare_their_sizebuilds two unused archives (let _ = (zip, second)).- Limit errors always say "100 MB", even when a smaller test cap applies.
- From round 1: the desktop lockfile edges are re-pointed downward, there are no
Upstream-PR:trailers, and the bareFixes #69…#93lines point at unrelated items that are already closed. - 54e49e0 has no
Co-Authored-Bytrailer.
Not merged; that's for oversight. This verdict covers only this head SHA. A new head needs a fresh review.
Summary
Cherry-picks the xbmc4lyfe batch 2 fixes onto our main, keeping original authorship (
git cherry-pick -x). Head13932cb.Upstream was re-listed before opening: open issues are exactly #69 #71 #73 #75 #77 #79 #81 #83 #85 #87 #89 #91 #93 #95, and no newer fix PRs exist beyond #96. Older open PRs (#6 #8 #36 #49 #51 #56) were brought in by earlier batches.
Upstream map
30523abfetch_allowed_hostsinstead of theALLOW_LOOPBACK_FOR_TESTSseam102a3abstarted_atkepta5937726d05f7abd2b41e8a639b2login()helper56dee5387635fa5d505b2ebfb91c098bf57faebca6. Adapted: gating sits on top of ourMAX_OUTAGE_TRIES_PER_SERVERcap; connect-time failures still always count03a1f85b1536a2post_processing_levelandsanitize_job_namef8dac41claim_unique_output_dir13932cbcargo fmtplus removal of an unused import left by the archive portAlready covered, not cherry-picked
a0a2dcb, part of postproc/config/auth/web: upstream fixes from xbmc4lyfe (#4 #5 #7 #35 #47 #48 #50 #55 #63 #67) #158. Same files, same fix.af8e52e, then tightened byefcd5fe(the security-review follow-up). Not re-applied.Test plan
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace(all green)harness_article_errors,harness_speed_limit,harness_unique_output_dir,harness_incomplete_cleanupsabnzbd_compat::tests(70),api_contracts(11),workflow_fixtures(7),nzb-dispatchlib (59)Do not merge until the security review records "no blockers" against this head.