Skip to content

Run update-ca-certificates as root #1505

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JamieMagee
Copy link
Contributor

This is equivalent to dependabot/cli#466

Required by dependabot/dependabot-core#9627

@Copilot Copilot AI review requested due to automatic review settings June 24, 2025 18:56
@JamieMagee JamieMagee requested a review from a team as a code owner June 24, 2025 18:56
Copilot

This comment was marked as outdated.

@JamieMagee JamieMagee force-pushed the jamiemagee/update-ca-certificates-root branch 2 times, most recently from 1bdad52 to 54934a6 Compare June 25, 2025 04:49
@JamieMagee JamieMagee requested a review from Copilot June 25, 2025 04:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures update-ca-certificates runs as root before executing Dependabot commands under the dependabot user in the updater container, and fixes a test directory creation call.

  • Split command execution into root vs. non-root phases in the container service
  • Updated UpdaterBuilder to launch the container as dependabot with an interactive shell
  • Added { recursive: true } to fs.mkdirSync in the integration test

Reviewed Changes

Copilot reviewed 3 out of 9 changed files in this pull request and generated 1 comment.

File Description
src/updater-builder.ts Set container User to dependabot and override Cmd/Tty flags
src/container-service.ts Introduce execCommand helper, run CA update as root, then user
tests/updater-builder-integration.test.ts Add recursive flag to mkdirSync to avoid missing parent folders
Comments suppressed due to low confidence (2)

tests/updater-builder-integration.test.ts:44

  • The new root-phase update-ca-certificates logic in ContainerService.run isn’t covered by existing integration tests. Consider adding a test that verifies the CA certificates step succeeds when running as root.
    fs.mkdirSync(workingDirectory, {recursive: true})

src/updater-builder.ts:55

  • Overriding the container command to just /bin/sh without -c or a no-op entrypoint leaves the shell open and may prevent the intended workflow. Consider specifying a simple no-op (e.g., ['/bin/sh', '-c', 'while true; do sleep 1; done']) or removing the Cmd override to use the image’s default entrypoint.
      Cmd: ['/bin/sh'],

['/bin/sh', '-c', cmd],
'dependabot'
)
}
Copy link
Preview

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

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

In the dependabot branch, the container process (an interactive shell) is never stopped or waited on before removal, which can lead to resource leaks or hanging. Add an explicit container.stop() or a final container.wait() in the dependabot path once all execCommand calls complete.

Suggested change
}
}
// Stop the container after all commands are executed
await container.stop()

Copilot uses AI. Check for mistakes.

@JamieMagee JamieMagee force-pushed the jamiemagee/update-ca-certificates-root branch from 54934a6 to b518b30 Compare June 25, 2025 04:56
@JamieMagee
Copy link
Contributor Author

JamieMagee commented Jun 25, 2025

Not sure why the difference in dist check is failing. I am running npm run clean-install and npm run package.

EDIT: I'm running Node 24 locally, and GitHub Actions is running Node 20. For some reason these were giving different results.

@JamieMagee JamieMagee force-pushed the jamiemagee/update-ca-certificates-root branch from b518b30 to 866cefc Compare June 25, 2025 19:01
@JamieMagee JamieMagee force-pushed the jamiemagee/update-ca-certificates-root branch from 866cefc to ecfa025 Compare June 25, 2025 19:06
Copy link

@jpinz jpinz left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants