~cadence/tube-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH cloudtube v2] Proxy captions via new /proxy route

~lomanic
Details
Message ID
<161946728082.11208.14185327030477598006-0@git.sr.ht>
DKIM signature
missing
Download raw message
Patch: +29 -25
From: Lomanic <lomanic@hotmail.fr>

We can add more authorized paths to authorizedPaths if we need
more resources to be pulled from the NewLeaf/Invidious backend
on the same domain
---
 api/captions.js | 25 -------------------------
 api/proxy.js    | 24 ++++++++++++++++++++++++
 api/video.js    |  5 +++++
 3 files changed, 29 insertions(+), 25 deletions(-)
 delete mode 100644 api/captions.js
 create mode 100644 api/proxy.js

diff --git a/api/captions.js b/api/captions.js
deleted file mode 100644
index 29611ef..0000000
--- a/api/captions.js
@@ -1,25 +0,0 @@
const fetch = require("node-fetch")
const {getUser} = require("../utils/getuser")
const constants = require("../utils/constants.js")

module.exports = [
	{
		route: `/api/v1/captions/(${constants.regex.video_id})`, methods: ["GET"], code: async ({req, fill, url}) => {
			const instanceOrigin = getUser(req).getSettingsOrDefaults().instance
			const fetchURL = new URL(`${url.pathname}${url.search}`, instanceOrigin)
			return fetch(fetchURL.toString()).then(res => {
				return res.text().then(text => {
					if (res.status === 200) {
						// Remove the position annotations that youtube unhelpfully provides
						text = text.replace(/(--> \S+).*/g, "$1")
					}
					return {
						statusCode: res.status,
						contentType: res.headers.get("content-type"),
						content: text
					}
				})
			})
		}
	}
]
diff --git a/api/proxy.js b/api/proxy.js
new file mode 100644
index 0000000..6b3af8c
--- /dev/null
+++ b/api/proxy.js
@@ -0,0 +1,24 @@
const {proxy} = require("pinski/plugins")
const {getUser} = require("../utils/getuser")
const constants = require("../utils/constants.js")

// list of paths relative to the backend this route is authorized to serve
const authorizedPaths = [`/api/v1/captions/(${constants.regex.video_id})`]

module.exports = [
	{
		route: `/proxy`, methods: ["GET"], code: async ({req, fill, url}) => {
			const instanceOrigin = getUser(req).getSettingsOrDefaults().instance
			const remotePath = url.searchParams.get("url")
			const fetchURL = new URL(remotePath, instanceOrigin)
			if (!authorizedPaths.some(element => fetchURL.pathname.match(new RegExp(`^${element}$`)))) {
				return {
					statusCode: 401,
					content: "CloudTube: Unauthorized",
					contentType: "text/plain"
				}
			}
			return proxy(fetchURL)
		}
	}
]
diff --git a/api/video.js b/api/video.js
index ad82499..c00f731 100644
--- a/api/video.js
+++ b/api/video.js
@@ -123,6 +123,11 @@ async function renderVideo(video, {user, settings, id, instanceOrigin}, locals =
		// rewrite description
		video.descriptionHtml = converters.rewriteVideoDescription(video.descriptionHtml, id)

		// rewrite captions urls so they are served on the same domain via the /proxy route
		for (const caption of video.captions) {
			caption.url = `/proxy?${new URLSearchParams({"url": caption.url})}`
		}

		return render(200, "pug/video.pug", Object.assign(locals, {video, formats, subscribed, instanceOrigin}))
	} catch (e) {
		// show an appropriate error message
-- 
2.30.2
Details
Message ID
<20210502005431.1fae107f4007cb3d5f4e6978@disroot.org>
In-Reply-To
<161946728082.11208.14185327030477598006-0@git.sr.ht> (view parent)
DKIM signature
pass
Download raw message
Okay, I finally got to review this properly. It looks good.

There is one comment from the previous revision that you did not
address:

> Note that if this were to be used for video streams, you also need
> to forward some headers like `Range` / `Accept-Ranges` /
> `Content-Range`.

If the /proxy route shouldn't be used for video stream proxying, then
I'm still ok to apply this patch. I just need to know that you've made
that decision.

If this is the decision you've made, I'll add a code comment when I
apply the patch, just to make sure other people don't get tripped up
in the future.

Otherwise, if you want it to work for video stream proxying right now,
you should check those headers.

Just let me know which choice you've made.
Details
Message ID
<VI1PR0701MB236676E8799B5B99E86FE86FD15D9@VI1PR0701MB2366.eurprd07.prod.outlook.com>
In-Reply-To
<20210502005431.1fae107f4007cb3d5f4e6978@disroot.org> (view parent)
DKIM signature
missing
Download raw message
I just forgot this remark indeed.

As-is it's only made to relay requests to the
NewLeaf/Invidious backend on the same
domain, the question to ask is more about if 
videos streams will go through both the 
NL/IV backend and the Cloudtube frontend or 
not. I wouldn't like to mix both proxies, to YT 
and to the backend, on the same route. 

I had the mindset to have only NewLeaf directly
talk to YouTube, and the frontend only care about presenting the data from the NL/IV API
(to be agnostic between backends), so looks
like /proxy will relay streams via the backend,
but it's also dependent on how Invidious implements proxying.

On May 1, 2021 2:54:31 PM GMT+02:00, Cadence Ember <cadence@disroot.org> wrote:
>Okay, I finally got to review this properly. It looks good.
>
>There is one comment from the previous revision that you did not
>address:
>
>> Note that if this were to be used for video streams, you also need
>> to forward some headers like `Range` / `Accept-Ranges` /
>> `Content-Range`.
>
>If the /proxy route shouldn't be used for video stream proxying, then
>I'm still ok to apply this patch. I just need to know that you've made
>that decision.
>
>If this is the decision you've made, I'll add a code comment when I
>apply the patch, just to make sure other people don't get tripped up
>in the future.
>
>Otherwise, if you want it to work for video stream proxying right now,
>you should check those headers.
>
>Just let me know which choice you've made.
Details
Message ID
<e7abfdc0-63c9-4f14-92a8-4799a6bd6696@disroot.org>
In-Reply-To
<VI1PR0701MB236676E8799B5B99E86FE86FD15D9@VI1PR0701MB2366.eurprd07.prod.outlook.com> (view parent)
DKIM signature
pass
Download raw message
Semi-related: If we wanted to use videojs for dash playback (I _don't_, 
but, just hypothetically) then do we have to serve the streams from the 
same domain as the frontend, or can they be proxied through just the 
selected instance?
Reply to thread Export thread (mbox)