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

fix: add reject in handleError in Windows verifySignature function #7380

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

beyondkmp
Copy link
Contributor

@beyondkmp beyondkmp commented Jan 17, 2023

We cannot catch the whoops error. Because all synchronous errors are handled. But here the error is generated not while the executor is running, but later. So the promise can’t handle it.

new Promise(function(resolve, reject) {
  setTimeout(() => {
    throw new Error("Whoops!");
  }, 1000);
}).catch(e => console.log('test is here:', e));

The verifySignature function has the same problem. we cannot catch the error throw by the function handleError.

export function verifySignature(publisherNames: Array<string>, unescapedTempUpdateFile: string, logger: Logger): Promise<string | null> {
  return new Promise<string | null>(resolve => {
    execFile(
      "chcp 65001 >NUL & powershell.exe",
      ["-NoProfile", "-NonInteractive", "-InputFormat", "None", "-Command", `"Get-AuthenticodeSignature -LiteralPath '${tempUpdateFile}' | ConvertTo-Json -Compress"`],
      {
        shell: true,
        timeout: 20 * 1000,
      },
      (error, stdout, stderr) => {
        try {
          if (error != null || stderr) {
            handleError(logger, error, stderr)
            resolve(null)
            return
          }

Impact

We want to catch all the powershell error because the users have a variety of powershell errors. But his bug, we cannot catch the errors and some users did not succeed in upgrading.

We can use the following code to test this bug in windows

const { execFile } = require('child_process');

function runCMD() {
  return new Promise((resolve, reject) => {
    execFile(
      'powershell.exe',
      ['test xxx'],
      (error, stdout, stderr) => {
        try {
          if (error != null || stderr) {
            handleErrorByThrow(error, stderr, reject);
            resolve(null);
            return;
          }
        } catch (e) {
          handleErrorByThrow(e, null, reject);
          return;
        }
      },
    );
  });
}

function handleError(error, stderr, reject) {
  if (error != null) {
    reject(error);
  }

  if (stderr) {
    reject(
      new Error(
        `Cannot execute Get-AuthenticodeSignature, stderr: ${stderr}. Failing signature validation due to unknown stderr.`,
      ),
    );
  }
}

function handleErrorByThrow(error, stderr, reject) {
  if (error != null) {
    throw error;
  }

  if (stderr) {
    throw new Error(`Cannot execute Get-AuthenticodeSignature, stderr: ${stderr}. Failing signature validation due to unknown stderr.`)
  }
}


async function test() {
  try {
    const result = await runCMD();
    console.log(result);  
  } catch (e) {
    console.log('catch is called');
    console.log(e);
  }
}
test();

@changeset-bot
Copy link

changeset-bot bot commented Jan 17, 2023

🦋 Changeset detected

Latest commit: 9e57da9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
electron-updater Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Jan 17, 2023

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit 9e57da9
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/63c6d7fb2eeade0009532008
😎 Deploy Preview https://deploy-preview-7380--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mmaietta mmaietta changed the title fix: add reject in handleError fix: add reject in handleError in Windows verifySignature function Jan 17, 2023
@mmaietta mmaietta merged commit 7862e38 into electron-userland:master Jan 17, 2023
@github-actions github-actions bot mentioned this pull request Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants