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

Question: [js-flipper] supported device types, localhost connection, unique device id #3319

Closed
usrbowe opened this issue Jan 19, 2022 · 8 comments
Assignees

Comments

@usrbowe
Copy link
Contributor

usrbowe commented Jan 19, 2022

I've been testing the new js-flipper, and was planning to have a way to connect RN apps, without the native Flipper SDK (this might be also the way Expo could adopt Flipper).

There are few problems so far:

  1. the os package is no longer polyfilled by webpack (so also latest create react app will not be able to use the js-flipper).

  2. supported devicesBrowser or Node.JS is quite limiting factor. I think we could have a way to pass on device type when we run flipperClient.start to tell Flipper how to display the device and list supported plugins.

    export const detectOS = (): OS => {
    if (detectDeviceType() === 'Browser') {
    return 'Browser';
    }
    switch (require('os').type()) {
    case 'Linux':
    return 'Linux';
    case 'Darwin':
    return 'MacOS';
    default:
    return 'Windows';
    }

3. Valid websocket connection prefixes will only accept 'chrome-extension://', 'localhost:', 'http://localhost:', 'app://'. Which makes it impossible to connect from other device. Actually would like to know more details on why this limitation exist, maybe could have some opt out or other way to accept connections from non localhost.
For Node/RN can provide custom origin: localhost header.

const ok = getFlipperServerConfig().validWebSocketOrigins.some(
(validPrefix) => info.origin.startsWith(validPrefix),
);

  1. When running react-flipper-example, every time the connection is established there will be new device added and previous connection marked s Offline. I think we might need have some way to assign same browser/apps to existing connection.

image

@aigoncharov aigoncharov self-assigned this Jan 19, 2022
@usrbowe
Copy link
Contributor Author

usrbowe commented Jan 21, 2022

  1. IDEA: Actually for the non-localhost connections, would it be viable to provide QR code mechanism? To first get the Flipper host IP address and probably we could also exchange some passphrase to establish secure connection.

@aigoncharov
Copy link
Member

Hey @usrbowe! Thank you for your interest in js-flipper!

the os package is no longer polyfilled by webpack (so also latest create react app will not be able to use the js-flipper).

Did you try it or is it just a theory? I was thinking that the require block is never going to be reached by browsers, so it doesn't make sense to care much about polyfilling it.

I think we could have a way to pass on device type when we run flipperClient.start to tell Flipper how to display the device

Great idea! We definitely should add it to our list of options for start

list supported plugins

Do you think it would add significant value considering that we have addPlugin API?

Valid websocket connection prefixes will only accept 'chrome-extension://', 'localhost:', 'http://localhost:', 'app://'. Which makes it impossible to connect from other device. Actually would like to know more details on why this limitation exist, maybe could have some opt out or other way to accept connections from non localhost.

It should be possible to connect from any non-browser device by passing "Origin" header set to "localhost". For browsers, we have this list to prevent malicious sites developers could visit easily accessing Flipper's data. We should make it configurable by reading it from settings.json. Would it help?

When running react-flipper-example, every time the connection is established there will be new device added and previous connection marked s Offline. I think we might need have some way to assign same browser/apps to existing connection.

Nice catch! I guess start could accept getDeviceId: () => Promise<string>. Then for browsers we could write to LocalStorage. Node.js apps could implement their own persistence according to their app's needs.

@aigoncharov
Copy link
Member

  1. IDEA: Actually for the non-localhost connections, would it be viable to provide QR code mechanism? To first get the Flipper host IP address and probably we could also exchange some passphrase to establish secure connection.

Good thinking! We are actually exploring various ideas including one that looks really close to yours to re-think the entire certificate exchange flow.
The question here is which environment are we discussing? If it is a browser, what would be the case when a non-localhost client needs to connect to Flipper? If it is not a browser, then couldn't it simply pass "Origin" header set to "localhost" to get past the validator?

Speaking about security, it sounds like a great way to authorize connections between JS devices and Flipper! The only concern is with how it affects developer experience. @passy @mweststrate WDYT?

@usrbowe
Copy link
Contributor Author

usrbowe commented Mar 16, 2022

@aigoncharov thanks for reply, somehow missed this. I forgot to point that I'm testing this connection for simpler React Native connection (without the native Flipper SDK).

1. package os not available

update: resolved by #3319 (comment)
React: this is from fresh Create React App, after running yarn start:

Failed to compile.

Module not found: Error: Can't resolve 'os' in '/Users/username/my-app/node_modules/js-flipper/lib'
BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "os": require.resolve("os-browserify/browser") }'
        - install 'os-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "os": false }
...

React Native:, it seems metro will still execute the require by default (but there seems to be option to lazily invoke require).

2. Support devices

Actually, the deviceType would be only needed for device plugins (for other plugins, it would be invoked by addPlugin method).

3. WS localhost validation

I tested now again on React Native and realised it also have an option to specify origin headers (similar as ws package on NodeJS):

// reference for other connecting React Native
// let metroServerIP = set IP address of metro server (by default: localhost, for physical devices it will be the metro server host IP)
const flipperPort = 8333;
flipperClient.start('React Native App', {
  urlBase: `${metroServerIP}:${flipperPort}`,
  websocketFactory: url =>
    new WebSocket(url, undefined, {
      headers: {
        origin: `localhost:${flipperPort}`,
      },
    }),
});

So feels actionable items could be only:

  • 1. Add getDeviceId: () => Promise<string> to restore previous session
  • 2. Easier connection for Flipper (also allow discovery of devices/apps so don't have to rely on adb/idb)
  • 3. Resolve the os package issue, we could even remove the device detection and make it required enum option for .start method

@usrbowe
Copy link
Contributor Author

usrbowe commented Mar 16, 2022

The question here is which environment are we discussing? If it is a browser, what would be the case when a non-localhost client needs to connect to Flipper? If it is not a browser, then couldn't it simply pass "Origin" header set to "localhost" to get past the validator?

I think we could probably provide QR code on Flipper app, which devices could scan to at least get the IP address of host. Also this could cover the secure connection exchange.

@aigoncharov
Copy link
Member

Polyfilling issue for the os should be resolved by 245d263

facebook-github-bot pushed a commit that referenced this issue Jun 1, 2022
Summary: Linked to #3319

Reviewed By: passy

Differential Revision: D36786952

fbshipit-source-id: f3214f35039845a8e35fa14e63f509a6fbdddb1f
@aigoncharov
Copy link
Member

getDeviceId should be resolved by 055b14c

@usrbowe
Copy link
Contributor Author

usrbowe commented Jul 6, 2022

Nice, this works really well 👍

I think we can close this issues, as all major issues were resolved. For the connection, that can be done in general for entire Flipper, not just js connection.

@usrbowe usrbowe closed this as completed Jul 6, 2022
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

No branches or pull requests

2 participants