OfficeDev / Office-Addin-Scripts

A set of scripts and packages that are consumed in Office add-ins projects.
MIT License
152 stars 93 forks source link

Issues with AppContainer Loopback Logic in `office-addin-debugging` and `office-addin-dev-settings` #840

Closed ZYUN-MSFT closed 2 months ago

ZYUN-MSFT commented 4 months ago

Hi all, I recently found some issues/strange behavior with the loopback logic.

Background

I know that AppContainer tech make some Apps likes UWP can't access localhost (loopback) by default. In previously, I think Webview2 && Edge need that loopback exempt for Add-in side-load. But some test I did recently shows that things may be different.

Tools I used:

Issue 1: Type No during Loopback question prompt

Code in office-addin-debugging dosen't use the result from ensureLoopbackIsEnabled:

// enable loopback for Edge
if (isWindowsPlatform && parseInt(os.release(), 10) === 10) {
  const name = isDesktopAppType ? "EdgeWebView" : "EdgeWebBrowser";
  await devSettings.ensureLoopbackIsEnabled(name);
}
export async function ensureLoopbackIsEnabled(
  manifestPath: string,
  askForConfirmation: boolean = true
): Promise<boolean> {
  const name = await getAppcontainerNameFromManifestPath(manifestPath);
  let isEnabled = await isLoopbackExemptionForAppcontainer(name);

  if (!isEnabled) {
    if (!askForConfirmation || (await getUserConfirmation(getDisplayNameFromManifestPath(manifestPath)))) {
      await addLoopbackExemptionForAppcontainer(name);
      isEnabled = true;
    }
  }

  return isEnabled;  // if user type No, this will return false
}

So, when ensureLoopbackIsEnabled detect that loopback is not exempted and user type No, the logic in office-addin-debugging will carried on the following steps (start the dev server and side load the add-in).

Then the things get interested:

  1. I first delete all loopback exempt No Loopback exempt
  2. Then I try start debug in add-in project and type No during loopback question asked
  3. I found that add-in side load successfully even without Edge/Webview2 loopback exempt. result
  4. I also try to start in web, the result is the same result-web

These result shows that maybe the Loopback exempt is no needed for user to side load the add-in.

Issue 2: Edge App Container Name

In this code file, we can see the Edge Browser App Container Name (AC Name):

export const EdgeBrowserAppcontainerName: string = "Microsoft.MicrosoftEdge_8wekyb3d8bbwe";
export const EdgeWebViewAppcontainerName: string = "Microsoft.win32webviewhost_cw5n1h2txyewy";

However, on my new Win11 23H2 machine, I don't have Edge that with AC Name, instead I have a Edge with AC Name of Microsoft.MicrosoftEdge.Stable_8wekyb3d8bbwe:

ac name1

But in my two other machine (one is Win11 which upgrade from Win10, another is Win10), they both have two Edge App Container and one is Microsoft.MicrosoftEdge_8wekyb3d8bbwe another is Microsoft.MicrosoftEdge.Stable_8wekyb3d8bbwe

Win10 ac name

Win10-Win11 ac name

Issue 3: The way to check whether Edge Browser have Loopback exempt maybe wrong

This code shows that how to check the app have the exempt:

export function isLoopbackExemptionForAppcontainer(name: string): Promise<boolean> {
  if (!isAppcontainerSupported()) {
    throw new ExpectedError(`Platform not supported: ${process.platform}.`);
  }

  return new Promise((resolve, reject) => {
    const command = `CheckNetIsolation.exe LoopbackExempt -s`;

    childProcess.exec(command, (error, stdout) => {
      if (error) {
        reject(stdout);
      } else {
        const expr = new RegExp(`Name: ${name}`, "i");
        const found: boolean = expr.test(stdout);
        resolve(found);
      }
    });
  });
}

But I did some test here:

test

We can see that for EdgeWebViewAppcontainerName, this way works. But for EdgeBrowserAppcontainerName this way not work. Though Microsoft.MicrosoftEdge_8wekyb3d8bbwe already have exempt but .\CheckNetIsolation.exe LoopbackExempt -s will not list it.

Issue 4: Only Add Edge Browser to loopback Exempt maybe not correct.

When sideload in web, this code show that open is used for open the sideload link.

async function launchApp(app: OfficeApp, sideloadFile: string) {
  console.log(`Launching ${app} via ${sideloadFile}`);
  if (sideloadFile) {
    if (app === OfficeApp.Outlook) {
      // put the Outlook.exe path in quotes if it contains spaces
      if (sideloadFile.indexOf(" ") >= 0) {
        sideloadFile = `"${sideloadFile}"`;
      }

      startDetachedProcess(sideloadFile);
    } else {
      await open(sideloadFile, { wait: false });
    }
  }
}

But open seems to open link with system default browser, which may not be Edge.

So only exempt Edge Browser maybe not correct. But accroding to above information, maybe exempt browser is not needed.

hermanwenhe commented 4 months ago

More thoughts on this:

Since the CheckNetIsolation logic is added 5 years ago and we do find out this logic is not needed on Windows 11. Maybe we should re-check this and find out is this check a required step? Maybe we can remove this check for certain OS version or we can set the default value to "NO" to avoid the "Access Denied" issue.

Add @millerds @akrantz for discussion.

akrantz commented 4 months ago

I'm not working in this space now, so Darren can investigate. It may be that the webview2 code allows the localhost loopback when debugging or this was allowed in a mechanism which CheckNetIsolation doesn't report. Certainly there are different flavors and ways of installing webview2, as you mention the different app ids.

ZYUN-MSFT commented 3 months ago

I make a PR with simple change to keep behavior the same when user type Yes or No when asking loopback question.


Some other investigation I found:

I found the following docs:

In the first doc, it said that for Legacy Edge, it need the loopback exemption, but for Webview 2 it doesn't need so.

Maybe this is the kernel cause of all my above issues.

Adrian-MSFT commented 2 months ago

Thanks for reporting this issue. After careful consideration, our team has decided to not fix this issue in the short term due to the amount of effort needed for this and the fact that the PR you created provides a workaround for this as well. If we end up working on this issue we will re-activate as needed.