Replace all uses of promiseExec with promiseExecFile #4514

Nokel81 · 2021-12-06T20:31:50Z

Signed-off-by: Sebastian Malton sebastian@malton.name

resolves https://github.com/lensapp/lens-security/issues/7

src/main/helm/helm-release-manager.ts

src/common/system-ca.ts

src/main/helm/helm-release-manager.ts

src/main/helm/helm-repo-manager.ts

Signed-off-by: Sebastian Malton <sebastian@malton.name>

jim-docker

tested on mac and linux (for regression, not security impact)

Signed-off-by: Sebastian Malton <sebastian@malton.name>

jim-docker

So this PR can be cherry-picked for 5.3.4 (and won't suffer this flaky test). And you can turn the test back on in #4564 for master, right? As soon as it is approved :)

Nokel81 · 2021-12-14T20:56:45Z

@jim-docker Yes that is the plan

jansav · 2021-12-16T06:18:43Z

src/main/helm/helm-repo-manager.ts

      const { repositories } = await this.readConfig();

      if (!repositories.length) {
+        if (previousAttempt) {
+          throw new Error("Previous add repo called did not add repo");
+        }
+
        await HelmRepoManager.addRepo({ name: "bitnami", url: "https://charts.bitnami.com/bitnami" });

-        return await this.repositories();
+        return await this.repositories(true);
      }


Having recursion here seems bit odd.

Could we just:

const { repositories: storedRepositories } = await this.readConfig(); let repositories: HelmRepo[] = storedRepositories; if (!repositories.length) { await HelmRepoManager.addRepo({ name: "bitnami", url: "https://charts.bitnami.com/bitnami", }); // Even better would be if we could trust that the HelmRepo used as argument of addRepo is the exact same that "readConfig" returns. ({ repositories } = (await this.readConfig())); }

In bigger picture, I think that adding the default repository should be extracted away because now this function has two separate responsibilities.

Yes I agree that having a default registry should be abstracted away. I have already done this in my follow up branch
Which I will make into a PR once this is merged.

We felt that this PR shouldn't have too many non-necessary changes.

Good points all, but this was a change unrelated to the security fix which has effectively delayed approval of this review.

aleksfront · 2021-12-16T11:22:00Z

Probably not related to current PR, but found that we need to refresh helm chart after go from Preferences page. Right now, it might show stale repos and charts in list. If you click them, you got an app crash:

stale.helm.repos.mov

Nokel81 · 2021-12-16T12:08:45Z

Probably not related to current PR, but found that we need to refresh helm chart after go from Preferences page. Right now, it might show stale repos and charts in list. If you click them, you got an app crash:

stale.helm.repos.mov

I will investigate since I didn't think that this was possible before this PR

Nokel81 · 2021-12-16T16:30:47Z

@aleksfront That also currently exists in master so I don't think we should fix it in this security PR.

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com>

* Use electron.clipboard for all clipboard uses (#4535) Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Fix ERR_UNSAFE_PORT from LensProxy (#4558) * Fix ERR_UNSAFE_PORT from LensProxy - Use the current list of ports from chromium as it is much easier to just reject using one of those instead of trying to handle the ERR_UNSAFE_PORT laod error from a BrowserWindow.on("did-fail-load") Signed-off-by: Sebastian Malton <sebastian@malton.name> * Move all port handling into LensProxy Signed-off-by: Sebastian Malton <sebastian@malton.name> * don't use so many exceptions Signed-off-by: Sebastian Malton <sebastian@malton.name> Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Fix not being able to clear set cluster icon (#4555) Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Fix extension engine range not working for some ^ ranges (#4554) Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Fix crash on NetworkPolicy when matchLabels is missing (#4500) Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Replace all uses of promiseExec with promiseExecFile (#4514) Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Less noisy metrics-not-available error logging (#4602) Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com> Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Fix close button overflow in Preferences (#4611) * Adding basic colors to tailwind theme Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com> * Using tailwind inline to style close button Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com> * Make Select look similar to inputs Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com> * Moving styles into separate module Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com> * Convert tailwind commands to css Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com> Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Fix prometheus operator metrics work out of the box (#4617) Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com> Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Fix CRD.getPreferedVersion() to work based on apiVersion (#4553) * Fix CRD.getPreferedVersion() to work based on apiVersion Signed-off-by: Sebastian Malton <sebastian@malton.name> * Add tests Signed-off-by: Sebastian Malton <sebastian@malton.name> Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Fix crash for KubeObjectStore.loadAll() (#4675) Signed-off-by: Sebastian Malton <sebastian@malton.name> Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * Convert CloseButton styles out from css modules (#4723) * Convert CloseButton styles out from css modules Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com> * Fix close button styling Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com> * release v5.3.4 Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> Co-authored-by: Sebastian Malton <sebastian@malton.name> Co-authored-by: Jari Kolehmainen <jari.kolehmainen@gmail.com> Co-authored-by: Alex Andreev <alex.andreev.email@gmail.com> Co-authored-by: Lauri Nevala <lauri.nevala@gmail.com>

Nokel81 added bug Something isn't working area/security labels Dec 6, 2021

Nokel81 requested a review from a team as a code owner December 6, 2021 20:31

Nokel81 requested review from nevalla and jim-docker and removed request for a team December 6, 2021 20:31

Nokel81 added this to the 5.3.3 milestone Dec 6, 2021

jim-docker reviewed Dec 7, 2021

View reviewed changes

Nokel81 modified the milestones: 5.3.3, 5.3.4 Dec 9, 2021

jim-docker reviewed Dec 13, 2021

View reviewed changes

src/main/helm/helm-repo-manager.ts Outdated Show resolved Hide resolved

Nokel81 added 3 commits December 13, 2021 11:52

Replace all uses of promiseExec with promiseExecFile

e9f6330

Signed-off-by: Sebastian Malton <sebastian@malton.name>

review comments

844b7be

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Fix addCustomRepo getResources error output

e704572

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 force-pushed the security/remove-promiseExec branch from d0edbbf to e704572 Compare December 13, 2021 16:52

Nokel81 mentioned this pull request Dec 13, 2021

Cleanup HelmRepoManager and <HelmCharts> #4564

Merged

jim-docker previously approved these changes Dec 13, 2021

View reviewed changes

Skip integrationt test, will turn it on again in next PR

f185c08

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 dismissed jim-docker’s stale review via f185c08 December 14, 2021 17:02

jim-docker previously approved these changes Dec 14, 2021

View reviewed changes

jansav reviewed Dec 16, 2021

View reviewed changes

Remove non-security bug fix

07d829a

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 dismissed jim-docker’s stale review via 07d829a December 17, 2021 14:42

jim-docker approved these changes Dec 17, 2021

View reviewed changes

jansav approved these changes Dec 17, 2021

View reviewed changes

Iku-turso approved these changes Dec 17, 2021

View reviewed changes

Nokel81 merged commit e9d99d8 into master Dec 17, 2021

Nokel81 deleted the security/remove-promiseExec branch December 17, 2021 15:29

jim-docker pushed a commit that referenced this pull request Jan 19, 2022

Replace all uses of promiseExec with promiseExecFile (#4514)

857dc1a

jim-docker mentioned this pull request Jan 19, 2022

Release/v5.3.4 #4721

Merged

jim-docker pushed a commit that referenced this pull request Jan 19, 2022

Replace all uses of promiseExec with promiseExecFile (#4514)

6ee62ba

Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com>

jim-docker mentioned this pull request Feb 1, 2022

release v5.4.0-alpha.0 #4780

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace all uses of promiseExec with promiseExecFile #4514

Replace all uses of promiseExec with promiseExecFile #4514

Nokel81 commented Dec 6, 2021

jim-docker left a comment

jim-docker left a comment

Nokel81 commented Dec 14, 2021

jansav Dec 16, 2021 •

edited

Nokel81 Dec 16, 2021

jim-docker Dec 16, 2021

aleksfront commented Dec 16, 2021

Nokel81 commented Dec 16, 2021

Nokel81 commented Dec 16, 2021

Replace all uses of promiseExec with promiseExecFile #4514

Replace all uses of promiseExec with promiseExecFile #4514

Conversation

Nokel81 commented Dec 6, 2021

jim-docker left a comment

Choose a reason for hiding this comment

jim-docker left a comment

Choose a reason for hiding this comment

Nokel81 commented Dec 14, 2021

jansav Dec 16, 2021 • edited

Choose a reason for hiding this comment

Nokel81 Dec 16, 2021

Choose a reason for hiding this comment

jim-docker Dec 16, 2021

Choose a reason for hiding this comment

aleksfront commented Dec 16, 2021

Nokel81 commented Dec 16, 2021

Nokel81 commented Dec 16, 2021

jansav Dec 16, 2021 •

edited