Fix NIP-05 redirect SSRF and archive-transition draft loss#492
Conversation
|
Ready to review this PR? Stage has broken it down into 2 individual chapters for you:
Chapters generated by Stage for commit 914614e on Jul 13, 2026 12:22pm UTC. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesNIP-05 redirect validation
Chat list state preservation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
whitenoise-mac/Core/NIP05Resolver.swift (1)
81-101: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo redirect hop-count cap, despite the doc comment claiming parity with
CappedImageDownloadDelegate.The comment says this "Mirrors the avatar path's
CappedImageDownloadDelegate," but that delegate also caps redirect chains at 5 hops (RemoteImageLoader.swift), whileNIP05RedirectPolicyonly re-validates the target host on each hop and will follow an unbounded number of allowed redirects. Impact is bounded today bytimeoutIntervalForRequest/timeoutIntervalForResourceon the session, so this isn't exploitable beyond a resource-timeout-bounded delay, but it's a real gap versus the stated intent and the sibling implementation.🔒 Proposed fix to mirror the hop-count cap
final class NIP05RedirectPolicy: NSObject, URLSessionTaskDelegate { + private let lock = NSLock() + private var redirectHopCount = 0 + func urlSession( _ session: URLSession, task: URLSessionTask, willPerformHTTPRedirection response: HTTPURLResponse, newRequest request: URLRequest, completionHandler: `@escaping` (URLRequest?) -> Void ) { + let hopCount = lock.withLock { + redirectHopCount += 1 + return redirectHopCount + } + if hopCount > 5 { + completionHandler(nil) + task.cancel() + return + } guard let url = request.url, RemoteImageURLPolicy.isAllowed(url) else { completionHandler(nil) task.cancel() return } completionHandler(request) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@whitenoise-mac/Core/NIP05Resolver.swift` around lines 81 - 101, Update NIP05RedirectPolicy’s urlSession(_:task:willPerformHTTPRedirection:newRequest:completionHandler:) to track redirect hops and stop following redirects after the same five-hop cap used by CappedImageDownloadDelegate in RemoteImageLoader.swift. Cancel or reject requests beyond the limit while preserving the existing RemoteImageURLPolicy.isAllowed validation for each permitted target.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@whitenoise-mac/Core/NIP05Resolver.swift`:
- Around line 81-101: Update NIP05RedirectPolicy’s
urlSession(_:task:willPerformHTTPRedirection:newRequest:completionHandler:) to
track redirect hops and stop following redirects after the same five-hop cap
used by CappedImageDownloadDelegate in RemoteImageLoader.swift. Cancel or reject
requests beyond the limit while preserving the existing
RemoteImageURLPolicy.isAllowed validation for each permitted target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eabe09da-4013-4f41-badc-feda814ba04e
📒 Files selected for processing (3)
whitenoise-mac/Core/NIP05Resolver.swiftwhitenoise-mac/Core/WorkspaceState+ChatList.swiftwhitenoise-macTests/whitenoise_macTests.swift
1 participant
Two self-contained fixes, each with a unit test — no on-device verification needed.
NIP05Resolvervalidated only the initial.well-known/nostr.jsonURL against the SSRF host policy; itsURLSessionhad no redirect delegate, so a3xxfrom the well-known host was auto-followed to any host — including loopback/private/link-local addresses the up-front guard blocks — turning a contact lookup into a blind internal probe. AddedNIP05RedirectPolicy(aURLSessionTaskDelegate) that re-runsRemoteImageURLPolicy.isAllowedon every redirect target and cancels the task on a disallowed one, mirroring the avatar path'sCappedImageDownloadDelegate. Test drives the delegate directly: loopback, link-local, private, IPv6-loopback, and scheme-downgrade targets are blocked; a publichttpstarget is followed unchanged.applyChatRowscomputed "removed" chats as the union of per-list removals, so a chat that merely moved active↔archived (absent from one "next" list, present in the other) was wrongly treated as removed and had its composer draft cleared and group-member cache invalidated. Now the removed set is(previousActive ∪ previousArchived) − (nextActive ∪ nextArchived), so only truly-gone chats are cleared. Test archives a chat holding a draft through the snapshot reload and asserts the draft survives.Closes #448
Closes #466
CI note: this branch is cut from
master, which is currently red on two pre-existing tests unrelated to this change —messageMediaDiskCacheReadDeletionMaintainsTrackedFootprint(an obsolete assertion from the #444 plaintext-hash isolation change, fixed separately) and the flakyworkspaceCoalescesAndCachesSourceEpochZeroMediaReferenceResolution. Both fail onmasteritself; the two new tests here pass and this change adds no new failures.Summary by CodeRabbit
Bug Fixes
Tests