diff --git a/lib/src/auth/solid_oidc_manager_factory.dart b/lib/src/auth/solid_oidc_manager_factory.dart index 582a639..4da730b 100644 --- a/lib/src/auth/solid_oidc_manager_factory.dart +++ b/lib/src/auth/solid_oidc_manager_factory.dart @@ -134,31 +134,49 @@ abstract class SolidOidcManagerFactory { return Future.value(hookRequest); }, - - // 20260920 gjw Report what the token endpoint actually objected to. - // OidcUserManagerBase wraps any failure here in its RFC 9207 "the - // authorization error response is missing the `iss` parameter" message - // and discards the original. An error from the TOKEN endpoint never - // carries `iss`, so every token failure — invalid_grant, an - // unacceptable DPoP proof, a rejected client — reaches the app looking - // like a mix-up attack. This hook runs first, while the server's own - // error code is still attached, which is the difference between a - // diagnosable failure and a guess. + // Report what the token endpoint actually said. + // + // OidcUserManagerBase.tryGetAuthResponse() runs the code exchange + // INSIDE the try block that guards the authorization response, and its + // catch applies the RFC 9207 mix-up defence to any OidcException that + // carries an errorResponse. A token-endpoint error body carries one — + // and never carries `iss`, which is an authorization-response + // parameter — so a plain `invalid_grant` comes back to the caller as + // + // The authorization server advertises + // `authorization_response_iss_parameter_supported` but the + // authorization error response is missing the `iss` parameter + // (RFC 9207 §2.4); refusing as a possible mix-up attack. + // + // which names neither the endpoint that failed nor the reason. Rethrow + // the code-exchange failure without an errorResponse so that catch + // rethrows it untouched, with the server's own error in the message. + // + // Only the authorization_code grant is unwrapped. refresh_token errors + // are left exactly as they are, because oidc_core reads their + // errorResponse to decide whether a refresh failure means re-login. modifyExecution: (hookRequest, defaultExecution) async { try { return await defaultExecution(hookRequest); - } on OidcException catch (e) { - final response = e.errorResponse; - + } on OidcException catch (e, st) { + final errorResponse = e.errorResponse; + if (errorResponse == null || + hookRequest.request.grantType != + OidcConstants_GrantType.authorizationCode) { + rethrow; + } + final description = errorResponse.errorDescription; + final detail = description == null ? '' : ': $description'; _log.severe( - 'Token endpoint rejected the request: ' - 'error=${response?.error ?? '(none)'} ' - 'description=${response?.errorDescription ?? '(none)'} ' - 'message=${e.message} ' - 'clock_offset=${ServerClock.offset.inSeconds}s', + 'Token endpoint rejected the code exchange: ' + '${errorResponse.error}$detail', + ); + throw OidcException( + 'The code exchange at ${hookRequest.tokenEndpoint} failed: ' + '${errorResponse.error}$detail', + internalException: e, + internalStackTrace: st, ); - - rethrow; } }, ); diff --git a/lib/src/auth/solid_token_store.dart b/lib/src/auth/solid_token_store.dart index d8b1448..d2ba218 100644 --- a/lib/src/auth/solid_token_store.dart +++ b/lib/src/auth/solid_token_store.dart @@ -28,6 +28,7 @@ library; import 'package:flutter_secure_storage/flutter_secure_storage.dart'; +import 'package:oidc_core/oidc_core.dart'; import 'package:oidc_default_store/oidc_default_store.dart'; /// Builds the store used for everything solid_auth persists between runs. @@ -45,36 +46,105 @@ import 'package:oidc_default_store/oidc_default_store.dart'; /// impersonation. Ordinary home-directory backups collect the file /// (RFC 9700 §4.14). /// -/// Android and iOS take [OidcDefaultStore]'s own hardened recommendations: an -/// Android-Keystore-backed key on Android, and first-unlock-this-device -/// keychain items (never iCloud-synced) on iOS. +/// The per-platform options are [OidcDefaultStore]'s own hardened +/// recommendations on Android and iOS: an Android-Keystore-backed key, and +/// first-unlock-this-device keychain items (never iCloud-synced). /// -/// 20260920 gjw macOS departs from the recommendation, turning OFF the data -/// protection keychain. That keychain requires the app to hold a keychain -/// access group, which comes from the `keychain-access-groups` entitlement or -/// from the `com.apple.application-identifier` an embedded provisioning -/// profile supplies. An app distributed with Developer ID has neither, so -/// every write failed: todopod 1.0.46 logged "tried writing secure tokens -/// using package:flutter_secure_storage, but it failed" and fell back to -/// shared_preferences, while READS returned errSecItemNotFound, which the -/// plugin reports as a plain null rather than an error. Nothing then caught a -/// failure to fall back on, so the PKCE `code_verifier` written before the -/// browser flow read back as null, the code exchange went out without it, and -/// the server answered invalid_grant — surfacing as an RFC 9207 mix-up -/// warning. The legacy file-based keychain needs no entitlement and works for -/// a signed, unsandboxed app. The cost is that `kSecAttrAccessible` is ignored -/// there, so items follow the login keychain rather than being pinned to -/// first-unlock-this-device. An App Store build is sandboxed and ships a -/// provisioning profile, so it can use the data protection keychain: give it -/// its own options rather than reusing these. +/// macOS takes those same recommendations with one change: +/// `usesDataProtectionKeychain` is turned OFF. `flutter_secure_storage` +/// defaults it on, and the macOS data protection keychain is reachable only by +/// a process that carries a keychain access group — either declared as the +/// restricted `keychain-access-groups` entitlement, or defaulted from the +/// `com.apple.application-identifier` that an embedded provisioning profile +/// supplies. Developer ID distribution embeds no profile, so a notarized app +/// has neither, and the OS then answers asymmetrically: +/// +/// - `SecItemAdd` fails with `errSecMissingEntitlement` (-34018). The plugin +/// turns that into a `PlatformException`, [OidcDefaultStore] catches it and +/// silently falls back to `package:shared_preferences` — so the secret is +/// written, in the clear, to the app's plist. +/// - `SecItemCopyMatching` returns `errSecItemNotFound` (-25300), which is not +/// an error at all. The plugin returns null, [OidcDefaultStore] reads that as +/// "never stored" and does NOT fall back — so the value just written is +/// invisible. +/// +/// Every secret in this namespace is therefore both leaked to disk and lost on +/// read. For `package:oidc` 4.x that includes the PKCE `code_verifier` (stored +/// under `code_verifier.`), so the code exchange goes out without +/// one and the OP rejects it with `invalid_grant - PKCE verification failed`: +/// login is impossible in a notarized build. +/// +/// Turning the flag off moves macOS to the file-based (login) keychain, which +/// needs no entitlement, works whether or not the app is sandboxed, and is +/// where a Developer ID app's secrets belong. `accessibility` is a data +/// protection attribute and is simply ignored there. +/// +/// 20260920 tonypioneer Diagnosed against the notarized todopod 1.0.46 DMG. -OidcDefaultStore createSolidTokenStore() => OidcDefaultStore( +OidcDefaultStore createSolidTokenStore() => _LoggingStore( secureStorageInstance: const FlutterSecureStorage( aOptions: OidcDefaultStore.recommendedAndroidOptions, iOptions: OidcDefaultStore.recommendedIOSOptions, - mOptions: MacOsOptions( - accessibility: KeychainAccessibility.first_unlock_this_device, - usesDataProtectionKeychain: false, - ), + mOptions: macOsKeychainOptions, ), ); + +/// The macOS keychain options used for everything solid_auth persists. +/// +/// [OidcDefaultStore.recommendedMacOsOptions] with the data protection +/// keychain turned off — see [createSolidTokenStore] for why that flag cannot +/// be left on in a Developer ID build. + +const MacOsOptions macOsKeychainOptions = MacOsOptions( + accessibility: KeychainAccessibility.first_unlock_this_device, + usesDataProtectionKeychain: false, +); + +// TEMP DIAGNOSTIC - remove. +class _LoggingStore extends OidcDefaultStore { + _LoggingStore({super.secureStorageInstance}); + + @override + Future setMany( + OidcStoreNamespace namespace, { + required Map values, + String? managerId, + }) async { + // ignore: avoid_print + print( + 'STORE set ${namespace.name} keys=${values.keys.toList()} ' + 'values=${namespace == OidcStoreNamespace.state ? values : ''}', + ); + return super.setMany(namespace, values: values, managerId: managerId); + } + + @override + Future> getMany( + OidcStoreNamespace namespace, { + required Set keys, + String? managerId, + }) async { + final res = await super.getMany( + namespace, + keys: keys, + managerId: managerId, + ); + // ignore: avoid_print + print( + 'STORE get ${namespace.name} keys=$keys -> found=${res.keys.toList()}' + '${namespace == OidcStoreNamespace.state ? ' values=$res' : ''}', + ); + return res; + } + + @override + Future removeMany( + OidcStoreNamespace namespace, { + required Set keys, + String? managerId, + }) async { + // ignore: avoid_print + print('STORE remove ${namespace.name} keys=$keys'); + return super.removeMany(namespace, keys: keys, managerId: managerId); + } +}