Add more web storage options #3

layoutd · 2024-01-25T13:49:34Z

This PR enables the user to specify which type of storage to use via the web_storage configuration parameter:

web_storage: 'cookies' (default) - multiple cookies as now
- Proven implementation.
- Some users don't like having so many cookies (extra requests, etc).
- Some cookies trigger a false positive for Web Application Firewalls (like ModSecurity) using the Comodo WAF ruleset.
web_storage: 'singleCookie' - one JSON-encoded sbjs_current cookie that contains all data stored in the cookies
- Workaround for Web Application Firewall 403 errors which generally have sbjs_current whitelisted.
- Includes a method for handling session length (able to count user sessions)
- Reduces the number of cookies passed back and forth to the server.
- Limited to 4kb in data, which could be a problem if there are many 4-byte values (emoji, Sumerian, Hieroglyphics, etc.)
web_storage: 'sessionStorage' - keep all data in the browser's session storage
- No cookies
- All data expires once the session ends (no counting user sessions)
- Data isn't synced across multiple browser windows (opening a link in a new tab loses the data)
web_storage: 'localStorage' - keep all the data in the browser's local storage
- No cookies
- Data will persist indefinitely
- Includes a method for handling session length (able to count user sessions)
- Data will persist indefinitely

Notes

This doesn't currently handle moving from one storage type to another. If a user visits the site with one storage method configured, then the configuration changes during their session or while navigating, the original data will be lost (at best) or corrupted.
Please reread the sub-bullet points above as they illustrate pros and cons of each method!
The localStorage uses the maximum of the session_length (minutes) and lifetime (months converted to minutes) to create an expiry date for the values. Technically they'll live "forever" in the browser's local storage, but whenever Sourcebuster tries to access them, if the expiry timestamp has passed, the values will be discarded. This workaround was added to prevent 0-lifetime configuration (all cookies expire with the session) from creating 0-lifetime local storage, which effectively is erased and set every time a page is loaded.

Testing

Clone the repo into a web-accessible directory.
Navigate to /public/index.html in a browser.
At the bottom of the index.html file, find the Sourcebuster JavaScript configuration. Initially, it should have web_storage set as cookies.
You can add a session_length parameter with value of 1 minute if you would like to test transitions from session to session (default is 30 minutes).
Perform some testing with the cookies value for web_storage.
- Ensure that Referral (coming from woo.com), Organic (coming from google.com), TypeIn/Direct, UTM (with ?utm_source=[VALUE]…) all capture source values correctly.
- Change source method and see that the "first" source values and "current" source values are correct (UTM parameters will always override "first").
- Check for incrementing pages viewed in the session.
- Wait for session timeout then refresh, and see pages viewed return to 1 and user sessions increase.
- Confirm multiple cookies in the Application tab of the dev panel.
Close the browser (or erase all sbjs_ cookies) and change the web_storage config value to singleCookie (camel cased).
Run the above tests again and verify that the behavior is consistent with the previous tests.
Also, check in the Application tab in the dev panel to confirm that only one cookie is being created.
Repeat the tests with both localStorage and sessionStorage options.
- Be sure to close the browser (and/or erase previous config values in storage or cookies) as the current implementation doesn't erase any old data when changing the web_storage value.
- Confirm that no cookies are created when those these methods are enabled.

ecgan

Did some simple testing with the web_storage: 'cookies', seems to work as expected.

I have reviewed 15 out of 18 files. I left some comments below. I have also edited the PR description to fix a code mistake.

In the following line:

sourcebuster-js/src/js/migrations.js

Line 57 in bd51443

// Switch delimiter and renew cookies

We have a code comment that mentions "cookies", I think it may not be accurate anymore since the code is about web_storage, so we may need to change the code comment? 🤔

I'll continue review the three remaining helpers files and do some testings later, if nobody else beat me to it.

ecgan · 2024-01-31T18:03:26Z

README.md

