PostHog / posthog-js

Send usage data from your web app or site to PostHog, with autocapture.
https://posthog.com/docs/libraries/js
Other
267 stars 111 forks source link

Unable to use with Manifest v3 due to remote code execution #1394

Open damnmicrowave opened 1 month ago

damnmicrowave commented 1 month ago

Bug Description

Bug description

I've created a simple reproduction for this issue. You can find it here.

Since 1st of August, we started getting rejections from Chrome Store for our browser extension. The reason for these rejections is that remote code execution is not allowed in Manifest v3 extensions.

Some posthog features are using remote code execution via injecting a script tag:

Unfortunately, they are included in the bundle even if not used.

Here are the emails that we got from the Chrome Store: image image

How to reproduce

  1. Clone the reporduction repo
  2. Run pnpm i and then pnpm build
  3. Open generated dist/content-scripts/main.js
  4. Find 4 places with remote code execution (script tag injection) for:
    • /static/exception-autocapture.js
    • /static/surveys.js
    • /static/recorder.js
    • /static/toolbar.js

Additional context

This is not a bug, and I don't really know how this can be solved. Right now I've just forked the repo and removed all these features by hand. Probably, the best way to enable them in the extensions would be to import the source code directly, instead of injecting script tags.

Debug info

No response

seanwessmith commented 1 month ago

same issue here, seems to be occuring due to posthog.capture

import posthog from "posthog-js";
posthog.capture()

