Skip to content

ci: set min age for npm installed packages - #1003

Merged
google-oss-prow[bot] merged 3 commits into
kubeflow:notebooks-v2from
christian-heusel:ci/set-min-age
Apr 2, 2026
Merged

google-oss-prow[bot] merged 3 commits into
kubeflow:notebooks-v2from
christian-heusel:ci/set-min-age

Conversation

@christian-heusel

@christian-heusel christian-heusel commented Apr 1, 2026 •

Copy link
Copy Markdown
Member
  • chore: Infer node version in CI from packages.json
  • ci: Enforce a minimum npm release age for increased supply chain security
  • ci: Use npm ci instead of npm install in CI

@github-project-automation github-project-automation Bot moved this to Needs Triage in Kubeflow Notebooks Apr 1, 2026
@google-oss-prow google-oss-prow Bot added the area/ci area - related to ci label Apr 1, 2026
@google-oss-prow
google-oss-prow Bot requested review from andyatmiami and ederign April 1, 2026 09:23
@google-oss-prow google-oss-prow Bot added area/frontend area - related to frontend components area/v2 area - version - kubeflow notebooks v2 size/S labels Apr 1, 2026
@christian-heusel

Copy link
Copy Markdown
Member Author

/ok-to-test

@andyatmiami andyatmiami left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

overall this looks just - just a minor nit and looking for simply some clarification on one of the changes.

Comment thread workspaces/frontend/package.json Outdated
},
"engines": {
"node": ">=20.0.0"
"npm" : "^11.10.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

minor nit:

Suggested change
"npm" : "^11.10.0",
"npm": "^11.10.0",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I recall correctly, npm 11 ships with Node 24 (not yet LTS). Node 20 ships with npm 10.x. Are we sure we want to pinning to ^20 for Node but requiring a npm version that doesn't ship with Node 20?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, we actually need to (I outlined the reasoning in the commit message for 790e0e1) if we want to use the min-release-age attrbute. Before this version it is not respected.

Comment thread workspaces/frontend/package.json Resolved
Comment thread .github/workflows/ws-frontend-test.yml Resolved
Comment thread workspaces/frontend/.npmrc Resolved
@ederign

ederign commented Apr 2, 2026 •

Copy link
Copy Markdown
Member

@christian-heusel thanks for working on this! Just some questions in the PR.

@thesuperzapper

Copy link
Copy Markdown
Member

I think this change is critical for the security of maintainers themselves, as there have been many recent cases where these supply chain attacks target the developer computers, rather than the end user consuming the javascript service.

@ederign

ederign commented Apr 2, 2026 •

Copy link
Copy Markdown
Member

@thesuperzapper I agree with you, the only point that I'm making is the corner case/tradeoff if there is a day zero CVE and a patch recently released, we would need to patch both package and min-release-age=7 (something that Dependabot doesn't do).

Signed-off-by: Christian Heusel <christian@heusel.eu>
…rity

This feature is available the earliest in `npm` with version "v11.10.0"

The upstream `actions/setup-node` GHA does not allow to specify a fixed
`npm` version, but we need it to be a specific version in an upcoming
change, on its own the action itself installs version "10.8.2" right now.

To keep the setup maintainable the version is extracted from
`packages.json`.

Related to actions/setup-node#529
Related to npm/cli#8570

Signed-off-by: Christian Heusel <christian@heusel.eu>
This way the `packages-lock.json` stays untouched and other sane
defaults are set for the CI.

Link: https://docs.npmjs.com/cli/v8/commands/npm-ci
Signed-off-by: Christian Heusel <christian@heusel.eu>
@ederign

ederign commented Apr 2, 2026

Copy link
Copy Markdown
Member

Follow up for my comment will be part of : #818

@ederign

ederign commented Apr 2, 2026

Copy link
Copy Markdown
Member

/approve
/lgtm

@google-oss-prow google-oss-prow Bot added the lgtm label Apr 2, 2026
@ederign

ederign commented Apr 2, 2026

Copy link
Copy Markdown
Member

Thanks @christian-heusel

@ederign

ederign commented Apr 2, 2026

Copy link
Copy Markdown
Member

/approve

@ederign

ederign commented Apr 2, 2026

Copy link
Copy Markdown
Member

@andyatmiami @thesuperzapper we need someone with approval on CI.

@andyatmiami

Copy link
Copy Markdown
Contributor

/lgtm
/approve

thanks @christian-heusel - we have been discussing this on/off on various slack channels today and feel like this improves our security posture. we can/will adjust/adapt moving forward as needs arise

  • i.e. if 7 day window blocks us from "need now" updates

But i think we have good ideas about how that can work - so it won't leave us in a real bind to get these changes in now.

@google-oss-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyatmiami, ederign

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow
google-oss-prow Bot merged commit 8fdffe8 into kubeflow:notebooks-v2 Apr 2, 2026
8 checks passed
@github-project-automation github-project-automation Bot moved this from Needs Triage to Done in Kubeflow Notebooks Apr 2, 2026
andyatmiami added a commit to andyatmiami/kubeflow-notebooks that referenced this pull request Apr 3, 2026
The backend added a `namespaceFilter` query param to GET /workspacekinds
(kubeflow#1009). Without it, the endpoint requires cluster-wide `list workspacekinds`
permission (admin-only). With the filter, auth falls back to checking
`create workspaces` in the given namespace, allowing regular users access.

The frontend was always calling `listWorkspaceKinds()` without the filter,
so non-admin users would get 403 when auth is enabled.

Changes:
- Update swagger.version to include the namespaceFilter swagger changes
- Regenerate the API client from the updated swagger spec
- Add optional `namespaceFilter` param to the `useWorkspaceKinds` hook
- Thread namespace through WorkspaceFormKindSelection and WorkspaceTable
- Add unit test coverage for the new parameter
- Sync package-lock.json engines field with package.json
    - `package.json` updated as part of kubeflow#1003 but `npm` was not ran to
      reflect this in the lock file

Signed-off-by: Andy Stoneberg <astonebe@redhat.com>
google-oss-prow Bot pushed a commit that referenced this pull request Apr 3, 2026
The backend added a `namespaceFilter` query param to GET /workspacekinds
(#1009). Without it, the endpoint requires cluster-wide `list workspacekinds`
permission (admin-only). With the filter, auth falls back to checking
`create workspaces` in the given namespace, allowing regular users access.

The frontend was always calling `listWorkspaceKinds()` without the filter,
so non-admin users would get 403 when auth is enabled.

Changes:
- Update swagger.version to include the namespaceFilter swagger changes
- Regenerate the API client from the updated swagger spec
- Add optional `namespaceFilter` param to the `useWorkspaceKinds` hook
- Thread namespace through WorkspaceFormKindSelection and WorkspaceTable
- Add unit test coverage for the new parameter
- Sync package-lock.json engines field with package.json
    - `package.json` updated as part of #1003 but `npm` was not ran to
      reflect this in the lock file

Signed-off-by: Andy Stoneberg <astonebe@redhat.com>
thesuperzapper pushed a commit that referenced this pull request Apr 7, 2026
* chore: Infer node version in CI from `packages.json`

Signed-off-by: Christian Heusel <christian@heusel.eu>

* ci: Enforce a minimum npm release age for increased supply chain security

This feature is available the earliest in `npm` with version "v11.10.0"

The upstream `actions/setup-node` GHA does not allow to specify a fixed
`npm` version, but we need it to be a specific version in an upcoming
change, on its own the action itself installs version "10.8.2" right now.

To keep the setup maintainable the version is extracted from
`packages.json`.

Related to actions/setup-node#529
Related to npm/cli#8570

Signed-off-by: Christian Heusel <christian@heusel.eu>

* ci: Use `npm ci` instead of `npm install` in CI

This way the `packages-lock.json` stays untouched and other sane
defaults are set for the CI.

Link: https://docs.npmjs.com/cli/v8/commands/npm-ci
Signed-off-by: Christian Heusel <christian@heusel.eu>

---------

Signed-off-by: Christian Heusel <christian@heusel.eu>
thesuperzapper pushed a commit that referenced this pull request Apr 7, 2026
The backend added a `namespaceFilter` query param to GET /workspacekinds
(#1009). Without it, the endpoint requires cluster-wide `list workspacekinds`
permission (admin-only). With the filter, auth falls back to checking
`create workspaces` in the given namespace, allowing regular users access.

The frontend was always calling `listWorkspaceKinds()` without the filter,
so non-admin users would get 403 when auth is enabled.

Changes:
- Update swagger.version to include the namespaceFilter swagger changes
- Regenerate the API client from the updated swagger spec
- Add optional `namespaceFilter` param to the `useWorkspaceKinds` hook
- Thread namespace through WorkspaceFormKindSelection and WorkspaceTable
- Add unit test coverage for the new parameter
- Sync package-lock.json engines field with package.json
    - `package.json` updated as part of #1003 but `npm` was not ran to
      reflect this in the lock file

Signed-off-by: Andy Stoneberg <astonebe@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved area/ci area - related to ci area/frontend area - related to frontend components area/v2 area - version - kubeflow notebooks v2 lgtm ok-to-test size/S

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants