fix: run all containers as non-root users - #138
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThree Dockerfiles: add Corepack/PNPM setup (create /opt/corepack and /pnpm), create non-root users (node or appuser), ensure /ml-models/embedding-models exists with adjusted ownership/permissions, chown /app in development/runtime stages, and switch production/development to run as the non-root user. ChangesNon-root user execution and package manager configuration
🎯 4 (Complex) | ⏱️ ~40 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docker/Dockerfile.background-jobs (1)
12-21: 💤 Low valueMissing apt cache cleanup in base-cpu stage.
The
base-gpustage (line 56) includesrm -rf /var/lib/apt/lists/*butbase-cpudoes not. Adding this cleanup would reduce the final image size.Suggested fix
RUN apt-get update && apt-get install -y --no-install-recommends \ openssl \ ffmpeg \ curl \ libgomp1 \ ca-certificates \ && corepack enable \ && corepack prepare pnpm@${PNPM_VERSION} --activate \ && mkdir -p /opt/corepack /pnpm \ - && chmod -R a+rx /opt/corepack /pnpm + && chmod -R a+rx /opt/corepack /pnpm \ + && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/Dockerfile.background-jobs` around lines 12 - 21, The base-cpu stage is missing apt cache cleanup like base-gpu; update the RUN layer that executes apt-get update && apt-get install ... in the base-cpu stage to remove /var/lib/apt/lists/* after installs (e.g., append && rm -rf /var/lib/apt/lists/*) so the image size is reduced; mirror the same cleanup placement/ordering used in the base-gpu stage to keep layers consistent (modify the RUN that installs openssl/ffmpeg/curl/libgomp1/ca-certificates and corepack/pnpm setup).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docker/Dockerfile.background-jobs`:
- Around line 12-21: The base-cpu stage is missing apt cache cleanup like
base-gpu; update the RUN layer that executes apt-get update && apt-get install
... in the base-cpu stage to remove /var/lib/apt/lists/* after installs (e.g.,
append && rm -rf /var/lib/apt/lists/*) so the image size is reduced; mirror the
same cleanup placement/ordering used in the base-gpu stage to keep layers
consistent (modify the RUN that installs
openssl/ffmpeg/curl/libgomp1/ca-certificates and corepack/pnpm setup).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f23e2a30-1650-48f2-bc11-f43ec28a6bd4
📒 Files selected for processing (3)
docker/Dockerfile.background-jobsdocker/Dockerfile.mldocker/Dockerfile.web
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/Dockerfile.web
All three service containers were running as root. This PR fixes each one for both production and development targets. **Dockerfile.background-jobs** - Set COREPACK_HOME=/opt/corepack so the pnpm binary is stored in a shared, world-readable path - Create the node user explicitly in the CUDA base image - USER node in production and development stages - Pre-create /ml-models/embedding-models owned by node (chmod 777) so the embedding-core module can write its model cache at startup **Dockerfile.web** - Same COREPACK_HOME + mkdir pattern in the base stage - USER node in production and development stages - Pre-create /ml-models/embedding-models owned by node (chmod 777) **Dockerfile.ml** - Create appuser (UID/GID 1001) in both base-cpu and base-gpu stages - Replace chmod -R 777 /ml-models with chown -R appuser:appgroup + chmod 777 so the shared ml-models volume is also writable by the node user - USER appuser in all four stages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
77bd3c4 to
2c28ae7
Compare
Non-root containers cannot chmod system binaries installed via apt. The chmod was unnecessary since apt-installed binaries are already executable. Drop ensureBinaryPermissions entirely and make the validate/spawn functions sync. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove .then() wrappers that assumed the spawn functions returned Promises — they now return ChildProcess directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/media-utils/src/utils/audio.ts (1)
185-214: ⚡ Quick winAdd stderr collection for better error diagnostics.
Unlike
extractSceneAudiowhich includes stderr in failure messages,readAudioonly reports the exit code. Collecting stderr would help diagnose FFmpeg failures.♻️ Proposed fix
const ffmpeg = spawnFFmpeg(args) const chunks: Buffer[] = [] + let stderr = '' ffmpeg.stdout?.on('data', (chunk) => { chunks.push(chunk) }) + ffmpeg.stderr?.on('data', (data) => { + stderr += data.toString() + }) + ffmpeg.on('close', (code) => { if (code !== 0) { - reject(new Error(`FFmpeg process exited with code ${code}`)) + reject(new Error(`FFmpeg process exited with code ${code}: ${stderr.trim() || 'Unknown error'}`)) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/media-utils/src/utils/audio.ts` around lines 185 - 214, The readAudio function currently only reports FFmpeg's exit code on failure; update it to collect ffmpeg.stderr output (similar to extractSceneAudio) by creating a stderrChunks array and attaching ffmpeg.stderr?.on('data', ...) to push chunks, then on 'close' include the concatenated stderr string in the reject(new Error(...)) when code !== 0 to aid diagnostics; also include the stderr text in the ffmpeg.on('error', ...) rejection message and ensure you reference spawnFFmpeg, ffmpeg.stdout, ffmpeg.stderr, and the existing chunks/close/error handlers when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/media-utils/src/utils/audio.ts`:
- Around line 185-214: The readAudio function currently only reports FFmpeg's
exit code on failure; update it to collect ffmpeg.stderr output (similar to
extractSceneAudio) by creating a stderrChunks array and attaching
ffmpeg.stderr?.on('data', ...) to push chunks, then on 'close' include the
concatenated stderr string in the reject(new Error(...)) when code !== 0 to aid
diagnostics; also include the stderr text in the ffmpeg.on('error', ...)
rejection message and ensure you reference spawnFFmpeg, ffmpeg.stdout,
ffmpeg.stderr, and the existing chunks/close/error handlers when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cb8e037-cc15-4993-bc45-1e448d6dfe63
📒 Files selected for processing (6)
docker/Dockerfile.background-jobsdocker/Dockerfile.mldocker/Dockerfile.webpackages/media-utils/src/lib/ffmpeg.tspackages/media-utils/src/utils/audio.tspackages/media-utils/src/utils/frame.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docker/Dockerfile.background-jobs
- docker/Dockerfile.web
- docker/Dockerfile.ml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/media-utils/src/lib/ffmpeg.ts`:
- Around line 6-24: The current validateBinary uses existsSync which only checks
file existence; update it to call fs.accessSync(binaryPath, constants.X_OK) to
ensure the binary is executable and throw a clear Error if accessSync raises (or
wrap in try/catch and throw new Error including the original error.message).
Apply this change where validateBinary is defined so validateBinaries(),
spawnFFmpeg(), and spawnFFprobe() all benefit; keep the same error message
formatting but include access failures (use symbols: validateBinary,
validateBinaries, spawnFFmpeg, spawnFFprobe, existsSync -> accessSync, and
constants.X_OK).
In `@packages/media-utils/src/utils/audio.ts`:
- Around line 185-213: The readAudio routine currently never reads ffmpeg.stderr
which can block the child and loses FFmpeg diagnostics; modify the code around
spawnFFmpeg and the ffmpeg listeners to add a stderr drain: attach
ffmpeg.stderr?.on('data', chunk => stderrChunks.push(chunk)) and collect into a
string/Buffer (e.g., stderrChunks: Buffer[]), ensure you include that joined
stderr content in both the rejection when ffmpeg 'close' returns a non-zero code
and in the ffmpeg 'error' rejection (e.g., include stderrText in the Error
message), and also make sure to clear/drain stderr on successful resolve so the
pipe is consumed and won't block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b1e94ca7-24bc-454c-9940-05c849b70d90
📒 Files selected for processing (6)
docker/Dockerfile.background-jobsdocker/Dockerfile.mldocker/Dockerfile.webpackages/media-utils/src/lib/ffmpeg.tspackages/media-utils/src/utils/audio.tspackages/media-utils/src/utils/frame.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docker/Dockerfile.ml
- docker/Dockerfile.web
- docker/Dockerfile.background-jobs
| const validateBinary = (binaryPath: string, name: string): void => { | ||
| if (!existsSync(binaryPath)) { | ||
| throw new Error(`${name} binary not found at path: ${binaryPath}`) | ||
| } | ||
| } | ||
|
|
||
| export const validateBinaries = async (): Promise<void> => { | ||
| await Promise.all([validateBinary(FFMPEG_PATH, 'FFmpeg'), validateBinary(FFPROBE_PATH, 'FFprobe')]) | ||
| export const validateBinaries = (): void => { | ||
| validateBinary(FFMPEG_PATH, 'FFmpeg') | ||
| validateBinary(FFPROBE_PATH, 'FFprobe') | ||
| } | ||
|
|
||
| export const spawnFFmpeg = async (args: string[]): Promise<ChildProcess> => { | ||
| await validateBinary(FFMPEG_PATH, 'FFmpeg') | ||
| export const spawnFFmpeg = (args: string[]): ChildProcess => { | ||
| validateBinary(FFMPEG_PATH, 'FFmpeg') | ||
| return spawn(FFMPEG_PATH, args) | ||
| } | ||
|
|
||
| export const spawnFFprobe = async (args: string[]): Promise<ChildProcess> => { | ||
| await validateBinary(FFPROBE_PATH, 'FFprobe') | ||
| export const spawnFFprobe = (args: string[]): ChildProcess => { | ||
| validateBinary(FFPROBE_PATH, 'FFprobe') | ||
| return spawn(FFPROBE_PATH, args) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
find . -name "ffmpeg.ts" -type f | head -5Repository: IliasHad/edit-mind
Length of output: 146
🏁 Script executed:
# Read the file to see current implementation
cat -n packages/media-utils/src/lib/ffmpeg.ts | head -40Repository: IliasHad/edit-mind
Length of output: 1655
🏁 Script executed:
# Search for usage of validateBinaries and spawn calls
rg "validateBinaries|spawnFFmpeg|spawnFFprobe" -A 2 -B 2 --type tsRepository: IliasHad/edit-mind
Length of output: 9142
Use accessSync to check executability, not just file existence.
Line 7 only verifies that the path exists via existsSync. In non-root containers, FFmpeg/FFprobe can exist but lack executable permissions, causing EACCES failures at runtime instead of failing during the preflight validateBinaries() check. This weakens the validation contract and defers the error to the first media operation.
Use fs.accessSync() with constants.X_OK to verify executability:
Suggested fix
-import { existsSync } from 'fs'
+import { accessSync, constants } from 'fs'
const validateBinary = (binaryPath: string, name: string): void => {
- if (!existsSync(binaryPath)) {
- throw new Error(`${name} binary not found at path: ${binaryPath}`)
+ try {
+ accessSync(binaryPath, constants.X_OK)
+ } catch {
+ throw new Error(`${name} binary is missing or not executable: ${binaryPath}`)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/media-utils/src/lib/ffmpeg.ts` around lines 6 - 24, The current
validateBinary uses existsSync which only checks file existence; update it to
call fs.accessSync(binaryPath, constants.X_OK) to ensure the binary is
executable and throw a clear Error if accessSync raises (or wrap in try/catch
and throw new Error including the original error.message). Apply this change
where validateBinary is defined so validateBinaries(), spawnFFmpeg(), and
spawnFFprobe() all benefit; keep the same error message formatting but include
access failures (use symbols: validateBinary, validateBinaries, spawnFFmpeg,
spawnFFprobe, existsSync -> accessSync, and constants.X_OK).
| const ffmpeg = spawnFFmpeg(args) | ||
| const chunks: Buffer[] = [] | ||
|
|
||
| ffmpeg.stdout?.on('data', (chunk) => { | ||
| chunks.push(chunk) | ||
| }) | ||
|
|
||
| ffmpeg.on('close', (code) => { | ||
| if (code !== 0) { | ||
| reject(new Error(`FFmpeg process exited with code ${code}`)) | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| const buffer = Buffer.concat(chunks) | ||
| const audioData = new Float32Array( | ||
| buffer.buffer, | ||
| buffer.byteOffset, | ||
| buffer.length / Float32Array.BYTES_PER_ELEMENT | ||
| ) | ||
| resolve(audioData) | ||
| } catch (error) { | ||
| reject(error) | ||
| } | ||
| }) | ||
|
|
||
| ffmpeg.on('error', (error) => { | ||
| reject(new Error(`Failed to spawn ffmpeg: ${error.message}`)) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the audio.ts file
find . -name "audio.ts" -type f | head -5Repository: IliasHad/edit-mind
Length of output: 104
🏁 Script executed:
# Get more details about the file structure
git ls-files | grep -E "(audio\.ts|ffmpeg)" | head -20Repository: IliasHad/edit-mind
Length of output: 224
🏁 Script executed:
# Check the size of audio.ts to see if we can safely read it
wc -l packages/media-utils/src/utils/audio.tsRepository: IliasHad/edit-mind
Length of output: 106
🏁 Script executed:
# Read the audio.ts file to see lines around 185-213
cat -n packages/media-utils/src/utils/audio.ts | sed -n '170,230p'Repository: IliasHad/edit-mind
Length of output: 1353
🏁 Script executed:
# Also check the spawnFFmpeg function to understand stderr handling
cat -n packages/media-utils/src/lib/ffmpeg.ts | head -100Repository: IliasHad/edit-mind
Length of output: 1980
🌐 Web query:
Node.js child_process spawn default stdio behavior stderr pipe
💡 Result:
In Node.js, child_process.spawn defaults to creating pipes for stdin, stdout, and stderr between the parent and the child. 1) Default stdio behavior - By default, spawn sets stdio equivalent to ['pipe', 'pipe', 'pipe'] (so the child’s stdin/stdout/stderr are connected to streams on the ChildProcess object). [1][2] - The documentation also states that pipes have limited capacity; if the child writes more than the buffer while the parent isn’t consuming it, the child can block (like shell pipes). [1][2] 2) What this means for stderr specifically - With the default stdio ('pipe' for stderr), child.stderr is a readable stream you can consume (and it is not automatically inherited to the parent’s terminal). [1][2] - If instead you set stdio to inherit, then the child’s stderr/stdout are passed through to the parent process streams rather than being exposed as pipe streams. [2] 3) Related options you may see - stdio: 'inherit' passes through parent stdio; in contrast, the default uses 'pipe'. [2] - The silent option (default false) determines whether stdio is piped or inherited; but stdio (if provided) overrides silent. [2][1] 4) Quick example - Default (pipes for everything): spawn('cmd') gives you child.stdout and child.stderr as streams you can read. - To forward child stderr directly to the terminal: spawn('cmd', { stdio: ['pipe','pipe','inherit'] }) or stdio: 'inherit' depending on your needs. [2] If you tell me your exact spawn call and desired behavior (e.g., “show stderr live in my terminal and also capture it”), I can suggest the correct stdio tuple.
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://nodejs.org/download/release/v18.20.4/docs/api/child_process.html
Drain stderr in readAudio.
The function does not read ffmpeg.stderr. Since spawn() defaults to piping stderr, an unread pipe can fill up and block the child process. Additionally, FFmpeg's diagnostic output is lost, making failures harder to debug. Capture stderr and include it in the rejection error.
Suggested fix
const ffmpeg = spawnFFmpeg(args)
const chunks: Buffer[] = []
+ let stderr = ''
ffmpeg.stdout?.on('data', (chunk) => {
chunks.push(chunk)
})
+ ffmpeg.stderr?.on('data', (chunk) => {
+ stderr += chunk.toString()
+ })
+
ffmpeg.on('close', (code) => {
if (code !== 0) {
- reject(new Error(`FFmpeg process exited with code ${code}`))
+ reject(new Error(`FFmpeg process exited with code ${code}: ${stderr.trim() || 'Unknown error'}`))
return
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/media-utils/src/utils/audio.ts` around lines 185 - 213, The
readAudio routine currently never reads ffmpeg.stderr which can block the child
and loses FFmpeg diagnostics; modify the code around spawnFFmpeg and the ffmpeg
listeners to add a stderr drain: attach ffmpeg.stderr?.on('data', chunk =>
stderrChunks.push(chunk)) and collect into a string/Buffer (e.g., stderrChunks:
Buffer[]), ensure you include that joined stderr content in both the rejection
when ffmpeg 'close' returns a non-zero code and in the ffmpeg 'error' rejection
(e.g., include stderrText in the Error message), and also make sure to
clear/drain stderr on successful resolve so the pipe is consumed and won't
block.
1 participant
Summary
All three service containers were running as root. This PR fixes each one for both production and development targets.
Dockerfile.background-jobs
COREPACK_HOME=/opt/corepackso the pnpm binary is stored in a shared, world-readable path (not root's home)mkdir -p /opt/corepack /pnpmbeforechmodso both directories exist at build time (fixeschmod: cannot access '/pnpm'on the CUDA base)nodeuser explicitly in the CUDA base — the nvidia image has nonodeuser unlike the officialnode:imageUSER nodein production and development stages;chown -R node:node /appin dev so tsx watch can write at runtimeDockerfile.web
COREPACK_HOME+mkdirpattern in the base stageUSER nodein production and development stages;chown -R node:node /appin dev so Vite can write its cacheDockerfile.ml
appuser(UID/GID 1001) in bothbase-cpuandbase-gpustageschmod -R 777 /ml-modelswithchown -R appuser:appgroup /ml-models && chmod 755chown -R appuser:appgroup /app+USER appuserin all four stages:production,production-gpu,development,development-gpuTest results
All 6 image targets built and verified locally:
background-jobsdevnodebackground-jobsprodnodewebdevnodewebprodnodemldevappuser/ml-modelswritable ✓, Python 3.11.15 ✓mlprodappuser/ml-modelswritable ✓, Python 3.11.15 ✓🤖 Generated with Claude Code
Summary by CodeRabbit
Security
Infrastructure
Bug Fixes / Improvements