Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#7356 closed change (fixed)

Allow wrapping of function properties in abort-current-inline-script snippet

Reported by: agiammarchi Assignee: agiammarchi
Priority: P2 Milestone:
Module: Core Keywords: circumvention
Cc: amr Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/28

Description (last modified by agiammarchi)

Background

We currently hook only into objects, making the ability to hook into commonly needed/used public static utilities, such as Object.keys or String.fromCharCode, impossible.

What to change

In lib/content/snippets.js, relax the split API check within the loop, to enable checks through global constructors too (typeof function).

Hints for testers

It should now be possible to prevent access to properties on functions. e.g. try the filter #$#abort-current-inline-script Object.keys and add <script>console.log(Object.keys({a: 1}))</script> to your HTML document; it should not print ["a"] in the console.

Change History (21)

comment:1 Changed 7 months ago by agiammarchi

  • Review URL(s) modified (diff)
  • Verified working set

comment:2 Changed 7 months ago by amrmak

  • Cc amr added

comment:3 Changed 7 months ago by amrmak

  • Keywords circumvention added

comment:4 Changed 7 months ago by agiammarchi

  • Review URL(s) modified (diff)

comment:5 Changed 7 months ago by mjethani

  • Verified working unset

comment:6 follow-up: Changed 7 months ago by mjethani

The "Verified working" flag should not be set, that is what QA does.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 months ago by agiammarchi

Replying to mjethani:

The "Verified working" flag should not be set, that is what QA does.

fair enough, but in various people here have verified this actually works (I've created a build with this change in and shared it)

comment:8 in reply to: ↑ 7 Changed 7 months ago by mjethani

Replying to agiammarchi:

Replying to mjethani:

The "Verified working" flag should not be set, that is what QA does.

fair enough, but in various people here have verified this actually works (I've created a build with this change in and shared it)

That's nice to know.

The flag though is for QA testing, which is a more formal process.

comment:9 Changed 7 months ago by abpbot

A commit referencing this issue has landed:
Issue 7356 - Allow wrapping of function properties in snippets

comment:10 Changed 7 months ago by abpbot

A commit referencing this issue has landed:
Issue 7356 - Allow wrapping of function properties in snippets

comment:11 Changed 7 months ago by mjethani

  • Description modified (diff)
  • Owner set to agiammarchi
  • Priority changed from Unknown to P2
  • Ready set
  • Summary changed from Relax abortCurrentInlineScript to Relax Allow wrapping of function properties in snippets

comment:12 Changed 7 months ago by mjethani

  • Summary changed from Relax Allow wrapping of function properties in snippets to Allow wrapping of function properties in snippets

comment:13 Changed 7 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed

comment:14 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:15 Changed 7 months ago by Ross

This does not appear to be working in Chrome or Firefox.

With an external script:

if( window.blah ){
  console.log("blah");
}

console.log("before");
console.log(Object.keys({a: 1}));
console.log("after");

On a test page:

<!DOCTYPE html>
<html>
  <body>
    <script src="script.js"></script>
  </body>
</html>

The filter host#$#abort-on-property-read Object.keys does not appear to do anything. Have double checked that snippets are actually being applied to the page using the filter host#$#abort-on-property-read blah which works as expected. No errors appear in the page or extension background consoles. Have also tried using wrapping the console logs in a setTimeout.

ABP 3.5.0.2283
Chrome 73.0.3683.86 / Windows 10
Firefox 65.0.1 / Windows 10

comment:16 Changed 7 months ago by agiammarchi

  • Description modified (diff)

comment:17 Changed 7 months ago by agiammarchi

my apologies the ticket had bad copy/paste mistake, it's abort-current-inline-script that should work.

the property read bit hasn't changed in here.

Last edited 7 months ago by agiammarchi (previous) (diff)

comment:18 Changed 7 months ago by mjethani

There is currently no snippet called abort-on-function-call.

comment:19 Changed 7 months ago by mjethani

  • Summary changed from Allow wrapping of function properties in snippets to Allow wrapping of function properties in abort-current-inline-script snippet

comment:20 Changed 7 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as expected in the browsers below. It fails in Edge, but that looks to be because the snippet as a whole does not work in Edge (#7420). I could not get it working in Firefox Mobile either, but that might be related to #7410.

ABP 3.5.0.2283
Chrome 73.0.3683.86 / Windows 10
Chrome 49.0.2623.75 / Windows 10
Firefox 65.0.1 / Windows 10
Firefox 51.0 / Windows 10
Opera 58.0.3135.118 / Windows 10
Opera 36.0.2130.80 / Windows 10

comment:21 Changed 7 months ago by mjethani

  • Sensitive unset
Note: See TracTickets for help on using tickets.