Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add response_mode=form_post to legacy WebView authorization requests #2780

Merged
merged 8 commits into from
Jul 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ public CustomWebBrowserEvent(CustomWebBrowser parent)
public void NavigateError(object pDisp, ref object url, ref object frame, ref object statusCode,
ref bool cancel)
{
string uriString = (url == null) ? "" : ((string) url);
string frameString = (frame == null) ? "" : ((string) frame);
int statusCodeInt = (statusCode == null) ? 0 : ((int) statusCode);
string uriString = (url == null) ? "" : ((string)url);
string frameString = (frame == null) ? "" : ((string)frame);
int statusCodeInt = (statusCode == null) ? 0 : ((int)statusCode);

#pragma warning disable 618 // WebBrowserNavigateErrorEventArgs is marked obsolete
WebBrowserNavigateErrorEventArgs e = new WebBrowserNavigateErrorEventArgs(uriString, frameString,
Expand All @@ -33,27 +33,19 @@ public void NavigateError(object pDisp, ref object url, ref object frame, ref ob
cancel = e.Cancel;
}

// This method are empty because we agree with their implementation in base class event handler System.Windows.Forms.WebBrowser+WebBrowserEvent.
// We disagree with empty implementation of NavigateError.
// We also could tweak implementation of base class if we disagree by implementing this method.
// This is COM events handler, defined in COM interface, however this model works as Events in .NET
// Multiple handlers are possible, so empty method just called and do nothing.
public void BeforeNavigate2(object pDisp, ref object urlObject, ref object flags, ref object targetFrameName,
public void BeforeNavigate2(object pDisp, ref object url, ref object flags, ref object targetFrameName,
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
ref object postData, ref object headers, ref bool cancel)
{
// TODO: Navigating event from public class could be called for internal object.
// Current implementation of System.Windows.Forms.WebBrowser doesn't allow you to track who issues this event this control or IFrame,
// internal IFrame will have different pDisp, so we need filter events from internal IFrames by analyzing this field:
//
// if ( this.webBrowser.ActiveXInstance != e.WebBrowserActiveXInstance )
// {
// // this event came from internal frame, ignore this.
// return;
// }
//
// See WindowsFormsWebAuthenticationDialogBase.WebBrowserNavigateErrorHandler( object sender, WebBrowserNavigateErrorEventArgs e )
// Thus, before making any decision it will be safe to check if Navigating event comes from right object.
// This not a P0 bug, as it final URL with auth code could came only in main frame, however it could give issue with more complicated logic.
string urlString = (url == null) ? string.Empty : ((string)url);
int flagsInt = (flags == null) ? 0 : ((int)flags);
string targetFrameNameString = (targetFrameName == null) ? string.Empty : ((string)targetFrameName);
byte[] postDataBytes = (byte[])postData;
string headersString = (headers == null) ? string.Empty : ((string)headers);

WebBrowserBeforeNavigateEventArgs e = new WebBrowserBeforeNavigateEventArgs(urlString, postDataBytes,
headersString, flagsInt, targetFrameNameString, pDisp);
parent.OnBeforeNavigate(e);
cancel = e.Cancel;
}

public void ClientToHostWindow(ref long cX, ref long cY)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected override void CreateSink()
{
webBrowserEvent = new CustomWebBrowserEvent(this);
webBrowserEventCookie = new AxHost.ConnectionPointCookie(activeXInstance, webBrowserEvent,
typeof (NativeWrapper.DWebBrowserEvents2));
typeof(NativeWrapper.DWebBrowserEvents2));
}
}

Expand All @@ -66,9 +66,16 @@ protected virtual void OnNavigateError(WebBrowserNavigateErrorEventArgs e)
}
#pragma warning restore 618

protected virtual void OnBeforeNavigate(WebBrowserBeforeNavigateEventArgs e)
{
BeforeNavigate?.Invoke(this, e);
}

public event WebBrowserNavigateErrorEventHandler NavigateError;

[ComVisible(true), ComDefaultInterface(typeof (NativeWrapper.IDocHostUIHandler))]
public event WebBrowserBeforeNavigateEventHandler BeforeNavigate;

