Skip to content

Commit d446153

Browse files
josephpabjosephpab
authored
Merge pull request #1543 from AzureAD/josephpab/dunaStateValidation
Duna State Validation
2 parents fa8e395 + 4e0072a commit d446153

9 files changed

+140
-26
lines changed

‎IdentityCore/src/oauth2/aad_base/MSIDAADWebviewFactory.m‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,12 +206,14 @@ - (MSIDWebviewResponse *)oAuthResponseWithURL:(NSURL *)url
206206
{
207207
MSIDSwitchBrowserResponse *switchBrowserResponse = [[MSIDSwitchBrowserResponse alloc] initWithURL:url
208208
redirectUri:endRedirectUri
209+
requestState:requestState
209210
context:context
210211
error:nil];
211212
if (switchBrowserResponse) return switchBrowserResponse;
212213

213214
MSIDSwitchBrowserResumeResponse *switchBrowserResumeResponse = [[MSIDSwitchBrowserResumeResponse alloc] initWithURL:url
214215
redirectUri:endRedirectUri
216+
requestState:requestState
215217
context:context
216218
error:nil];
217219
if (switchBrowserResumeResponse) return switchBrowserResumeResponse;

‎IdentityCore/src/webview/operations/ios/MSIDSwitchBrowserOperation.m‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,12 @@ - (void)invokeWithRequestParameters:(nonnull MSIDInteractiveTokenRequestParamete
8282
NSMutableDictionary *queryItems = [NSMutableDictionary new];
8383
queryItems[@"code"] = self.switchBrowserResponse.switchBrowserSessionToken;
8484
queryItems[MSID_OAUTH2_REDIRECT_URI] = requestParameters.redirectUri;
85+
86+
if (self.switchBrowserResponse.state)
87+
{
88+
queryItems[MSID_OAUTH2_STATE] = self.switchBrowserResponse.state;
89+
}
90+
8591
NSURLComponents *requestURLComponents = [[NSURLComponents alloc] initWithString:self.switchBrowserResponse.actionUri];
8692
requestURLComponents.percentEncodedQuery = [queryItems msidURLEncode];
8793
NSURL *startURL = requestURLComponents.URL;

‎IdentityCore/src/webview/operations/ios/MSIDSwitchBrowserResumeOperation.m‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,19 @@ - (void)invokeWithRequestParameters:(nonnull MSIDInteractiveTokenRequestParamete
8787
webviewResponseCompletionBlock:(nonnull MSIDWebviewAuthCompletionHandler)webviewResponseCompletionBlock
8888
authorizationCodeCompletionBlock:(nonnull MSIDInteractiveAuthorizationCodeCompletionBlock)authorizationCodeCompletionBlock
8989
{
90+
MSIDSwitchBrowserResponse *parentResponse = (MSIDSwitchBrowserResponse *)self.switchBrowserResumeResponse.parentResponse;
91+
NSError *stateValidationError = nil;
92+
93+
BOOL stateValidated = [MSIDSwitchBrowserResponse validateStateParameter:self.switchBrowserResumeResponse.state
94+
expectedState:parentResponse.state
95+
error:&stateValidationError];
96+
if (!stateValidated)
97+
{
98+
MSID_LOG_WITH_CTX(MSIDLogLevelError, requestParameters, @"Resume operation rejected due to state validation failure");
99+
if (webviewResponseCompletionBlock) webviewResponseCompletionBlock(nil, stateValidationError);
100+
return;
101+
}
102+
90103
webRequestConfiguration.startURL = [[NSURL alloc] initWithString:self.switchBrowserResumeResponse.actionUri];
91104
NSMutableDictionary *customHeaders = [webRequestConfiguration.customHeaders mutableCopy] ?: [NSMutableDictionary new];
92105
customHeaders[@"Authorization"] = [NSString stringWithFormat:@"Bearer %@", self.switchBrowserResumeResponse.switchBrowserSessionToken];

‎IdentityCore/src/webview/response/MSIDSwitchBrowserResponse.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ typedef NS_OPTIONS(NSInteger, MSIDSwitchBrowserModes) {
3535
@property (nonatomic, readonly) NSString *actionUri;
3636
@property (nonatomic, readonly) NSString *switchBrowserSessionToken;
3737
@property (nonatomic, readonly) BOOL useEphemeralWebBrowserSession;
38+
@property (nonatomic, readonly) NSString *state;
3839

3940
- (instancetype )init NS_UNAVAILABLE;
4041
+ (instancetype)new NS_UNAVAILABLE;
@@ -45,9 +46,13 @@ typedef NS_OPTIONS(NSInteger, MSIDSwitchBrowserModes) {
4546

4647
- (instancetype)initWithURL:(NSURL *)url
4748
redirectUri:(NSString *)redirectUri
49+
requestState:(NSString *)requestState
4850
context:(id<MSIDRequestContext>)context
4951
error:(NSError *__autoreleasing*)error;
5052

5153
+ (BOOL)isDUNAActionUrl:(NSURL *)url operation:(NSString *)operation;
5254

55+
+ (BOOL)validateStateParameter:(NSString *)receivedState
56+
expectedState:(NSString *)expectedState
57+
error:(NSError *__autoreleasing*)error;
5358
@end

‎IdentityCore/src/webview/response/MSIDSwitchBrowserResponse.m‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ + (NSString *)operation
3838

3939
- (instancetype)initWithURL:(NSURL *)url
4040
redirectUri:(NSString *)redirectUri
41+
requestState:(NSString *)requestState
4142
context:(id<MSIDRequestContext>)context
4243
error:(NSError *__autoreleasing*)error
4344
{
@@ -48,8 +49,23 @@ - (instancetype)initWithURL:(NSURL *)url
4849
if (self)
4950
{
5051
if (![self isMyUrl:url redirectUri:redirectUri]) return nil;
52+
53+
NSError *stateCheckError = nil;
54+
BOOL stateValidated = [MSIDSwitchBrowserResponse validateStateParameter:self.parameters[MSID_OAUTH2_STATE]
55+
expectedState:requestState
56+
error:&stateCheckError];
57+
if (!stateValidated)
58+
{
59+
if (stateCheckError && error)
60+
{
61+
*error = stateCheckError;
62+
}
63+
return nil;
64+
}
65+
5166
_actionUri = self.parameters[@"action_uri"];
5267
_useEphemeralWebBrowserSession = YES;
68+
_state = self.parameters[MSID_OAUTH2_STATE];
5369

5470
NSString* browserOptionsString = self.parameters[@"browser_modes"];
5571
if (browserOptionsString)
@@ -105,6 +121,45 @@ + (BOOL)isDUNAActionUrl:(NSURL *)url operation:(NSString *)operation
105121
return NO;
106122
}
107123

124+
+ (BOOL)validateStateParameter:(NSString *)receivedState
125+
expectedState:(NSString *)expectedState
126+
error:(NSError *__autoreleasing*)error
127+
{
128+
if (!receivedState && !expectedState)
129+
{
130+
return YES;
131+
}
132+
133+
if (!expectedState || !receivedState)
134+
{
135+
if (error)
136+
{
137+
*error = MSIDCreateError(MSIDOAuthErrorDomain,
138+
MSIDErrorServerInvalidState,
139+
[NSString stringWithFormat:@"Missing or invalid state returned state: %@", receivedState],
140+
nil, nil, nil, nil, nil, YES);
141+
}
142+
return NO;
143+
}
144+
145+
BOOL result = [receivedState isEqualToString:expectedState];
146+
147+
if (!result)
148+
{
149+
MSID_LOG_WITH_CTX(MSIDLogLevelError, nil, @"State parameter mismatch");
150+
if (error)
151+
{
152+
*error = MSIDCreateError(MSIDOAuthErrorDomain,
153+
MSIDErrorServerInvalidState,
154+
[NSString stringWithFormat:@"State parameter mismatch. Expected: %@, Received: %@", expectedState, receivedState],
155+
nil, nil, nil, nil, nil, YES);
156+
}
157+
return NO;
158+
}
159+
160+
return YES;
161+
}
162+
108163
#pragma mark - Private
109164

110165
- (BOOL)isMyUrl:(NSURL *)url

‎IdentityCore/tests/MSIDSwitchBrowserOperationTest.swift‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,8 @@ class MSIDAuthorizeWebRequestConfigurationMock : MSIDAuthorizeWebRequestConfigur
7878
final class MSIDSwitchBrowserOperationTest: XCTestCase
7979
{
8080
lazy var validSwitchBrowserResponse: MSIDSwitchBrowserResponse? = {
81-
let url = URL(string: "msauth.com.microsoft.msaltestapp://auth/switch_browser?action_uri=some_uri&code=some_code")!
82-
return try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", context: nil)
81+
let url = URL(string: "msauth.com.microsoft.msaltestapp://auth/switch_browser?action_uri=some_uri&code=some_code&state=state")!
82+
return try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", requestState: "state", context: nil)
8383
}()
8484

8585
override func setUpWithError() throws
@@ -148,7 +148,7 @@ final class MSIDSwitchBrowserOperationTest: XCTestCase
148148

149149
XCTAssertEqual(1, certAuthManagerMock.startWithUrlInvokedCount)
150150
XCTAssertEqual(1, certAuthManagerMock.resetStateInvokedCount)
151-
XCTAssertEqual(URL(string: "some_uri?code=some_code"), certAuthManagerMock.startURLProvidedParam)
151+
XCTAssertEqual(URL(string: "some_uri?state=state&code=some_code"), certAuthManagerMock.startURLProvidedParam)
152152
}
153153

154154
func testInvoke_whenWebRequestConfigurationReturnError_shouldReturnError() async throws
@@ -186,7 +186,7 @@ final class MSIDSwitchBrowserOperationTest: XCTestCase
186186

187187
XCTAssertEqual(1, certAuthManagerMock.startWithUrlInvokedCount)
188188
XCTAssertEqual(1, certAuthManagerMock.resetStateInvokedCount)
189-
XCTAssertEqual(URL(string: "some_uri?code=some_code"), certAuthManagerMock.startURLProvidedParam)
189+
XCTAssertEqual(URL(string: "some_uri?state=state&code=some_code"), certAuthManagerMock.startURLProvidedParam)
190190
XCTAssertEqual(1, webRequestConfigurationMock.responseWithResultURLInvokedCount)
191191
}
192192

@@ -222,7 +222,7 @@ final class MSIDSwitchBrowserOperationTest: XCTestCase
222222

223223
XCTAssertEqual(1, certAuthManagerMock.startWithUrlInvokedCount)
224224
XCTAssertEqual(1, certAuthManagerMock.resetStateInvokedCount)
225-
XCTAssertEqual(URL(string: "some_uri?code=some_code"), certAuthManagerMock.startURLProvidedParam)
225+
XCTAssertEqual(URL(string: "some_uri?state=state&code=some_code"), certAuthManagerMock.startURLProvidedParam)
226226
XCTAssertEqual(1, webRequestConfigurationMock.responseWithResultURLInvokedCount)
227227
}
228228
}

‎IdentityCore/tests/MSIDSwitchBrowserResponseTest.swift‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ final class MSIDSwitchBrowserResponseTest: XCTestCase
4242
{
4343
let url = URL(string: "msauth.com.microsoft.msaltestapp://auth/switch_browser?action_uri=some_uri&code=some_code")!
4444

45-
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", context: nil)
45+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", requestState: nil, context: nil)
4646

4747
XCTAssertNotNil(response)
4848
XCTAssertEqual(response?.actionUri, "some_uri")
@@ -53,7 +53,7 @@ final class MSIDSwitchBrowserResponseTest: XCTestCase
5353
{
5454
let url = URL(string: "MSAUTH.COM.MICROSOFT.msaltestapp://auth/switch_browser?action_uri=some_uri&code=some_code")!
5555

56-
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://AUTH", context: nil)
56+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://AUTH", requestState: nil, context: nil)
5757

5858
XCTAssertNotNil(response)
5959
XCTAssertEqual(response?.actionUri, "some_uri")
@@ -64,7 +64,7 @@ final class MSIDSwitchBrowserResponseTest: XCTestCase
6464
{
6565
let url = URL(string: "msauth.com.microsoft.msaltestapp://auth/switch_browser?action_uri=some_uri&code=some_code#ff")!
6666

67-
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth#fragment", context: nil)
67+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth#fragment", requestState: nil, context: nil)
6868

6969
XCTAssertNotNil(response)
7070
XCTAssertEqual(response?.actionUri, "some_uri")
@@ -75,42 +75,59 @@ final class MSIDSwitchBrowserResponseTest: XCTestCase
7575
{
7676
let url = URL(string: "msauth://broker_bundle_id//switch_browser?action_uri=some_uri&code=some_code")!
7777

78-
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth://broker_bundle_id", context: nil)
78+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth://broker_bundle_id", requestState: nil, context: nil)
7979

8080
XCTAssertNotNil(response)
8181
XCTAssertEqual(response?.actionUri, "some_uri")
8282
XCTAssertEqual(response?.switchBrowserSessionToken, "some_code")
8383
}
8484

85-
func testInit_whenValidBrowserMode_hasBitmaskPrivateSessionShouldBeTrue() throws
85+
func testInit_whenStateIsPresentInUrl_shouldCreateObject() throws
86+
{
87+
let url = URL(string: "msauth://broker_bundle_id//switch_browser?action_uri=some_uri&code=some_code&browser_modes=AAAAAA&state=state")!
88+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth://broker_bundle_id", requestState: "state", context: nil)
89+
90+
XCTAssertNotNil(response)
91+
XCTAssertEqual(response?.state, "state")
92+
}
93+
94+
func testInit_whenValidBrowserMode_hasBitmaskPrivateSession_shouldBeTrue() throws
8695
{
8796
let url = URL(string: "msauth://broker_bundle_id//switch_browser?action_uri=some_uri&code=some_code&browser_modes=AQAAAA")!
8897

89-
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth://broker_bundle_id", context: nil)
98+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth://broker_bundle_id", requestState: nil, context: nil)
9099

91100
XCTAssertNotNil(response)
92101
XCTAssertEqual(response?.actionUri, "some_uri")
93102
XCTAssertEqual(response?.switchBrowserSessionToken, "some_code")
94103
XCTAssertEqual(response?.useEphemeralWebBrowserSession, true)
95104
}
96105

97-
func testInit_whenInvalidBrowserMode_hasBitmaskPrivateSessionShouldBeFalse() throws
106+
func testInit_whenInvalidBrowserMode_hasBitmaskPrivateSession_shouldBeFalse() throws
98107
{
99108
let url = URL(string: "msauth://broker_bundle_id//switch_browser?action_uri=some_uri&code=some_code&browser_modes=AAAAAA")!
100109

101-
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth://broker_bundle_id", context: nil)
110+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth://broker_bundle_id", requestState: nil, context: nil)
102111

103112
XCTAssertNotNil(response)
104113
XCTAssertEqual(response?.actionUri, "some_uri")
105114
XCTAssertEqual(response?.switchBrowserSessionToken, "some_code")
106115
XCTAssertEqual(response?.useEphemeralWebBrowserSession, false)
107116
}
108117

118+
func testInit_whenStateIsMissingFromUrl_shouldReturnNil() throws
119+
{
120+
let url = URL(string: "msauth://broker_bundle_id//switch_browser?action_uri=some_uri&code=some_code&browser_modes=AAAAAA")!
121+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth://broker_bundle_id", requestState: "state", context: nil)
122+
123+
XCTAssertNil(response)
124+
}
125+
109126
func testInit_whenInvalidUrl_shouldReturnNil() throws
110127
{
111128
let url = URL(string: "msauth.com.microsoft.msaltestapp://auth/abc?action_uri=some_uri&code=some_code")!
112129

113-
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", context: nil)
130+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", requestState: nil, context: nil)
114131

115132
XCTAssertNil(response)
116133
}
@@ -119,7 +136,7 @@ final class MSIDSwitchBrowserResponseTest: XCTestCase
119136
{
120137
let url = URL(string: "abc.com.microsoft.msaltestapp://auth/switch_browser?action_uri=some_uri&code=some_code")!
121138

122-
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", context: nil)
139+
let response = try? MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", requestState: nil, context: nil)
123140

124141
XCTAssertNil(response)
125142
}
@@ -128,7 +145,7 @@ final class MSIDSwitchBrowserResponseTest: XCTestCase
128145
{
129146
let url = URL(string: "msauth.com.microsoft.msaltestapp://auth/switch_browser?code=some_code")!
130147

131-
XCTAssertThrowsError(try MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", context: nil)) { error in
148+
XCTAssertThrowsError(try MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", requestState: nil, context: nil)) { error in
132149
XCTAssertEqual((error as NSError).code, MSIDErrorCode.serverInvalidResponse.rawValue)
133150
XCTAssertEqual((error as NSError).domain, MSIDOAuthErrorDomain)
134151
XCTAssertEqual((error as NSError).userInfo["MSIDErrorDescriptionKey"] as? String, "action_uri is nil.")
@@ -139,7 +156,7 @@ final class MSIDSwitchBrowserResponseTest: XCTestCase
139156
{
140157
let url = URL(string: "msauth.com.microsoft.msaltestapp://auth/switch_browser?action_uri=some_uri")!
141158

142-
XCTAssertThrowsError(try MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", context: nil)) { error in
159+
XCTAssertThrowsError(try MSIDSwitchBrowserResponse(url: url, redirectUri: "msauth.com.microsoft.msaltestapp://auth", requestState: nil, context: nil)) { error in
143160
XCTAssertEqual((error as NSError).code, MSIDErrorCode.serverInvalidResponse.rawValue)
144161
XCTAssertEqual((error as NSError).domain, MSIDOAuthErrorDomain)
145162
XCTAssertEqual((error as NSError).userInfo["MSIDErrorDescriptionKey"] as? String, "code is nil.")

‎IdentityCore/tests/MSIDSwitchBrowserResumeOperationTest.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,10 @@ final class MSIDSwitchBrowserResumeOperationTest: XCTestCase
3030
lazy var validSwitchBrowserResumeResponse: MSIDSwitchBrowserResumeResponse? = {
3131

3232
let switchUrl = URL(string: "msauth.com.microsoft.msaltestapp://auth/switch_browser?action_uri=some_uri&code=some_code")!
33-
let switchBrowserResponse = try? MSIDSwitchBrowserResponse(url: switchUrl, redirectUri: "msauth.com.microsoft.msaltestapp://auth", context: nil)
33+
let switchBrowserResponse = try? MSIDSwitchBrowserResponse(url: switchUrl, redirectUri: "msauth.com.microsoft.msaltestapp://auth", requestState: nil, context: nil)
3434

3535
let resumeUrl = URL(string: "msauth.com.microsoft.msaltestapp://auth/switch_browser_resume?action_uri=some_uri&code=some_code")!
36-
let resumeResponse = try? MSIDSwitchBrowserResumeResponse(url: resumeUrl, redirectUri: "msauth.com.microsoft.msaltestapp://auth", context: nil)
36+
let resumeResponse = try? MSIDSwitchBrowserResumeResponse(url: resumeUrl, redirectUri: "msauth.com.microsoft.msaltestapp://auth", requestState: nil, context: nil)
3737
resumeResponse?.parent = switchBrowserResponse
3838

3939
return resumeResponse

0 commit comments

Comments
 (0)