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

Secure client-side storage on playground.wordpress.net to avoid going through the GitHub connection flow so often #1395

Open
adamziel opened this issue May 14, 2024 · 5 comments

Comments

@adamziel
Copy link
Collaborator

adamziel commented May 14, 2024

Authorizing GitHub is a painful and annoying process. We don't store the token anywhere so you have to do it every. single. time. In the process, you're losing the running Playground instance. Let's find a secure way to store that token for later.

The problem

Suppose we authorized GitHub and wanted to store the OAuth token for later. How can we do that securely?

If the "browser app" (index.html) stores it in localStorage, a clever attacker could use the fact that WordPress (remote.html) is running on the same origin and craft a Blueprint to simply steal it from localStorage. The same goes for IndexedDB, OPFS, and probably all forms of client-side storage.

If the "browser app" stores is on the server, it would mean sending an authorized fetch() request to save or expose the OAuth token. The fetch() would be authorized client based on a cookie, a GET or POST parameter, or a custom header. In each scenario, WordPress could issue the same fetch and steal the OAuth token.

Possible solutions

Expose a client-side storage only to index.html and no other page

Above I said there's no safe client-side storage option, but now I want to challenge that.

For once, I wonder if there's a way of storing credentials in a service worker in a way that would only be accessible via index.html but not remote.html.

  • Service workers also have access to an isolated persistent "cache storage".
  • I think you can't replace a Service Worker with a script from another domain or by a "virtual file" created via a Blueprint.
  • When serving fetch() requests, the service worker has access to information about the Client issuing the request:
const client = await self.clients.get(event.clientId);
client.frameType === 'top-level'  // or 'auxiliary', 'none', 'nested'
client.clientType === 'window' // or 'worker', 'sharedWorker', 'all'
client.url

Putting it all together, perhaps the service worker could provide a set of /kv/get, /kv/set, /kv/delete endpoints that would only respond to fetch() requests from top-level index.html pages, but not to ones made by WordPress – either the iframed one or a top-level one running via "open in a new tab".

This solution would require:

  • Rigorously confirming all the assumptions listed above.
  • A pre-mortem investigating all the potential ways this could go wrong. For example, could any clever in-WordPress index.html file work around our checks?
  • Cross-browser testing.

Even then, it would be risky as a single missed assumption could expose that storage to

Therefore, let's put index.html and remote.html on different domains.

Separate origins for 'the browser app' and remote.html

Either:

  1. Leave index.html on https://playground.wordpress.net and move remote.html to something like https://api.playground.wordpress.net.
  2. Leave remote.html on https://playground.wordpress.net and move index.html to something like https://app.playground.wordpress.net.

UX-wise, I like the option number 1 better as typing https://playground.wordpress.net feels more natural than https://api.playground.wordpress.net.

Here's what we'd need to consider before the split:

  • Update the local development setup to use the same domain setup.
  • Decide between subdomains (playground.wordpress.net and api.playground.wordpress.net) and same-level domains (playground.wordpress.net and playground-api.wordpress.net). A few factors to consider:
    • Mutual access to cookies, storage mechanisms, workers, service worker registration.
    • Ability to pass transferrable objects between the origins.
    • Ability to pass OPFS and NativeFS directory handles to Playground iframe. Otherwise the "browser storage" and "local directory" features would break.
  • Ensure remote.html and its related files can't be accessed via playground.wordpress.net and that index.html and its related files can't be accessed via api.playground.wordpress.net.
  • Assess all the consequences of not running the service worker at the "browser app" domain. Are there any fetch() requests assuming a service-worker-provided handler?
  • Ensure all the OAuth redirects point at the "browser app" domain.
  • Ensure all the existing Playground apps keep working. If someone embeds remote.html in an iframe, would that iframe still work if it 301 redirects to https://api.playground.wordpress.net/remote.html?queryParams#hashParams?
  • The "browser app" origin would have to be XSS–free. However, Blueprints are currently executed in index.html and we also have a builder.html app and some demos – is any of that a problem? I don't think there's a way to same-origin eval() JavaScript from a Blueprint. Are there any other risks?
  • Double-check any playground.wordpress.net–hosted HTML files for XSS

The Privacy Sandbox rabbit hole is relevant. If we do separate domains but need to mark them as "same site" for any reason, we could probably do that.

Any other options?

If you have any other, potentially simpler ideas, let's discuss them!

cc @bgrgicak @dmsnell @brandonpayton
FYI @henriqueiamarino @sarahnorris @beafialho

@adamziel adamziel changed the title Secure client-side storage on playground.wordpress.net Secure client-side storage on playground.wordpress.net to avoid going through the GitHub connection flow so often May 14, 2024
@adamziel adamziel added this to the User Experience milestone May 14, 2024
@bgrgicak
Copy link
Collaborator

Why is the current solution bad? GitHub access is extremely sensitive in some cases and asking for auth every time is acceptable for me personally.

If not storing the data is more secure could we optimize the current auth flow to make it easy to use?

@adamziel you mentioned a redirect that causes us to lose data, could we avoid losing the data?
We could try to use Window: postMessage(), open the auth flow in a new tab, and send the token back to the original tab without reloading it.
Alternatively, we could explore how to store the state and restore it after redirecting, but that sounds more complicated.

@adamziel
Copy link
Collaborator Author

could we avoid losing the data?

Oh, I like that! It would make the repeated OAuth flow unnoticeable. The only problem to solve now is – how to enable initiating and concluding the flow from index.html but not from remote.html.

@adamziel
Copy link
Collaborator Author

I wonder if this popup would do the trick:

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Popup</title>
    <script>
        window.addEventListener('message', (event) => {
            if (event.origin !== window.location.origin) return; // Ensure same origin
            if (event.source !== window.opener) return; // Ensure it's from the opener

            // Verify the opener's URL to ensure it's index.html
            try {
                const openerUrl = new URL(window.opener.location.href);
                if (openerUrl.pathname !== '/index.html') return; // Ensure it's index.html
            } catch (e) {
                return; // In case of any error, reject the message
            }

            if (event.data === 'Hello from index.html') {
                console.log('Message from index.html:', event.data);
                event.source.postMessage('Hello from popup.html', event.origin);
            }
        });

        function sendMessageToIndex() {
            if (window.opener) {
                window.opener.postMessage('Hello from popup.html', window.location.origin);
            }
        }
    </script>
</head>
<body>
    <button onclick="sendMessageToIndex()">Send Message to Index</button>
</body>
</html>

@bgrgicak
Copy link
Collaborator

This should work. If window.opener causes issues, we can do something like this https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#examples

@brandonpayton
Copy link
Member

Why is the current solution bad? GitHub access is extremely sensitive in some cases and asking for auth every time is acceptable for me personally.

@bgrgicak Thank you for bringing this up. It is a good observation, and I find auth-each-time not only acceptable but desirable compared to storing credentials.

In addition, would it be possible to:

  • put a short-lived lifespan on the authorization
  • switch to more specific, granular authorization
    ?

If not storing the data is more secure could we optimize the current auth flow to make it easy to use?

❤️

Possible solutions

Separate origins for 'the browser app' and remote.html

I wonder if this is a good idea to do eventually, regardless of what we do with GitHub auth flow.

On a related note, it might be good to ask adjacent security experts for a general audit of Playground on the web. It would be interesting to see if there any concerns we haven't considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants