kavimuru commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Issue was found when executing #9294

Action Performed:

Open app
Use autofill on the login screen
Navigate to Settings>Payments>Add Debit card
Use autofill

Expected Result:

Autofill should works during login Autofill should works while adding new Debit card

Actual Result:

No autofill for fields at login page No autofill for all fields while adding new Debit card

Workaround:

unknown

Platform:

Where is this issue occurring?

Desktop App

Version Number: Version 1.1.86-1

Reproducible in staging?: y

Reproducible in production?: y

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/181026173-67396956-4e27-4886-9d7b-047dd470e730.mp4

Upwork job URL: https://www.upwork.com/jobs/~01a9e983bc39d24141

Issue reported by: Applause internal team

Slack conversation:

View all open jobs on GitHub

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @marcochavezf (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

marcochavezf commented 2 years ago

I also noticed the password manager is not triggered when I enter the email in the desktop app and here's a discussion about a possible solution to enable autofill.

I think it's worth to make this issue External to see if we get a proposal to enable autofill on desktop. But feel free to close it if there's no possible solution.

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @michaelhaxhiu (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

michaelhaxhiu commented 2 years ago

Posted to upwork - https://www.upwork.com/jobs/~01a9e983bc39d24141

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @iwiznia (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

parasharrajat commented 2 years ago

The electron does not support autofill natively. This is a userland feature and saving username and passed is a security concern.

marcochavezf commented 2 years ago

The electron does not support autofill natively. This is a userland feature and saving username and passed is a security concern.

Ah yeah, definitively saving usernames and passwords in the electron app is not a good idea, but shouldn't password managers (like 1Password) detect the username/email field to autofill it automatically in macOS apps?

parasharrajat commented 2 years ago

Yes! the motive of this issue should be to set up the app to support native autofill services.

iwiznia commented 2 years ago

Still waiting for proposals. I think we need to increase price, no?

michaelhaxhiu commented 2 years ago

Yep, true. I got the reminder to double price on Friday last week but didn't have time to action it. Doubling now.

michaelhaxhiu commented 2 years ago

Not overdue as I just doubled the price

michaelhaxhiu commented 2 years ago

Doubling.

michaelhaxhiu commented 2 years ago

Doubling price again

michaelhaxhiu commented 2 years ago

Doubling

iwiznia commented 1 year ago

Not overdue

michaelhaxhiu commented 1 year ago

Doubling

tomivs commented 1 year ago

Is this issue still open to proposals? I might have a proposal. Asking because "Exported" label was removed.

parasharrajat commented 1 year ago

Yes, it is open

parasharrajat commented 1 year ago

@michaelhaxhiu let's pin this issue and post it in slack to bring to attention.

No one interested to catch this whale :scream:

aneequeahmad commented 1 year ago

@parasharrajat, i have been researching on this issue. I would post a proposal soon most prolly today or tomorrow .

michaelhaxhiu commented 1 year ago

Good call, I just pinned it. I'm going to double the value too, as it's been more than 7 days.

tomivs commented 1 year ago

Reason

Electron doesn't provide an interface for autofill because according to one of its main developers:

Electron doesn't ship the autofill feature of Chrome browser because only very few cases need it

Therefore, this feature has to be implemented on our side.

Proposal

We have two options:

Implementing our own autofill API (and storing and managing all data and passwords by ourselves) such as Brave did (https://github.com/brave/muon/pull/40)
Implementing autofill using something such as datalist along with node-keytar for managing and storing passwords using the system keychain.

In my case, I would rather go with the second one because this way we would be using the system keychain and therefore we would not have to manage and store passwords.

Here is a tutorial about node-keytar that breaks it down and explain some things in detail.

Also, I had found a repository that implemented autofill for Electron using node-keytar but I can't find it now. I'll keep trying to find it and I'll add it as reference to my proposal.

I'll be listening to any questions and clarifying any details about my proposal.

parasharrajat commented 1 year ago

We are not up for the first approach. This could create serious security concerns. Let's hear about the second approach.

you can go as much as detailed as you like or choose to use an external document if the proposal does not fit into the post.

azimgd commented 1 year ago

Proposal

To save the credentials, I propose to utilize Electron's safeStorage module to save the password or payment information. This module protects data stored on disk from being accessed by other applications or users with full disk access. Note that on Mac, access to the system Keychain is required (the same applies for Linux and Windows).

Create UI components for [save password] and [auto fill password]. Store encrypted information in AsyncStorage via Onyx.

(first string in the terminal above is encrypted console.log, second is decrypted)

UX

Create save password checkbox
Create a generic dropdown component to show saved credentials
- Do we want to show a checkbox [save this password] ? (Q to design-team)
- Do we want to show a popup [save this password] ? (Q to design-team)
Create a notification component that informs about private keychain access
- To avoid this: https://discussions.apple.com/thread/253741610
- Do we want to explain this to users ? (Q to design-team)
- Where do we want to show this ? (Q to design-team)

Auth credentials flow (first-time user)

User navigates to login screen, enters username
User enters password
User presses [save this password] checkbox
- We can also show the checkbox on successful login only, however this will trigger native popup
Native OS Keychain permissions popup is shown
Credentials are encrypted
Encrypted payload is stored into AsyncStorage via Onyx

Auth credentials autofill flow

User focuses on a username TextInput
Credentials are requested from Onyx
Dropdown with available credentials for that field are shown
User selects credential
Credentials are decrypted
Decrypted value is autofilled into TextInput

Authorize and save secret credentials

Check if OS supports Electron's safeStorage via safeStorage.isEncryptionAvailable()
Encrypt credentials via safeStorage.encryptString(plainText)
Store encrypted payload into RN-Onyx
- Migrations should be defined to avoid obsolete payload storage
Persist credentials on logout

Credentials schema

Store encrypted value mappings
- {key: 'credit-card', title: 'VISA **72', encrypted: '=123aosidj123'}
- This approach allows us to query sensitive information meta without decrypting
Define schema for available payload types
- authentication: {username, password}
- credit-card: {number, name, month, year, cvv}
Create a getter / setter class for keychain → safestorage → onyx interactions
Handle schema mismatch, e.g:
- user entered authentication credentials {username, password}
- credentials are encrypted and stored as a hash =123aosidj123
- application code was changed to utilize {login, password} format

TextInput Component

Distinguish between auto-fillable inputs, this should only be implemented for Electron environment
- Utilize secureTextEntry and autoCompleteType props
- Utilize process.env.ELECTRON_ENV
Subscribe to auto-fill event and override textinput when payload is received

// BaseTextInput [pseudo-code]

componentDidMount() {
  if (
    isElectronEnvironment && // only subscribe within electron environment
    this.props.autoCompleteType === 'password' // only subscribe on password or credit card information
  ) {
    Autofill.subscribeCredentialSelect(this.props.name, this.onAutofill) // this event will be triggered when user selects credential from UI Component
  }
}

componentWillUnmount() {
  Autofill.unsubscribeCredentialSelect(this.props.name, this.onAutofill) // unsubscribe when component is unmounted
}

onAutofill(response) {
  this.input.value = response // override with autofill response
}

onFocus() {
  if (
    isElectronEnvironment && // same conditions as in componentDidMount
    !this.input.value.length && // could be modified, but I assume user wants to auto-fill before typing
    this.props.autoCompleteType === 'password'
  ) {
    Autofill.showPopup(this.props.autoCompleteType) // Shows autofill popup with available credentials, such as username or a credit card
  }
}

render () {
  return <TextInput {...props} />
}

Autofill Library

let secureCredentials = [];

loadCredentials() {
  Onyx.connect({
    key: ONYXKEYS.SECURE_CREDENTIALS,
    callback: credentials => secureCredentials = credentials,
  });
}

getCredentialsOverview(credentialType = 'credit-card') {
  let credentials = secure.filter(item => item.type === credit-card) // filter by credential type e.g. so we don't return username 
  return credentials // encrypted values to show in UI e.g. 'VISA **72'
}

getCredential(credentialKey) {
  let credential = secure.find(item => item.key === credentialKey) // find credential to be decrypted
  safeStorage.decryptString(credential)
}

saveCredential(string) {
  let credential = safeStorage.encryptString(string)
  Onyx.merge(ONYXKEYS.SECURE_CREDENTIALS, [credential])
}

Autofill Component

Render popup component via React.Portal
Keep focused input reference we want to autofill
Similar to src/components/Popover.js

createPortal(
            <Modal
                type={CONST.MODAL.MODAL_TYPE.SECURE_AUTOFILL}
                popoverAnchorPosition={props.anchorPosition}
                shouldCloseOnOutsideClick
            />,
            document.body, // could be rendered as a global element, or straight into input field
);

Comparison against node-keytar

node-keytar will store each encrypted payload as a separate entry, which results in prompting OS authorization multiple times. safeStorage will only store a secret key which is used to decrypt secret payload.
node-keytar is not a storage service, but rather an api for OS specific keychain. Meaning that multiple node processes or chromium instances can access ~~secure~~ keychain entries.
node-keytar requires extra maintenance, specifically extra build steps per platform.

https://www.electronjs.org/ru/docs/latest/api/safe-storage
https://twitter.com/NurM4rvin/status/1494800475933356042 [discord uses safeStorage]
https://chromium.googlesource.com/chromium/src/+/master/docs/security/faq.md#Why-arent-physically_local-attacks-in-Chromes-threat-model follow up questions for physical attacks

mallenexpensify commented 1 year ago

@parasharrajat can you please review @azimgd 's proposal above?

parasharrajat commented 1 year ago

Thanks for the proposal. Sounds like a plan but I will have to do some research on this to understand.

Intitial Questions:

Do you have any example of a desktop app where autofill is working?

I propose to utilize Electron's safeStorage module to save the password or payment information.

But it looks are you are only using this module to encrypt. Then storing things on Onyx. Correct?

azimgd commented 1 year ago

Do you have any example of a desktop app where autofill is working?

https://user-images.githubusercontent.com/4882133/195590342-655d256e-4ee5-46c8-a135-9b5b3fc15a2a.mov

But it looks are you are only using this module to encrypt. Then storing things on Onyx. Correct?

Yes. Safe storage uses keychain to store an app specific cypher which is later utilized for any consecutive encryptString and decryptString method calls.

Storing encrypted payload as ArrayBuffer within IndexedDB (which is supported by Onyx via LocalForage). Note that decrypting requires correct cypher text stored in Keychain.

Here is an error I get when I try to decrypt a string with corrupt cypher (changed the cypher manually using Keychain Access).

Error: Error while decrypting the ciphertext provided to safeStorage.decryptString.

OS Prompt which requests access to Keychain

Sample code

Note: communication between node → react is done using ipcRenderer. sendSync blocks the node thread until callback is resolved.

// libs/SecureCredentials.js
import Onyx from 'react-native-onyx';
import ONYXKEYS from '../ONYXKEYS';
import ELECTRON_EVENTS from '../../desktop/ELECTRON_EVENTS';

let secureCredentials;
Onyx.connect({
    key: ONYXKEYS.SECURE_CREDENTIALS,
    waitForCollectionCallback: true,
    callback: val => secureCredentials = val,
});

function storeEncryptedCredentials() {
    const encrypted = window.electron.sendSync(ELECTRON_EVENTS.REQUEST_SECURE_CREDENTIALS, JSON.stringify({authorization: {username: 'azim', password: '123'}}));
    // eslint-disable-next-line rulesdir/prefer-actions-set-data
    Onyx.merge(ONYXKEYS.SECURE_CREDENTIALS, encrypted);
}

function fetchDecryptedCredentials() {
    const decrypted = window.electron.sendSync(ELECTRON_EVENTS.PERSIST_SECURE_CREDENTIALS, secureCredentials);
    return decrypted;
}

export {
    storeEncryptedCredentials,
    fetchDecryptedCredentials,
};

// desktop/main.js
ipcMain.on(ELECTRON_EVENTS.REQUEST_SECURE_CREDENTIALS, (event, credential) => {
  try {
      // eslint-disable-next-line no-param-reassign
      event.returnValue = safeStorage.encryptString(credential).toJSON();
  } catch (error) {
      // eslint-disable-next-line no-console
      console.log(error);
      // eslint-disable-next-line no-param-reassign
      event.returnValue = {};
  }
});

ipcMain.on(ELECTRON_EVENTS.PERSIST_SECURE_CREDENTIALS, (event, credential) => {
  try {
      // eslint-disable-next-line no-param-reassign
      event.returnValue = safeStorage.decryptString(Buffer.from(credential));
  } catch (error) {
      // eslint-disable-next-line no-console
      console.log(error);
      // eslint-disable-next-line no-param-reassign
      event.returnValue = {};
  }
});

therealJimoh commented 1 year ago

Is this issue still open?

parasharrajat commented 1 year ago

@therealJimoh Yes

azimgd commented 1 year ago

@parasharrajat any questions on the proposal above ?

parasharrajat commented 1 year ago

Haven't got the time to look into it? I need to review the overall approach to storing data in Onyx. This requires an internal discussion. I will try to summarize it on slack.

tomivs commented 1 year ago

Proposal

As I mentioned before, I am considering using node-keytar.

Some of the advantages:

Uses native system keychain
No need to worry about encrypting or decrypting
Gets system credentials straightforwardly from system keychain
No need to store data in Onyx

Example:

const keytar = require('keytar')

// add credentials
keytar.addPassword('ExpensifyApp', 'AccountName', 'secret');

// get all stored credentials to then use for suggestions
let credentials = keytar.findCredentials('ExpensifyApp');

Resources:

node-keytar GitHub repository with documentation and installing steps
node-keytar tutorial and break down
"WebCatalog" App that uses node-keytar (commit when it was implemented)

Clarifying some questions:

node-keytar will store each encrypted payload as a separate entry, which results in prompting OS authorization multiple times. safeStorage will only store a secret key which is used to decrypt secret payload.

They can certainly be separate entries, but the user will still be prompted OS authorization only for the first time.

node-keytar is not a storage service, but rather an api for OS specific keychain. Meaning that multiple node processes or chromium instances can access secure keychain entries.

While it's true that node-keytar is an API for system keychain, it's not true that any node processes will be able to access Expensify entries (or at least not in our case). The reason being that after we package our Electron app, the app name would be Expensify meaning that only Expensify process would be able to access it.

node-keytar requires extra maintenance, specifically extra build steps per platform.

It's a node module. So we would just have to install, import, and use it.

mallenexpensify commented 1 year ago

I will try to summarize it on slack.

@parasharrajat when do you do so, will you drop the link in here (for internal or #expensify-open-source post), thanks.

michaelhaxhiu commented 1 year ago

Would be good to get a small update here before EOW if possible 🙏

parasharrajat commented 1 year ago

I will share an update tomorrow. Haven't forgotten it. Basically, it will kick off in a few hours.

ntdiary commented 1 year ago

Proposal

Autofill is a component in the chromium project. Probably the most expensive way is that we enable it and modify its storage logic and compile our own electron. The first build took me several hours. (This way is not recommended 😂)

I think it's better to create a separate library to do it and modify our existing components as little as possible. We can learn the logic of chromium, just use js to render the popup and store password in a system password manager. (for example, use node-keytar, It doesn't prompt for OS authorization multiple times)

IMO, it's better to keep interactive habits similar to the browser:

can fill more than one field at a time, for example, it should also fill password after selecting username.
can filter candidates.
can manage the passwords or cards data (add / edit / delete).

Here are three demo videos, and I will update this proposal with more details in the next few days :

Some details

I create a new electron-autofill library, it will be published to npm after development. All our app needs to do is install and use it, minor modifications required, such as this:

-                    <View style={[this.props.style]}>
+                    <View style={[this.props.style]} accessibilityRole={ComponentUtils.ACCESSIBILITY_ROLE_FORM}>

It will add some event listeners on window or div#root (click for autofill, input for filtering)

When we click on an input that needs to be autofilled, it analyzes the input's attributes, even its label, to decide which type of pop-up box to open, such as password type or card type , address can also be supported. It also recognizes other input elements within the same form scope (The principle is complex and is still being perfected).

It will update the value of these inputs after we select a candidate. (maintain compatibility with react) After the form is submitted, it saves the relevant information. Password information will be saved in the system's keychain. The storage location of the card information can be discussed again, which can be localStorage, Onyx , or even keychain. (IMO, localStorage or Onyx are both ok)

In addition:

I think we shouldn't use safeStorage and store user password in front-end application. The cipher text can be decrypted, especially if the decrypted key is the same one shared by other electron applications. We can't modify this entry name by program unless we build electron from source code. It is a constant value in the chromium source code.

I guess these apps may have modified the entry name in their own chromium source code. 😂

parasharrajat commented 1 year ago

Looking now at all proposals.

parasharrajat commented 1 year ago

Looked all proposals. Going though node-keytar and safestorage for now.

@tomivs and @ntdiary your proposal are incomplete. Please provide more details when possible. Proposals should explain architecture of your code along with implementation in our app.

Note: A universal solution which can be applied to normally any autofillable form will be better. For now, this issue focuses on Add debit card and login forms.

ntdiary commented 1 year ago

I have updated some details in the Some details section, I'll update and discuss as necessary.

A universal solution which can be applied to normally any autofillable form will be better.

I hope to create a library (electron-autofill) for most electron apps that need it, and of course our app will be supported first. 😄

azimgd commented 1 year ago

Demo

https://user-images.githubusercontent.com/4882133/197191516-b5acb91d-292d-494c-ae40-f1de0cdbc326.mov

Known issues with demo

zIndex inconsistencies make some element overflow
Save password feature is not implemented, credentials are stored securely on first mount though

Source

https://github.com/azimgd/expensify-app/pull/1/files

A universal solution which can be applied to normally any autofillable form will be better

This approach is quite universal as I'm attaching (for demo purposes only) Dropdown component to src/component/RNTextInput. This could easily be decoupled further as it isn't dependent on anything else but TextInput and Onyx for now. Having a separate npm package is possible as well with this approach.

azimgd commented 1 year ago

Issues with node-keytar

https://github.com/atom/node-keytar/issues/88

the fact that when this is used in a Node process to store secrets any other Node process can trivially access those secrets is something that should be deeply troubling to project owners.

as it's clear that I'm not the only one confused by this - and people are most likely using this library blindly, and haven't gone to the effort that I did of creating another Electron app to test that the other app can't access the credentials - then I would strongly recommend updating the readme with a brief explanation of when this library should and shouldn't be used. Otherwise, it's doing more harm than good.

https://github.com/atom/node-keytar/issues/50

As I understand it, keytar stores credentials in the OS X keychain. Keychain automatically allows "the service which created the entry" to access or change the password. Testing this, however, it seems Keychain simply registers node as the service — meaning that any node process can arbitrarily look up any passwords set by keytar, with no confirmation by the user.

Build issues

https://github.com/atom/node-keytar/issues/359 https://github.com/atom/node-keytar/issues/406 https://github.com/atom/node-keytar/issues/245 https://github.com/atom/node-keytar/issues/50

ntdiary commented 1 year ago

I've read it all but I am still confused. Can someone make a conclusion, please?

Interesting. I also take this very seriously. I read the source code of both and tested some use cases, rather than just blindly choosing A or B based on the README or doc. I think I can do more testing for this and help come to a conclusion.

melvin-bot[bot] commented 1 year ago

@iwiznia, @michaelhaxhiu, @parasharrajat Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

@iwiznia, @michaelhaxhiu, @parasharrajat Whoops! This issue is 2 days overdue. Let's get this updated quick!

ntdiary commented 1 year ago

Test for `node-keytar` and `safeStorage`

repo

https://github.com/ntdiary/example, There are two electron project in this repo:

demo: this app wants to save passwords with node-keytar or safeStorage + indexedDB.
attacker: this app wants to steal passwords from demo app. (Just copy db files and key, very easy. For security reasons, more complex ways are not described here.)

platforms:

mac os 10.15.7
windows 10 21H1 (19043.2130)
Ubuntu 20.04.3 LTS

steps:

open demo with npm start or just double-click executable app.
open attacker app.
select demo app, fill in the password and save.
select attacker app, click steal button. See if it can steal the password quietly.

mac, unsigned app

https://user-images.githubusercontent.com/8579651/197464320-c88c2268-39ef-4fe6-9c2b-facc95da0650.mp4

safeStorage: yes
node-keytar: yes

mac, signed app

https://user-images.githubusercontent.com/8579651/197464338-7ade32ce-6f4c-44e0-a95e-87182d1d2d14.mp4

safeStorage: no. Authorization required.
node-keytar: no. Authorization required.

windows

https://user-images.githubusercontent.com/8579651/197464352-cbb39daa-8857-4ca3-b1b7-bdbfc2408c95.mp4

safeStorage: yes
node-keytar: yes

linux

https://user-images.githubusercontent.com/8579651/197464402-c089ae2f-b642-404f-b2e7-c608d13dc5fc.mp4

This repo doesn't yet support Linux, although it's easy to do. Now you can manually copy the key first if you want to test.

safeStorage: yes
node-keytar: yes

Some questions.

Q: Why do this test? I hope this test and post can help us better understand the security of these two modules.

Q: Why I didn't recommed safeStorage before? Actually, both modules are my alternatives. safeStorage had an initialization bug that happened to come across in my previous tests, which made me think it was using the key of Chromium Safe Storage and might conflict with other apps. But it has been fixed. the worry is gone.

Q: So is safeStorage safer? According to the test, I'm afraid not. node-keytar is just the interface of the system keychain. And safeStorage also use system keychain to store encryption key (It's different in Windows, where the key is stored in the Local State file). This problem requires OS support in the first place. I think I'd be happy to help do something as well if needed by then.

Extra

Just curious, do we need to explain the risk somewhere when we try to provide this autofill service? Looking forward to hearing more opinions. 🙂

Finally, I will provide more details and demo code about architecture logic in this week.

parasharrajat commented 1 year ago

Update: Reviewing Proposals...

azimgd commented 1 year ago

It's been almost 2 weeks without an update, so I would really appreciate if we can get any comment.

parasharrajat commented 1 year ago

There are holidays in India. I will try to share an update but expect some delay.

azimgd commented 1 year ago

Thank you for an update, do you have any estimates on when this could be reviewed ?

Expensify / App

[$32000] Desktop - Autofill doesn't work during login or when adding a debit card #10107

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

Reason

Proposal

Proposal

UX

Auth credentials flow (first-time user)

Auth credentials autofill flow

Authorize and save secret credentials

Credentials schema

TextInput Component

Autofill Library

Autofill Component

Comparison against node-keytar

Sample code

Proposal

Example:

Resources:

Clarifying some questions:

Proposal

mac

linux

windows

Some details

In addition:

Demo

Known issues with demo

Source

Issues with node-keytar

Build issues

Test for node-keytar and safeStorage

repo

platforms:

steps:

mac, unsigned app

mac, signed app

windows

linux

Some questions.

Extra

Test for `node-keytar` and `safeStorage`