fix: crash parsing CLSID in shell.readShortcutLink

[dlon]

Description of Change

Close #45166.

Shortcuts may store the PKEY_AppUserModel_ToastActivatorCLSID property as a string CLSID/UUID as opposed to VT_CLSID (i.e., bytes), resulting in NOTREACHED being hit. This PR adds support for VT_BSTR and VT_LPWSTR, and handles cases with and without braces around the ID.

Upstream CL: https://chromium-review.googlesource.com/c/chromium/src/+/6172220

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation, tutorials, templates and examples are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: fixed a crash caused by some PKEY_AppUserModel_ToastActivatorCLSID being represented by a string uuid rather than bytes

[welcome]

💖 Thanks for opening this pull request! 💖

Semantic PR titles

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Commit signing

This repo enforces commit signatures for all incoming PRs.
To sign your commits, see GitHub's documentation on Telling Git about your signing key.

PR tips

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[dlon]

Here is an upstream CL: https://chromium-review.googlesource.com/c/chromium/src/+/6172220

[codebytere]

hi @dlon - could you please describe the fix in the PR body as well?

[dlon]

@codebytere done!

[ckerr]

This looks familiar but I can't find the PR this reminds me of. 🤔 @dlon, is this a new iteration of another change?

Approving since the change LGTM. Also I appreciate that you're submitting the change upstream too. The best way to fix this in Electron is for the fix to land in Chromium and we pick it up automatically + cherry-pick the Chromium fix to older versions of Electron

[deepak1556]

@ckerr you are right, the previous one is #44784

[dlon]

It seems like upstream maintainers are reluctant to accept the patch, since they don't intend this to be a generic shortcut parser. Maybe it must remain a patch?

[codebytere]

Given we prefer to hew to upstream where possible - I don't think this passes the bar to be an indefinitely floated patch per our patch policy. It looks like upstream is willing to mitigate the crash, so adding that patch here would be fine by me but beyond that I think we should look to either document the issue or attempt mitigation within Electron's own codebase.

[dlon]

Given we prefer to hew to upstream where possible - I don't think this passes the bar to be an indefinitely floated patch per our patch policy. It looks like upstream is willing to mitigate the crash, so adding that patch here would be fine by me but beyond that I think we should look to either document the issue or attempt mitigation within Electron's own codebase.

The suggested upstream fix (suppressing NOTREACHED) will result in toastActivatorClsid not being set: https://www.electronjs.org/docs/latest/api/shell#shellreadshortcutlinkshortcutpath-windows. I think this works for our specific case, but is this acceptable?

As far as mitigating this within Electron, can it be done? One possible way is to implement the parser in Electron instead of using chromium for this.

[codebytere]

@dlon yes - as of now i think it's fine for us to mitigate the crash at cost of toastActivatorClsid not being set. @deepak1556 what do you think?

[dlon]

I've pushed the smaller upstream "fix". Also a check to prevent it from reading garbage as the field isn't zero-initialized.