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

[PM-11721] [BEEEP] Add initial password-protected zip export support #10926

Open
wants to merge 12 commits into
base: auth/beeep/zip-export
Choose a base branch
from

Conversation

quexten
Copy link
Contributor

@quexten quexten commented Sep 6, 2024

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-11721

📔 Objective

Use the zip.js library instead of jszip, in order to add support for large (>4GB) zip archives, and encrypted exports.

📸 Screenshots

image

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link
Contributor

github-actions bot commented Sep 6, 2024

Logo
Checkmarx One – Scan Summary & Details83b0af15-513f-4e69-95d7-0c31c902fc31

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Client_Privacy_Violation /libs/tools/export/vault-export/vault-export-core/src/services/individual-vault-export.service.ts: 78 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM Client_Privacy_Violation /libs/tools/export/vault-export/vault-export-core/src/services/individual-vault-export.service.ts: 73
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 246
MEDIUM Unpinned Actions Full Length Commit SHA /build-web.yml: 190
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop-beta.yml: 50
MEDIUM Unpinned Actions Full Length Commit SHA /publish-cli.yml: 103
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 186
MEDIUM Unpinned Actions Full Length Commit SHA /deploy-web.yml: 190
MEDIUM Unpinned Actions Full Length Commit SHA /publish-web.yml: 43
MEDIUM Unpinned Actions Full Length Commit SHA /build-web.yml: 341
MEDIUM Unpinned Actions Full Length Commit SHA /publish-desktop.yml: 121
MEDIUM Unpinned Actions Full Length Commit SHA /release-cli.yml: 43
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: 890
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: 1292
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 358
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 47
MEDIUM Unpinned Actions Full Length Commit SHA /release-browser.yml: 95
MEDIUM Unpinned Actions Full Length Commit SHA /deploy-web.yml: 221
MEDIUM Unpinned Actions Full Length Commit SHA /publish-cli.yml: 140
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 193
MEDIUM Unpinned Actions Full Length Commit SHA /build-browser.yml: 407
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 81
MEDIUM Unpinned Actions Full Length Commit SHA /publish-desktop.yml: 195
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 179
MEDIUM Unpinned Actions Full Length Commit SHA /build-browser.yml: 366
MEDIUM Unpinned Actions Full Length Commit SHA /staged-rollout-desktop.yml: 28
MEDIUM Unpinned Actions Full Length Commit SHA /publish-desktop.yml: 244
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 296
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop-beta.yml: 667
MEDIUM Unpinned Actions Full Length Commit SHA /deploy-web.yml: 269
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: 1338
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 200
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop.yml: 70
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop.yml: 93
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 498
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 506
MEDIUM Unpinned Actions Full Length Commit SHA /release-web.yml: 40
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: 287
MEDIUM Unpinned Actions Full Length Commit SHA /crowdin-pull.yml: 40
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop.yml: 43
MEDIUM Unpinned Actions Full Length Commit SHA /build-web.yml: 296
MEDIUM Unpinned Actions Full Length Commit SHA /build-cli.yml: 404
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 161
MEDIUM Unpinned Actions Full Length Commit SHA /release-web.yml: 56
MEDIUM Unpinned Actions Full Length Commit SHA /retrieve-current-desktop-rollout.yml: 22
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 514
MEDIUM Unpinned Actions Full Length Commit SHA /version-auto-bump.yml: 20
MEDIUM Unpinned Actions Full Length Commit SHA /publish-cli.yml: 180
MEDIUM Unpinned Actions Full Length Commit SHA /build-web.yml: 266
MEDIUM Unpinned Actions Full Length Commit SHA /crowdin-pull.yml: 34
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop-beta.yml: 247
MEDIUM Unpinned Actions Full Length Commit SHA /brew-bump-desktop.yml: 25
MEDIUM Unpinned Actions Full Length Commit SHA /release-browser.yml: 43
MEDIUM Unpinned Actions Full Length Commit SHA /release-cli.yml: 58
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 522

@djsmith85
Copy link
Contributor

@quexten I love that you continue to push forward with this. Let me know if I can assist, even with just feedback like this:

One thing to keep in mind for the sake of automation. A lot user want to use the CLI to create regular backups. Does this new lib work under node ? If it doesn't we'll have to think about how we would be able to support the new format within the CLI.

@quexten quexten changed the title Add initial password-protected zip export support [PM-11721] [BEEEP] Add initial password-protected zip export support Sep 6, 2024
@quexten
Copy link
Contributor Author

quexten commented Sep 6, 2024

@djsmith85 I did not test the library under node yet, but it seems to have support for node, deno and bun, going through docs / github issues, so anything that can run js/ts.

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.

Project coverage is 33.32%. Comparing base (02c957c) to head (f8a28d7).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...re/src/services/individual-vault-export.service.ts 75.00% 3 Missing and 1 partial ⚠️
...t-export-core/src/services/vault-export.service.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           auth/beeep/zip-export   #10926   +/-   ##
======================================================
  Coverage                  33.32%   33.32%           
======================================================
  Files                       2787     2787           
  Lines                      86619    86625    +6     
  Branches                   16523    16525    +2     
======================================================
+ Hits                       28868    28870    +2     
- Misses                     55446    55449    +3     
- Partials                    2305     2306    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@quexten
Copy link
Contributor Author

quexten commented Sep 9, 2024

Ok, @djsmith85 there are a few quirks with using the library in node, (noticed during fixing the jest tests). The blobreader / textreader and writers don't work because they rely on a global Response which we would have to polyfill for the tests. I chose to just use uint8reader/writer instead. Further, we have to require / conditionally set on globalThis the TransformStream and ReadableStream / WriteableStream for node. This is done by running "require" in eval in order to circumvent webpack causing build issues on web/desktop/browser. This is done by using a jest setup script. Would be neat if the package did this internally, but it's also not too bad in my books.

If we want to actually use the library for streaming data (not just encryption) for very large zip files (we can on both cli and web), we'd have to take another look, but then again this needs re-thinking about how we present the progress to the user anyways and is a future endeavor.

@quexten quexten marked this pull request as ready for review September 9, 2024 12:32
@quexten quexten requested a review from a team as a code owner September 9, 2024 12:32
@djsmith85 djsmith85 self-requested a review September 10, 2024 12:00
@djasonpenney
Copy link

djasonpenney commented Oct 9, 2024

Branch conflicts need to be resolved.

I also note some regressions in code coverage. It looks mainly like a few more unit tests need to be written.

@quexten
Copy link
Contributor Author

quexten commented Oct 19, 2024

@djasonpenney Was on vacation, apologies for the delayed response. This is waiting for review from the tools team. Once they reviewed, the minor package-lock conflict will be resolved in order not to have too much merge conflict resolution overhead.

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

Successfully merging this pull request may close these issues.

3 participants