Land F-05/F-08/F-09/F-12 + SnapshotSettings field-drop onto main - #340
Merged
Merged
Conversation
Re-pins ext/Lidarr.Plugin.Common to RicherTunes/Lidarr.Plugin.Common@ed3c27b (current main HEAD) to converge the streaming trio onto one Common SHA and satisfy the Ecosystem Pin Drift guard. Supersedes the stale bump to 79d2140 (#317). Additive forward bump (manifest parsers, hostbridge size-estimator seam, MSBuild deps.json-race fix); commonVersion stays 1.18.0-dev. Both the gitlink and ext-common-sha.txt now equal ed3c27b8edfd2a125665daf7b7d754b3ecf5c341. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… parity discipline Adds a short pointer (not a restatement — consolidation applies to docs too) to ext/Lidarr.Plugin.Common/AGENTS.md 'Ecosystem Consolidation & Parity Discipline': find-once -> sweep-five across the 5 copy-paste-adjacent plugins, push shared logic into Common (plugins adopt it; the out-of-tree DRM seam stays plugin-owned+public), the parity matrix is a contract, verify architecture before assuming a bug class sweeps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oss-plugin field-drop sweep) Cross-plugin sweep of the SnapshotSettings field-drop bug class (found first in amazonmusicarr, where the host snapshot dropped EnableDrm and DRM never activated in background downloads). Tidal's inline snapshotter copied 8 of the settings' 10 read-write properties — it silently dropped SaveSyncedLyrics + UseLRCLIB, so the background download (which reads the pre-await snapshot) ignored the user's lyric settings and used defaults. The existing host->core parity test didn't catch it (it checks a different mapping, not the snapshotter). Extracted the snapshotter into an internal SnapshotSettings that reflection-copies EVERY read-write property — structurally cannot drop a field. Added a unit test pinning the full contract (a reflection sweep over all properties) + the two previously-dropped lyric fields. Sweep results: apple's SnapshotSettings already copies all 10 fields (and has a test) — not vulnerable; qobuz/brainarr have no such snapshot pattern. Candidate to consolidate into a Common helper later. Build 0-error; 187 download-area tests pass + 2 new snapshot tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review F-09 (MEDIUM): TidalDownloadClient's public write boundaries (DownloadTrackEnhancedAsync, DownloadTrackWithMetadataAsync) created directories and wrote/moved/deleted files based on a caller-provided outputPath without checking it stays inside the configured DownloadPath — a hostile-controlled path (e.g. via crafted filename/segment) could write outside the download root. Fix: add IsOutputPathAllowed (canonical-form containment via Common's PathTraversalGuard.IsPathWithinRoot — resolves "..", defends sibling-prefix + case-twin) and call EnsureOutputPathAllowed as the first statement in both methods, before any mkdir/write/move/delete/tag. The system temp dir is allowed as an alternate root because DownloadTrackAsync stages there before the host imports. Mirrors amazon's existing IsPathWithinRoot guards (parity). Sibling sweep of the "download write containment" class: amazon already guarded, qobuz cleanup is the separate F-10, apple is parallel-AI-owned, brainarr has no download client. Tests: TidalDownloadClientPathContainmentTests — 5 pure-helper cases + 2 wired integration tests (escape path refused with a download-path error, no file written). RED→GREEN; full download-client suite 99/99. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ck MemoryStream (F-08) Adversarial review F-08 (MEDIUM): DownloadTrackEnhancedAsync assembled the full track via DownloadAndAssembleAsync(manifest), which buffers the entire decrypted track in a MemoryStream (File.ReadAllBytes + in-memory decrypt), then copied that to the output FileStream — pinning a large hi-res track's full size on the managed heap and double-buffering (disk → memory → disk). Fix: switch to the already-existing file-backed path DownloadAndAssembleToFileStreamAsync, which assembles to a temp file (DeleteOnClose), decrypts in place on the file stream, and returns a seekable FileStream. The downstream header-validate + copy logic is unchanged (works on any seekable stream). Sequential (maxConcurrency 1) to preserve this path's historical ordering contract. The MemoryStream overload stays — it remains public API and is covered by the chunk downloader's own tests; only the enhanced client path moves off it. Tidal-specific class (amazon/qobuz already stream to disk; apple parallel-AI-owned). Verified: Enhanced + ChunkDownloader suites 69/69, full Tidalarr.Tests 1315/1332 (17 pre-existing skips), 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…numbers (F-12) Identify allowlist entries by file + pattern + method instead of drift-prone line numbers (the linter matches by file/pattern and reports the accurate current line at scan time — the JSON 'line' field was redundant + stale, e.g. TidalLidarrIndexer listed 506 vs actual 596). Part of the cross-plugin F-12 sweep. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review F-05 (HIGH): re-pin tidal's ext/Lidarr.Plugin.Common submodule +
ext-common-sha.txt from ed3c27b to 961dfaea60710f61f885f101f3c233baf7a650cb
(origin/main after security PR #618 merged). commonVersion stays 1.18.0-dev (matches
merged Common — truthful).
Adoption fix for F-01's strict-by-default SSRF guard: the merged Common makes
ChunkedHttpAssembler SSRF-strict by default. TidalChunkDownloader now constructs it
with RemoteMediaUriPolicy { AllowHttp = true, ResolveDns = false } — the guard's value
for Tidal is blocking literal private/loopback/CGNAT/metadata-host targets (the real
SSRF vectors). AllowHttp preserves the pre-#618 behaviour (Tidal manifests can serve
http segment URLs; the assembler had no scheme restriction before), and ResolveDns is
skipped on the hot path (manifest source is authenticated/trusted). The production
orchestrator uses the stream-provider path, which doesn't hit the URL guard, so no
change needed there.
Verified against merged Common: download/chunk/orchestrator 160/160, full Tidalarr.Tests
1315/1332 (17 pre-existing skips), 0 failures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings the adversarial-review fixes carried on chore/converge-common-ed3c27b onto main: - F-05: Common pin converged to 961dfae (PR #618 security batch) - F-08: enhanced track assembly streams to file (no whole-track MemoryStream) - F-09: download output paths root-contained (PathTraversalGuard) - F-12: sync-over-async allowlist made semantic (drop stale line numbers) - SnapshotSettings field-drop sweep (SaveSyncedLyrics + UseLRCLIB) Resolves ext-common-sha.txt to 961dfae (newest Common; submodule gitlink fast-forwards to match). # Conflicts: # ext-common-sha.txt
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+398
to
+404
| foreach (var p in typeof(TidalLidarrDownloadClientSettings).GetProperties()) | ||
| { | ||
| if (p.CanRead && p.CanWrite && p.GetIndexParameters().Length == 0) | ||
| { | ||
| p.SetValue(snapshot, p.GetValue(live)); | ||
| } | ||
| } |
… (de-flake) DownloadAndAssembleToFileStreamAsync_WithProgress_ReportsProgress used System.Progress<T>, whose callbacks marshal asynchronously with no ordering guarantee, so progressReports[0] could observe the count-2 report before count-1 (Assert.Equal Expected:1 Actual:2 — flaked in CI's Test-with-Coverage, passed locally). Switched to the file's existing SyncProgress<T> (synchronous, inline) which the other ordered-assert tests already use, making order + count deterministic. Pre-existing flake; not introduced by the F-05/F-08 work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 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.
Lands the adversarial-review fixes that were validated on
chore/converge-common-ed3c27bonto main (squash-merge for linear history).ext-common-sha.txt resolved to 961dfae; submodule gitlink fast-forwards to match. Full Tidalarr.Tests green locally (1321 passed, 17 skipped, 0 failed).
🤖 Generated with Claude Code