feat: Blossom BUD-01/03/04/06/11 compliance, fallback retrieval, and security hardening#383
Open
advorzhak wants to merge 11 commits into
Open
feat: Blossom BUD-01/03/04/06/11 compliance, fallback retrieval, and security hardening#383advorzhak wants to merge 11 commits into
advorzhak wants to merge 11 commits into
Conversation
…security hardening - BUD-01: Update blossomPathRegex to accept optional file extensions. - BUD-03: Anchor SHA-256 regex to end of path to prevent hash confusion. - BUD-03: Add BlossomFallbackFetcher with strict SHA-256 verification. - BUD-04: Add concurrent background mirroring (mirrorBlob). - BUD-06: Add pre-flight HEAD check to avoid redundant uploads. - BUD-11: Migrate to strict Base64URL encoding for auth headers. - Security: Add strict host validation to prevent SSRF in mirror payloads. - Security: Only send auth signature on 401 challenge in fallback fetcher. - Tests: Add BlossomEncodingTests, ContentParserHashTests, BlossomClientTests. Closes barrydeen#382
Loosen the URL sanitization used when picking the mirror payload URL so that CDN-enabled servers can deliver via their CDN subdomain (e.g., an upload to blossom.azzamo.media returning https://cdn.azzamo.media/<hash>) instead of forcing every mirror server to fetch from the rate-limited blossom endpoint. The check now accepts the server's own response URL when either: - the response host equals the upload host, or - the response host shares the same registrable domain (last-2 label match) with the upload host (sibling subdomains). mirrorBlob itself remains hardened: it still only targets the user's explicitly configured kind-10063 server list, and the mirror server is expected to independently validate the fetched blob hash against the authorized x tag per BUD-04. Adds unit tests for both helpers.
- CRITICAL: HEAD check now uses correct 'get' auth action (BUD-11 compliance) - CRITICAL: Wire authorPubkey through ContentParser to reach fallback fetcher - WARNING: Add legacy Base64 fallback for older Blossom servers (BUD-11 backward compat) - WARNING: Lazy-generate uploadAuth only when needed (saves remote-signer roundtrip) - WARNING: Reduce HEAD timeout from 15s to 5s (non-fatal pre-flight check) - WARNING: Use canonical AnimatedImageDecoder.decodeStatic for fallback decode - Remove trailing whitespace from all modified files The BUD-03 fallback retrieval path is now reachable through: - RichContentView passes authorPubkey to ContentParser.parse - InlineImageView passes meta.authorPubkey to RetryingAsyncImage - ArticleCardView passes event.pubkey to RetryingAsyncImage - MediaLookaheadPrefetcher passes event.pubkey to ContentParser.parse
Critical fixes: - Mirror blob auth: correctly detect and handle auth rejections (401/404/405) with proper legacy Base64 fallback and retry logic (BUD-11 compliance) - Fallback fetcher: add cooldown cache to prevent rapid retry storms - Upload flow: properly handle legacy Base64 fallback in all code paths Performance improvements: - Use cached server list for fallback fetches (avoid N+1 relay queries) - Parallelize HEAD pre-flight checks across all servers Code quality: - Restore doc comments on ContentParser and RetryingAsyncImage - Add comprehensive tests for legacy Base64 conversion All tests passing. No breaking changes.
CRITICAL FIXES (4 HIGH severity): - Fix barrydeen#1: Change mirror action from 'mirror' to 'upload' (BUD-11 spec compliance) - Fix barrydeen#2: Clarify BUD-06 comment to BUD-01 blob-existence HEAD (accurate spec reference) - Fix barrydeen#4: Parallelize HEAD pre-flight checks with withTaskGroup (5s → N×5s latency fix) - Fix barrydeen#5: Add legacy Base64 fallback to headOnce (fix lost optimization on old servers) MEDIUM SEVERITY FIXES (9 total): - Fix barrydeen#6: Remove auth from fallback fetcher (privacy: prevent deanonymization) - Fix barrydeen#7: Make makeAuthHeader nonisolated (fix MainActor serialization bottleneck) - Fix barrydeen#9: Propagate network errors properly in fetchOnce (use specific URLError cases) - Fix barrydeen#10: Cache converted legacy auth per server (avoid per-iteration conversions) - Fix barrydeen#11: Simplify uploadAuth generation (eliminate redundant guard) - Fix barrydeen#12: Extract withLegacyFallback helper (single source of truth for retry logic) - Fix barrydeen#13: Make mirrorPUT return status code (retry strictly on 401, not all non-2xx) ARCHITECTURE IMPROVEMENTS: - Shared withLegacyFallback<T> helper eliminates 3 duplicate retry patterns (BUD-11 legacy fallback) - Parallel HEAD checks reduce upload latency from O(N×5s) to O(1×5s) in best case - Privacy-safe fallback: no auth sent to author-controlled servers (prevents deanonymization) - Cleaner error propagation: specific URLError cases instead of generic badServerResponse SPEC COMPLIANCE: - BUD-01: Blob existence check (not BUD-06) - BUD-11: Mirror endpoint uses 'upload' action tag (not custom 'mirror') - BUD-04: CDN sibling subdomain support preserved (areSameRegistrableDomain intact) All tests pass. No breaking changes. Backward compatible with BUD-11 legacy servers.
Author
|
@claude review |
Author
|
Hi, @barrydeen Could you give me some guidance what needs to be added to testing section of PR to be acceptable for this PR to get reviewed? |
HIGH PRIORITY (4 issues): - Fix barrydeen#1: Document BlossomFallbackFetcher limitation - clarifies that only the current user's server list is cached, falls back to primal.net for other authors - Fix barrydeen#3: Bound NSCache in BlossomFallbackFetcher - limit to 512 entries to prevent unbounded memory growth - Fix barrydeen#4: Improve areSameRegistrableDomain documentation - clarify limitation with multi-level TLDs - Fix barrydeen#5: Fix normalizeServerURL to strip all trailing slashes, not just one MEDIUM PRIORITY (3 issues): - Fix barrydeen#6: Add documentation for fire-and-forget mirror task lifecycle - Fix barrydeen#8: Add JSON validation to convertToLegacyBase64Auth - Fix barrydeen#9: Improve error handling in withLegacyFallback LOW PRIORITY (5 issues): - Fix barrydeen#7: Add privacy annotations to os_log calls - Fix barrydeen#10: Add comprehensive test coverage for convertToLegacyBase64Auth - Fix barrydeen#11: Document sha256Hash(fromUrl:) visibility change - Fix barrydeen#12: Clarify purpose of two Blossom path regexes All tests pass. No breaking changes.
Critical (bugs with user-visible impact): - C-1: Move legacyBase64 flag inside for-server loop so BUD-11 servers that follow a pre-BUD-11 server still receive a BUD-11 attempt - C-2: Key fallback cooldown cache by (hash|authorPubkey) so different authors with different server lists get independent cooldowns; record the timestamp only on failure, not unconditionally upfront High: - H-1: Simplify BlossomError.authRejected — payload was always 401 - H-2: Enforce HTTPS in normalizeServerURL (returns nil for http://) - H-3: Descope authorPubkey from ContentParser/MediaMeta; pass through InlineImageView.authorPubkey instead (parser stays pure, view layer owns identity context) Medium: - M-1: Hoist legacyAuthPair to function scope — convert once per upload call rather than once per server, correcting the misleading comment - M-2: Update withLegacyFallback doc to reflect dual-implementation reality and document the required error contract for callers - M-3: Remove dead guard !servers.isEmpty (BlossomServerList.cached always returns ≥1 entry) - M-4: Parallelize three makeAuthHeader calls with async let - M-5: Cap BlossomFallbackFetcher concurrency to mirrorMaxConcurrent (3) - M-6: Keep BlossomFallbackFetcher/Data+Base64URL in wisp/ (auto-synced via PBXFileSystemSynchronizedRootGroup; pbxproj edit not required) Low / informational: - Remove all (Fix #N) development-round markers from production code - Replace serverAttempt:while true with clearer for-triedLegacy pattern - Document error contract on withLegacyFallback<T> - Drop .fragmentsAllowed from JSONSerialization (unnecessary after cast) Tests (T-1..T-4): - T-1: Fix padding test to assert '=' char count, not just roundtrip - T-2: Add tests for withLegacyFallback error contract, areSameRegistrable domain symmetry, sanitizeMirrorURL empty-uploadHost, IPv6/IP behaviour - T-3: Add normalizeServerURL HTTPS rejection and fragment/query tests - T-4: Add sha256Hash tests for port numbers, URL fragments, invalid URL All tests pass (** TEST SUCCEEDED **)
… error paths Addresses HIGH findings from frontier review: - barrydeen#3: BlossomFallbackFetcher now has URLProtocol-backed test coverage (SHA-256 verification, 401/500 handling, default-server fallback, no-hash URL rejection) - barrydeen#4: upload() error paths tested (empty server list, HTTP-only servers) - barrydeen#5: makeAuthHeader tag structure verified (kind=24242, correct t/x/ expiration tags, Base64URL encoding, nil returns for invalid keys) - barrydeen#11: Case-insensitive hash extraction test added Test infrastructure: - Added nonisolated(unsafe) static URLSession override to BlossomClient and BlossomFallbackFetcher for URLProtocol injection - MockURLProtocol handles HTTP response simulation without network - @MainActor/@suite(.serialized) on new test suite per project default actor isolation setting New tests: 10 total - BlossomFallbackFetcherTests: 6 (hash match, hash mismatch, 401 no-auth, 500 server error, default server fallback, no-hash URL) - BlossomClientTests: 4 (auth header tag structure for all actions, invalid privkey nil, Base64URL encoding verification, upload error paths) - ContentParserHashTests: 1 (case-insensitive hex extraction)
Critical fixes: - BlossomFallbackFetcher: HTTP fallback bug — normalizeServerURL now returns nil for non-HTTPS servers, and servers are properly skipped (no more falling back to unencrypted HTTP with ?? server) - Cooldown cache scoped to network-level failures only; SHA-256 hash mismatches do not suppress retries on other servers (data integrity issues signal corrupt copy, not a network failure) - normalizeServerURL output is now lowercased for consistent string comparison in mirror targets and other downstream callers Security & design: - FullScreenImageView: custom init now accepts authorPubkey parameter (previously only synthesized memberwise init had it, causing call-site mismatch when adding authorPubkey to MediaGridView) - BloomonClient/BlossomFallbackFetcher session test seams wrapped in #if DEBUG and exposed via sessionOverride pattern (computed property that falls back to .shared in production) - fetchOncePublic: added assert() enforcing no Authorization header (structural privacy invariant enforcement) BUD-03 authorPubkey propagation (all display surfaces): - InlineImageView: fullscreen viewer (imageContent) now passes authorPubkey - ArticleView: header cover + inline images pass article.pubkey - ProfileTabs: GalleryTile passes event.pubkey; MediaTile passes item.authorPubkey (via ProfileViewModel.MediaItem model extension) - MusicTrackCardView: artwork() passes author - DraftsScheduledView: thumbnailRow() passes authorPubkey - MediaGridView: MediaItem model extended with publisherPubkey; passes to RetryingAsyncImage (tile + poster) and FullScreenImageView (pager) - RichContentView: mediaItem(from:) passes self.authorPubkey - ProfileViewModel: MediaItem model extended; construction passes event.pubkey Test additions: - BlossomClientTests: legacyBase64RetryPathViaUpload test exercises the withLegacyFallback dispatch mechanism through the upload path — verifies (1) 401 triggers retry, (2) retry uses standard Base64 not Base64URL, (3) upload succeeds after legacy retry - BlossomClientTests: added @mainactor @suite(.serialized) to prevent race condition with shared MockURLProtocol state - BlossomFallbackFetcherTests: each test now resets MockURLProtocol.handlers and sessionOverride in defer blocks for proper isolation All tests pass (222 unit tests, 0 failures)
Resolves 9 findings from the final 6-track review (security, business logic, performance, dead code, deploy safety, duplication). Highlights: - AsyncSemaphore rewritten with CheckedContinuation parking instead of busy-wait Task.yield() loop (no more cooperative-thread cycle burn during concurrent media scroll). - BlossomFallbackFetcher cooldown race fixed: removed the in-flight Set that caused concurrent image cells to permanently show failure placeholders; URLSession URLCache now coalesces identical concurrent requests. - IntegrityTracker.recordFailure() now invoked on network-level failures (was dead code); mixed integrity/network failures correctly record cooldown. - HEAD legacy retry switched to two-axis strategy: kind 24242 + standard Base64 for pre-BUD-03/pre-BUD-11 servers (previously only kind changed, Base64URL still rejected). - makeAuthHeader unified with optional kind and AuthHeaderEncoding parameters (Base64URL default, .standardBase64 for legacy). - normalizeServerURL preserves path component so users with path-prefixed kind-10063 entries keep working; strips query/fragment only. - BlossomFallbackImage helper collapsed to a single generic fetchDecodeAndCache<T> (was two near-identical static/animated variants). - Fullscreen media pager now forwards NIP-92 posterUrl/blurhash/sha256 via MediaItem to InlineVideoView so pager videos don't regress to black placeholders. - Imeta mirror dedup decoupled from isBlossomUrl guard so non-Blossom mirrors with matching 64-hex paths dedup correctly. - Shared MockURLProtocol helper extracted to wispTests/MockURLProtocol.swift; inline copies removed from both test suites. - BlossomClient.sessionOverride made thread-safe with NSLock (was a plain static var read by URLSession's delegate queue). Tests: all wispTests + wispUITests pass on iPhone Air simulator.
Concurrency (H-4/5/6, M-2):
AsyncSemaphore.acquire() is now cancellation-aware via
withTaskCancellationHandler; cancelled waiters are removed from the
queue so release() never resumes a dead task. chunkedFirstSuccess and
chunkedFanOut release the semaphore inline (await release()) instead
of via defer { Task { await release() } } to avoid unstructured
continuation leaks. HEAD precheck in upload() now caps concurrency
via chunkedFirstSuccess.
Security (H-1/2/3, M-12):
sanitizeMirrorURL now rejects non-HTTPS server-returned URLs.
convertToLegacyBase64Auth recomputes the canonical NIP-01 event id
and rejects tampered payloads. BlossomServerList.parseServers
validates relay-sourced URLs through normalizeServerURL.
BlossomServerList ships multi-default community servers
(primal.net, nostr.build, cdn.nostrcheck.me); BlossomFallbackFetcher
skips fallback entirely for authors with no stored kind-10063 list.
Privacy / correctness (H-8, M-8, M-9, H-1):
Fallback fetcher uses a dedicated no-redirect URLSession so 301/302
cannot silently switch hosts. In-flight await-registry dedupes
concurrent fetches for the same (hash, author) under a single
network request. areSameRegistrableDomain ships a hardcoded
multi-level TLD set (.co.uk/.com.au/.ac.uk/etc) for correct
registrable-domain comparison. normalizeServerURL collapses
double-slash path runs.
Robustness (M-1, M-5/6/7, M-13, L-1/2):
BlossomFallbackFetcher.sessionOverride is now NSLock-guarded. NSCache
cooldown replaced by plain locked dictionary with manual eviction.
activeMirrors now stores insertion timestamps and prunes stale
entries on each lookup. Mirror/fetcher os_log .fault downgraded
to .error (server failures are not programming bugs).
Cleanup (H-7, L-4, L-6-L-10):
Deleted unused makeAuthHeaderWithKind. isBlossomUrl now enforces
HTTP/HTTPS scheme so WebSocket URLs are not misclassified. Added
9 tests: semaphore cancel safety, double-slash, HTTPS enforcement,
multi-level TLD (.co.uk/.com.au), id tamper detection, WebSocket
classification, unknown-author gate, in-flight dedup sharing.
All wispTests + wispUITests pass on iPhone Air simulator.
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
1 participant
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.
feat: Blossom BUD-01/03/04/06/11 compliance, fallback retrieval, and security hardening
Closes #382
What This PR Does
Implements five Blossom Upgrade Document (BUD) specifications and addresses every finding from multiple rounds of code review (critical, medium, low, and informational). The result is a production-grade, security-hardened, CDN-aware Blossom media stack.
16 files changed · 1,440 lines · 10 commits · 80+ tests · All passing
BUD Specifications
blossomPathRegexextended; dual regex design documented;normalizeServerURLpreserves user-configured path prefixesBlossomFallbackFetcherwith SHA-256 verification, cooldown cache, parallel fan-out viaAsyncSemaphoremirrorBlobwithBlossomClient.chunkedFanOut(continuation-based parallel throttle at 3)makeAuthHeaderwith optionalkindandAuthHeaderEncodingparameters; automatic legacy-Base64 fallback on 401Architecture
authorPubkeythreading — scoped to the view layer only.ContentParserandMediaMetaremain pure content→segments functions with no identity context.authorPubkeyflows:RichContentView→InlineImageView→RetryingAsyncImage/AnimatedImageView→BlossomFallbackFetcher→BlossomFallbackImage.fetchDecodeAndCache<T>. Threaded to every image/video media surface across the app.Shared
BlossomClient.chunkedFirstSuccess/chunkedFanOut— single helper used by bothmirrorBlobandBlossomFallbackFetcher.fetch. Backs ontoAsyncSemaphore(actor withCheckedContinuationparking — no busy-wait) for the concurrency cap. Replaces prior sequential chunking so first-success latency is bounded bytimeoutIntervalregardless of total server count.Unified
makeAuthHeader— single function with optionalkind: Int?andencoding: AuthHeaderEncodingparameters. BUD-03 GETs default to kind 24243 + Base64URL; legacy retry paths passkind: 24242, encoding: .standardBase64. Removes the previous near-duplicatemakeAuthHeaderWithKind.BlossomFallbackImage.fetchDecodeAndCache<T>— single generic helper that consolidates fetch → off-main decode → cache write for both static (UIImage) and animated (AnimatedImagePayload) paths. Both image views now share identical fallback semantics.AsyncSemaphore— actor-based semaphore withCheckedContinuationqueue. Replaces a busy-waitTask.yield()loop that was burning cooperative-thread-pool cycles during concurrent media scroll.normalizeServerURL— preserves path component (backward-compat for users with path-prefixed kind-10063 entries likehttps://blossom.example.com/custom/), strips query/fragment only, lowercases host, brackets IPv6, preserves non-standard ports.BlossomFallbackFetchercooldown — keyed by(hash, authorPubkey), recorded only on network-level failures (4xx/5xx/timeout). SHA-256 integrity mismatches don't record cooldown — a corrupt copy at one server doesn't mean others are corrupt.IntegrityTrackerdistinguishes the two. In-flight de-dup removed (was causing concurrent image cells to permanently show failure placeholders); URLSession's URLCache coalesces identical concurrent requests.MediaGridView.MediaItem— extended withposterUrl,blurhash,sha256so the fullscreen pager can reconstruct video posters/blurhash placeholders without dropping metadata.ContentParser imeta dedup — hash extraction for imeta dedup no longer gated by
isBlossomUrl, so non-Blossom mirrors with matching 64-hex paths correctly dedup. Link-dedup pass keeps the guard (avoids false positives in note text from blog posts that end a path in a hex token).Security
normalizeServerURLreturnsnilfor non-HTTPS servers → fallback fetcher skips them entirely (no cleartext leaks)normalizeServerURL(server) ?? serverwas a bug that silently fell back to HTTP; now usesguard let ... else continuesanitizeMirrorURLrejects server-returned URLs unless the host matches the upload host or a CDN sibling on the same registrable domainassert()infetchOncePublicverifies noAuthorizationheader is set on the requestconvertToLegacyBase64Authvalidateskind == 24242 || 24243before re-encoding#if DEBUG; lock-protected thread-safe getter/setterPerformance
AsyncSemaphore: waits suspend viaCheckedContinuationinstead of spinning withTask.yield()mirrorBlobandBlossomFallbackFetcher.fetchboth usechunkedFirstSuccess/chunkedFanOut; first success latency is independent of total target countgetAuth,mediaAuth,uploadAuthgenerated concurrently withasync letBlossomClient.maxConcurrentOperations = 3shared by uploads, mirrors, and fallback fetchesTest Infrastructure
MockURLProtocol: extracted towispTests/MockURLProtocol.swiftwith lock-protected handler map. Inline copies removed from both test suites.AsyncSemaphoretest not needed — exercised indirectly via fallback fetcher tests with >3 servers.BlossomClient.sessionOverrideis now thread-safe withNSLock(was a plain static var read by URLSession's delegate queue from background threads while tests set it from the main actor).Tests
80+ tests across four test suites (500+ total including existing suites, all passing):
BlossomClientTests (~50 tests):
normalizeServerURL: trailing slashes, HTTPS enforcement, HTTP rejection, lowercase output, IPv6 brackets, non-standard ports, path preservationareSameRegistrableDomain: siblings, nested, identical, different, case-insensitive, IP addresses, IPv6 exact-match, symmetrysanitizeMirrorURL: same host, CDN sibling, different domain, malformed, empty uploadHostconvertToLegacyBase64Auth: roundtrip, padding, Base64URL→Base64 swap, JSON kind validation (24242 || 24243)makeAuthHeader: tag structure, kind=24242 or 24243 by action, Base64URL encoding, nil for invalid privkeylegacyBase64RetryPathViaUpload: exerciseswithLegacyFallbackdispatch — verifies 401 triggers retry, retry uses standard Base64, upload succeeds after legacy retryBlossomFallbackFetcherTests (~15 tests):
fetchChunksServersBeyondConcurrencyCap: server beyondmaxConcurrentFetchesstill tried (parallel fan-out, not sequential chunking)BlossomEncodingTests (~5 tests): padding, special chars, empty data, URL-safe chars
ContentParserHashTests (~15 tests): path end, no extension, query params, mid-path, non-hex, trailing slash, port, fragment, invalid URL, case-insensitive hex, non-Blossom imeta mirror dedup
Commits
1c4280d3ea652d942e75524b872e383022ebceb175496cf72ba3d490fa111191d5cf26