Skip to content

fix: yt-dlp update failure when run as non-root user (#451)#458

Merged
dialmaster merged 1 commit intodevfrom
fix/ytdlp-update-nonroot-permissions
Mar 15, 2026
Merged

fix: yt-dlp update failure when run as non-root user (#451)#458
dialmaster merged 1 commit intodevfrom
fix/ytdlp-update-nonroot-permissions

Conversation

@dialmaster
Copy link
Collaborator

  • Move yt-dlp binary from /usr/local/bin/ to /opt/yt-dlp/ with world-writable permissions so non-root containers (YOUTARR_UID/GID) can perform self-updates
  • Detect yt-dlp's "Unable to write to" error message in addition to "Permission denied" so users see the helpful platform-specific message instead of a generic exit code error
  • Add test coverage

- Move yt-dlp binary from /usr/local/bin/ to /opt/yt-dlp/ with
  world-writable permissions so non-root containers (YOUTARR_UID/GID)
  can perform self-updates
- Detect yt-dlp's "Unable to write to" error message in addition to
  "Permission denied" so users see the helpful platform-specific
  message instead of a generic exit code error
- Add test coverage
@github-actions
Copy link
Contributor

📊 Test Coverage Report

Backend Coverage

Type Coverage Status
Lines 79.58% 🟡
Statements 79.45% 🟡
Functions 80.70% 🟢
Branches 72.58% 🟡

Frontend Coverage

Type Coverage Status
Lines 87.40% 🟢
Statements 86.71% 🟢
Functions 80.45% 🟢
Branches 81.51% 🟢

Coverage Requirements

  • Minimum threshold: 70% line coverage
  • Backend: ✅ Passes
  • Frontend: ✅ Passes

Coverage report generated for commit 45021fa

@claude
Copy link

claude bot commented Mar 15, 2026

PR Review: fix: yt-dlp update failure when run as non-root user

The intent is solid. The JS-layer fix and test are clean. The Dockerfile approach has a security problem worth addressing before merge.


Security Issue: World-Writable Binary (chmod 0777)

Making both the directory and the binary itself world-writable means any process in the container can silently replace yt-dlp with arbitrary code. This is particularly risky because yt-dlp processes user-supplied URLs. If a malicious payload escaped the yt-dlp sandbox, a world-writable binary is an easy pivot point for persistence or further exploitation.

Recommended fix: handle ownership in the entrypoint rather than relaxing permissions globally.

Dockerfile at build-time with normal permissions:

RUN mkdir -p /opt/yt-dlp && curl ... -o /opt/yt-dlp/yt-dlp && chmod 0755 /opt/yt-dlp && chmod 0755 /opt/yt-dlp/yt-dlp
ENV PATH=/opt/yt-dlp:$PATH

docker-entrypoint-simple.sh before starting the server:

chown -R $YOUTARR_UID:$YOUTARR_GID /opt/yt-dlp

This way only the designated app user can write to the directory; all other processes get read/execute only.


JS Fix (ytdlpModule.js): looks good

The added "Unable to write to" detection is correct and minimal. That string is what yt-dlp emits on permission errors, so catching it here is the right fix.


Test Coverage: looks good

The new test accurately exercises the new error-string branch and verifies the user-facing result.message surfaces as "Permission denied". No issues.


Minor Notes

  • chmod 0777 on the directory alone would be sufficient for the update use-case, since yt-dlp replaces itself by writing a new file into the directory. Making the existing binary also 0777 adds no benefit and increases attack surface.
  • The ENV PATH line is placed after the RUN block; grouping it adjacent to the RUN would improve readability, though it has no functional impact.

Summary: please fix the chmod 0777 before merging. The entrypoint-based chown approach achieves the same goal without the world-writable binary risk. Everything else looks good.

@dialmaster
Copy link
Collaborator Author

The Claude review is not quite correct:

The entrypoint runs as the non-root user (set via user: "${YOUTARR_UID:-0}:${YOUTARR_GID:-0}" in docker-compose), so chown in the entrypoint would fail because there's no root phase. Making this work with chown would require adding gosu, restructuring the entrypoint to start as root and drop privileges, and removing the user: directive, a much larger change. The fix as-is is good.

@dialmaster dialmaster merged commit e2e13ba into dev Mar 15, 2026
7 checks passed
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.

1 participant