Vulnerability in dependency `glob-parent` via `broccoli-merge-files` about ember-intl HOT 10 CLOSED

@LucasHillDex Thanks for working on #1863 and buschtoens/broccoli-merge-files#431.

I will have a look at the former PR (I'll need time to check how the change affects a work project), and can ask for feedback from @buschtoens.

from ember-intl.

@ijlee2 I had to re-open the PR under a different account: #1864

I'm hoping the changes have zero impact on the behavior of the addon, let me know if you find otherwise. The tests were really good so that gave me a lot of confidence.

My PR to broccoli-merge-files is hung up because of other brocolli packages that are not updated (and maybe the fast-glob update causing public API changes to broccoli-merge-files). As our team has had to take vulnerabilities more seriously I'm definitely starting to appreciate removing dependencies when possible, especially if what the dependency was providing wasn't huge (no shade to broccoli-merge-files, just in this case it felt like an extra step that the translation-reducer could skip if it constructed the final translations.js itself).

from ember-intl.

broccoli-merge-files has additional vulnerabilities coming in through broccoli-test-helper so it is the source of quite a few vulnerabilities. Could be nice to get rid of it if it is no longer maintained.

from ember-intl.

@ijlee2 would you mind taking a look at this proposed fix? #1863

from ember-intl.

Hi, @LucasHill. I messaged Jan for feedback. Here's the reply that I received:

I don’t really have the time & interest to reasonably maintain broccoli-merge-files, so I would recommend to migrate off of it. Sorry. :/

Option 2 [Remove the use of broccoli-merge-files in ember-intl] would be my recommendation. Do you require my help for reviewing it?

I briefly looked at the PR: I can imagine that the removal of intermediary .json files will break ember-intl for CLARK, as AFAIR we rely on that feature.

I think it should be possible to do a minimal change by really only replacing the usage of broccoli-merge-files with manual broccoli plumbing.

So both Jan and I are, in general, in favor of removing unnecessary dependencies from ember-intl.

However, because (1) #1864 shows changes to the expected outputs in test files and (2) ember-intl currently doesn't check in CI more complex ways of loading translations, such as writing translations to dist folder and pushing translations into ember-intl, I need to go with an approach that will be the safest for many Ember projects, not just your team's and mine.

In addition, my perspective on vulnerability reports (e.g. from Dependabot) is similar to that of https://overreacted.io/npm-audit-broken-by-design/: For frontend, they tend to produce many false positives, with little actionable information on how a vulnerability can be exploited in practice (especially, in the context of Ember), not just in theory.

For example, broccoli-merge-files indirectly depends on glob-parent through fast-glob, but fast-glob is used just once to read translation files. (Here, I may be incorrect, since I'm not too familiar with how ember-intl has used broccoli-merge-files over the years.)

At work, we use yarn's overrides to install [email protected], and this works fine with [email protected] and 7.0.0-beta.2. Therefore, I think asking end-developers to use their package manager to install [email protected] is a safer option than merging #1864.

As I mentioned earlier, it will take me some time (about a week) to learn more about broccoli-merge-files and test the proposed change in #1864 against my team's production project. In the meantime, you could help me out by:

• Creating an additional Ember app(s) in the docs folder, so that, in CI, we can check that we can continue to (1) push translations into ember-intl and (2) write translations to dist folder.
• Looking into "only replacing the usage of broccoli-merge-files with manual broccoli plumbing" than Jan mentioned. That is, the expected outputs in test files are to remain the same.
• Talking to your security team about considering the risk of glob-parent from ember-intl to be low. Together, figure out how fg.stream() (from fast-glob) can actually be exploited.

What do you think?

from ember-intl.

@ijlee2 thanks for the detailed feedback.

We have certain regulatory requirements that make working with these vulnerabilities more difficult and while I agree there is a great argument for how low the risk is for these vulnerabilities in build tools we are still making an effort to remove them.

I understand now the effect of removing the intermediate json files, my original interpretation was those were thrown out in favor of the final translations.js file. I should be able to modify my PR to restore those json files pretty easily.

Could you tell me a little more of what you'd want with the additional docs app?

from ember-intl.

@ijlee2 I updated my PR to get back the behavior I had inadvertently removed. If I'm understanding correctly, we need to support the publicOnly=true case where the translations get loaded asynchronously rather than being combined into the bundle. Could you let me know if my new approach is right? Is the additional docs app you want to enable tests that the publicOnly=true case works?

from ember-intl.

@LucasHill For additional docs-apps, let's start with #1866 and #1867, because I'm not sure yet how we could test the dist folder easily in CI, when setting publicOnly to true.

#1866 is easier to complete so I recommend that you start with that one (with create 1 PR per issue). Let me know if you need clarifications and I can reply to you next week.

Thanks,

from ember-intl.

@ijlee2 Unfortunately I don't have the bandwidth currently to take on those projects, are they a prerequisite for patching the vulnerability? Do you have concerns my PR as it is now may break compatibility with other projects? To me is looks like the output will be the exact same as it is currently, but I could still be missing some context.

from ember-intl.

@LucasHill Sorry for a late reply. I definitely want to see #1866 and #1867 completed before #1864 is tested, since these two apps will give us more confidence in merging #1864.

No worries about currently lacking bandwidth. I suggest that we rely on the Ember community to lend help with the two issues (if no one takes them, I'll get to them later). In the meantime, will you use your package manager's overrides feature to install [email protected]?

from ember-intl.