Code snippet: popup.js: loadScript("/static/recorder.js?v=".concat(ar.LIB_VERSION), Code snippet: content_script.js: loadScript("/static/toolbar.js?t=".concat(s), Code snippet: report.js: loadScript(this.instance.requestRouter.endpointFor("assets", "/static/exception-autocapture.js?v="

marandaneto commented 1 month ago

@damnmicrowave @seanwessmith have you tried those options https://posthog.com/docs/advanced/browser-extension ?

damnmicrowave commented 3 weeks ago

@damnmicrowave @seanwessmith have you tried those options https://posthog.com/docs/advanced/browser-extension ?

I find this documentation a bit confusing. Don't fully understand what should I do. Even if I import "posthog-js/dist/recorder.js", the recording script tag insertion code still ends up in the bundle.

seanwessmith commented 3 weeks ago

Right. It's not properly tree shaked.

AruSeito commented 2 weeks ago

Is there any progress on this issue now? I have same issue

marandaneto commented 2 weeks ago

@damnmicrowave @seanwessmith have you tried those options posthog.com/docs/advanced/browser-extension ?

I find this documentation a bit confusing. Don't fully understand what should I do. Even if I import "posthog-js/dist/recorder.js", the recording script tag insertion code still ends up in the bundle.

have you tried to do what the docs tell here https://posthog.com/docs/advanced/browser-extension#session-replay about the array.full.js bits?

marandaneto commented 2 weeks ago

The array.full.js should have all the bundled code so it should not do any remote code execution. Unless the Chrome store is unhappy even if there's code that does that but does not call the code execution at all, in this case, a solution would be a simpler version of the SDK (eg https://github.com/PostHog/posthog-js-lite/tree/main/posthog-web)

Can you confirm if using array.full.js still works, or if using https://www.npmjs.com/package/posthog-js-lite is ok?

oliverdunk commented 2 weeks ago

Hi all, I work on the Chrome Extensions team at Google. We came across this issue as we've been seeing quite a number of violations along the lines described here.

Just to clarify, the Additional requirements for Manifest V3 (usually referred to as our "Remote Hosted Code" policy) applies to all code included in the submission, even if it isn't executed. Tree shaking can often help to remove code but that depends a lot on how it is imported.

Do feel free to reach out if you have any questions (oliverdunk@google.com). Definitely keen to help make things work better out of the box.

pauldambra commented 1 week ago

@oliverdunk I emailed you to start the conversation - happy to wait in case you're on summer leave but messaging here as well in case the email didn't make it :)

DophinL commented 1 week ago

Do feel free to reach out if you have any questions (oliverdunk@google.com). Definitely keen to help make things work better out of the box.

However, the code is not actually executing. Please adjust the logic for code detection. I've had three consecutive versions rejected a total of nine times, and each time I have to manually appeal and wait over three days for a resolution. This is seriously impacting my product's iteration progress.

DophinL commented 1 week ago

@oliverdunk Please resolve this ASAP. Thank you! It really affects the development experience. 🫠

ryan-greenfieldlabs commented 1 week ago

Just sharing for others - I migrated to https://www.npmjs.com/package/posthog-js-lite for my Chrome extension and that was approved by the Chrome Web Store team 👍

ebloom19 commented 1 week ago

I managed to get it to work with the following.

I was using:

import { PostHogProvider } from "posthog-js/react";
import { usePostHog } from "posthog-js/react";

I had to create custom replacements for PostHogProvider & usePostHog I made sure to create an array.full.js file in the public directory https://us-assets.i.posthog.com/static/array.full.js

Included in the index.html inside the <script src="array.full.js"></script>

postHogProvider.tsx

import "posthog-js/dist/recorder";

// If updating the posthog-js package, please also update the array.full.js from https://us-assets.i.posthog.com/static/array.full.js

import posthogType from "posthog-js";
import React, { useEffect } from "react";

declare global {
    const posthog: typeof posthogType;
}

interface PostHogProviderProps {
    children: React.ReactNode;
}

export const PostHogContext = React.createContext<typeof posthog | undefined>(
    undefined
);

export const PostHogProvider: React.FC<PostHogProviderProps> = ({
    children,
}) => {
    useEffect(() => {
        posthog.init(import.meta.env.VITE_PUBLIC_POSTHOG_KEY, {
            api_host: import.meta.env.VITE_PUBLIC_POSTHOG_HOST,
            persistence: "localStorage",
        });
    }, []);

    return (
        <PostHogContext.Provider value={posthog}>
            {children}
        </PostHogContext.Provider>
    );
};

usePostHog.ts

import { useContext } from "react";

import { PostHogContext } from "@/providers";

export const usePostHog = () => {
    const context = useContext(PostHogContext);
    if (context === undefined) {
        throw new Error("usePostHog must be used within a PostHogProvider");
    }
    return context;
};

I am yet to publish these updates to the Chrome Web Store yet fingers crossed it does not get flagged!

DophinL commented 1 week ago

I was rejected 12 times. Could everyone please check if the following code is present in the popup.js?

image

I find this code detection quite unreasonable, as it actually follows the this._onScriptLoaded() logic. My extension relies on the recorder, so I can't switch to posthog-js-lite.

I'm really frustrated right now. This issue has been ongoing for a month without resolution. Is anyone paying attention to the developers' concerns?

seanwessmith commented 1 week ago

@DophinL rhe best solution I found is to use the HTTP endpoints

wong2 commented 4 days ago

@ryan-greenfieldlabs Have you encountered any issues while using the lite version?

DophinL commented 4 days ago

@DophinL rhe best solution I found is to use the HTTP endpoints @seanwessmith

My solution was manually removing the code. Currently I write a postbuild script to remove the code(my posthog-js version is 1.130.2):

import fs from "fs"
import path from "path"

const buildDir = path.join(__dirname, "..", "build", "chrome-mv3-prod")
const filesToModify = ["popup", "tabs/app", "tabs/unauthorized"]

function modifyFile(filePath: string) {
  let content = fs.readFileSync(filePath, "utf-8")

  content = content.replace(
    /this\.rrwebRecord\?this\._onScriptLoaded\(\):e[so]\(this\.instance\.requestRouter\.endpointFor\("assets","\/static\/recorder\.js\?v="\s*\.concat\(g\.LIB_VERSION\)\),function\(t\)\{if\(t\)return K\.error\(r_\+"\s*could not load recorder\.js",t\);e\._onScriptLoaded\(\)\}\)/,
    "this._onScriptLoaded()"
  )

  content = content.replace(
    /&&e[so]\(this\.instance\.requestRouter\.endpointFor\("assets","\/static\/exception-autocapture\.js"\),function\(r\)\{if\(r\)return K\.error\("Could not load exception autocapture script",r\);_\.extendPostHogWithExceptionAutocapture\(t\.instance,e\)\}\)/,
    ""
  )

  content = content.replace(
    /e[so]\(this\.instance\.requestRouter\.endpointFor\("assets","\/static\/toolbar\.js\?t="\s*\.concat\(a\)\),function\(e\)\{if\(e\)return K\.error\("Failed to load toolbar",e\),void\(t\._toolbarScriptLoaded=!1\);t\._callLoadToolbar\(n\)\}\),/,
    ""
  )

  content = content.replace(
    /\|\|e[so]\(this\.instance\.requestRouter\.endpointFor\("assets","\/static\/surveys\.js"\),function\(t\)\{if\(t\)return K\.error\("Could not load surveys script",t\);H\.extendPostHogWithSurveys\(e\.instance\)\}\)/,
    ""
  )

  fs.writeFileSync(filePath, content, "utf-8")
  console.log(`Modified: ${filePath}`)
}

function main() {
  const files = fs.readdirSync(buildDir, { recursive: true })

  filesToModify.forEach((filePattern) => {
    const regex = new RegExp(
      `${filePattern.replace("/", "\\/")}\\.[a-z0-9]+\\.js$`
    )
    const matchedFiles = files.filter((file) => regex.test(file.toString()))

    if (matchedFiles.length > 0) {
      matchedFiles.forEach((matchedFile) => {
        const filePath = path.join(buildDir, matchedFile.toString())
        modifyFile(filePath)
      })
    } else {
      console.warn(`No files found matching pattern: ${filePattern}`)
    }
  })
}

main()
pauldambra commented 4 days ago

Ah that's a really cool workaround @DophinL and leads into my update nicely 😊


I've been chatting to @oliverdunk. Their analysis means even if we allow you to configure posthog-js in such a way that it will never load remote code their analysis can't detect that. Frustrating with a posthog hat on, but totally understandable with an "extensions should be certified safe" hat on :)

We're going to need to bundle a version of posthog-js that doesn't include the remote lazy-loading functionality. We should be able to do something very similar to the workaround above that lets rollup/terser shake the code away during bundling

I'd normally be bullish about how fast we can do that but we're onboarding a new team member next week... Watch this space though!

ryan-greenfieldlabs commented 4 days ago

@ryan-greenfieldlabs Have you encountered any issues while using the lite version?

Nope. I am using persistence: 'localStorage' for initializing it though. Have submitted multiple builds to the web store and they've all been accepted.

DophinL commented 3 days ago

@DophinL rhe best solution I found is to use the HTTP endpoints @seanwessmith

My solution was manually removing the code. Currently I write a postbuild script to remove the code(my posthog-js version is 1.130.2):

import fs from "fs"
import path from "path"

const buildDir = path.join(__dirname, "..", "build", "chrome-mv3-prod")
const filesToModify = ["popup", "tabs/app", "tabs/unauthorized"]

function modifyFile(filePath: string) {
  let content = fs.readFileSync(filePath, "utf-8")

  content = content.replace(
    /this\.rrwebRecord\?this\._onScriptLoaded\(\):e[so]\(this\.instance\.requestRouter\.endpointFor\("assets","\/static\/recorder\.js\?v="\s*\.concat\(g\.LIB_VERSION\)\),function\(t\)\{if\(t\)return K\.error\(r_\+"\s*could not load recorder\.js",t\);e\._onScriptLoaded\(\)\}\)/,
    "this._onScriptLoaded()"
  )

  content = content.replace(
    /&&e[so]\(this\.instance\.requestRouter\.endpointFor\("assets","\/static\/exception-autocapture\.js"\),function\(r\)\{if\(r\)return K\.error\("Could not load exception autocapture script",r\);_\.extendPostHogWithExceptionAutocapture\(t\.instance,e\)\}\)/,
    ""
  )

  content = content.replace(
    /e[so]\(this\.instance\.requestRouter\.endpointFor\("assets","\/static\/toolbar\.js\?t="\s*\.concat\(a\)\),function\(e\)\{if\(e\)return K\.error\("Failed to load toolbar",e\),void\(t\._toolbarScriptLoaded=!1\);t\._callLoadToolbar\(n\)\}\),/,
    ""
  )

  content = content.replace(
    /\|\|e[so]\(this\.instance\.requestRouter\.endpointFor\("assets","\/static\/surveys\.js"\),function\(t\)\{if\(t\)return K\.error\("Could not load surveys script",t\);H\.extendPostHogWithSurveys\(e\.instance\)\}\)/,
    ""
  )

  fs.writeFileSync(filePath, content, "utf-8")
  console.log(`Modified: ${filePath}`)
}

function main() {
  const files = fs.readdirSync(buildDir, { recursive: true })

  filesToModify.forEach((filePattern) => {
    const regex = new RegExp(
      `${filePattern.replace("/", "\\/")}\\.[a-z0-9]+\\.js$`
    )
    const matchedFiles = files.filter((file) => regex.test(file.toString()))

    if (matchedFiles.length > 0) {
      matchedFiles.forEach((matchedFile) => {
        const filePath = path.join(buildDir, matchedFile.toString())
        modifyFile(filePath)
      })
    } else {
      console.warn(`No files found matching pattern: ${filePattern}`)
    }
  })
}

main()

After dealing with the remote loading code, I was fortunate enough to be rejected twice more, this time by PostHog, but for a different reason—code obfuscation issues. 💢 Should I switch from PostHog to Microsoft Clarity, or should I just give the source code to Google? 🫠 @marandaneto @oliverdunk

image image
oliverdunk commented 3 days ago

After dealing with the remote loading code, I was fortunate enough to be rejected twice more, this time by PostHog, but for a different reason—code obfuscation issues.

Thanks for sharing this - I've reached out to the team, and I'll try to follow up here and directly to Paul in the next day or so. I want to have a quick chat about this and make sure we're aligned.

Generally, we allow minification, but don't allow obfuscation. As I hope you can appreciate from a review perspective this walks the line a little bit. I understand it isn't code you added though and comes from a reasonable set of steps (ultimately a dependency of PostHog seems to be using https://www.npmjs.com/package/rollup-plugin-web-worker-loader). I think we just need to make a decision about if this is something we consider reasonable or not.

pauldambra commented 3 days ago

Thanks for replying @oliverdunk

Folk can audit rrweb to see what it's doing - IIRC the web worker is to offload compression to a worker instead of blocking the main thread

We're not using the compression right now but we plan to so it'd be painful to remove that - would need someone to contribute upstream or for us to start to maintain our own fork of rrweb


Separately we're figuring out how we can provide a bundle with no remote execution here https://github.com/PostHog/posthog-js/pull/1413

DophinL commented 3 days ago

Thank you for the feedback. I look forward to further progress, and if there are any temporary workarounds that could help, please let me know. 🫡 @oliverdunk @pauldambra

seanwessmith commented 3 days ago

A good workaround is to use their HTTP api instead

https://posthog.com/docs/api/capture

import fetch from "node-fetch";

async function sendPosthogEvent() {
  const url = "https://us.i.posthog.com/capture/";
  const headers = {
      "Content-Type": "application/json",
  };
  const payload = {
    api_key: "xxx",
    event: "event name",
    distinct_id: "user distinct id",
    properties: {
      account_type: "pro",
    },
    timestamp: "[optional timestamp in ISO 8601 format]",
  };

  const response = await fetch(url, {
    method: "POST",
    headers: headers,
    body: JSON.stringify(payload),
  });

  const data = await response.json();
  console.log(data);
}

sendPosthogEvent()