[BUG] [LINUX] All video downloads fail apparently due to unescaped spaces in shell command

[DarwinAwardWinner]

Describe the bug When trying to download any video, the audio download fails. The error log contains lines like:

{
  "error": {
    "cmd": "/opt/Crunchyroll Downloader/resources/shaka/shaka input=\"/tmp/crd-tmp-jcl66swogb/temp-main.m4s\",stream=audio,output=\"/tmp/crd-tmp-jcl66swogb/main.m4s\" --enable_raw_key_decryption --keys key_id=XXXXXX:key=XXXXXX",
    "code": 127,
    "killed": false,
    "signal": null,
    "stderr": "/bin/sh: 1: /opt/Crunchyroll: not found\n",
    "stdout": ""
  },
  "level": "error",
  "message": "Error while merging Audio",
  "section": "audioCrunchyrollMerging",
  "service": "user-service",
  "timestamp": "2024-07-04T15:34:21.084Z"
}

The stderr seems to show that the space in "Crunchyroll Downloader" needs to be escaped.

To Reproduce Steps to reproduce the behavior: Attempt to download any video normally.

Expected behavior Video should download successfully, including all selected audio streams.

Screenshots State of the UI after attempting to download. It gets "stuck" on merging since the audio files aren't available.

Desktop (please complete the following information):

• OS: Ubuntu 23.10
• Browser: Firefox
• Version: CrunchyDL v1.2.4

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 23.10
Release:    23.10
Codename:   mantic

[DarwinAwardWinner]

It looks like the issue is here: https://github.com/OpenSTDL/CrunchyDL/commit/1902f58f3771698e1975f3df61d84be35bad904b#diff-77479a096112b1a9c4877ee7445be4509c509d0deefa924c512b8a501575bba7R165 and also video processing would encounter the same issue here: https://github.com/OpenSTDL/CrunchyDL/commit/1902f58f3771698e1975f3df61d84be35bad904b#diff-aead198a48c793c0c340312b5720382be9ad7786c624a37d3649d1046bf5a0f6R1307. Although looking at the code, I'm unclear as to how this same problem didn't occur in previous versions as well. Did the installation path change from something without spaces to /opt/Crunchyroll Downloader? Anyway, it seems that a similar error could occur if ${tmp} (or any other variable) has spaces in it, which is a problem because that is user-specified. Ideally every variable interpolated into a shell command should be properly shell-quoted (i.e. not just surrounded with quotes).

[DarwinAwardWinner]

Looking into the specifics for JS/typescript, I think the right thing to do is use execFile instead of exec so that no shell interpolation ever happens at all. If I understand correctly, this should eliminate the need to any quoting.

[DarwinAwardWinner]

I'm not a JS programmer, but I think the following (untested) patch should fix things:

0001-Use-child_process.execFile-to-avoid-unintended-shell.patch:

From 3f66538a4df4c0c04d811a76f0d0377e6b38ddf6 Mon Sep 17 00:00:00 2001
From: "Ryan C. Thompson" <rct@thompsonclan.org>
Date: Thu, 4 Jul 2024 12:19:32 -0400
Subject: [PATCH] Use child_process.execFile to avoid unintended shell
 interpolation

---
 src/api/routes/service/service.service.ts |  9 ++++++---
 src/api/services/audio.ts                 | 11 +++++++----
 src/api/services/shaka.ts                 |  2 +-
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/api/routes/service/service.service.ts b/src/api/routes/service/service.service.ts
index 5c3aa39..b71c99b 100644
--- a/src/api/routes/service/service.service.ts
+++ b/src/api/routes/service/service.service.ts
@@ -23,7 +23,7 @@ import settings from 'electron-settings'
 import { server } from '../../api'
 import { createChapterFile } from '../../services/chapter'
 import { app } from 'electron'
-const exec = util.promisify(require('child_process').exec)
+const execFile = util.promisify(require('child_process').execFile)

 settings.configure({
     dir: app.getPath('documents') + '/Crunchyroll Downloader/settings/'
@@ -1353,9 +1353,12 @@ async function mergeParts(parts: { filename: string; url: string }[], downloadID
             await updatePlaylistByID(downloadID, 'decrypting video')
             console.log('Video Decryption started')

-            const command = `${shaka} input="${tmp}/temp-main.m4s",stream=video,output="${tmp}/main.m4s" --enable_raw_key_decryption --keys key_id=${drmkeys[1].kid}:key=${drmkeys[1].key}`
+            await execFile(shaka, [
+                `input=${tmp}/temp-main.m4s,stream=video,output=${tmp}/main.m4s`,
+                '--enable_raw_key_decryption',
+                '--keys', `key_id=${drmkeys[1].kid}:key=${drmkeys[1].key}`
+            ])

-            await exec(command)
             console.log('Video Decryption finished')
             concatenatedFile = `${tmp}/main.m4s`
         }
diff --git a/src/api/services/audio.ts b/src/api/services/audio.ts
index dcbd549..9a5b640 100644
--- a/src/api/services/audio.ts
+++ b/src/api/services/audio.ts
@@ -9,7 +9,7 @@ const ffmpegP = getFFMPEGPath()
 const shaka = getShakaPath()
 import util from 'util'
 import { server } from '../api'
-const exec = util.promisify(require('child_process').exec)
+const execFile = util.promisify(require('child_process').execFile)
 import { finished } from 'stream/promises'
 import { messageBox } from '../../electron/background'

@@ -160,12 +160,15 @@ async function mergePartsAudio(
             if (dn) {
                 dn.status = 'decrypting'
             }
-            
+
             console.log(`Audio Decryption started`)

-            const command = `${shaka} input="${tmp}/temp-main.m4s",stream=audio,output="${tmp}/main.m4s" --enable_raw_key_decryption --keys key_id=${drmkeys[1].kid}:key=${drmkeys[1].key}`
+            await execFile(shaka, [
+                `input=${tmp}/temp-main.m4s,stream=audio,output=${tmp}/main.m4s`,
+                '--enable_raw_key_decryption',
+                '--keys', `key_id=${drmkeys[1].kid}:key=${drmkeys[1].key}`
+            ])

-            await exec(command)
             concatenatedFile = `${tmp}/main.m4s`
             console.log(`Audio Decryption finished`)
         }
diff --git a/src/api/services/shaka.ts b/src/api/services/shaka.ts
index 13c0227..483c8d2 100644
--- a/src/api/services/shaka.ts
+++ b/src/api/services/shaka.ts
@@ -14,7 +14,7 @@ export function getShakaPath() {

         return shaka
     } else {
-        const shaka = `"${path.join(shakaFolderPath, 'shaka.exe')}"`
+        const shaka = path.join(shakaFolderPath, 'shaka.exe')

         return shaka
     }
-- 
2.45.2

[stratuma]

Thank you for the bugreport. I think this issue appears due to a electron bug. The function app.getAppPath() does somehow not return the actual app path. I will investigate the error and create a fix as soon as I have time.

[stratuma]

This behaviour somehow only seems to be on linux.

[DarwinAwardWinner]

I've further edited and tested the above patch, and I confirm it works for me.

(For reasons I don't understand, when I build the app myself, it requires the shaka and ffmpeg binary file names to end with ".exe". I'm unsure if this is related to this bug and/or my fix.)

[stratuma]

Yeah because you forgot to remove the .exe from the fix you wrote: const shaka = path.join(shakaFolderPath, 'shaka.exe') to const shaka = path.join(shakaFolderPath, 'shaka')

[stratuma]

you also have to go into the ffmpeg.ts and remove the .exe endings