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.

Sign up for GitHub

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

Jump to bottom

chore: Show FIDO devices in the chooser if allowed #40216

Merged
merged 4 commits into from Oct 18, 2023
Merged

chore: Show FIDO devices in the chooser if allowed #40216

merged 4 commits into from Oct 18, 2023

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 16, 2023 •
edited

Description of Change

Refs CL:3309164

This CL allows devices containing a top-level collection with a usage
from the FIDO usage page to appear in the WebHID device chooser if
the requesting origin is allowed to access FIDO.

Refs CL:3595839

This CL makes sure users can see why a HID device does not show up in the browser picker to help debugging. See https://stackoverflow.com/a/71922823/422957 for context

Refs CL:3809913

There is a smaller window between BrowserContext and HidService
destruction timing in service worker condition, which creates an UAF
issue as the stored BrowserContext pointer can still be accessed by the
HidService. This is fixed by using weak pointer of
ServiceWorkerContextCore to access BrowserContext when it is valid.

Checklist

Release Notes

Notes: none

@codebytere codebytere added semver/patch backwards-compatible bug fixes 27-x-y 28-x-y labels Oct 16, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 16, 2023
jkleinsc
jkleinsc requested changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change removes functionality currently supported by Electron and replaces it with Chromium specific logic around allowing devices when a specific extension is installed. I'm not sure I see the use case to do this.

@codebytere codebytere force-pushed the better-hid-debugging branch 2 times, most recently from dd28916 to e124680 Compare October 16, 2023 19:16
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 17, 2023
@codebytere
Copy link
Member Author

Changes addressed following offline conversation ☝️

1 similar comment
@codebytere
Copy link
Member Author

Changes addressed following offline conversation ☝️

jkleinsc
jkleinsc requested changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM; just needs a few tweaks to more closely follow the changes in https://chromium-review.googlesource.com/c/chromium/src/+/3809913

shell/browser/hid/electron_hid_delegate.cc Outdated Show resolved Hide resolved
shell/browser/hid/electron_hid_delegate.cc Outdated Show resolved Hide resolved
shell/browser/hid/electron_hid_delegate.cc Outdated Show resolved Hide resolved
shell/browser/hid/electron_hid_delegate.cc Show resolved Hide resolved
shell/browser/hid/electron_hid_delegate.cc Outdated Show resolved Hide resolved
@jkleinsc jkleinsc added target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. and removed 27-x-y 28-x-y labels Oct 18, 2023
@jkleinsc jkleinsc merged commit 025af35 into main Oct 18, 2023
18 checks passed
@jkleinsc jkleinsc deleted the better-hid-debugging branch October 18, 2023 23:19
@release-clerk
Copy link

release-clerk bot commented Oct 18, 2023

No Release Notes

@trop trop bot mentioned this pull request Oct 18, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Oct 18, 2023

I have automatically backported this PR to "27-x-y", please check out #40274

@trop trop bot added in-flight/27-x-y and removed target/27-x-y PR should also be added to the "27-x-y" branch. labels Oct 18, 2023
@trop trop bot mentioned this pull request Oct 18, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Oct 18, 2023

I have automatically backported this PR to "28-x-y", please check out #40275

@trop trop bot added in-flight/28-x-y and removed target/28-x-y PR should also be added to the "28-x-y" branch. labels Oct 18, 2023
codebytere added a commit that referenced this pull request Oct 19, 2023
@codebytere @jkleinsc
chore: Show FIDO devices in the chooser if allowed (#40216)
f05a03e 
* chore: Show FIDO devices in the chooser if allowed

* chore: tweak HidChooserContext::IsFidoAllowedForOrigin

* chore: feedback from review

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
@trop trop bot added merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. and removed in-flight/27-x-y in-flight/28-x-y labels Oct 19, 2023
codebytere added a commit that referenced this pull request Oct 20, 2023
@trop @codebytere @jkleinsc
chore: Show FIDO devices in the chooser if allowed (#40275)
d1bf107 
chore: Show FIDO devices in the chooser if allowed (#40216)

* chore: Show FIDO devices in the chooser if allowed

* chore: tweak HidChooserContext::IsFidoAllowedForOrigin

* chore: feedback from review

---------

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
felixrieseberg pushed a commit to felixrieseberg/electron that referenced this pull request Oct 25, 2023
@codebytere @jkleinsc @felixrieseberg
chore: Show FIDO devices in the chooser if allowed (electron#40216)
fa8ee7c 
* chore: Show FIDO devices in the chooser if allowed

* chore: tweak HidChooserContext::IsFidoAllowedForOrigin

* chore: feedback from review

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@codebytere @jkleinsc
chore: Show FIDO devices in the chooser if allowed (electron#40216)
d17a00a 
* chore: Show FIDO devices in the chooser if allowed

* chore: tweak HidChooserContext::IsFidoAllowedForOrigin

* chore: feedback from review

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@zcbenz zcbenz Awaiting requested review from zcbenz

@deepak1556 deepak1556 Awaiting requested review from deepak1556

Assignees
No one assigned
Labels
merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@codebytere @jkleinsc