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

Avoid blob url for files downloads #17986

Merged

Conversation

weizman
Copy link
Member

@weizman weizman commented Mar 5, 2023

  • In order to allow the user to download log reports, we use a function called exportAsFile
  • exportAsFile creates a URL object from a Blob and downloads that (common trick)
  • The combination of URL object and Blob is dangerous in realms security and therefore is forbidden by new Snow version because it's very easy to use it to bypass Snow
  • In order to migrate into the new Snow version (PR), I propose this change to the exportAsFile function, which will achieve the exact same goal but without the dangerous combo, but rather with data: URI instead
  • (This modification is covered by tests in CI which proves it works 👍)

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2023

Codecov Report

Merging #17986 (6e13ea8) into develop (abff495) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff            @@
##           develop   #17986   +/-   ##
========================================
  Coverage    63.44%   63.44%           
========================================
  Files          903      903           
  Lines        35267    35263    -4     
  Branches      8920     8918    -2     
========================================
- Hits         22373    22371    -2     
+ Misses       12894    12892    -2     
Impacted Files Coverage Δ
ui/helpers/utils/export-utils.js 0.00% <0.00%> (ø)
app/scripts/lib/segment/analytics.js 49.02% <0.00%> (-2.61%) ⬇️
app/scripts/lib/segment/index.js 78.05% <0.00%> (+4.88%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@weizman weizman marked this pull request as ready for review March 5, 2023 15:06
@weizman weizman requested a review from a team as a code owner March 5, 2023 15:06
@weizman weizman requested a review from garrettbear March 5, 2023 15:06
digiwand
digiwand previously approved these changes Mar 6, 2023
@@ -1,19 +1,23 @@
import { getRandomFileName } from './util';

Copy link
Member

Choose a reason for hiding this comment

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

prefer async over callbacks

Copy link
Member Author

Choose a reason for hiding this comment

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

const elem = window.document.createElement('a');
elem.target = '_blank';
elem.href = window.URL.createObjectURL(blob);
elem.href = `data:${type};Base64,${b64}`;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately btoa fails on special chars that we have in the exported json files that TextEncoder successfully deals with (I learned the hard way)

Copy link
Member

Choose a reason for hiding this comment

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

interesting -- what about Buffer.from(xyz, 'utf8').toString('base64')

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -1,19 +1,13 @@
import { getRandomFileName } from './util';

export function exportAsFile(filename, data, type = 'text/csv') {
export async function exportAsFile(filename, data, type = 'text/csv') {
Copy link
Member

Choose a reason for hiding this comment

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

no longer needs to be async

Copy link
Member Author

Choose a reason for hiding this comment

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

@weizman weizman requested a review from digiwand March 7, 2023 02:32
@weizman weizman merged commit bfbd652 into MetaMask:develop Mar 7, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants