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

Protected namespace in localstorage #15943

Open
sualko opened this issue Jun 12, 2019 · 14 comments
Open

Protected namespace in localstorage #15943

sualko opened this issue Jun 12, 2019 · 14 comments

Comments

@sualko
Copy link
Member

sualko commented Jun 12, 2019

Is your feature request related to a problem? Please describe.
Starting from version 16, Nextcloud deletes the complete local/sessionstorage. This can cause issues with different apps like ojsxc. I understand the intend to protect users privacy, but it would be nice if this could be configured by the app developer.

server/core/src/login.js

Lines 32 to 33 in 6f941a7

window.localStorage.clear();
window.sessionStorage.clear();

Describe the solution you'd like
Black or white list of prefixes which should be deleted or preserved. For example:

   let prefixRegex = new RegExp('^' + prefix);
   let keys = Object.keys(localStorage);

   for (let key of keys) {
      if (prefixRegex.test(key)) {
         localStorage.removeItem(key);
      }
   }
@kesselb
Copy link
Contributor

kesselb commented Jun 13, 2019

cc @rullzer @ChristophWurst

As user i would expect that logout clears all data.

@ChristophWurst
Copy link
Member

What data would the app keep?

@sualko
Copy link
Member Author

sualko commented Jun 13, 2019

For example we cache the complete contact list (we have users with hundreds of contacts), conversations and encryption keys. I know the discussion about encryption on the client side, but under the assumption that Nextcloud is secure (same assumption Nextcloud does for every security feature) it's a effective protection against malicious third-party servers (XMPP supports server-to-server). Sure we could store all this data in Nextcloud, but this would require a lot of requests and a smart conflict management in the case someone is using Nextcloud in two browsers at the same time.

I agree that all data should be deleted on public computers, but I see no need for this on private machines.

A notice about this change had also been helpful in #15339.

@sualko
Copy link
Member Author

sualko commented Jun 13, 2019

Btw. if Nextcloud is installed in a subdirectory this deletes all data for the whole domain. This means if someone uses one domain for multiple applications, this results in side effects for all other applications.

I would volunteer to write a pull request to delete only Nextcloud specific data.

@e-alfred
Copy link

I agree that all data should be deleted on public computers, but I see no need for this on private machines.

Maybe adding a "this is a public computer" selection menu on the login page could solve this?

@sualko
Copy link
Member Author

sualko commented Jun 13, 2019

Maybe adding a "this is a public computer" selection menu on the login page could solve this?

Yes, but it could still impact other applications on the same domain. In my opinion the only correct solution would be to use a namespace for all NC related entries and to delete only those.

@ChristophWurst
Copy link
Member

Sure we could store all this data in Nextcloud, but this would require a lot of requests and a smart conflict management in the case someone is using Nextcloud in two browsers at the same time.

Can't you just prefix the cached data wit the session id?

@sualko
Copy link
Member Author

sualko commented Jun 17, 2019

Can't you just prefix the cached data wit the session id?

If I store my cache, encryption keys and so on on the server, the idea would be that the user has the same data in every browser session. So I have to merge them. The use of a session key would only make sense, if I would bind a session to a browser, but without localstorage I can't detect if it's a new browser or not.

Nonetheless I think the Nextcloud should not mess with the domain wide storage is more important.

@kesselb
Copy link
Contributor

kesselb commented Jan 21, 2020

cc @ChristophWurst @rullzer 🏓

I guess that's still an issue for ojsxc. It's definitely a tricky one. Probably we need something like https://www.npmjs.com/package/store2 for a namespace aware localstorage.

@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 21, 2020

Looks good! We could make this available as a npm wrapper package @nextcloud/local-storage so it's used in a unified way.

@sualko
Copy link
Member Author

sualko commented Jan 22, 2020

That would be great. Please let me know if I can help.

@ChristophWurst
Copy link
Member

Voilá: nextcloud-libraries/nextcloud-browser-storage#1. Let's discuss the API details there.

@szaimen

This comment has been minimized.

@szaimen szaimen closed this as completed May 28, 2021
@sualko

This comment has been minimized.

@szaimen szaimen reopened this May 28, 2021
@szaimen szaimen added 1. to develop Accepted and waiting to be taken care of and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels May 28, 2021
@joshtrichards joshtrichards added developer experience 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of labels Sep 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants