Skip to content

Fix direct URL relative destination not resolved against DOWNLOADS_DIR#19

Merged
loganbuilt merged 1 commit intosudoStacks:mainfrom
bradleesand:fix/direct-url-relative-destination
Apr 15, 2026
Merged

Fix direct URL relative destination not resolved against DOWNLOADS_DIR#19
loganbuilt merged 1 commit intosudoStacks:mainfrom
bradleesand:fix/direct-url-relative-destination

Conversation

@bradleesand
Copy link
Copy Markdown
Contributor

Summary

  • _run_direct_url_with_cli receives destination from the UI as a relative path (e.g. Singles) but passed it directly to ensure_dir() and finalize_download_artifact(), causing os.makedirs to resolve it against the process CWD (/app) instead of DOWNLOADS_DIR (/downloads)
  • When running as a non-root user this raises PermissionError: [Errno 13] Permission denied: 'Singles'; when running as root it silently writes files inside the container rather than the mounted downloads volume
  • The job queue path correctly calls resolve_dir(raw, paths.single_downloads_dir) for this reason — this fix applies the same logic to the direct URL CLI path

Test plan

  • Submit a direct URL (e.g. Facebook reel) with a relative single_download_folder configured (e.g. Singles) — file should land in $DOWNLOADS_DIR/Singles/ instead of erroring
  • Submit a direct URL with no destination set — file should land in $DOWNLOADS_DIR/ as before
  • Submit a direct URL with an absolute destination — should still work unchanged

_run_direct_url_with_cli received the destination from the UI as a
relative path (e.g. 'Singles') but passed it directly to ensure_dir()
and finalize_download_artifact(), causing os.makedirs to resolve it
against the process CWD (/app) instead of DOWNLOADS_DIR (/downloads).

The job queue path correctly calls resolve_dir(raw, paths.single_downloads_dir)
for this reason; apply the same logic here: if the destination is relative,
join it against DOWNLOADS_DIR before use.
@loganbuilt
Copy link
Copy Markdown
Collaborator

Thanks for catching this!!

The bug is real, but before merging I want this path to use the same resolution logic as the queue path, including rejecting destinations that escape DOWNLOADS_DIR via inputs like ../.... Can you update this to use the shared resolve_dir(...) helper and add a regression test for both a relative destination like Singles and an escape case? I’m happy to re-review after that.

I appreciate your contribution!

@loganbuilt loganbuilt self-requested a review April 8, 2026 19:04
Copy link
Copy Markdown
Collaborator

@loganbuilt loganbuilt left a comment

Choose a reason for hiding this comment

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

Will merge as-is for to main and will implement the hard fix on the latest dev branch

@loganbuilt loganbuilt merged commit a12c873 into sudoStacks:main Apr 15, 2026
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.

2 participants