send uppyAuthToken via wss #4110

S0c5 · 2022-09-14T15:15:08Z

Hello guys!

This PR solves #4107,

when users have Instagram installed on android and get redirected to authorize, the app is opened and after this, the app opens the browser to the authorization url, it provokes a lost of context for window.opener, meaning that the token is not sent to the client via postMessage

this PR sends the uppyAuthToken via WSS using a token reference for wss.

It was tested locally and added the tests to callbac.js in the companion package.

… when plugins authenticate via companion on mobile devices

mifi

Hi and thanks for attempting to fix this!
I think your PR contains some unrelated changes that should be removed.
Also I'm not sure I understand why it doesn't work currently. Is it because on Android window.opener is undefined?
I think also some tests broke (hang forever)

mifi · 2022-09-14T19:47:01Z

.yarnrc.yml

+enableGlobalCache: false
+
 initScope: uppy

-enableGlobalCache: false
 nodeLinker: node-modules

 plugins:
  - path: .yarn/plugins/@yarnpkg/plugin-workspace-tools.cjs
    spec: "@yarnpkg/plugin-workspace-tools"
  - path: .yarn/plugins/@yarnpkg/plugin-version.cjs
    spec: "@yarnpkg/plugin-version"
+
+yarnPath: .yarn/releases/yarn-3.2.1.cjs


these changes seem unrelated

mifi · 2022-09-14T19:47:32Z

packages/@uppy/companion/src/server/controllers/send-token.js

