FlowCrypt / flowcrypt-browser

FlowCrypt Browser extension for Chrome and Firefox
https://flowcrypt.com
Other
374 stars 46 forks source link

find a way to stop using tabs permission #2497

Open tomholub opened 4 years ago

tomholub commented 4 years ago

I played around with the permissions for a bit

It seems that the highest permission we require is the tabs permission. When present, this will show:

image

So we'll have to figure out how to stop using the tabs permission, but I'll leave that for another issue.

For this issue, I'll take the risk. I'll update 10% of users first and wait for complaints to roll in, if any. If they don't, we'll update everybody. If they do, we'll re-evaluate and maybe roll back in next update, maybe not.

Originally posted by @tomholub in https://github.com/FlowCrypt/flowcrypt-browser/issues/1897#issuecomment-577833259

tomholub commented 4 years ago

One way we use .tabs is to get information about active tab:

  public static getActiveTabInfo: Bm.AsyncRespondingHandler = () => new Promise((resolve, reject) => {
    chrome.tabs.query({ active: true, currentWindow: true, url: ["*://mail.google.com/*", "*://inbox.google.com/*"] }, (activeTabs) => {
      if (activeTabs.length) {
        if (activeTabs[0].id !== undefined) {
          type ScriptRes = { acctEmail: string | undefined, sameWorld: boolean | undefined }[];
          chrome.tabs.executeScript(activeTabs[0].id!, { code: 'var r = {acctEmail: window.account_email_global, sameWorld: window.same_world_global}; r' }, (result: ScriptRes) => {
            resolve({ provider: 'gmail', acctEmail: result[0].acctEmail, sameWorld: result[0].sameWorld === true });
          });
        } else {
          reject(new Error('tabs[0].id is undefined'));
        }
      } else {
        resolve({ provider: undefined, acctEmail: undefined, sameWorld: undefined });
      }
    });
  })

They say, for this we can instead use activeTab permission from the popup, which does not cause warning (meaning it should not cause app disabling either), see https://developer.chrome.com/extensions/activeTab

image

Another way is to find out if settings are already open and if yes, replace existing settings tab when opening new settings tab. This is not so crucial, can probably use some more standard way to do something similar, even <a href target> should do the job:

  public static openExtensionTab = async (url: string) => {
    const openedTab = await BgUtils.getFcSettingsTabIdIfOpen();
    if (!openedTab) {
      chrome.tabs.create({ url });
    } else {
      chrome.tabs.update(openedTab, { url, active: true });
    }
  }

  public static getFcSettingsTabIdIfOpen = async (): Promise<number | undefined> => {
    return await new Promise(resolve => {
      chrome.tabs.query({ currentWindow: true }, tabs => {
        const extensionUrl = chrome.runtime.getURL('/');
        for (const tab of tabs) {
          if (tab.url && tab.url.includes(extensionUrl)) {
            resolve(tab.id);
            return;
          }
        }
        resolve(undefined);
      });
    });
  }

We also use it for injecting content scripts, and this will be difficult, if not impossible to replace. I think it will affect updates and also certain other situations:

export const injectFcIntoWebmail = () => {
  const contentScriptGroups = chrome.runtime.getManifest().content_scripts!; // we know it's in the manifest
  // one time when extension installed or on browser start - go through all matching tabs and inject
  for (const group of contentScriptGroups) {
    getContentScriptTabIds(group.matches || [], (tabIds) => {
      for (const tabId of tabIds) {
        injectContentScriptIntoTabIfNeeded(tabId, group.js || []);
      }
    });
  }
  // on Firefox, standard way of loading content scripts stopped working. We have to listen to tab loaded events, and inject then
  // basically here we do what normally the browser is supposed to do (inject content scripts when page is done loading)
  if (Catch.browser().name === 'firefox') {
    chrome.tabs.onUpdated.addListener((tabId, changed, tab) => {
      if (changed.status === 'complete' && tab.active && tab.url) {
        for (const group of contentScriptGroups) {
          for (const groupMatchUrl of group.matches || []) {
            if (tab.url.startsWith(groupMatchUrl.replace(/\*$/, ''))) {
              injectContentScriptIntoTabIfNeeded(tabId, group.js || []);
            }
          }
        }
      }
    });
  }
};

export const injectContentScriptIntoTabIfNeeded = (tabId: number, files: string[]) => {
  isContentScriptInjectionNeeded(tabId, (alreadyInjected) => {
    if (!alreadyInjected) {
      console.info("Injecting FlowCrypt into tab " + tabId);
      injectContentScripts(tabId, files);
    }
  });
};

const getContentScriptTabIds = (matches: string[], callback: (tabIds: number[]) => void) => {
  chrome.tabs.query({ 'url': matches }, result => {
    callback(result.filter(tab => typeof tab.id !== 'undefined').map((tab) => tab.id) as number[]);
  });
};

const isContentScriptInjectionNeeded = (tabId: number, callback: (injected: boolean) => void) => {
  chrome.tabs.executeScript(tabId, { code: 'Boolean(window.injected)' }, (results: (boolean | undefined)[]) => {
    callback(results[0] === true);
  });
};

const injectContentScripts = (tabId: number, files: string[], callback?: () => void) => {
  const filesCopy = files.slice();
  chrome.tabs.executeScript(tabId, { file: filesCopy.shift() }, results => {
    if (filesCopy.length) {
      injectContentScripts(tabId, filesCopy, callback);
    } else if (callback) {
      callback();
    }
  });
};

Then finally we use chrome.tabs.sendMessage but that should continue working even without the permission.

I'll split this into separate tasks.

tomholub commented 4 years ago

As for the injecting, there is a (slim) chance that it may work with currentTab - we would inject (or update) the content scripts after user clicks browser action button (the flowcrypt icon on top right). That means during plugin update, we may need to guide the user to click on it. Not as smooth, but workable, and probably worth the tradeoff of fewer permissions.