[ComVisible(true), ComDefaultInterface(typeof(NativeWrapper.IDocHostUIHandler))]
protected class CustomSite : WebBrowserSite, NativeWrapper.IDocHostUIHandler, ICustomQueryInterface
{
private const int NotImplemented = -2147467263;
Expand All @@ -85,9 +92,9 @@ public CustomSite(WebBrowser host)
public CustomQueryInterfaceResult GetInterface(ref Guid iid, out IntPtr ppv)
{
ppv = IntPtr.Zero;
if (iid == typeof (NativeWrapper.IDocHostUIHandler).GUID)
if (iid == typeof(NativeWrapper.IDocHostUIHandler).GUID)
{
ppv = Marshal.GetComInterfaceForObject(this, typeof (NativeWrapper.IDocHostUIHandler),
ppv = Marshal.GetComInterfaceForObject(this, typeof(NativeWrapper.IDocHostUIHandler),
CustomQueryInterfaceMode.Ignore);
return CustomQueryInterfaceResult.Handled;
}
Expand Down Expand Up @@ -213,8 +220,8 @@ public int TranslateAccelerator(ref NativeWrapper.MSG msg, ref Guid group, int n
{
if (ModifierKeys == Keys.Shift || ModifierKeys == Keys.Alt || ModifierKeys == Keys.Control)
{
int num = ((int) msg.wParam) | (int) ModifierKeys;
Shortcut s = (Shortcut) num;
int num = ((int)msg.wParam) | (int)ModifierKeys;
Shortcut s = (Shortcut)num;
if (shortcutDisallowedList.Contains(s))
{
return S_OK;
Expand Down Expand Up @@ -243,4 +250,7 @@ public int UpdateUI()
/// </summary>
internal delegate void WebBrowserNavigateErrorEventHandler(object sender, WebBrowserNavigateErrorEventArgs e);
#pragma warning restore 618
/// <summary>
/// </summary>
internal delegate void WebBrowserBeforeNavigateEventHandler(object sender, WebBrowserBeforeNavigateEventArgs e);
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ public void CloseBrowser()
/// </summary>
private void SuppressBrowserSubDialogs()
{
var webBrowser2 = (NativeWrapper.IWebBrowser2) WebBrowser.ActiveXInstance;
var webBrowser2 = (NativeWrapper.IWebBrowser2)WebBrowser.ActiveXInstance;
webBrowser2.Silent = true;
}

/// <summary>
/// </summary>
protected override void WebBrowserNavigatingHandler(object sender, WebBrowserNavigatingEventArgs e)
protected override void WebBrowserBeforeNavigateHandler(object sender, WebBrowserBeforeNavigateEventArgs e)
{
if (null == timer)
{
Expand All @@ -73,12 +73,12 @@ protected override void WebBrowserNavigatingHandler(object sender, WebBrowserNav
// Reset the expiry time so that it isn't relevant until the next document complete.
navigationExpiry = DateTime.MaxValue;

base.WebBrowserNavigatingHandler(sender, e);
base.WebBrowserBeforeNavigateHandler(sender, e);
}

private static Timer CreateStartedTimer(Action onTickAction, int interval)
{
Timer timer = new Timer {Interval = interval};
Timer timer = new Timer { Interval = interval };
timer.Tick += (notUsedsender, notUsedEventArgs) => onTickAction();
timer.Start();
return timer;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.ComponentModel;

namespace Microsoft.Identity.Client.Platforms.Features.WinFormsLegacyWebUi
{
/// <summary>
/// Event arguments for <c>BeforeNavigate</c> event.
/// </summary>
public class WebBrowserBeforeNavigateEventArgs : CancelEventArgs
{
/// <summary>
/// Initializes a new instance of <c>WebBrowserBeforeNavigateEventArgs</c>.
/// </summary>
public WebBrowserBeforeNavigateEventArgs(
string url,
byte[] postData,
string headers,
int flags,
string targetFrameName,
object webBrowserActiveXInstance)
{
Url = url;
PostData = postData;
Headers = headers;
Flags = flags;
TargetFrameName = targetFrameName;
WebBrowserActiveXInstance = webBrowserActiveXInstance;
}

/// <summary>
/// The URL to be navigated to.
/// </summary>
public string Url { get; }

/// <summary>
/// The data to send to the server, if the HTTP POST transaction is used.
/// </summary>
public byte[] PostData { get; }

/// <summary>
/// Additional HTTP headers to send to the server
/// </summary>
public string Headers { get; }

/// <summary>
/// The following flag, or zero.
/// beforeNavigateExternalFrameTarget (H0001)
/// Internet Explorer 7 or later. This navigation is the result of
/// an external window or tab that targets this browser.
/// </summary>
public int Flags { get; }

/// <summary>
/// The name of the frame in which to display the resource,
/// or null if no named frame is targeted for the resource.
/// </summary>
public string TargetFrameName { get; }

/// <summary>
/// A pointer to the IDispatch interface for the WebBrowserControl object that represents the window or frame.
/// </summary>
public object WebBrowserActiveXInstance { get; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public WebBrowserNavigateErrorEventArgs(string url, string targetFrameName, int
/// </summary>
public string TargetFrameName { get; }

// url as a string, as in case of error it could be invalid url
// URL as a string, as in case of error it could be invalid URL
/// <summary>
/// </summary>
public string Url { get; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
// Licensed under the MIT License.

using System;
using System.Net.NetworkInformation;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Identity.Client.Http;
using Microsoft.Identity.Client.Internal;
using Microsoft.Identity.Client.Platforms.Features.DesktopOs;
using Microsoft.Identity.Client.UI;
using Microsoft.Identity.Client.Utils;

namespace Microsoft.Identity.Client.Platforms.Features.WinFormsLegacyWebUi
{
Expand All @@ -29,6 +29,10 @@ public async Task<AuthorizationResult> AcquireAuthorizationAsync(
{
AuthorizationResult authorizationResult = null;

var authUriBuilder = new UriBuilder(authorizationUri);
authUriBuilder.AppendOrReplaceQueryParameter("response_mode", "form_post");
authorizationUri = authUriBuilder.Uri;
pmaytak marked this conversation as resolved.
Show resolved Hide resolved

var sendAuthorizeRequest = new Action(() =>
{
authorizationResult = Authenticate(authorizationUri, redirectUri);
Expand All @@ -44,7 +48,7 @@ public async Task<AuthorizationResult> AcquireAuthorizationAsync(
catch (Exception e)
{
// Need to catch the exception here and put on the TCS which is the task we are waiting on so that
// the exception comming out of Authenticate is correctly thrown.
// the exception coming out of Authenticate is correctly thrown.
((TaskCompletionSource<object>)tcs).TrySetException(e);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@ public void ShowBrowser()

/// <summary>
/// </summary>
protected override void WebBrowserNavigatingHandler(object sender, WebBrowserNavigatingEventArgs e)
protected override void WebBrowserBeforeNavigateHandler(object sender, WebBrowserBeforeNavigateEventArgs e)
{
SetBrowserZoom();
base.WebBrowserNavigatingHandler(sender, e);
base.WebBrowserBeforeNavigateHandler(sender, e);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ private void WebBrowser_PreviewKeyDown(object sender, PreviewKeyDownEventArgs e)

/// <summary>
/// </summary>
protected virtual void WebBrowserNavigatingHandler(object sender, WebBrowserNavigatingEventArgs e)
protected virtual void WebBrowserBeforeNavigateHandler(object sender, WebBrowserBeforeNavigateEventArgs e)
{
if (DialogResult == DialogResult.OK)
{
Expand All @@ -117,24 +117,33 @@ protected virtual void WebBrowserNavigatingHandler(object sender, WebBrowserNavi
e.Cancel = true;
}

if (string.IsNullOrEmpty(e.Url))
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
{
RequestContext.Logger.Verbose("[Legacy WebView] URL in BeforeNavigate is null or empty.");
e.Cancel = true;
return;
}

Uri url = new Uri(e.Url);

// we cancel further processing, if we reached final URL.
// Security issue: we prohibit navigation with auth code
// if redirect URI is URN, then we prohibit navigation, to prevent random browser popup.
e.Cancel = CheckForClosingUrl(e.Url);
// if redirect URI is URN, then we prohibit navigation, to prevent random browser pop-up.
e.Cancel = CheckForClosingUrl(url, e.PostData);

// check if the url scheme is of type browser-install://
// check if the URL scheme is of type browser-install://
// this means we need to launch external browser
if (e.Url.Scheme.Equals("browser", StringComparison.OrdinalIgnoreCase))
if (url.Scheme.Equals("browser", StringComparison.OrdinalIgnoreCase))
{
Process.Start(e.Url.AbsoluteUri.Replace("browser://", "https://"));
Process.Start(url.AbsoluteUri.Replace("browser://", "https://"));
e.Cancel = true;
}

if (!e.Cancel)
{
string urlDecode = CoreHelpers.UrlDecode(e.Url.ToString());
RequestContext.Logger.VerbosePii(
string.Format(CultureInfo.InvariantCulture, "Navigating to '{0}'.", urlDecode),
string.Format(CultureInfo.InvariantCulture, "[Legacy WebView] Navigating to '{0}'.", urlDecode),
string.Empty);
}
}
Expand All @@ -149,7 +158,7 @@ private void WebBrowserNavigatedHandler(object sender, WebBrowserNavigatedEventA

string urlDecode = CoreHelpers.UrlDecode(e.Url.ToString());
RequestContext.Logger.VerbosePii(
string.Format(CultureInfo.InvariantCulture, "Navigated to '{0}'.", urlDecode),
string.Format(CultureInfo.InvariantCulture, "[Legacy WebView] Navigated to '{0}'.", urlDecode),
string.Empty);
}

Expand Down Expand Up @@ -188,23 +197,23 @@ protected virtual void WebBrowserNavigateErrorHandler(object sender, WebBrowserN
OnNavigationCanceled(e.StatusCode);
}

private bool CheckForClosingUrl(Uri url)
private bool CheckForClosingUrl(Uri url, byte[] postData = null)
{
bool readyToClose = false;

if (url.Authority.Equals(_desiredCallbackUri.Authority, StringComparison.OrdinalIgnoreCase) &&
url.AbsolutePath.Equals(_desiredCallbackUri.AbsolutePath))
{
RequestContext.Logger.Info("Redirect Uri was reached. Stopping webview navigation...");
Result = AuthorizationResult.FromUri(url.OriginalString);
RequestContext.Logger.Info("[Legacy WebView] Redirect URI was reached. Stopping WebView navigation...");
Result = AuthorizationResult.FromPostData(postData);
readyToClose = true;
}

if (!readyToClose && !url.Scheme.Equals("https", StringComparison.OrdinalIgnoreCase) &&
!url.AbsoluteUri.Equals("about:blank", StringComparison.OrdinalIgnoreCase) && !url.Scheme.Equals("javascript", StringComparison.OrdinalIgnoreCase))
{
RequestContext.Logger.Error(string.Format(CultureInfo.InvariantCulture,
"Redirection to non-HTTPS scheme ({0}) found! Webview will fail...", url.Scheme));
"[Legacy WebView] Redirection to non-HTTPS scheme ({0}) found! WebView will fail...", url.Scheme));
Result = AuthorizationResult.FromStatus(
AuthorizationStatus.ErrorHttp,
MsalError.NonHttpsRedirectNotSupported,
Expand Down Expand Up @@ -233,14 +242,14 @@ private void StopWebBrowser()
}

RequestContext.Logger.Verbose(string.Format(CultureInfo.InvariantCulture,
"WebBrowser state: IsBusy: {0}, ReadyState: {1}, Created: {2}, Disposing: {3}, IsDisposed: {4}, IsOffline: {5}",
"[Legacy WebView] WebBrowser state: IsBusy: {0}, ReadyState: {1}, Created: {2}, Disposing: {3}, IsDisposed: {4}, IsOffline: {5}",
_webBrowser.IsBusy, _webBrowser.ReadyState, _webBrowser.Created,
_webBrowser.Disposing, _webBrowser.IsDisposed, _webBrowser.IsOffline));

_webBrowser.Stop();

RequestContext.Logger.Verbose(string.Format(CultureInfo.InvariantCulture,
"WebBrowser state (after Stop): IsBusy: {0}, ReadyState: {1}, Created: {2}, Disposing: {3}, IsDisposed: {4}, IsOffline: {5}",
"[Legacy WebView] WebBrowser state (after Stop): IsBusy: {0}, ReadyState: {1}, Created: {2}, Disposing: {3}, IsDisposed: {4}, IsOffline: {5}",
_webBrowser.IsBusy, _webBrowser.ReadyState, _webBrowser.Created,
_webBrowser.Disposing, _webBrowser.IsDisposed, _webBrowser.IsOffline));
});
Expand All @@ -262,7 +271,7 @@ internal AuthorizationResult AuthenticateAAD(Uri requestUri, Uri callbackUri)
// The WebBrowser event handlers must not throw exceptions.
// If they do then they may be swallowed by the native
// browser com control.
_webBrowser.Navigating += WebBrowserNavigatingHandler;
_webBrowser.BeforeNavigate += WebBrowserBeforeNavigateHandler;
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
_webBrowser.Navigated += WebBrowserNavigatedHandler;
_webBrowser.NavigateError += WebBrowserNavigateErrorHandler;

Expand All @@ -285,7 +294,7 @@ protected virtual void OnAuthenticate()
/// <param name="action"></param>
protected void InvokeHandlingOwnerWindow(Action action)
{
// We only support WindowsForms (since our dialog is winforms based)
// We only support Windows Forms (since our dialog is Windows Forms based)
if (ownerWindow != null && ownerWindow is Control winFormsControl)
{
winFormsControl.Invoke(action);
Expand Down Expand Up @@ -381,7 +390,7 @@ protected MsalClientException CreateExceptionForAuthenticationUiFailed(int statu
string messageUnknown = string.Format(CultureInfo.InvariantCulture, formatUnknown, statusCode);
return new MsalClientException(MsalError.AuthenticationUiFailedError, messageUnknown);
}

/// <summary>
/// </summary>
internal static class NativeMethods
Expand Down
Loading