@@ -17,7 +18,7 @@ const htmlContent = (token, origin) => {
    <head>
        <meta charset="utf-8" />
        <script>
-          window.opener.postMessage(${serialize({ token })}, ${serialize(origin)})
+          if (window.opener) window.opener.postMessage(${serialize({ token })}, ${serialize(origin)})


why is this needed?

given that window.opener is null, next line is not executed when window.close.

According to https://developer.mozilla.org/en-US/docs/Web/API/Window/opener:

In the following cases, the browser does not populate window.opener, but leaves it null:

The opener can be omitted by specifying rel=noopener on a link, or passing noopener in the windowFeatures parameter.

Windows opened because of links with a target of _blank don't get an opener, unless explicitly requested with rel=opener.

Having a Cross-Origin-Opener-Policy header with a value of same-origin prevents setting opener. Since the new window is loaded in a different browsing context, it won't have a reference to the opening window.

Writing the code in order that it doesn't crash when window.opener === null is definitely worth it 👍

I vaguely remember the discussion around the reason for not checking window.opener before calling it. IIRC the reason was that it is better that the page crashes with an error message in the browser console than having it just close the page without any error or anything happening. I think if we add this check we will not why it's not working if this happens again in the future.

mifi · 2022-09-14T19:48:32Z

packages/@uppy/companion/src/server/emitter/index.js

@@ -11,7 +11,7 @@ let emitter
 module.exports = (redisUrl, redisPubSubScope) => {
  if (!emitter) {
    emitter = redisUrl ? redisEmitter(redisUrl, redisPubSubScope) : nodeEmitter()
+    Object.assign(emitter, { __TEST__: Math.random() })


what's the purpose of this?

mifi · 2022-09-14T19:50:54Z

packages/@uppy/companion/test/__tests__/callback.js

    <!DOCTYPE html>
    <html>
    <head>
        <meta charset="utf-8" />
        <script>
-          window.opener.postMessage({"token":"${token}"}, "http:\\u002F\\u002Flocalhost:3020")
+          if (window.opener) window.opener.postMessage({"token":"${token}"}, "http:\\u002F\\u002Flocalhost:3020")


mifi · 2022-09-14T19:51:51Z

packages/@uppy/companion/test/mockoauthstate.js

@@ -3,6 +3,8 @@ module.exports = () => {
    generateState: () => 'some-cool-nice-encrytpion',
    addToState: () => 'some-cool-nice-encrytpion',
    getFromState: (state, key) => {
+      if (key === 'callbackToken') return 'client-token'


what's the purpose of this?

I can remove that, I was reading the code and adding a few lines to test how that mocks works.

mifi · 2022-09-14T19:54:31Z

packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

@@ -255,26 +256,30 @@ export default class ProviderView extends View {
      // Check if it's a string before doing the JSON.parse to maintain support
      // for older Companion versions that used object references
      const data = typeof e.data === 'string' ? JSON.parse(e.data) : e.data
+      window.removeEventListener('message', handlePostMessageToken)
+      handleToken(data)


not sure what's happening here, care to explain with a comment?

mifi · 2022-09-14T20:08:12Z

yarn.lock

@@ -34991,7 +34992,7 @@ hexo-filter-github-emojis@arturi/hexo-filter-github-emojis:

 "typescript@patch:typescript@*#~builtin<compat/typescript>, typescript@patch:typescript@^4.0.3#~builtin<compat/typescript>, typescript@patch:typescript@^4.6.2#~builtin<compat/typescript>, typescript@patch:typescript@~4.8#~builtin<compat/typescript>":
  version: 4.8.2
-  resolution: "typescript@patch:typescript@npm%3A4.8.2#~builtin<compat/typescript>::version=4.8.2&hash=f456af"
+  resolution: "typescript@patch:typescript@npm%3A4.8.2#~builtin<compat/typescript>::version=4.8.2&hash=7ad353"


what's the purpose of this change?

mifi · 2022-09-15T10:05:02Z

After digging and thinking some more, I think I understand the problem here:

In the normal auth flow with Uppy:

User clicks auth
Browser Tab1 (Uppy) pops up another browser Tab2 (Auth flow)
Tab2 runs the auth flow with the provider
Tab2 auth flow redirects to companion's callback endpoint, which returns HTML that calls window.opener.postMessage(token) to send the token back to Tab1
Tab2 calls window.close() to close Tab2
Tab1 finishes the auth with the received auth token

However in the case of Instagram, it's a bit different:

Browser Tab1 opens Tab2 with Instagram auth flow
Tab2 opens Instagram app
Instagram app runs auth flow and returns to Tab2
Tab2 auth flow redirects to companion's callback endpoint, which returns HTML that calls window.opener.postMessage(token) to send the token back to Tab1
However Tab2 window.opener is now null and it crashes, and there is no way for Tab2 to message the token back to Tab1.

With your PR it will instead do this:

Browser Tab1 connects to companion's websocket
Tab1 opens Tab2 with the Instagram auth
Tab2 opens Instagram app
Instagram app runs auth flow and returns to Tab2
Tab2 auth flow redirects to companion's callback endpoint, which returns HTML that calls window.close() to close Tab2.
Companion will also emit a token websocket event to all connected websockets
Tab1 will receive this token websocket token event to finish the auth

Is my understanding correct? If so, maybe it would make sense to completely remove the whole window.opener logic and instead only go with websockets if that is 100% guaranteed to work, while window.opener is not guaranteed to work.

related: https://stackoverflow.com/questions/51180645/window-opener-is-null-on-android-chrome-but-not-in-incognito

Murderlon · 2023-09-07T07:04:18Z

here are GPT's thoughts on handling the Instagram auth token problem:

https://chat.openai.com/share/ac97acff-bfab-49c3-8997-e9d6a111bdc2

github-actions bot added the pending end-to-end tests label Sep 14, 2022

send uppyAuthToken via wss to avoid lost of context for window.opener…

d11dab1

… when plugins authenticate via companion on mobile devices

S0c5 force-pushed the fix/send-uppy-auth-token-via-wss branch from de8a938 to d11dab1 Compare September 14, 2022 15:15

Murderlon requested review from aduh95 and mifi September 14, 2022 15:23

mifi requested changes Sep 14, 2022

View reviewed changes

aduh95 assigned mifi May 24, 2023

StrixOSG mentioned this pull request Aug 16, 2023

Auth redirect to blank page & Cannot read properties of null (reading 'postMessage') #4107

Open

2 tasks

aduh95 removed their request for review September 25, 2023 07:50

edanweis mentioned this pull request Jul 15, 2024

Uncaught TypeError: Cannot read properties of null (reading 'postMessage') in Companion's Callback Endpoint #5334

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send uppyAuthToken via wss #4110

send uppyAuthToken via wss #4110

S0c5 commented Sep 14, 2022 •

edited

Loading

mifi left a comment

mifi Sep 14, 2022

mifi Sep 14, 2022

S0c5 Sep 14, 2022

aduh95 Sep 15, 2022 •

edited

Loading

mifi Sep 15, 2022

mifi Sep 14, 2022

mifi Sep 14, 2022

mifi Sep 14, 2022

S0c5 Sep 14, 2022

mifi Sep 14, 2022

mifi Sep 14, 2022

mifi commented Sep 15, 2022 •

edited

Loading

Murderlon commented Sep 7, 2023

send uppyAuthToken via wss #4110

Are you sure you want to change the base?

send uppyAuthToken via wss #4110

Conversation

S0c5 commented Sep 14, 2022 • edited Loading

mifi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mifi commented Sep 15, 2022 • edited Loading

Murderlon commented Sep 7, 2023

S0c5 commented Sep 14, 2022 •

edited

Loading

aduh95 Sep 15, 2022 •

edited

Loading

mifi commented Sep 15, 2022 •

edited

Loading