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

feat: webxdc: make isSecureContext === true #3413

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

WofWca
Copy link
Collaborator

@WofWca WofWca commented Sep 25, 2023

...for the apps.

Related test: webxdc/webxdc-test#24

I have tested it with webxdc/webxdc-test#24 and it works.

@Simon-Laux
Copy link
Member

changelog missing

@WofWca
Copy link
Collaborator Author

WofWca commented Sep 26, 2023

Added a changelog entry

@@ -35,6 +35,25 @@ protocol.registerSchemesAsPrivileged([
{
scheme: 'webxdc',
privileges: {
// This gives apps access to APIs such as
// - Web Cryptography
// - Clipboard
Copy link
Collaborator

Choose a reason for hiding this comment

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

So whenever I open a webXDC app it will be able to take my clipboard and paste it into the chat?

Copy link
Member

Choose a reason for hiding this comment

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

probably not, but I guess we should test against that before merging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have tested it. It's already the case, even without isSecureContext. Try

window.addEventListener('DOMContentLoaded', () => {
  const textarea = document.createElement('textarea');
  document.body.appendChild(textarea);
  textarea.focus();
  setTimeout(() => {
    document.execCommand('paste');
  });
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

:( Then I guess it is a bug, would be nice to work around this somehow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@WofWca WofWca Sep 27, 2023

Choose a reason for hiding this comment

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

So I removed the "Clipboard" point because yeah, it requires permission, which is currently denied (the write() API too)
(but document.execCommand('paste') still works, but it's not directly related to this MR).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is to say, clipboard-related security is not affected by this MR.

@r10s
Copy link
Member

r10s commented Sep 27, 2023

did not follow all discussions, but only looking at the pr, it is not clear to me, which real-world-issue this pr will fix.

esp. as it seems there are some potential side effects, it would be helpful to elaborate the gist in the pr description a bit more

@WofWca
Copy link
Collaborator Author

WofWca commented Sep 27, 2023

it is not clear to me, which real-world-issue this pr will fix

If you want an example, crypto.randomUUID() will become available, which, for example, is used in the calendar app, with a fallback to Math.random(). Web Crypto API also offers some cryptography stuff, which might be useful for a "secret chat" webxdc app we talked about, like https://github.com/DavidSM100/CryptoText.
I have looked for a bit for other examples besides Web Share and Web Crypto API that require a secure context but don't require a permission but couldn't find one.

The main point of this MR as I see it is to bring us a step closer to giving apps access to camera, geolocation and other stuff that requires permissions. See the forum post I linked in the comments.

Also FYI isSecureContext is already true for DC iOS and Android.

@WofWca
Copy link
Collaborator Author

WofWca commented Sep 27, 2023

To give a background for what's "secure context": isSecureContext is true for all websites served with https (well, except for those that are opened in an iframe on a website that is not served securely (i.e. http), and some other cases).

@r10s
Copy link
Member

r10s commented Sep 27, 2023

thanks for more detailed explanations! (it is much clearer now than trying to search information from some linked issues with linked URLs each. a good PR description cannot be overestimated :)

having crypto api would be nice, indeed. also indeed, it now sounds reasonable that webxdc apps should be considered "secure context" if https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts says "Locally-delivered resources [...] are also considered to have been delivered securely."

however, i would leave final decision up to the desktop maintainers or ppl who are more into security.

@Simon-Laux Simon-Laux merged commit ba86a92 into deltachat:master Sep 27, 2023
5 of 6 checks passed
@WofWca WofWca deleted the webxdc-isSecureContext branch October 31, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants