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(useClipboard): fix issue when permission is not defined #3812

Merged
merged 3 commits into from Feb 27, 2024

Conversation

Mister-Hope
Copy link
Contributor

@Mister-Hope Mister-Hope commented Feb 22, 2024

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.

  • Read the Pull Request Guidelines.

  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.

  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).

  • Ideally, include relevant tests that fail without this PR but pass with it.

    No, it's hard to simulate a browser like wechat browser

Description

There are a lot of built-in browsers (Chinese app likes them), which does not implement permission api return value, for example on latest Wechat client 8.0.47, the demo of the usePermission page looks like this:

Screenshot_2024-02-22-14-21-02-41_e39d2c7de19156b

That is because when calling with usePermission('xxx'), these browsers return undefined (including the 'clipboard-write' and 'clipboard-read' used in useClipboard

When the value is undefined, we'd better treat it as denied, e.g. wechat browser just denied it with out any asking for security reasons:

Screenshot_2024-02-22-13-58-03-41_e39d2c7de19156b

The issue is included in #3379 as a feature for falling back to legacy if possible.

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Feb 22, 2024
Copy link
Contributor

@posva posva left a comment

Choose a reason for hiding this comment

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

In this case, it's more about the feature being supported or not. So the fix should likely go in isClipboardApiSupported

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Feb 22, 2024

I think in the example case I showed, clipboard related api is supported, but they just being disabled for security concern. So the original check is valid, we just need a stricter check before using it.

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Feb 22, 2024

@posva Also I would request you to review #3813. This fix still can not cover all use cases.

with undefined or prompt, but still always refused to read and write when requesting for security.

some app may return prompt in read, but no dialog could be triggered, and the read request always fail.

Copy link
Contributor

posva commented Feb 22, 2024

If it always fail, what part of the API is exactly supported?

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Feb 22, 2024

The clipboard API is supported by WeChat browsers, and a whitelist of websites can call clipboard.write() (don't be too amazing, this is how big company in China works). It's just normal website is not allowed "for security consideration".

China is an amazing place where you can find loan feature in almost every app, like food delivery apps, social media apps, shopping app and so on. Every app want you to stay in its ecosystem, so we have to deal with internal browsers in these apps. A lot of their behavior is non-standard, while they may inject some special Apis to global, and allow you to call "share menu" and such things.

Anyway, it makes no sense that a browser have a clipboard object on navigator but with no functionality. It's just they are disabled in normal case "for users' security", I hate these either. So IMO, the original isSupported logic is good to display where clipboard is available, both users and the browser could disable write and read permission in cases, which this pr try to take better handling.

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Feb 22, 2024

I made an update on the above comment, and if you inisit, I can do a full refactor, to only set isSupported to true if clipboard.write() permission is granted only. Reply this thread to confirm that.

@posva
Copy link
Contributor

posva commented Feb 22, 2024

I think the version is fine. I guess the undefined value is not meant to be valid? 😅 . I'm not 100% sure but it doesn't seem to be covered by the spec. I think it's worth adding a short comment about why not just !== denied for future maintainers. FYI I'm not making any calls here, just trying to help in the maintenance 😄

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 27, 2024
@antfu antfu enabled auto-merge February 27, 2024 04:28
@antfu antfu added this pull request to the merge queue Feb 27, 2024
Merged via the queue into vueuse:main with commit a9f02dd Feb 27, 2024
8 checks passed
noook pushed a commit to noook/vueuse that referenced this pull request Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants