Stop raising exceptions for optional discovery fields

CHANGELOG.md

@@ -2,6 +2,7 @@
 - Improved the state validation error messages to distinguish a missing `state` parameter from a mismatched one. The end session message now notes that a provider's `end_session_endpoint` may point at a legacy or SAML single-logout endpoint, which does not echo `state`. Addresses issue #956.
 - Fixed `OIDExternalUserAgentIOSCustomBrowser` failing the flow with "Unable to open Safari." when `canOpenURL:` returns `NO` for a URL that the system can open, for example an https URL when Safari is restricted and another browser is the default. The flow now fails only if the URL really cannot be opened. Addresses issue #980.
 - Fixed a crash when the user cancels sign-in on iOS, caused by `ASWebAuthenticationSession` being deallocated during its completion handler or dismiss animation. Based on [#917](https://github.com/openid/AppAuth-iOS/pull/917) by @Prash88.
+- Discovery documents containing a JSON `null` or a non-string value in a URL-valued field now fail with `OIDErrorCodeInvalidDiscoveryDocument` instead of raising `NSInvalidArgumentException`. An `authorization_endpoint` that does not parse now fails the document too, like the other required URL fields.
 
 # 3.0.0
 - BREAKING: Updates made to support Xcode 27. ([#972](https://github.com/openid/AppAuth-iOS/pull/972), [#973](https://github.com/openid/AppAuth-iOS/pull/973))

Sources/AppAuthCore/OIDServiceDiscovery.m

@@ -127,7 +127,9 @@ - (nullable instancetype)initWithDictionary:(NSDictionary *)serviceDiscoveryDict
     @discussion This test is not meant to provide semantic analysis of the document (eg. fields
         where the value @c none is not an allowed option would not cause this method to fail if
         their value was @c none.) We are just testing to make sure we can meet the nullability
-        contract we promised in the header.
+        contract we promised in the header. A URL-valued field that is present must be a string,
+        or the getters would raise. A URL field that does not parse is an error only for the
+        nonnull URL properties. The nullable ones return nil for it instead.
  */
 + (BOOL)dictionaryHasRequiredFields:(NSDictionary<NSString *, id> *)dictionary
                               error:(NSError **_Nullable)error {
@@ -156,15 +158,47 @@ + (BOOL)dictionaryHasRequiredFields:(NSDictionary<NSString *, id> *)dictionary
     }
   }
 
-  // Check required URL fields are valid URLs.
-  NSArray *requiredURLFields = @[
+  // Every URL-valued field that is present must be a string. NSJSONSerialization turns a JSON
+  // null into NSNull, and +[NSURL URLWithString:] raises on anything that is not a string.
+  NSArray<NSString *> *urlFields = @[
     kIssuerKey,
+    kAuthorizationEndpointKey,
+    kDeviceAuthorizationEndpointKey,
+    kTokenEndpointKey,
+    kUserinfoEndpointKey,
+    kJWKSURLKey,
+    kRegistrationEndpointKey,
+    kEndSessionEndpointKey,
+    kServiceDocumentationKey,
+    kOPPolicyURIKey,
+    kOPTosURIKey
+  ];
+
+  for (NSString *field in urlFields) {
+    id value = dictionary[field];
+    if (value && ![value isKindOfClass:[NSString class]]) {
+      if (error) {
+        NSString *errorText = [NSString stringWithFormat:kInvalidURLFieldErrorText, field];
+        *error = [OIDErrorUtilities errorWithCode:OIDErrorCodeInvalidDiscoveryDocument
+                                  underlyingError:nil
+                                      description:errorText];
+      }
+      return NO;
+    }
+  }
+
+  // The nonnull URL properties must also parse. The nullable ones return nil for a value that
+  // does not parse, so a sloppy optional field does not reject the whole document. Keep this
+  // list in sync with the nonnull NSURL properties in OIDServiceDiscovery.h.
+  NSArray<NSString *> *requiredURLFields = @[
+    kIssuerKey,
+    kAuthorizationEndpointKey,
     kTokenEndpointKey,
     kJWKSURLKey
   ];
 
   for (NSString *field in requiredURLFields) {
-    if (![NSURL URLWithString:dictionary[field]]) {
+    if (![NSURL URLWithString:(NSString *)dictionary[field]]) {
       if (error) {
         NSString *errorText = [NSString stringWithFormat:kInvalidURLFieldErrorText, field];
         *error = [OIDErrorUtilities errorWithCode:OIDErrorCodeInvalidDiscoveryDocument

UnitTests/OIDServiceDiscoveryTests.m

@@ -70,7 +70,7 @@ - (void)encodeWithCoder:(NSCoder *)coder {
 static NSString *const kTestURL = @"http://www.google.com/";
 
 /*! A string for testing an invalid URL. */
-static NSString *const kTestURLInvalid = @"abc";
+static NSString *const kTestURLInvalid = @"http://foo bar/";
 
 /*! Field keys associated with an OpenID Connect Discovery Document. */
 static NSString *const kIssuerKey = @"issuer";
@@ -337,8 +337,7 @@ - (void)testNoErrorWhenNoMissingFields {
 
 /*! @brief Tests that we get an error when the required fields are present but invalid.
  */
-// TODO: this test is failing
-- (void)pendingTestErrorWhenMalformedFields {
+- (void)testErrorWhenMalformedFields {
   NSError *error;
   // builds a discovery document with a munged field
   NSMutableDictionary *serviceDiscoveryDictionary =
@@ -358,8 +357,7 @@ - (void)pendingTestErrorWhenMalformedFields {
 
 /*! @brief Tests that we get an error when null is passed in through JSON.
  */
-// TODO: this test is failing
-- (void)pendingTestErrorWhenJSONNullField {
+- (void)testErrorWhenJSONNullField {
   NSError *error;
   OIDServiceDiscovery *discovery =
       [[OIDServiceDiscovery alloc] initWithJSON:kDiscoveryDocumentNullField error:&error];
@@ -370,6 +368,41 @@ - (void)pendingTestErrorWhenJSONNullField {
   XCTAssertEqual(error.code, OIDErrorCodeInvalidDiscoveryDocument, @"");
 }
 
+/*! @brief Tests that a present optional URL-valued field that is not a string produces an error
+        instead of raising.
+ */
+- (void)testErrorWhenOptionalURLFieldIsNotAString {
+  NSError *error;
+  NSMutableDictionary *serviceDiscoveryDictionary =
+      [[[self class] minimumServiceDiscoveryDictionary] mutableCopy];
+  serviceDiscoveryDictionary[kOPPolicyURIKey] = @42;
+
+  OIDServiceDiscovery *discovery =
+      [[OIDServiceDiscovery alloc] initWithDictionary:serviceDiscoveryDictionary error:&error];
+  XCTAssertNil(discovery, @"When initializing a discovery document, it should not return an "
+                          "instance if an optional URL field is not a string.");
+  XCTAssertNotNil(error, @"There should be an error indicating the discovery document is invalid.");
+  XCTAssertEqualObjects(error.domain, OIDGeneralErrorDomain, @"");
+  XCTAssertEqual(error.code, OIDErrorCodeInvalidDiscoveryDocument, @"");
+}
+
+/*! @brief Tests that a present optional URL-valued field that does not parse is accepted, with
+        its property reading as nil.
+ */
+- (void)testNoErrorWhenOptionalURLFieldDoesNotParse {
+  NSError *error;
+  NSMutableDictionary *serviceDiscoveryDictionary =
+      [[[self class] minimumServiceDiscoveryDictionary] mutableCopy];
+  serviceDiscoveryDictionary[kOPPolicyURIKey] = kTestURLInvalid;
+
+  OIDServiceDiscovery *discovery =
+      [[OIDServiceDiscovery alloc] initWithDictionary:serviceDiscoveryDictionary error:&error];
+  XCTAssertNotNil(discovery, @"An optional URL field that does not parse should not reject the "
+                             "discovery document.");
+  XCTAssertNil(error, @"");
+  XCTAssertNil(discovery.OPPolicyURI, @"");
+}
+
 /*! @brief Tests that the JSON String version results in a valid object.
  */
 - (void)testJSONStringDecoding {