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-12102 | Fix LastPass importer not properly de-encrypting URLs #11366

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

Tyrrrz
Copy link
Contributor

@Tyrrrz Tyrrrz commented Oct 2, 2024

🎟️ Tracking

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

Closes #11051

📔 Objective

LastPass encrypts URLs of items in their vault. When importing, they need to be decrypted to appear properly.

I used this code as reference for my implementation.

📸 Screenshots

Before:

image

After:

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

@Tyrrrz Tyrrrz requested a review from a team as a code owner October 2, 2024 18:39
@Tyrrrz Tyrrrz requested a review from djsmith85 October 2, 2024 18:40
Comment on lines 26 to 27
TODO: Add a test for the folder case!
TODO: Add a test case that covers secure note account!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also couldn't find any existing tests for this parser. Should I leave it like that or add them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally you would add them. In the meantime I'm going to approve and move this to QA, so we can hopefully include the fix soon.

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 33.26%. Comparing base (398f9be) to head (fed7fe5).
Report is 32 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...r/src/importers/lastpass/access/services/parser.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11366      +/-   ##
==========================================
+ Coverage   33.24%   33.26%   +0.02%     
==========================================
  Files        2728     2728              
  Lines       85392    85413      +21     
  Branches    16275    16278       +3     
==========================================
+ Hits        28387    28414      +27     
+ Misses      54755    54749       -6     
  Partials     2250     2250              

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

Copy link
Contributor

github-actions bot commented Oct 2, 2024

Logo
Checkmarx One – Scan Summary & Detailse2f634fe-fa54-4a57-8e94-1bca76c7bae6

No New Or Fixed Issues Found

@Tyrrrz Tyrrrz enabled auto-merge (squash) October 2, 2024 18:50
@djsmith85 djsmith85 disabled auto-merge October 7, 2024 10:57
@djsmith85 djsmith85 added needs-qa Marks a PR as requiring QA approval and removed needs-qa Marks a PR as requiring QA approval labels Oct 7, 2024
@djsmith85
Copy link
Contributor

Spoke with QA and merging this, to be included with the rc-cut and continue with any further testing there.

@djsmith85 djsmith85 merged commit 359b6e0 into main Oct 7, 2024
70 of 72 checks passed
@djsmith85 djsmith85 deleted the pm12102 branch October 7, 2024 14:55
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.

Firefox Extension import from LastPass fails with encrypted URLs
2 participants