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

Fix stats leakage #86

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,14 @@ jobs:

strategy:
matrix:
node-version: [14.x, 16.x, 18.x]
node-version: [18, 19]

steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- name: Upgrade npm
run: npm install -g npm
- run: npm ci
- run: ./fern.js build configs/ci/unit-tests.js
- run: ./fern.js test configs/ci/unit-tests.js -l unit-node --environment testing --no-build --ci report.xml
2 changes: 1 addition & 1 deletion .tool-versions
Original file line number Diff line number Diff line change
@@ -1 +1 @@
nodejs 19.8.1
nodejs 19.9.0
6 changes: 4 additions & 2 deletions modules/adblocker/sources/adblocker.es
Original file line number Diff line number Diff line change
Expand Up @@ -290,13 +290,15 @@ export default class Adblocker {
* engine decides.
*/
onBeforeRequest(context, response) {
if (context.frameId === 0 && context.isMainFrame) {
this.stats.addNewPage(context);
}

if (this.shouldProcessRequest(context, response) === false) {
return false;
}

if (context.isMainFrame) {
this.stats.addNewPage(context);

// HTML filtering is a feature from the adblocker which is able to
// intercept streaming responses from main documents before they are
// parsed by the browser. This means that we have a chance to remove
Expand Down
38 changes: 31 additions & 7 deletions modules/adblocker/sources/statistics.es
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { nextTick } from '../core/decorators';
import TrackerCounter from '../core/helpers/tracker-counter';
import events from '../core/events';
import pacemaker from '../core/services/pacemaker';
import { parse } from '../core/url';
import { browser } from '../platform/globals';

const FIRSTPARTY = 'First party';

Expand All @@ -20,6 +22,7 @@ class PageStats {
this.tabUrl = context.tabUrl;
this.hostGD = context.tabUrlParts.generalDomain;
this.blockedRequests = [];
this.createdAt = Date.now();

// Lazily computed once a report is requested
this._counter = new TrackerCounter();
Expand Down Expand Up @@ -138,30 +141,51 @@ export default class AdbStats {
}

init() {
browser.webNavigation.onBeforeNavigate.addListener(this.onBeforeNavigate);
philipp-classen marked this conversation as resolved.
Show resolved Hide resolved
this.clearInterval = pacemaker.everyFewMinutes(() => { this.clearStats(); });
}

unload() {
browser.webNavigation.onBeforeNavigate.removeListener(this.onBeforeNavigate);
pacemaker.clearTimeout(this.clearInterval);
}

onBeforeNavigate = (event) => {
if (event.frameId !== 0 || event.frameType === 'outermost_frame') {
return;
}
const urlParts = parse(event.url);
this.addNewPage({
tabId: event.tabId,
tabUrl: event.url,
tabUrlParts: urlParts,
});
};

addBlockedUrl(context) {
// Check if we already have a context for this tab
let page = this.tabs.get(context.tabId);
if (page === undefined) {
page = this.addNewPage(context);
}

// If it's a new url in an existing tab
if (context.tabUrl !== page.tabUrl) {
if (!page) {
page = this.addNewPage(context);
}

page.addBlockedUrl(context);
// ignore requests that may related to previous page
if (context.tabUrl === page.tabUrl) {
page.addBlockedUrl(context);
}
}

addNewPage(context) {
const existingPage = this.tabs.get(context.tabId);

if (
existingPage
&& existingPage.tabUrl === context.tabUrl
&& existingPage.createdAt + 200 > Date.now()
) {
return existingPage;
}

if (existingPage !== undefined) {
// Emit summary of stats from previous page
nextTick(() => {
Expand Down
21 changes: 11 additions & 10 deletions modules/hpn-lite/tests/unit/server-public-key-accessor-test.es
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,14 @@ export default describeModule('hpn-lite/server-public-key-accessor',
await storage.set(someStorageKey, entry);
};

const oldCrypto = global.crypto;
const oldImportKey = global.crypto && global.crypto.subtle.importKey;
let hadNoCrypto = false;
beforeEach(async function () {
/* eslint-disable-next-line global-require */
global.crypto = global.crypto || {
subtle: {
importKey(...args) {
return MOCKS.importKey(...args);
},
}
};
if (!global.crypto) {
Copy link
Member

@philipp-classen philipp-classen Oct 12, 2023

Choose a reason for hiding this comment

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

As it is currently written (after the PR), the tests have the side effects to change
global.crypto === undefined (before tests) to global.crypto !== undefined (after tests)

hadNoCrypto = true;
global.crypto = { subtle: {} };
}
global.crypto.subtle.importKey = (...args) => MOCKS.importKey(...args);

// in-memory implementation of storage
MemoryPersistentMap = (await this.system.import('core/helpers/memory-map')).default;
Expand All @@ -117,7 +115,10 @@ export default describeModule('hpn-lite/server-public-key-accessor',
});

afterEach(function () {
global.crypto = oldCrypto;
global.crypto.subtle.importKey = oldImportKey;
if (hadNoCrypto) {
delete global.crypto;
}
});

it('should be able to retrieve a key and cache it (happy path)', async function () {
Expand Down
13 changes: 11 additions & 2 deletions modules/webrequest-pipeline/sources/page-store.es
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,17 @@ export default class PageStore {
}

onBeforeNavigate = (details) => {
const { frameId, tabId, url } = details;
const { frameId, tabId, url, timeStamp } = details;
const tabContext = this.tabs.get(tabId);
if (frameId === 0) {
if (
tabContext
&& tabContext.id === tabId
&& tabContext.url === url
&& tabContext.created + 200 > timeStamp
) {
return;
}
// We are starting a navigation to a new page - if the previous page is complete (i.e. fully
// loaded), stage it before we create the new page info.
if (tabContext && tabContext.state === PAGE_LOADING_STATE.COMPLETE) {
Expand All @@ -150,7 +158,8 @@ export default class PageStore {
id: tabId,
active: false,
url,
incognito: tabContext ? tabContext.isPrivate : false
incognito: tabContext ? tabContext.isPrivate : false,
created: timeStamp,
});
nextContext.previous = tabContext;
this.tabs.set(tabId, nextContext);
Expand Down
8 changes: 6 additions & 2 deletions modules/webrequest-pipeline/sources/page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ interface FirefoxWebRequestDetails extends chrome.webRequest.WebRequestDetails {
documentUrl?: string
}

interface ExtentedTab extends chrome.tabs.Tab {
created: any;
}

export default class Page {

id: number
Expand Down Expand Up @@ -46,13 +50,13 @@ export default class Page {

previous?: Page

constructor({ id, active, url, incognito }: chrome.tabs.Tab) {
constructor({ id, active, url, incognito, created }: ExtentedTab) {
this.id = id || 0;
this.url = url;
this.isRedirect = false;
this.isPrivate = incognito;
this.isPrivateServer = false;
this.created = Date.now();
this.created = created || Date.now();
this.destroyed = null;
this.lastRequestId = null;
this.frames = new Map([[0, {
Expand Down
Loading