+#### web_storage
+``` javascript
+web_storage: 'cookies'
+```
+See [PR 3](https://github.com/woocommerce/sourcebuster-js/pull/3) for more details.
+
+Basically, configure where the Sourcebuster data will be stored and manipulated:
+- `cookies` (default) — in multiple cookies
+- `singleCookie` — in a single, JSON-encoded cookie
+- `localStorage` — in browser `local storage`
+- `sessionStorage` — in browser `session storage`
+
+Any other value (or no value) will default to `cookies`.


Paragraph spacing, to be consistent with other spacing in the document:

Suggested change

#### web_storage

``` javascript

web_storage: 'cookies'

```

See [PR 3](https://github.com/woocommerce/sourcebuster-js/pull/3) for more details.

Basically, configure where the Sourcebuster data will be stored and manipulated:

- `cookies` (default) — in multiple cookies

- `singleCookie` — in a single, JSON-encoded cookie

- `localStorage` — in browser `local storage`

- `sessionStorage` — in browser `session storage`

Any other value (or no value) will default to `cookies`.

#### web_storage

```javascript

web_storage: 'cookies'

```javascript

See https://github.com/woocommerce/sourcebuster-js/pull/3 for more details.

Basically, configure where the Sourcebuster data will be stored and manipulated:

- `cookies` (default) — in multiple cookies

- `singleCookie` — in a single, JSON-encoded cookie

- `localStorage` — in browser `local storage`

- `sessionStorage` — in browser `session storage`

Any other value (or no value) will default to `cookies`.

ecgan · 2024-01-31T18:11:23Z

src/js/data.js

@@ -12,7 +12,8 @@ var data = {
    first_extra:      'sbjs_first_add',
    session:          'sbjs_session',
    udata:            'sbjs_udata',
-    promocode:        'sbjs_promo'
+    promocode:        'sbjs_promo',
+    single:           'sbjs_current',


Can we have an explanation of the meaning of this single here? In a documentation, or maybe in this PR description? 🤔

ecgan · 2024-01-31T18:11:47Z

src/js/data.js

-    promo:            'code'
+    promo:            'code',
+
+    single_expire:    'sxp'


Can we have an explanation of the meaning of this single_expire here? In a documentation, or maybe in the PR description? 🤔

ecgan · 2024-01-31T18:25:38Z

src/js/helpers/storage_init.js

+	validateType: function( storage_type ) {
+		// Default to valid_values[0] if storage_type is not in valid_values
+		var valid_values = ['cookies', 'singleCookie', 'localStorage', 'sessionStorage'];
+		return valid_values.indexOf( storage_type ) > -1 ? storage_type : valid_values[0];


We can use array includes here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes. Also, I think it is better to be explicit and use 'cookies' instead of valid_values[0].

Suggested change

return valid_values.indexOf( storage_type ) > -1 ? storage_type : valid_values[0];

return valid_values.includes( storage_type ) ? storage_type : 'cookies';

ecgan · 2024-01-31T18:32:57Z

src/js/helpers/storage_init.js

+			case 'cookies':
+			default:
+				storage_module = cookies;


Specifying break for default statement. Some mention it is a good practice. See Break on default case in switch.

Suggested change

case 'cookies':

default:

storage_module = cookies;

case 'cookies':

default:

storage_module = cookies;

break;

ecgan

Reviewed codes and tested some scenarios. cookies, singleCookie and localStorage seem to work as expected. 👍

📝 💡 When I was testing sessionStorage, I forgot that it was tied to the browser tab session, and the session_length value is actually meaningless. I think we should mention it somewhere, or even better, make it work like localStorage, so that it will expire when it exceeds session_length or when the browser tab is close.

🚧 In my testing, I have session_length: 1. When I set web_storage: 'singleCookie', I can't seem to get session or "visits" to go greater than 1; it is always 1, and the "Pages seen" is always incrementing. I think this is a bug?

I am leaving 12 comments below. Important ones are marked with ❓ . I think there are a few code mistakes that need to be fixed.

Requesting changes due to the above 🚧 and ❓ .

ecgan · 2024-02-01T14:48:16Z

src/js/helpers/local_storage.js

+  set: function(name, value, minutes, domain, excl_subdomains) {
+    // Don't break the build
+    domain = '';
+    excl_subdomains = false;


Since domain and excl_subdomains are the last two parameters, we can just remove them and things should still work.

Suggested change

set: function(name, value, minutes, domain, excl_subdomains) {

// Don't break the build

domain = '';

excl_subdomains = false;

set: function(name, value, minutes) {

ecgan · 2024-02-01T14:49:28Z

src/js/helpers/local_storage.js

+  destroy: function(name, domain, excl_subdomains) {
+    // Don't break the build
+    domain = '';
+    excl_subdomains = false;
+


Since domain and excl_subdomains are the last two parameters, we can just remove them and things should still work.

Suggested change

destroy: function(name, domain, excl_subdomains) {

// Don't break the build

domain = '';

excl_subdomains = false;

destroy: function(name) {

ecgan · 2024-02-01T15:19:41Z

src/js/helpers/local_storage.js

+    // as well as the time when it's supposed to expire
+    var item = {
+      value: this.encodeData( value ),
+      expiry: (new Date()).getTime() + ( Math.max( minutes, local_storage_session_length ) * 60 * 1000 ),


❓ Is the Math.max expected? 🤔 I'm thinking it might give unexpected / undesired behavior.

Consider this scenario:

We set the session length by calling setSessionLength( 15 ), i.e. 15 minutes "default" session length.

We call set('abc', '123', 5), i.e. we intentionally want this item to have 5 minutes expiry.

Because of the Math.max, 15 minutes expiry will be set, instead of the 5 minutes expiry that we intentionally want.

I think it may be more sensible to make session length as default parameter, when the minutes parameter is not supplied by the caller.

I tried to make session length as default parameter like this:

set: function(name, value, minutes = local_storage_session_length ) {

But there is a build error that says:

src/js/helpers/local_storage.js: line 8, col 38, 'default parameters' is only available in ES6 (use 'esversion: 6').

Alternatively, we can try this:

Suggested change

expiry: (new Date()).getTime() + ( Math.max( minutes, local_storage_session_length ) * 60 * 1000 ),

expiry: (new Date()).getTime() + ( ( minutes || local_storage_session_length ) * 60 * 1000 ),

If minutes is falsy (including 0), it will use local_storage_session_length.

This means that with the scenario I mentioned above, when we call set('abc', '123', 5), the expiry will be 5 minutes instead of 15 minutes.

If minutes is falsy (including 0), it will use local_storage_session_length.

We could also do an explicit minutes === undefined check, as sometimes 0 has a convention to denote no limit.

ecgan · 2024-02-01T15:36:59Z

src/js/helpers/local_storage.js

+    // `item` is an object which contains the original value
+    // as well as the time when it's supposed to expire
+    var item = {
+      value: this.encodeData( value ),


❔ Because we are using a JavaScript object and we are setting value into item.value, I'm thinking we don't really need this.encodeData (and the corresponding this.decodeData when reading the value). I think we can just do this:

Suggested change

value: this.encodeData( value ),

value: value,

(Note: We can't use object short notation because "'object short notation' is available in ES6".)

In cookies.js file, encodeData and decodeData is needed to prevent character collision in the larger document.cookie string.

ecgan · 2024-02-01T15:39:29Z

src/js/helpers/session_storage.js

+    domain = '';
+    excl_subdomains = false;
+
+    localStorage.removeItem(name);


❓ This should be sessionStorage?

Suggested change

localStorage.removeItem(name);

sessionStorage.removeItem(name);

ecgan · 2024-02-01T15:44:31Z

src/js/helpers/session_storage.js

+  destroy: function(name, domain, excl_subdomains) {
+    // Don't break the build
+    domain = '';
+    excl_subdomains = false;
+


Remove the unneeded parameters:

Suggested change

destroy: function(name, domain, excl_subdomains) {

// Don't break the build

domain = '';

excl_subdomains = false;

destroy: function(name) {

ecgan · 2024-02-01T16:37:13Z

src/js/helpers/single_cookie.js

+    if( name === data.containers.session ) {
+      var session = single_cookie[default_cookies.unsbjs(name)];
+      if( session ) {
+        var sts = session.match(/sts=(\d+)/);


❓ I think this should be sxp, as per data.aliases.single_expire?

Suggested change

var sts = session.match(/sts=(\d+)/);

var sxp = session.match(/sxp=(\d+)/);

ecgan · 2024-02-01T16:45:41Z

src/js/helpers/single_cookie.js

+
+    // Set an expiration for the values
+    if( name === data.containers.session ) {
+      value += data.delimiter + data.aliases.single_expire + '=' + (Math.floor(Date.now() / 1000) + minutes * 60);


In local storage and in cookies, we are using time in milliseconds, but for single_cookie here, we are using minutes? I'm thinking it may be better to make them consistent? 🤔

ecgan · 2024-02-01T16:55:17Z

src/js/helpers/single_cookie.js

+        }
+      }
+    }
+    return single_cookie[default_cookies.unsbjs(name)] || default_cookies.get(name);


Why do we call default_cookies.get(name)? I'm just thinking / concerned that this might cause issues because we are getting data from two different data structures. 🤔

ecgan · 2024-02-01T17:04:34Z

src/js/helpers/single_cookie.js

+    default_cookies.set(data.containers.single, JSON.stringify(single_cookie), lifetime, domain, isolate);
+  },
+
+  deleteOld: function( domain, isolate ) {


Can we have an explanation or documentation on this deleteOld function? What's the purpose etc? I don't see it being used in the code, and it is not being mentioned in the PR description.

tomalec · 2024-02-01T20:08:25Z

src/js/helpers/local_storage.js

+    // if the item doesn't exist, return null
+    if (!item_raw) {


Actually, the code would do "if the item is falsy, return null", and localStorage.getItem returns null for non-existing items.

Is that the case that we consider falsy values (", 0, false, ...) "non-existing"?

If so, I'd rather rephrase the code comment to explain from the "why?" perspective, not the "what?" perspective. ("What?" is answered in the code.)

Suggested change

// if the item doesn't exist, return null

if (!item_raw) {

// We consider items set to a falsy value as nonexistent and return `null` consistently.

if (!item_raw) {

But, I don't know if that's the actual intention.

Or if we want to just forward the behavior, and return null for non existing, I'd rather change the code:

Suggested change

// if the item doesn't exist, return null

if (!item_raw) {

// if the item doesn't exist, return null

if (!item_raw === null) {

Currently, we do set the item to be stringified JSON object, so it will always be nonfalsy if existing, but I'd rather make the code resilient for future changes. Plus, somebody may use this method to ask for an object set by a different piece of software.

tomalec · 2024-02-01T20:27:33Z

src/js/helpers/local_storage.js

+    // as well as the time when it's supposed to expire
+    var item = {
+      value: this.encodeData( value ),
+      expiry: (new Date()).getTime() + ( Math.max( minutes, local_storage_session_length ) * 60 * 1000 ),


If minutes is falsy (including 0), it will use local_storage_session_length.

We could also do an explicit minutes === undefined check, as sometimes 0 has a convention to denote no limit.

tomalec · 2024-02-01T20:46:35Z

src/js/helpers/local_storage.js

+    // if the item doesn't exist, return null
+    if (!item_raw) {


Or if we want to just forward the behavior, and return null for non existing, I'd rather change the code:

Suggested change

// if the item doesn't exist, return null

if (!item_raw) {

// if the item doesn't exist, return null

if (!item_raw === null) {

Currently, we do set the item to be stringified JSON object, so it will always be nonfalsy if existing, but I'd rather make the code resilient for future changes. Plus, somebody may use this method to ask for an object set by a different piece of software.

tomalec · 2024-02-03T20:22:40Z

src/js/init.js

+  // Select web storage method
+  storage_init.set(p.web_storage, p.session_length);
+  web_storage = storage_init.get();


❓
I don't get the rationale behind this architecture, and it looks bug-prone and confusing to me.

In the PR description, you mentioned, that changing webstorage in runtime is currently not supported, and may result in corrupted data.
And there is nothing in this code that would prevent such corruption, or discourage it anyhow.
sbjs.init() may be called multiple times (and in WC Core we do so), that technically can happen with different prefs every time.

We set it to immediately get it at the line bellow and get it again inside the migrations.go another line bellow.

Do we even need that storage_init module? Can sbjs.init just have web_storage variable and pass it to migrations.go.

I could imagine it would be useful, if it would not be ..._init but a storage singleton, that would prevent from changing it in runtime.

tomalec · 2024-02-03T20:25:27Z

src/js/helpers/storage_init.js

+	cookies       = require('./cookies');
+
+module.exports = {
+	validateType: function( storage_type ) {


This method is not used anywhere outside, so I'd rather make it private.

layoutd added 12 commits January 25, 2024 10:56

Implement local storage

6019a36

Abstract web storage

7186357

Only valid web_storage types

7665c12

Validate types of web storage

18e9156

Add session storage optoin

2addcb8

Gitignore update

e1bad77

Add gulp watch and update npm deps

d7c86b3

Default to cookies again

8874217

Include favicon and self-link in test page

6d6412e

Add single cookie web storage method

740aec0

No longer handle deleting multi cookies

fc5ea00

Specify single-cookie session param in data

28be8d1

layoutd self-assigned this Jan 25, 2024

layoutd added this to the 1.2 milestone Jan 25, 2024

layoutd mentioned this pull request Jan 25, 2024

Add the single cookie option #2

Closed

layoutd added 2 commits January 25, 2024 14:59

Add web_storage param to the readme

8eeb055

Note about default web_storage value

6137b50

This was referenced Jan 26, 2024

Stores way too many cookies alex35mil/sourcebuster-js#43

Open

Firewall blocks clients with 403 because of Sourcebuster since 8.5.1 woocommerce/woocommerce#43681

Closed

8.5 Adds Cookies that get blocked by WaFs woocommerce/woocommerce#43413

Closed

Use max session length / cookie lifetime for local storage

bd51443

layoutd requested a review from a team January 30, 2024 14:48

ecgan reviewed Jan 31, 2024

View reviewed changes

ecgan requested changes Feb 1, 2024

View reviewed changes

tomalec reviewed Feb 1, 2024

View reviewed changes

tomalec reviewed Feb 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more web storage options #3

Add more web storage options #3

layoutd commented Jan 25, 2024 •

edited by ecgan

ecgan left a comment •

edited

ecgan Jan 31, 2024

ecgan Jan 31, 2024

ecgan Jan 31, 2024

ecgan Jan 31, 2024

ecgan Jan 31, 2024

ecgan left a comment

ecgan Feb 1, 2024

ecgan Feb 1, 2024

ecgan Feb 1, 2024

tomalec Feb 1, 2024

ecgan Feb 1, 2024

ecgan Feb 1, 2024

ecgan Feb 1, 2024

ecgan Feb 1, 2024

ecgan Feb 1, 2024

ecgan Feb 1, 2024

ecgan Feb 1, 2024

tomalec Feb 1, 2024

tomalec Feb 1, 2024

tomalec Feb 1, 2024

tomalec Feb 1, 2024

tomalec Feb 3, 2024

tomalec Feb 3, 2024

	return valid_values.indexOf( storage_type ) > -1 ? storage_type : valid_values[0];
	return valid_values.includes( storage_type ) ? storage_type : 'cookies';

	expiry: (new Date()).getTime() + ( Math.max( minutes, local_storage_session_length ) * 60 * 1000 ),
	expiry: (new Date()).getTime() + ( ( minutes \|\| local_storage_session_length ) * 60 * 1000 ),

	localStorage.removeItem(name);
	sessionStorage.removeItem(name);

	var sts = session.match(/sts=(\d+)/);
	var sxp = session.match(/sxp=(\d+)/);

Add more web storage options #3

Are you sure you want to change the base?

Add more web storage options #3

Conversation

layoutd commented Jan 25, 2024 • edited by ecgan

Notes

Testing

ecgan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layoutd commented Jan 25, 2024 •

edited by ecgan

ecgan left a comment •

edited