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

packages: adblocker-electron{-preload}: make use of executeJavaScript #4278

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

nullptropy
Copy link
Contributor

@nullptropy nullptropy commented Sep 14, 2024

Use WebContents.executeJavaScript for adblocker script injection.

Preload script injection is subject to CSP, breaking functionality on some websites.
This change uses executeJavaScript API to bypass CSP:

  • Replace preload injection with IPC to main process
  • Add adblocker-inject-script handler in main process
  • Throw an error in the renderer process if the injected script fails

Fixes #4234

@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch 2 times, most recently from fd03028 to efc1dca Compare September 14, 2024 12:38
@seia-soto seia-soto added the PR: Bug Fix 🐛 Increment patch version when merged label Sep 17, 2024
Copy link
Member

@chrmod chrmod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we inject scriplets directly from get-cosmetic-filters-first handler?

this will avoid unnecessary communication as what is happening here is:

    1. preload requests scriptlets by get-cosmetic-filters-first
    1. background matches the scritlets and sends it back to preload
    1. preload sends scriptlets back to background to perform the injection with adblocker-inject-script
    1. background injects scripts in the main world

We can avoid steps 2 and 3, but injecting the scriptlets directly in the step 1.

@chrmod chrmod self-requested a review September 17, 2024 09:40
@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch 3 times, most recently from 22f5b22 to 987d06b Compare September 17, 2024 13:10
@nullptropy
Copy link
Contributor Author

@seia-soto @chrmod I tried to streamline the implementation and fix the issues, please take another look at the patch.

@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch 2 times, most recently from 0a91253 to 03e4f4d Compare September 17, 2024 13:23
Copy link
Member

@seia-soto seia-soto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution. I found some headrooms to improve, but I really appreciate introducing the changes.

packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
@nullptropy
Copy link
Contributor Author

nullptropy commented Sep 19, 2024

@seia-soto Thank you for the review! PTAL. I'll squash the commits once the review is through.

@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch 3 times, most recently from 82154cb to 5c335de Compare September 23, 2024 09:09
Copy link
Member

@seia-soto seia-soto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks very great. No other comments from me. I requested a review from our team.

@seia-soto
Copy link
Member

I found that we were missing tests from adblocker library from #4302 but this PR doesn't have any relationship with that. Therefore, I think it's fine to continue without additional rebasing.

@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch from 5c335de to 7709fd8 Compare September 23, 2024 12:22
Copy link
Member

@chrmod chrmod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for pushing this forward. It is clear that electron deserves improvements.

This review may take a bit more time as the entire team is catching up with electron changes and we want to be sure we can get most out of your efforts, so please bear with us :)

Was thinking, how about instead of requesting scriptlets from the preload script, we push them directly based on did-navigate and did-frame-navigate which would make it closer to the way the Ghostery extension does the scritplet injection

With that approach, changes to the other cosmetic filters injection could be minimal.

packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
@nullptropy
Copy link
Contributor Author

Thank you for the review @chrmod! I've replied most of the comments, and I think the most important one to address is that DOMUpdater expects a sync function.

I’d prefer to handle broader changes in other PRs since the electron package isn't fully functional right now. We rely on @ghostery/adblocker-electron in our product, so we'll definitely be here to help maintain the project as issues arise.

packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
@nullptropy
Copy link
Contributor Author

@chrmod PTAL, updated the branch!

@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch 2 times, most recently from 0939766 to 430c734 Compare October 9, 2024 08:58
@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch from 891c59e to 198d3fc Compare October 11, 2024 14:36
Copy link
Member

@chrmod chrmod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're almost there :)

Still need to ensure everything happens in the right moment.

Please let me know if you have any questions.

packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch from 198d3fc to ece9443 Compare October 17, 2024 13:53
@nullptropy
Copy link
Contributor Author

@chrmod Pushed some changes that hopefully addresses the review. I'm losing the thread a bit, please let me know if the changes conform to what you suggested.

Copy link
Member

@chrmod chrmod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need one more change around the async code.

Can you already confirm that this works with youtube.com ads?

packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
packages/adblocker-electron/src/index.ts Outdated Show resolved Hide resolved
@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch from ece9443 to 65c0cde Compare October 17, 2024 16:40
@nullptropy
Copy link
Contributor Author

@chrmod Committed the suggestions. I can confirm that this works on youtube.com.

Copy link
Member

@chrmod chrmod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like one last thing is missing - I've made a code suggestion

packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch from 565e649 to 1cf603a Compare October 18, 2024 10:03
Copy link
Member

@chrmod chrmod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now merge ready, we only have to fix the linter.

packages/adblocker-electron-preload/src/index.ts Outdated Show resolved Hide resolved
@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch from 1181919 to 90eed89 Compare October 21, 2024 08:33
Signed-off-by: nullptropy <nullptropy@tutanota.com>
@nullptropy nullptropy force-pushed the brkp/electron-bypass-csp-headers branch from 90eed89 to 8ec3afe Compare October 21, 2024 08:48
@seia-soto
Copy link
Member

Running CI. 👍

@seia-soto seia-soto merged commit ebc1d72 into ghostery:master Oct 21, 2024
4 checks passed
@seia-soto
Copy link
Member

We would like to extend our sincere appreciation and congratulations to you for your valuable contribution to the adblocker library. We are pleased to merge this fix, which will benefit all users, and we are grateful for the time and effort you invested in addressing this issue.

@chrmod
Copy link
Member

chrmod commented Oct 21, 2024

Many thanks @nullptropy !

@nullptropy
Copy link
Contributor Author

Thanks for being patient with me and for all the hard work you guys put into this library, I really appreciate it! @seia-soto @chrmod

@PascalPixel
Copy link

wonderful work, thank you for your time and effort (ノ´ヮ´)ノ*: ・゚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 Increment patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[adblocker-electron] fails to inject scriptlet due to CSP issue
5 participants