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.

Sign up for GitHub

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

Jump to bottom

Add a tus-js-client urlStorage implementation based on JS maps #31926

Closed
timabbott opened this issue Oct 9, 2024 · 5 comments · Fixed by #32838
Closed

Add a tus-js-client urlStorage implementation based on JS maps #31926

timabbott opened this issue Oct 9, 2024 · 5 comments · Fixed by #32838
Assignees
@shubham-padia
Labels
area: uploads in progress priority: high

Comments

@timabbott
Copy link
Member

As documented in https://github.com/tus/tus-js-client/blob/main/docs/api.md#urlstorage, it should be possible to instead of disabling this bit of tusd functionality, just provide a little module that uses JS maps to implement the API that tus-js-client expects here, without using browser local storage and the security concerns that come with it.
@zulipbot
Copy link
Member

zulipbot commented Oct 9, 2024

Hello @zulip/server-misc members, this issue was labeled with the "area: uploads" label, so you may want to check it out!

@devyk100
Copy link
Collaborator

devyk100 commented Oct 9, 2024

@zulipbot claim

@zulipbot
Copy link
Member

zulipbot commented Oct 9, 2024

Hello @devyk100!

Thanks for your interest in Zulip! You have attempted to claim an issue without the label "help wanted". You can only claim and submit pull requests for issues with the help wanted label.

If this is your first time here, we recommend reading our guide for new contributors before getting started.

@hustlernik
Copy link

@timabbott Can I take up this issue?

@hustlernik
Copy link

hustlernik commented Oct 16, 2024 •
edited
Loading

@timabbott
I have gone through the PS, looked into tus-js-client npm package and tus protocol.

This is how I am thinking to implement this:

The MapUrlStorage class uses JavaScript's Map to store and retrieve upload URLs for tus-js-client, following the required UrlStorage interface. Each upload is associated with a file's fingerprint, and a unique urlStorageKey is generated for each upload. This in-memory storage avoids using browser local storage, making it suitable for environments where local storage may raise security concerns.

Sample code Snippet:

class MapUrlStorage {
  constructor() {
    this.storage = new Map();
  }

  async findAllUploads() {
    return Array.from(this.storage.values()).flat();
  }

  async findUploadsByFingerprint(fingerprint) {
    return this.storage.get(fingerprint) || [];
  }

  async removeUpload(urlStorageKey) {
    for (const [fingerprint, uploads] of this.storage.entries()) {
      const updated = uploads.filter(entry => entry.urlStorageKey !== urlStorageKey);
      updated.length ? this.storage.set(fingerprint, updated) : this.storage.delete(fingerprint);
    }
  }

  async addUpload(fingerprint, upload) {
    const urlStorageKey = `upload-${Date.now()}-${Math.random().toString(36).slice(2)}`;
    const newUpload = { ...upload, urlStorageKey };
    this.storage.set(fingerprint, [...(this.storage.get(fingerprint) || []), newUpload]);
    return urlStorageKey;
  }
}

@shubham-padia shubham-padia self-assigned this Dec 28, 2024
shubham-padia added a commit to shubham-padia/zulip that referenced this issue Dec 28, 2024
@shubham-padia
upload: Implement InMemoryUrlStorage for tus-js-client.
31f0f11 
Fixes zulip#31926.
The tus-js-client fingerprinting feature stores metadata on
previously uploaded files in browser local storage, to
allow resuming the upload / avoiding a repeat upload in
future browser sessions.

This is not a feature we need across browser sessions. Since
these local storage entries are never garbage-collected,
they can be accessed via the browser console even after
logging out, and contain some metadata about previously
uploaded files, which seems like a security risk for
using Zulip on a public computer.

We use our own implementation of url storage that saves urls
in memory instead. We won't be able to retain this history
across reloads unlike local storage, which is a tradeoff we
are willing to make.
@shubham-padia shubham-padia mentioned this issue Dec 28, 2024
Merged
shubham-padia added a commit to shubham-padia/zulip that referenced this issue Jan 6, 2025
@shubham-padia
upload: Implement InMemoryUrlStorage for tus-js-client.
f1b41f4 
Fixes zulip#31926.
The tus-js-client fingerprinting feature stores metadata on
previously uploaded files in browser local storage, to
allow resuming the upload / avoiding a repeat upload in
future browser sessions.

This is not a feature we need across browser sessions. Since
these local storage entries are never garbage-collected,
they can be accessed via the browser console even after
logging out, and contain some metadata about previously
uploaded files, which seems like a security risk for
using Zulip on a public computer.

We use our own implementation of url storage that saves urls
in memory instead. We won't be able to retain this history
across reloads unlike local storage, which is a tradeoff we
are willing to make.
shubham-padia added a commit to shubham-padia/zulip that referenced this issue Jan 6, 2025
@shubham-padia
upload: Implement InMemoryUrlStorage for tus-js-client.
fe10b6f 
Fixes zulip#31926.
The tus-js-client fingerprinting feature stores metadata on
previously uploaded files in browser local storage, to
allow resuming the upload / avoiding a repeat upload in
future browser sessions.

This is not a feature we need across browser sessions. Since
these local storage entries are never garbage-collected,
they can be accessed via the browser console even after
logging out, and contain some metadata about previously
uploaded files, which seems like a security risk for
using Zulip on a public computer.

We use our own implementation of url storage that saves urls
in memory instead. We won't be able to retain this history
across reloads unlike local storage, which is a tradeoff we
are willing to make.
shubham-padia added a commit to shubham-padia/zulip that referenced this issue Jan 7, 2025
@shubham-padia
mention: Mentions via subgroup should be highlighted.
612a470 
Fixes zulip#31926.
This commit does not add highlighting to any pre-existing group mentions
for which a user was part of the mentioned group via a subgroup. This
only fixes it for mentions moving forward.
Fixes https://chat.zulip.org/#narrow/channel/9-issues/topic/group.20mention.20not.20highlighted/near/2008541
shubham-padia added a commit to shubham-padia/zulip that referenced this issue Jan 7, 2025
@shubham-padia
upload: Implement InMemoryUrlStorage for tus-js-client.
a920949 
Fixes zulip#31926.
The tus-js-client fingerprinting feature stores metadata on
previously uploaded files by default in browser local storage.
Since these local storage entries are never garbage-collected,
they can be accessed via the browser console even after
logging out, and contain some metadata about previously
uploaded files, which seems like a security risk for
using Zulip on a public computer.

We use our own implementation of url storage that saves urls
in memory instead. We won't be able to retain this history
across reloads unlike local storage, which is a tradeoff we
are willing to make.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@shubham-padia shubham-padia

Labels
area: uploads in progress priority: high
Projects
Zulip release priorities
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

upload: Implement InMemoryUrlStorage for tus-js-client. shubham-padia/zulip
5 participants
@timabbott @shubham-padia @zulipbot @hustlernik @devyk100