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

Proxy calls to localStorage.getItem() for sandboxing and ensuring value is current #293

Closed
westonruter opened this issue Nov 5, 2022 · 3 comments

Comments

@westonruter
Copy link
Contributor

Is your feature request related to a problem? Please describe.

I was experimenting with sandboxing a script to block access to localStorage, but I found that this was not currently possible. Namely I configured Partytown as follows:

var partytown = {
    apply: (opts) => {
        console.log('apply:', opts.name);
        if (opts.constructor === 'Window' && opts.name.startsWith('localStorage.')) {
            return opts.prevent;
        }
        return opts.continue;
    }
};

I then had this script on the page:

<script type="text/partytown">
localStorage.setItem('greeting', 'Hello World');
console.info(localStorage.getItem('greeting'));
localStorage.removeItem('greeting');
localStorage.clear();
</script>

In the console I then see only these calls logged out:

apply: localStorage.setItem
apply: localStorage.removeItem
apply: localStorage.clear

The call to localStorage.getItem is missing. This appears to have been done intentionally, as the entire localStorage data is sent to the worker when it is initialized:

const initWebWorkerData: InitWebWorkerData = {
$config$,
$interfaces$: readImplementations(impls, initialInterfaces),
$libPath$: new URL(libPath, mainWindow.location as any) + '',
$origin$: origin,
$localStorage$: readStorage('localStorage'),
$sessionStorage$: readStorage('sessionStorage'),
};

The initially-synced value is then referred referenced locally instead of invoking callMethod to obtain the value from the main thread:

let storage: Storage = {
getItem(key) {
index = getIndexByKey(key);
return index > -1 ? getItems()[index][STORAGE_VALUE] : null;
},
setItem(key, value) {
index = getIndexByKey(key);
if (index > -1) {
getItems()[index][STORAGE_VALUE] = value;
} else {
getItems().push([key, value]);
}
if (isSameOrigin) {
callMethod(win, [storageName, 'setItem'], [key, value], CallType.NonBlocking);
} else {
warnCrossOrgin('set', storageName, env);
}
},

Was this done for the sake of performance?

As it stands right now, it is quite possible for the storage in the worker to get out of sync with the storage in the main thread. If the main thread calls localStorage.setItem() after the worker is initialized, the stored value is not seen in the worker. There would seem at least a need to add a storage event listener on the main thread and continuously sync changes back to the copy in the worker. Nevertheless, this could be avoided if calls to localStorage.getItem() were proxied. This would also eliminate duplication of the localStorage data, reducing memory usage (although at the cost of slower execution time).

Describe the solution you'd like

Discontinue sending the entire localStorage and sessionStorage datasets into the worker at initialization, and instead proxy all calls to getItem to obtain the canonical current value from the main thread.

@n0th1ng-else
Copy link
Contributor

Hey any update on this issue? I find it makes a lot of sense, considering we can control the cookies, I wonder what is the reason to leave localStorage untracked?

@gioboa
Copy link
Collaborator

gioboa commented Jan 26, 2024

Hi @n0th1ng-else I didn't look at this issue. would you like to drop a PR for that?

n0th1ng-else pushed a commit to n0th1ng-else/partytown that referenced this issue Jan 31, 2024
In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of
worker creation. Although this could be considered as performance optimization, it leads to the
implementation inconsistency:
- Storages easily go out of sync as there is no proper synchronization with main window setup.
- One can not override the storage access easily.

FOr the latter point, this means we can not take on the methods like `localStorage.getItem()` in the
Partytown proxy methods (specifically in `apply`). Hence there is no way to shield the storage data
from 3rd party scripts.

In this commit we fully rely on the storage values the main window sends into the worker. As the result,
we can always receive relevant value and also can override the storage api with `apply` and `get` hooks
n0th1ng-else pushed a commit to n0th1ng-else/partytown that referenced this issue Jan 31, 2024
In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of
worker creation. Although this could be considered as performance optimization, it leads to the
implementation inconsistency:
- Storages easily go out of sync as there is no proper synchronization with main window setup.
- One can not override the storage access easily.

For the latter point, this means we can not take on the methods like `localStorage.getItem()` in the
Partytown proxy methods (specifically in `apply`). Hence there is no way to shield the storage data
from 3rd party scripts.

In this commit we fully rely on the storage values the main window sends into the worker. As the result,
we can always receive relevant value and also can override the storage api with `apply` and `get` hooks
n0th1ng-else pushed a commit to n0th1ng-else/partytown that referenced this issue Jan 31, 2024
In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of
worker creation. Although this could be considered as performance optimization, it leads to the
implementation inconsistency:
- Storage data leaks into 3rd party script
- Storages easily go out of sync as there is no proper synchronization with main window setup.
- One can not override the storage access easily.

For the latter point, this means we can not take on the methods like `localStorage.getItem()` in the
Partytown proxy methods (specifically in `apply`). Hence there is no way to shield the storage data
from 3rd party scripts.

In this commit we fully rely on the storage values the main window sends into the worker. As the result,
we can always receive relevant value and also can override the storage api with `apply` and `get` hooks
n0th1ng-else added a commit to n0th1ng-else/partytown that referenced this issue Jan 31, 2024
In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of
worker creation. Although this could be considered as performance optimization, it leads to the
implementation inconsistency:
- Storages data leaks into 3rd party script
- Storages easily go out of sync as there is no proper synchronization with main window setup.
- One can not override the storage access easily.

For the latter point, this means we can not take on the methods like `localStorage.getItem()` in the
Partytown proxy methods (specifically in `apply`). Hence there is no way to shield the storage data
from 3rd party scripts.

In this commit we fully rely on the storage values the main window sends into the worker. As the result,
we can always receive relevant value and also can override the storage api with `apply` and `get` hooks
n0th1ng-else added a commit to n0th1ng-else/partytown that referenced this issue Jan 31, 2024
In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of
worker creation. Although this could be considered as performance optimization, it leads to the
implementation inconsistency:
- Storages data leaks into 3rd party script
- Storages easily go out of sync as there is no proper synchronization with main window setup.
- One can not override the storage access easily.

For the latter point, this means we can not take on the methods like `localStorage.getItem()` in the
Partytown proxy methods (specifically in `apply`). Hence there is no way to shield the storage data
from 3rd party scripts.

In this commit we fully rely on the storage values the main window sends into the worker. As the result,
we can always receive relevant value and also can override the storage api with `apply` and `get` hooks
@n0th1ng-else
Copy link
Contributor

@gioboa could you please check the PR when you have time? looking forward to hear what you think

n0th1ng-else added a commit to n0th1ng-else/partytown that referenced this issue Feb 1, 2024
In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of
worker creation. Although this could be considered as performance optimization, it leads to the
implementation inconsistency:
- Storages data leaks into 3rd party script
- Storages easily go out of sync as there is no proper synchronization with main window setup.
- One can not override the storage access easily.

For the latter point, this means we can not take on the methods like `localStorage.getItem()` in the
Partytown proxy methods (specifically in `apply`). Hence there is no way to shield the storage data
from 3rd party scripts.

In this commit we fully rely on the storage values the main window sends into the worker. As the result,
we can always receive relevant value and also can override the storage api with `apply` and `get` hooks
@gioboa gioboa closed this as completed in 66db16d Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants