Skip to content

Support for private registry NPM packages by using NPM_TOKEN and .npmrc (#1390) #1405

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Support for private registry NPM packages by using NPM_TOKEN and .npmrc (#1390) #1405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47b11ff
Support for private registry NPM packages by using NPM_TOKEN and .npmrc
yadavshubham01 Oct 12, 2024
126b78f
Update Dockerfile
yadavshubham01 Oct 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ jobs:

- name: 📥 Download deps
run: pnpm install --frozen-lockfile
env:
NPM_TOKEN: ${{ secrets.NPM_TOKEN }} # Use the NPM_TOKEN here

- name: 📀 Generate Prisma Client
run: pnpm run generate
Expand Down
3 changes: 2 additions & 1 deletion .npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
//registry.npmjs.org/:_authToken=${NPM_TOKEN}
link-workspace-packages=false
public-hoist-pattern[]=*prisma*
public-hoist-pattern[]=*prisma*
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
v20.11.1
v20.11.1
16 changes: 16 additions & 0 deletions docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@ RUN find . -name "node_modules" -type d -prune -exec rm -rf '{}' +
FROM node:20.11.1-bullseye-slim@sha256:5a5a92b3a8d392691c983719dbdc65d9f30085d6dcd65376e7a32e6fe9bf4cbe AS base
RUN apt-get update && apt-get install -y openssl dumb-init
WORKDIR /triggerdotdev

# Copy .npmrc and use the NPM_TOKEN for private registry
ARG NPM_TOKEN
COPY .npmrc .npmrc
RUN if [ -f .npmrc ] && [ -n "${NPM_TOKEN}" ]; then \
echo "//registry.npmjs.org/:_authToken=${NPM_TOKEN}" >> .npmrc; \
fi

# For better security, use Docker BuildKit secrets
RUN --mount=type=secret,id=npm_token \
echo "//registry.npmjs.org/:_authToken=$(cat /run/secrets/npm_token)" >> .npmrc
Comment on lines +13 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Streamline NPM_TOKEN handling

The current implementation has two methods for adding the NPM_TOKEN to .npmrc:

  1. Using an ARG and conditional check (lines 15-19)
  2. Using Docker BuildKit secrets (lines 22-23)

To simplify and improve security, consider using only the Docker BuildKit secrets method. This approach prevents the token from being stored in intermediate layers.

Replace the current NPM_TOKEN handling with:

# Remove these lines
ARG NPM_TOKEN
COPY .npmrc .npmrc
RUN if [ -f .npmrc ] && [ -n "${NPM_TOKEN}" ]; then \
      echo "//registry.npmjs.org/:_authToken=${NPM_TOKEN}" >> .npmrc; \
    fi

# Keep only this part
COPY .npmrc .npmrc
RUN --mount=type=secret,id=npm_token \
    if [ -f /run/secrets/npm_token ]; then \
      echo "//registry.npmjs.org/:_authToken=$(cat /run/secrets/npm_token)" >> .npmrc; \
    fi

This change ensures that the NPM_TOKEN is only handled securely using Docker BuildKit secrets.


COPY --chown=node:node .gitignore .gitignore
COPY --from=pruner --chown=node:node /triggerdotdev/out/json/ .
COPY --from=pruner --chown=node:node /triggerdotdev/out/pnpm-lock.yaml ./pnpm-lock.yaml
Expand All @@ -21,6 +33,8 @@ WORKDIR /triggerdotdev
# Corepack is used to install pnpm
RUN corepack enable
ENV NODE_ENV development
# Copy .npmrc again for dev-deps stage
COPY .npmrc .npmrc
RUN pnpm install --ignore-scripts --no-frozen-lockfile
Comment on lines 35 to 38
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve dependency installation in dev-deps stage

  1. Copying .npmrc again might be redundant if it's already in the base stage.
  2. The pnpm install command uses flags that might lead to inconsistent builds:
    • --ignore-scripts skips running scripts specified in package.json
    • --no-frozen-lockfile allows installing newer versions of dependencies

Consider the following changes:

  1. Remove the redundant COPY of .npmrc if it's not necessary.
  2. Modify the pnpm install command to ensure reproducible builds:
RUN pnpm install --frozen-lockfile

This change ensures that the exact versions specified in the lockfile are installed, promoting build consistency across environments.


## Production deps
Expand All @@ -29,6 +43,8 @@ WORKDIR /triggerdotdev
# Corepack is used to install pnpm
RUN corepack enable
ENV NODE_ENV production
# Copy .npmrc again for production-deps stage
COPY .npmrc .npmrc
RUN pnpm install --prod --no-frozen-lockfile
COPY --from=pruner --chown=node:node /triggerdotdev/internal-packages/database/prisma/schema.prisma /triggerdotdev/internal-packages/database/prisma/schema.prisma
# RUN pnpm add @prisma/client@5.1.1 -w
Expand Down
12 changes: 12 additions & 0 deletions docker/scripts/build-extension.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/bin/bash

# Copy the .npmrc file into the build directory
echo "Copying .npmrc to build context..."
cp .npmrc /app/.npmrc

# Set the NPM_TOKEN environment variable for the build
export NPM_TOKEN=$1
Comment on lines +7 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance security and robustness of NPM_TOKEN handling.

While setting the NPM_TOKEN is necessary, the current implementation has some potential issues:

  1. It assumes the token is always provided as the first argument.
  2. There's no validation of the token.
  3. The token is exposed in the environment variables.

Consider implementing these improvements:

 # Set the NPM_TOKEN environment variable for the build
-export NPM_TOKEN=$1
+if [ -z "$1" ]; then
+    echo "Error: NPM_TOKEN not provided"
+    exit 1
+fi
+
+# Basic validation of the token (adjust regex as needed)
+if ! [[ $1 =~ ^[a-zA-Z0-9_-]+$ ]]; then
+    echo "Error: Invalid NPM_TOKEN format"
+    exit 1
+fi
+
+# Use read -s to avoid showing the token in process list
+read -r -s NPM_TOKEN <<< "$1"
+export NPM_TOKEN

This change adds error checking, basic token validation, and uses read -s to reduce token visibility.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Set the NPM_TOKEN environment variable for the build
export NPM_TOKEN=$1
# Set the NPM_TOKEN environment variable for the build
if [ -z "$1" ]; then
echo "Error: NPM_TOKEN not provided"
exit 1
fi
# Basic validation of the token (adjust regex as needed)
if ! [[ $1 =~ ^[a-zA-Z0-9_-]+$ ]]; then
echo "Error: Invalid NPM_TOKEN format"
exit 1
fi
# Use read -s to avoid showing the token in process list
read -r -s NPM_TOKEN <<< "$1"
export NPM_TOKEN


# Run the npm install command
echo "Running npm install with NPM_TOKEN..."
pnpm install
Comment on lines +10 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize and improve error handling for package installation.

The use of pnpm for package installation is fine if it's the intended package manager. However, there are a few improvements we can make:

  1. Consider adding the --production flag to avoid installing dev dependencies if this is for a production build.
  2. Add error handling to catch installation failures.
  3. Optionally, add a --frozen-lockfile flag to ensure consistent installations.

Here's a suggested improvement:

 # Run the npm install command
 echo "Running npm install with NPM_TOKEN..."
-pnpm install
+pnpm install --production --frozen-lockfile || { echo "Error: pnpm install failed"; exit 1; }

This change will:

  • Only install production dependencies
  • Ensure consistent installations across environments
  • Exit the script if the installation fails

Note: If dev dependencies are needed, remove the --production flag.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Run the npm install command
echo "Running npm install with NPM_TOKEN..."
pnpm install
# Run the npm install command
echo "Running npm install with NPM_TOKEN..."
pnpm install --production --frozen-lockfile || { echo "Error: pnpm install failed"; exit 1; }

Loading