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

Migration to community WebView #4

Closed
vitvly opened this issue Oct 28, 2019 · 20 comments · Fixed by status-im/status-mobile#10090
Closed

Migration to community WebView #4

vitvly opened this issue Oct 28, 2019 · 20 comments · Fixed by status-im/status-mobile#10090
Assignees
Labels

Comments

@vitvly
Copy link

vitvly commented Oct 28, 2019

Migration to community WebView

Introduction

Currently we rely on react-native-webview-bridge library. It is an extension of RN's WebView that injects a special script into each webpage it loads. Its raison d'etre is allowing bidirectional communication between RN and WebView.

Current status

We maintain our own fork of react-native-webview-bridge, adding fixes for web3.js support for dapps, webview caching, permissions fixes, etc. Comprehensive list is in a comparison sheet.

webview-bridgelibrary uses sendToBridge() method for RN->WebView communication and send() method for WebView->RN communication.

In status-react codebase, we use them as

  • :browser/send-to-bridge fx (status-im.browser.core ns). Also used by :browser.callback/call-rpc fx)
  • :on-bridge-message (status-im.browser.views ns). Dispatches a :browser/bridge-message-received event.

Correspondingly, on WebView side there is a WebViewBridge global variable that has a send() method and a onMessage() method.

The problem

There are some issues with it, e.g.:

Why Don't We Just...?

We could go on with maintaining our webview-bridge fork, but then we'd have to roll out our own WKWebView (UIWebView replacement) support, and would skip on community updates to WebView.

Proposal

We should migrate to community WebView. There would be some slight API changes, e.g.:

Direction webview-bridge community webview
RN -> WebView WebViewBridge.sendToBridge() injectJavaScript() method or injectedJavaScript prop
RN -> WebView onMessage onMessage
WebView -> RN WebViewBridge.send() ReactNativeWebView.postMessage()
WebView -> RN onBridgeMessage onMessage

However, currently there are unsolved issues with it that prevent using community WebView as an (almost) drop-in replacement. We will maintain our own fork until these are merged upstream.

The most important is allowing for JS injection before the document load. It is required by web3.js, and previously Status team implemented their own fixes for Android and iOS.

A proposed plan is:

Stage 1

JS injection implementation. We might borrow ideas from react-native-webview/react-native-webview#229 so that:

  • on Android, override RNCWebViewClient.shouldInterceptRequest() and RNCWebViewClient.interceptRequest(). Use OkHttpClient during request interception to re-send a request.
    Another option is overriding ReactWebViewClient.onPageStarted() and invoking evaluateJavascript() as implemented in status-im/react-native-webview-bridge@3814679#diff-0182b1b9ed632c87dec03071732bcbddR186.
  • on iOS, use WKUserScript with injectionTime:WKUserScriptInjectionTimeAtDocumentStart param, and then call WKUserContentController.addUserScript.

Stage 2

Caching, cookies persistence.

Alternate route

Or, alternatively, we can extend the API that Status offers to Dapps. There is currently a StatusAPI object and a sendAPIRequest() function in provider.js. They provide endpoints for contact code fetching and QR code scanning.

There are some security issues associated with exposing more of Status internal API. These should be solved with a number of approaches, namely (credit goes to @corpetty for elaborating on this):

  • sanitization: making sure calls are properly formed according to a specification
  • permissions: dapps requesting access to chat key, profile attributes, some account, etc. should request specific permissions
  • sandboxing: dapps (and websites) shouldn't be able to talk outside of their context and given permissions

Stage 3

Permissions fixes + other.

Stage 4

To be elaborated on. We can expose Status chat functionality as an API to embedded WebViews. A postMessage/onMessage mechanism could be used to provide a nice, more abstract API to dapps so that they can use what Status already has as part of status-go. This will help to avoid duplicate functionality in Status itself and nested WebViews.

@rasom
Copy link

rasom commented Oct 28, 2019

dependency on UIWebView

I used WKWebView there because that was the only way to inject js before page loading on iOS. So I'm not sure why we depend on UIWebView, but probably it has been changed...

upd.
it is still there https://github.com/status-im/react-native-webview-bridge/blob/master/ios/RCTWKWebView.m#L55

@andremedeiros
Copy link

andremedeiros commented Oct 28, 2019

Some concerns from the Teller swarm that we should address:

@hesterbruikman
Copy link

hesterbruikman commented Oct 28, 2019

The ⬅️ on top of the UI doesn't take you back, instead it "closes" the browser /cc @hesterbruikman

This is a definite pain that results from our current browser navigation. The in-browser navigation (back, forward) is actually at the bottom. Recently we updated the back button at the top to a cross to at least better communicate that it closes the tab (status-im/status-mobile#8847).

The cross doesn't actually close the browser, only the tab. Improving tab-based browsing is part of window-based navigation but has lower priority compared to Chat in the Core team. If this 'kills' Teller we need to shout louder:)

GPS access would be nice to have

@andremedeiros we might need a process around API management when it comes to sharing Profile, Location or any other information the Core app has.

@vitvly
Copy link
Author

vitvly commented Oct 29, 2019

@rasom UIWebView might be a transitive dependency, I'll check, thanks for pointing this out.
Update: react-native-webview-bridge depends on "react-native-webview" "^6.11.1".

@yenda
Copy link
Contributor

yenda commented Oct 29, 2019

@andremedeiros I don't think murmur is useful here, there is already a whisper capable node in Status, no need to run the whole stack again in the browser. Instead we could provide access to whisper api with permissions or even better the higher level chat api of Status

@andremedeiros
Copy link

we could provide access to whisper api with permissions

@siphiuel do you think this could work? Us exposing custom APIs to dapps that can request permissions to use?

@vitvly
Copy link
Author

vitvly commented Oct 29, 2019

@andremedeiros in theory yes, postMessage/onMessage mechanism should handle this, i think we could build an abstraction on top of it. Saying "in theory" because i'd love to build an mvp of it :)

@corpetty
Copy link

While I love the idea of updating the webview to be more seemless with the rest of Status, I worry about permissions and how well we are sanitizing injected JS. The webview is going to be (already is) the most attacked surface on the entire app. We'll have to be damn sure we don't allow full access through this.

@corpetty
Copy link

GPS access would be nice to have

as of now we still request android.permission.ACCESS_FINE_LOCATION in permissions (and whatever the equivalent is in iOS). The audit asked why we do this and we haven't been able to answer, yet.

Why do we want location data?

@yenda
Copy link
Contributor

yenda commented Oct 31, 2019

@andremedeiros @rachelhamlin a high priority issue that could be in the scope of this one, or maybe outside because it is even more urgent is the fact that subscription to events are not supported in Status right now, and that is a pretty big deal for dapps status-im/status-mobile#8577

@andremedeiros
Copy link

andremedeiros commented Oct 31, 2019

Why do we want location data?

The Teller project has (had?) a feature where it would show a seller's approximate location on a map.

@iurimatias is this still a thing?

@richard-ramos
Copy link
Member

In regards to this feature: Exposing the Whisper API
We had a meeting today about a requirement we have in Teller Network where users should be notified when an action happens in a trade they're involved.

Two possible solutions were suggested by @yenda:

  1. Expose the whisper api thru via a one time permission to send messages from the dapp (the permission access would be requested each time the dapp wants to send a message)
  2. Enable a functionality to watch over contract events in the Status App, that will emit a notification if an event is emitted.

Of the two solutions, the first one seemed to be easiest to implement and include along with this webview change, so I'd like this feature to be priorized above the other issues mentioned here: #4 (comment)

@andremedeiros
Copy link

one time permission

I'm not sure we can support this. Reason being, I can envision scenarios where dapps will share hostnames for IPFS gateways, so it's hard to come up with a rule where the permissions are remembered and assigned to the correct dapps.

I could also be missing something and would love to be proven wrong.

@andremedeiros
Copy link

users should be notified when an action happens in a trade they're involved

Wouldn't this only work when the app is active and dapp is open?

@vitvly
Copy link
Author

vitvly commented Oct 31, 2019

Regarding web3.eth.subscribe support - some notes after call with @yenda:

  1. Our web3 provider right now is completely custom, and does not support subscriptions. In order to support subscriptions, it has to either override WebsocketProvider or IpcProvider; alternatively we could implement own logic for handling onMessage (do not confuse with WebViewBridge's onMessage) and dispatching to listeners
  2. Need to implement handling of subscriptions on status-react side, so that when we get a signal from status-go, we can dispatch an event to the right listener. Also we need to persist these subscriptions.

@richard-ramos
Copy link
Member

users should be notified when an action happens in a trade they're involved

Wouldn't this only work when the app is active and dapp is open?

The notification would be emited by an user actively using the dapp. i.e.: If a buyer is opening an escrow, the dapp would request permissions to that user to send a whisper message to the seller. Same case if the seller is funding an escrow, a message would be sent from seller to buyer.

@guylouis
Copy link

guylouis commented Nov 4, 2019

For Status (browser) to be used as a point of sales, we are implementing status-im/status-mobile#9275
web3.keycard.signTypedData can be called by any dApp and this pop-ups a screen requesting a keycard (not paired to the phone) to be tapped and it signs the transaction without PIN

Should this be listed here ?

cc @gravityblast

@vitvly
Copy link
Author

vitvly commented Nov 5, 2019

@yenda @guylouis I'd suggest moving things related to web3 opt-in extension (like API, web3.eth.subscribe, web3.keycard.signTypedData) to another issue. Seems that they would not affect the WebView engine itself, but rather content of the scripts we're injecting. What do you think?

@guylouis
Copy link

guylouis commented Nov 5, 2019

@siphiuel yes, makes sense. Thanks for the explanations this morning about this btw.

@yenda
Copy link
Contributor

yenda commented Nov 6, 2019

@siphiuel Yes I agree as well, it should be a separate issue. Subscribing is actually more urgent as well and should be prioritized

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants