diff --git a/BookPlayer/AppDelegate.swift b/BookPlayer/AppDelegate.swift index 770d6d25b..3428fa247 100644 --- a/BookPlayer/AppDelegate.swift +++ b/BookPlayer/AppDelegate.swift @@ -351,9 +351,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate, BPLogger { // the raw query back under `http.query`, on both network breadcrumbs and `http.client` spans. // Only the userinfo portion is redacted for us. That leaks real secrets from hosts we don't // control: AudiobookShelf's OIDC exchange has to carry `code` and `code_verifier` in the query - // (its endpoint is GET-only), and Jellyfin's download URLs still carry `api_key`. Either one - // would be uploaded verbatim alongside the user's self-hosted hostname on the next captured - // event. + // (its endpoint is GET-only). Both would be uploaded verbatim alongside the user's self-hosted + // hostname on the next captured event. // // Our own backend authenticates with a bearer header and puts nothing sensitive in a query, so // its query strings are kept — that's where this data actually helps debugging. diff --git a/BookPlayer/Jellyfin/Network/JellyfinConnectionService.swift b/BookPlayer/Jellyfin/Network/JellyfinConnectionService.swift index f99a54aaf..032182834 100644 --- a/BookPlayer/Jellyfin/Network/JellyfinConnectionService.swift +++ b/BookPlayer/Jellyfin/Network/JellyfinConnectionService.swift @@ -723,17 +723,12 @@ class JellyfinConnectionService: BPLogger { return client } - func createItemDownloadUrl(_ item: JellyfinLibraryItem) throws -> URL { - guard let client else { - throw IntegrationError.noClient("Jellyfin") - } - + /// Kept private because the returned URL is *not* self-authenticating — handing it + /// straight to `URLSession`/`AVURLAsset` would 401. Go through + /// `createItemDownloadRequest(_:)`, which attaches the token. + private func createItemDownloadUrl(_ item: JellyfinLibraryItem) throws -> URL { let request = Paths.getDownload(itemID: item.id) - var components = try createUrlComponentsForApiRequest(request) - - var queryItems = components.queryItems ?? [] - queryItems.append(URLQueryItem(name: "api_key", value: client.accessToken)) - components.queryItems = queryItems + let components = try createUrlComponentsForApiRequest(request) guard let url = components.url else { throw IntegrationError.urlFromComponents(components) @@ -742,11 +737,19 @@ class JellyfinConnectionService: BPLogger { return url } - /// Returns a URLRequest for downloading a library item, carrying the user-defined - /// custom HTTP headers (needed for servers behind Cloudflare Access etc.). + /// Returns a URLRequest for downloading a library item, carrying the access token plus the + /// user-defined custom HTTP headers (needed for servers behind Cloudflare Access etc.). + /// + /// The token goes in the `Authorization` header, not in an `?api_key=` query item: Jellyfin 12 + /// servers can reject the query token with a 401, and a token in the URL leaks into the download task's + /// persisted `taskDescription`. It is set after the custom headers, so they can't clobber it. func createItemDownloadRequest(_ item: JellyfinLibraryItem) throws -> URLRequest { let url = try createItemDownloadUrl(item) - return wrapWithCustomHeaders(url) + var request = wrapWithCustomHeaders(url) + if let accessToken = client?.accessToken { + request.setValue("MediaBrowser Token=\"\(accessToken)\"", forHTTPHeaderField: "Authorization") + } + return request } /// Wraps an arbitrary URL (e.g. a cover image) in a URLRequest carrying the current diff --git a/BookPlayerTests/MediaServerIntegration/JellyfinQuickConnectTests.swift b/BookPlayerTests/MediaServerIntegration/JellyfinQuickConnectTests.swift index c4f42df80..b41589fb0 100644 --- a/BookPlayerTests/MediaServerIntegration/JellyfinQuickConnectTests.swift +++ b/BookPlayerTests/MediaServerIntegration/JellyfinQuickConnectTests.swift @@ -169,3 +169,52 @@ final class JellyfinQuickConnectTests: XCTestCase { } } } + +/// Covers the download request, which is built by hand instead of going through `JellyfinClient` +/// and so gets none of the client's automatic auth. Jellyfin 12 servers can reject the `?api_key=` query +/// token with a 401 and accept only the token in the `Authorization` header (#1598). +@MainActor +final class JellyfinDownloadRequestTests: XCTestCase { + private var sut: JellyfinConnectionService! + + override func setUp() { + super.setUp() + sut = JellyfinConnectionService(keychainService: KeychainServiceMock()) + sut.client = JellyfinClient( + configuration: JellyfinClient.Configuration( + url: URL(string: "https://jellyfin.example.com/jellyfin")!, + client: "BookPlayer", + deviceName: "Test Device", + deviceID: "test-device", + version: "1" + ), + accessToken: "s3cr3t" + ) + } + + override func tearDown() { + sut = nil + super.tearDown() + } + + private let audiobook = JellyfinLibraryItem(id: "item-1", name: "Book", kind: .audiobook) + + func testDownloadRequestCarriesTheTokenInTheAuthorizationHeader() throws { + let request = try sut.createItemDownloadRequest(audiobook) + + XCTAssertEqual( + request.value(forHTTPHeaderField: "Authorization"), + "MediaBrowser Token=\"s3cr3t\"" + ) + } + + /// The URL ends up in the download task's persisted `taskDescription`, so it must not carry the token. + func testDownloadURLDoesNotCarryTheToken() throws { + let request = try sut.createItemDownloadRequest(audiobook) + let url = try XCTUnwrap(request.url) + + XCTAssertEqual(url.path, "/jellyfin/Items/item-1/Download") + XCTAssertFalse(url.absoluteString.contains("s3cr3t")) + XCTAssertFalse(url.absoluteString.contains("api_key")) + } +}