Skip to content

Add support for SABR (v2) - #5814

Draft
unixfox wants to merge 15 commits into
iv-org:masterfrom
unixfox:sabr-shakajs-2
Draft

Add support for SABR (v2)#5814
unixfox wants to merge 15 commits into
iv-org:masterfrom
unixfox:sabr-shakajs-2

Conversation

@unixfox

@unixfox unixfox commented Jul 20, 2026

Copy link
Copy Markdown
Member

Checklist

  • I have read the AI Policy and understand the disclosure requirements

AI Disclosure

  • AI was not used to create this pull request
  • AI was used to fully create this pull request
  • AI was used to partially create this pull request

Model(s) used (and thinking/reasoning level if relevant):
Claude Opus 4.8

Tool(s) used:
Claude Code

How was AI used?

Was used to replicate the code from https://github.com/LuanRT/kira and freetube's SABR code.


Pull request description

This pull request is for adding SABR support to invidious.

Close #5263

TODO

  • Add support for https://github.com/LuanRT/ytc-bridge
  • Remove the dependency on Invidious companion (reuse work from trying to only use next endpoint for the major data #5003)
  • Make loading the video player faster
  • Fix glitch with loading icon in Shakajs
  • Wire potoken generation to Invidious companion since you can't generate a valid potoken directly in the browser anymore.
  • Avoid using esm.sh for fetch the dependencies.
  • Support extensions like SponsorBlock and DeArrow

Worth knowing

  • Potoken generation needs to be done in an external program because YouTube requires the potoken generation to be done as if you were connected to https://www.youtube.com. But this is impossible to do inside a browser.

Notes

  • Change This is not used for loading the video streams from YouTube servers (circumvent YouTube restrictions) in config.yml because wouldn't be true anymore since proxy is in invidious.

Summary by CodeRabbit

  • New Features
    • Added experimental SABR playback with adaptive streaming, captions, chapters, storyboards, audio-only mode, and playback-position persistence.
    • Added selectable SABR codecs: VP9, AV1, and H.264.
    • Added support for HTTP, SOCKS5, and SOCKS5H outbound proxies.
  • Bug Fixes
    • Improved handling of incomplete or unavailable video metadata and clearer unplayable-video messages.
    • Prevented fallback playback for premieres and videos with playback errors.
  • Style
    • Added responsive, themed player controls, loading indicators, captions, tooltips, and accessibility states.

@unixfox unixfox mentioned this pull request Jul 20, 2026
6 tasks
mdbraber added a commit to mdbraber/owntube that referenced this pull request Jul 30, 2026
docs/OWNTUBE-UPSTREAM-PLAN.md proposes replacing Invidious with a service we own,
built on youtubei.js. Two pressures point the same way: the fork is now 8 patches
rebased forever, and upstream is moving to SABR client-side only
(iv-org/invidious#5814), which is a viable answer for a web player but not
obviously one for a client set that includes expo-video on Android TV.

The plan records what was measured rather than assumed — youtubei.js' coverage
(FreeTube's 2,270-line local.js, shipped to 21.5k users), how much of Invidious we
actually use (8 endpoint families; ~40% of it is UI and accounts), and that
server-side SABR is feasible because googlevideo exports SabrStream for headless
use. It also records four corrections to my own earlier reasoning, including that
Android TV does *not* structurally force a server-side connector — Materialious
ships SABR on Android TV through a Capacitor WebView, so that constraint is our
choice of expo-video, not the platform.

The detector is the part worth having today. `sabrExposure` measures the share of
*non-live* videos that no longer carry init/index byte ranges — the thing that
would silently break /dash and /hls. Currently 0/12. Live streams are excluded
because they are legitimately segment-addressed; that exclusion leans on liveNow
being correct, which Phase 3.2 fixed and listLiveFlag guards.

It samples `popular` rather than trending, which was the first attempt and
SKIPped: trending is the livestreams feed now, so it yields no non-live sample at
all.

Canary is 8 PASS + 1 SKIP.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@TheFrenchGhosty
TheFrenchGhosty requested a review from Copilot August 2, 2026 08:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds frontend SABR (v2) playback support (Shaka + SABR scheme/manifest) and adjusts backend parsing/error-handling so watch/embed pages can still render using /next-only metadata when the /player endpoint is unavailable (e.g., no Invidious Companion).

Changes:

  • Add a SABR playback mode, including a new /proxy route and a new SABR player path in the watch/embed templates.
  • Extend user/video preferences to support quality=sabr and a SABR codec preference.
  • Make video parsing more resilient by falling back to /next data and surfacing improved unplayable error info.

Reviewed changes

Copilot reviewed 39 out of 46 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/invidious/yt_backend/youtube_api.cr Clarifies player returning nil without Companion to enable /next fallback.
src/invidious/yt_backend/extractors_utils.cr Adds helper to infer unlisted status from badges when videoDetails is missing.
src/invidious/views/watch.ecr Hides player when video.reason is present; surfaces reason/subreason UI.
src/invidious/views/user/preferences.ecr Adds SABR quality option and codec preference UI.
src/invidious/views/embed.ecr Shows unplayable reason/subreason instead of player when unavailable.
src/invidious/views/components/player.ecr Switches between video.js and SABR player, and loads SABR assets when enabled.
src/invidious/videos/video_preferences.cr Adds quality_sabr handling to processed per-video params.
src/invidious/videos/parser.cr Adds /next fallback parsing and enriches error handling (reason/subreason).
src/invidious/videos.cr Bumps schema version and adds subreason accessor + safer reason accessor.
src/invidious/user/preferences.cr Adds persisted preference for quality_sabr.
src/invidious/routing.cr Registers new proxy routes.
src/invidious/routes/watch.cr Avoids redirects/raw playback when the video is in an error state.
src/invidious/routes/proxy.cr Introduces SABR-oriented proxy endpoint for browser SABR traffic.
src/invidious/routes/preferences.cr Persists quality_sabr from preferences form submissions.
src/invidious/routes/before_all.cr Adjusts CSP to allow SABR/BotGuard dependencies and network calls.
src/invidious/config.cr Adds default config preference for quality_sabr.
spec/invidious/videos/regular_videos_extract_spec.cr Adds coverage for parsing without videoDetails and for subreason null behavior.
scripts/fetch-sabr-dependencies.cr Fetches SABR-related third-party browser artifacts into assets.
scripts/fetch-player-dependencies.cr Hooks SABR dependency fetch into existing player dependency fetch flow.
scripts/bundle-sabr-libs.js Bundles googlevideo/bgutils via esbuild for browser use.
package.json Adds Node dependencies and bundling script for SABR libraries.
package-lock.json Locks Node dependencies for bundling SABR libraries.
locales/en-US.json Adds UI strings for SABR preferences and unplayable error label.
assets/js/sabr/youtubei.js/versions.yml Records youtubei.js asset version.
assets/js/sabr/shaka-player/versions.yml Records shaka-player asset version.
assets/js/sabr/shaka-player/controls.css Adds Shaka UI stylesheet asset (with font adjustments).
assets/js/sabr/googlevideo/versions.yml Records googlevideo asset version.
assets/js/sabr/bgutils-js/versions.yml Records bgutils-js asset version.
assets/js/sabr/bgutils-js/bgutils.bundle.min.js Adds bundled bgutils-js browser artifact.
assets/js/sabr_ebml_parser.js Adds EBML parsing helper for SABR segment index parsing.
assets/js/sabr_mp4_index.js Adds MP4 segment index parser for SABR manifests.
assets/js/sabr_webm_index.js Adds WebM segment index parser for SABR manifests.
assets/js/sabr_manifest_parser.js Registers Shaka manifest parser for application/sabr+json.
assets/js/sabr_scheme_plugin.js Registers Shaka networking scheme sabr: and routes traffic through /proxy.
assets/js/sabr_helpers.js Adds shared helper utilities (proxy URL building, Onesie crypto helpers, caching).
assets/js/sabr_potoken.js Adds BotGuard/PoToken generation logic for SABR playback.
assets/js/sabr_onesie.js Adds Onesie player-response fetching for SABR URL portability.
assets/js/sabr_player.js Implements the SABR player orchestrator using Shaka + SABR scheme/manifest.
assets/js/sabr_loader.js Loads ES module SABR dependencies and exposes them on window.
assets/js/sabr_init.js Boots SABR player on page load once SABR libs are loaded.
assets/css/sabr_player.css Adds styling to align Shaka UI with existing player styling.
.gitignore Ignores node_modules/.
Files not reviewed (1)
  • assets/js/sabr/shaka-player/controls.css: Generated file
Suppressed comments (4)

src/invidious/routes/proxy.cr:213

  • The proxied response also reflects Origin and sets Access-Control-Allow-Credentials=true. Even if preflight is tightened, this still enables credentialed cross-origin reads for any allowed upstream host. If CORS is needed at all here, avoid Allow-Credentials and only set ACAO when an Origin is present (and add Vary: Origin).
    src/invidious/routes/proxy.cr:95
  • Custom headers from the __headers query param are currently accepted verbatim, but ALLOWED_HEADERS is not enforced. This allows clients to smuggle arbitrary headers upstream (e.g., Cookie, Host, X-Forwarded-*, etc.), which is a security risk even with an allowlisted host set. Filter __headers to the explicit allowlist (case-insensitive) before copying into request_headers.
    src/invidious/routes/proxy.cr:100
  • This route uses puts for warning output. Invidious routes elsewhere use LOGGER, which integrates with the app’s log level/formatting. Please use LOGGER.warn here so proxy warnings are captured consistently.
    src/invidious/routes/proxy.cr:193
  • For POST requests, the proxy reads the entire request body into memory via getb_to_end before forwarding. A malformed or unexpectedly large body can cause high memory usage per request. Consider streaming the request body to the upstream client (or enforcing a strict size limit / Content-Length cap) instead of buffering everything.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/invidious/routes/before_all.cr Outdated
Comment on lines +51 to +55
"script-src 'self' 'unsafe-eval' 'unsafe-inline'",
"style-src 'self' 'unsafe-inline'",
"img-src 'self' data:",
"font-src 'self' data:",
"connect-src 'self' " + COMPANION_CSP.companion_urls,
"connect-src 'self' https://*.googleapis.com https://*.youtube.com " + COMPANION_CSP.companion_urls,
Comment on lines +85 to +89
<script>
document.getElementById('quality').addEventListener('change', function() {
var sabrGroup = document.getElementById('sabr-codec-group');
if (this.value === 'sabr') {
sabrGroup.style.display = '';
Comment on lines +49 to +61
# OPTIONS /proxy
def self.options(env)
origin = env.request.headers["Origin"]? || "*"

env.response.headers["Access-Control-Allow-Origin"] = origin
env.response.headers["Access-Control-Allow-Methods"] = "GET, POST, OPTIONS"
env.response.headers["Access-Control-Allow-Headers"] = ALLOWED_HEADERS.join(", ")
env.response.headers["Access-Control-Max-Age"] = "86400"
env.response.headers["Access-Control-Allow-Credentials"] = "true"

env.response.status_code = 200
""
end
Comment thread package.json
Comment on lines +8 to +12
"dependencies": {
"googlevideo": "^4.0.4",
"youtubei.js": "^17.0.1",
"bgutils-js": "^3.2.0"
},
Copilot AI review requested due to automatic review settings August 2, 2026 08:37
@unixfox
unixfox marked this pull request as draft August 2, 2026 08:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 39 out of 46 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • assets/js/sabr/shaka-player/controls.css: Generated file
Suppressed comments (8)

src/invidious/routes/before_all.cr:53

  • The global CSP is being weakened for every response by adding both 'unsafe-eval' and 'unsafe-inline' to script-src. This substantially increases the blast radius of any XSS (inline script execution + eval) and also affects pages that never load the SABR player. Consider scoping these relaxations to SABR pages only (e.g., watch/embed when quality=sabr), or using nonces/hashes + avoiding eval where possible.
    package.json:12
  • package.json dependency versions don't match package-lock.json (e.g. package.json requests googlevideo ^4.1.1 / youtubei.js ^17.2.0 / bgutils-js ^4.0.2, but the lockfile pins 4.0.4 / 17.0.1 / 3.2.0). This will produce inconsistent installs depending on whether npm honors the lockfile, and can break bundling/reproducibility. Align the versions (or regenerate the lockfile).
  "dependencies": {
    "googlevideo": "^4.1.1",
    "youtubei.js": "^17.2.0",
    "bgutils-js": "^4.0.2"
  },

src/invidious/routes/proxy.cr:220

  • The proxy response sets Access-Control-Allow-Credentials: true. Removing this avoids credentialed cross-origin reads, and also avoids the invalid "" + credentials combination when Origin is missing (since this route currently falls back to "").
    src/invidious/routes/proxy.cr:59
  • CORS is currently reflecting the request Origin (or using "") while also advertising Access-Control-Allow-Credentials: true. Using "" with credentials is invalid per the CORS spec, and reflecting arbitrary origins with credentials can allow other sites to read proxied responses. Since /proxy is same-origin for the SABR player, it should not need credentialed CORS at all.
    src/invidious/routes/proxy.cr:104
  • This route logs parse failures with puts, which bypasses the app logger and can spam stdout in production. Use LOGGER.warn (or similar) for consistency with the rest of the routes.
    assets/js/sabr_player.js:42
  • BG_HELPER_URL is hardcoded to http://127.0.0.1:4416/generate, which is the end-user's localhost (browser-side), not the Invidious server. For remote instances this will always fail, and allowing/encouraging browser JS to talk to localhost can have security implications (probing local services). Consider making this a same-origin endpoint (e.g. /bg-helper/generate) and proxying server-side, or gating this feature behind an explicit opt-in.
  // bg-helper-server /generate endpoint. Mints an ATTESTED (StreamProtectionStatus=1)
  // PO token server-side, where jsdom presents a youtube.com origin. In-browser BotGuard
  // (Invidious origin) can only get status 2 (pending), so it's used only as a fallback.
  var BG_HELPER_URL = 'http://127.0.0.1:4416/generate';

assets/js/sabr_scheme_plugin.js:308

  • The SABR_REDIRECT handler assigns to currentState.sabrUrl, but the request loop uses currentState.sabrStreamState.sabrUrl. As written, the assignment has no effect and is misleading (and also contradicts the header comment claiming the redirect bug is fixed). If redirects are intentionally ignored for compatibility, remove this write entirely (or update the comments accordingly).
              // (sabrStreamState.sabrUrl). Earlier we "fixed" this to follow the redirect,
              // which pins the session to a specific CDN host (rrN---snXXX) that maintains a
              // sequential cursor and returns 0 media when the client jumps/seeks. Following
              // the redirect breaks seeking; ignoring it (as FreeTube does) keeps seeks working.
              currentState.sabrUrl = sabrRedirect.url;

scripts/fetch-sabr-dependencies.cr:5

  • require "digest/sha1" is unused in this script. Keeping unused requires increases load time and can trip linting/static checks. Remove it if not needed.
require "http"
require "yaml"
require "digest/sha1"
require "option_parser"
require "colorize"

Comment on lines +95 to +99
name = header_array[0]?.try &.as_s
value = header_array[1]?.try &.as_s
if name && value
custom_headers[name] = value
end
unixfox and others added 15 commits August 3, 2026 02:50
… 5.1.10), companion-optional watch page, no esm.sh
Segment index parsers (mp4/webm) declared the per-segment URI array with
`var` inside the loop, so every SegmentReference's getUris() closure
captured the same function-scoped binding and returned the LAST segment's
URL. YouTube received the final segment's startTimeMs/sq for every
segment, replied with policy-only UMP (no media), and the player looped
forever on a black screen ("SABR throttled by YouTube"). Use `let`
(block-scoped, fresh per iteration) to match FreeTube's `const`.

Also scope the big centered play button styling to
`.shaka-play-button-container`; the bare `.shaka-play-button` selector
also matched the control-bar play button (48px siblings), oversizing it
to ~54px and pushing it out of alignment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the plain /youtubei/v1/player request (made through the proxy,
and therefore bound to the proxy's egress IP) with an encrypted Onesie
request using the WEB client. Onesie is proxied by YouTube's "trusted
bandaid", so the returned player response - and the server_abr_streaming_url
inside it - is not tied to our egress IP. Media is still pulled over SABR
(sabr_scheme_plugin.js); only how the player response is obtained changes.

- sabr_onesie.js: new module, window.fetchOnesiePlayerResponse(). Builds the
  WEB player request, encrypts it (OnesieInnertubeRequest + OnesieRequest),
  POSTs to the onesie endpoint through /proxy, parses ONESIE_HEADER/ONESIE_DATA
  UMP parts, gunzips/decrypts, returns the raw player response JSON. Adapted
  from googlevideo/examples/onesie-request and invidious-secret-companion's
  WEB-client variant.
- sabr_helpers.js: add decryptResponse() (AES-CTR + HMAC verify), companion to
  the existing encryptRequest().
- sabr_loader.js: expose window.YT so a VideoInfo can be built from the raw
  onesie player response.
- sabr_player.js: in loadVideo(), fetch the player response via Onesie and wrap
  it in new YT.VideoInfo(...); fall back to innertube.getInfo() on failure.
- player.ecr: load sabr_onesie.js before sabr_player.js.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@TheFrenchGhosty

This comment was marked as outdated.

@coderabbitai

This comment was marked as outdated.

@coderabbitai

This comment was marked as outdated.

@TheFrenchGhosty

Copy link
Copy Markdown
Member

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 54

🤖 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 `@assets/css/sabr_player.css`:
- Around line 243-250: Update the mobile .shaka-controls-button-panel button
rule in the responsive styles to prevent padding from expanding Shaka’s declared
48px controls; remove the added padding rule, or enforce border-box sizing while
preserving the existing control dimensions.

In `@assets/js/sabr_helpers.js`:
- Around line 346-355: Update assets/js/sabr_helpers.js lines 346-355 in
isGoogleVideoURL to remove substring matching and accept only exact apex hosts
or .googlevideo.com/.youtube.com suffixes. Update assets/js/sabr_player.js lines
385-392 by removing the local isGoogleVideo and isYouTube checks and using
SABRHelpers.isGoogleVideoURL instead.
- Around line 328-338: Update the request preparation flow around the `headers`
serialization and `fetch` call: choose the intended behavior for `user-agent`
and make it consistent. To hide it from YouTube, move
`headers.delete('user-agent')` before the `__headers` value is created;
otherwise remove that deletion. Keep the serialized headers and outgoing request
behavior aligned.
- Around line 6-25: Wrap the entire contents of sabr_helpers.js, including the
top-level configuration constants and helper functions such as getProxyConfig,
encryptRequest, fetchWithProxy, and generateRandomString, in an IIFE. Keep the
existing window.SABRHelpers assignment as the sole public export, and close the
IIFE immediately after that assignment so these declarations no longer pollute
the global namespace or collide with sabr_potoken.js.
- Around line 286-339: Consolidate the duplicated proxied-URL construction by
making fetchWithProxy reuse proxyUrl for the URL transformation, preserving its
existing header and host parameters and request behavior. Also update the
corresponding construction in sabr_potoken.js to use the shared implementation
where compatible, while retaining its distinct __path contract; ensure proxy
host, port, protocol, and /proxy pathname logic has one source of truth.

In `@assets/js/sabr_init.js`:
- Around line 32-38: Update the error-display construction in
SABRPlayer.loadVideo’s error handler to avoid inserting dynamic error.message
through innerHTML. Create a separate message element, assign error.message via
textContent, and append it alongside the existing static error content and DASH
fallback link.

In `@assets/js/sabr_loader.js`:
- Around line 26-32: Update the SABR loading flow in the loader and its consumer
so static import failures are captured and exposed instead of leaving
initialization waiting indefinitely. Ensure sabr-libs-loaded is dispatched with
failure state or an equivalent error signal, and update sabr_init.js to detect
that failure and render the DASH fallback while preserving the
successful-loading path.

In `@assets/js/sabr_manifest_parser.js`:
- Around line 252-304: The stream-building loops capture function-scoped var
bindings, causing all segment-index closures to use the final iteration's
values. In assets/js/sabr_manifest_parser.js:252-304, change caption and stream
in createTextStreams to let; in assets/js/sabr_manifest_parser.js:308-321,
change storyboard and stream in the storyboard builder to let so each
createSegmentIndex and closeSegmentIndex closure retains its own iteration
values.
- Around line 308-321: In the storyboard loop, change the outer `storyboard` and
`stream` declarations to `let` so the `createSegmentIndex` and
`closeSegmentIndex` closures retain each iteration’s values. Leave the existing
`urls` declaration and surrounding stream construction unchanged.
- Around line 492-498: Remove the unconditional DRC exclusion in the
format-processing flow so Stable Volume audio remains available and
hasDrcAudio/createAudioStream labeling can function; if the workaround is still
required, replace the experimental continue with a clearly named, documented
configuration gate rather than dropping all DRC formats.
- Around line 154-155: Update SabrManifestParser.prototype.start and
createVideoStream to safely handle a null result from
format.mimeType.match(CODECS_REGEX) before accessing the capture group. Skip
formats whose MIME type lacks a codecs parameter, preventing malformed entries
from aborting manifest parsing or playback.
- Around line 36-38: Update the buffer handling before the initData and
indexData slices to preserve response.data’s byteOffset and byteLength when it
is an ArrayBuffer view. Ensure format.initRange and format.indexRange remain
relative to the returned segment while retaining direct handling for full
ArrayBuffers.
- Around line 506-511: Update the comparator in the videoStreams sort block to
map each findIndex result of -1 to a rank after VIDEO_CODEC_PRIORITIES.length,
ensuring unrecognised codecs sort below all known codecs while preserving the
existing priority order.

In `@assets/js/sabr_onesie.js`:
- Around line 157-162: Update the Onesie request in the relevant fetch flow to
use an AbortSignal timeout and check resp.ok before reading resp.arrayBuffer().
On non-success responses, fail immediately with the existing error-handling path
so loadVideo can fall back to getInfo instead of parsing the response body.

In `@assets/js/sabr_player.js`:
- Around line 144-155: Normalize the redirector URL at both storage boundaries:
in assets/js/sabr_player.js lines 144-155, require redirectorResponse.ok before
storing and trim redirectorResponse.text() before localStorage.setItem; in
assets/js/sabr_onesie.js lines 37-52, trim the value returned by
localStorage.getItem before using it as the base URL.
- Around line 83-100: Update savePlaybackPosition and all write call sites in
loadVideo to honor options.savePlayerPos before persisting playback data. Add
bounded retention to the youtube_playback_positions map by removing the oldest
entries once the configured maximum is exceeded, while preserving reads and
avoiding unbounded localStorage growth.
- Around line 630-657: Replace the user-facing SABR backoff and failure strings
in sabrStream.onBackoffRequested and sabrStream.onReloadOnce with localized
messages from the existing locale infrastructure and keys added to
locales/en-US.json, including the retry duration interpolation. Verify the
recovery link uses the correct query parameter for switching away from SABR; if
?quality=dash is not the established switch, update it to the appropriate
preference parameter while preserving the existing failure flow.
- Around line 38-41: Update BG_HELPER_URL and the fetchHelperPoToken path so
browser requests do not target the visitor’s loopback interface. Route
generation through a backend-proxied or instance-configurable endpoint, or
default the helper URL to disabled and skip this path until server-side wiring
exists; preserve the in-browser BotGuard fallback when the helper is
unavailable.
- Around line 117-124: Update Platform.shim.eval to stop interpolating env.n and
env.sig into the generated JavaScript source. Build the generated function with
parameter names for the required values, then pass env.n and env.sig as
arguments when invoking it, preserving the existing exportedVars.nFunction and
sigFunction behavior while preventing input from altering executable code.
- Around line 238-255: Update fetchHelperPoToken to enforce a finite timeout on
the BG_HELPER_URL request, ensuring timeout failures follow the existing
catch-and-return-null path so loadVideo cannot remain pending. Also update the
PO-token logging near mintContentWebPO to avoid exposing token contents, logging
only whether a token exists or removing the logs entirely.
- Around line 866-880: Update dispose() to destroy the Shaka UI overlay before
destroying the player: await ui.destroy() while player remains available, then
clear ui and destroy player only after no UI overlay exists. Preserve the
existing cleanup and state-reset behavior.

In `@assets/js/sabr_potoken.js`:
- Around line 159-173: Scope any CSP relaxation required by the BotGuard script
injection in the SABR watch-page path only, verifying the policy in
before_all.rb or relevant view templates does not affect other routes. Update
the SABR configuration description to document that YouTube-supplied BotGuard
JavaScript executes in the Invidious origin and has access to origin resources;
do not broaden the policy beyond this page.
- Around line 190-201: Guard the diagnostic logging in the GenerateIT response
handling so JSON.stringify(...map(...)) runs only when
integrityTokenResponseData is an array. Preserve the non-array token path and
ensure malformed or scalar responses reach the existing !integrityToken fallback
that returns botguardClient instead of throwing.
- Around line 39-43: Remove the unused useTrustedEnv parameter from buildURL and
update all callers, including the call sites near lines 133 and 180, to invoke
it with only the action argument. Preserve the existing YouTube base URL
behavior.
- Around line 52-83: Update the query-copy loop in fetchWithProxy to skip the
reserved parameters __host, __path, and __headers before setting values on
proxyUrl. Preserve copying all other source query parameters and the existing
explicitly constructed proxy parameters.
- Around line 89-114: Update init and/or setup so a previously initialized
botguardClient is returned immediately before starting _initBotguard again.
Preserve the existing initializationPromise sharing for concurrent calls, while
ensuring later init calls reuse the stored client and do not re-run
initialization.

In `@assets/js/sabr_scheme_plugin.js`:
- Around line 256-270: Update the SABR fetch handling around proxyFetch and
before response.body.getReader() to validate the response status and body. Route
non-OK responses, including status-only responses without a body, through the
existing BAD_HTTP_STATUS path so retry logic retains the HTTP status instead of
producing a generic HTTP_ERROR; only create the reader after confirming the
response is usable.
- Around line 209-228: Resolve the temporary seek diagnostics consistently
across the surrounding request and UMP processing code: either remove the
`window.SABR_DEBUG` branches and all associated `_dbg` logging, or formally
retain them by documenting `window.SABR_DEBUG` as a supported flag and renaming
the “TEMP SEEK DIAGNOSTICS” marker to non-temporary wording. Apply the chosen
approach to every diagnostic block, including those near the request setup,
parsing switch, and later processing sections.
- Around line 65-78: Guard the activeManifestVariant lookup in the SABR scheme
handler so unmatched manifest variants and audio-only variants do not cause a
dereference failure. Update both find predicates to tolerate missing
variant.video, then handle an undefined activeManifestVariant before accessing
its audio.segmentIndex by using the existing segment-request fallback/error path
and preventing the exception from escaping the sabr handler.
- Around line 641-649: Guard the audio-format selection in the SABR scheme
handler before dereferencing candidates[0], and handle an empty variantTracks
list at the related selection near line 635. Preserve the existing
preferred-track behavior when tracks exist, but avoid calling formatIdFromString
or accessing originalAudioId unless a valid probableAudioFormat was found; use
the handler’s existing fallback behavior for empty or roleless track lists.
- Around line 298-311: Update the file header to describe that SABR_REDIRECT
URLs are intentionally ignored to preserve seeking, rather than claiming the
redirect bug is fixed. In the SABR_REDIRECT case within the redirect decoder,
remove the assignment to the unused currentState.sabrUrl field and explicitly
discard sabrRedirect.url while retaining shouldRetry behavior.
- Around line 232-254: Update the backoff wait in the request flow around
currentState.abortController to handle an already-aborted signal immediately,
register the abort listener with one-shot cleanup, and clear the pending timeout
when abort wins; preserve normal timeout resolution. After setting
playerReloadRequested and aborting in the shouldReloadDueToBackoffLoop branch,
return before the fetch path so reload does not fall through to the request.
- Around line 485-542: Bound the SABR_REDIRECT retry path by adding a dedicated
counter and maximum retry limit, incrementing it before the recursive retry in
the branch controlled by shouldRetry. When the limit is exceeded, terminate
through the same ShakaError escape path used for exhausted attestation retries;
preserve existing handling for invalidPoToken, next-request policy retries, and
other retry conditions.

In `@config/config.example.yml`:
- Around line 250-265: Update the example outbound proxy block to include the
required host and port fields alongside type, using valid placeholder values so
uncommenting it deserializes successfully. In the same configuration comment
block, replace the stale statement claiming the proxy is not used for YouTube
video streams with wording that accurately reflects its use for the supported
googlevideo/SABR egress path.

In `@scripts/fetch-player-dependencies.cr`:
- Around line 166-168: Synchronize SABR dependency versions using
package-lock.json as the authoritative source: update package.json,
fetch-sabr-dependencies.cr, and both SABR versions.yml files to the resolved
versions. In scripts/fetch-player-dependencies.cr, perform a reproducible
dependency install and invoke the bundle-sabr step after fetching SABR
dependencies. Update scripts/bundle-sabr-libs.js as needed to consume the
lockfile versions, then rebuild and commit the tracked SABR bundles; affected
sites: scripts/fetch-player-dependencies.cr:166-168, package.json:9-11,
scripts/bundle-sabr-libs.js:55-84, assets/js/sabr/bgutils-js/versions.yml:1-2,
and assets/js/sabr/googlevideo/versions.yml:1-2.

In `@scripts/fetch-sabr-dependencies.cr`:
- Around line 10-26: Format the SABR_DEPENDENCIES declaration in
fetch-sabr-dependencies.cr using Crystal’s formatter, crystal tool format, and
commit the resulting formatting-only changes.
- Around line 84-86: Update the download flow around HTTP::Client.get to
maintain an expected SHA-256 digest for each asset, compute the digest of
response.body_io before File.write, and reject the response without writing when
it does not match. Ensure every downloaded file has a configured digest entry
rather than allowing an unverified asset.

In `@spec/invidious/socks_proxy_spec.cr`:
- Around line 53-76: Move the `@server.accept` call inside the begin/rescue/ensure
block in run so accept failures are captured via `@captured.send`(ex) and the
fiber still closes the socket when available. Preserve the existing handshake,
reply, echo, and cleanup behavior for successful accepts.

In `@src/invidious/config.cr`:
- Around line 69-79: Update Config.load to validate HTTPProxyConfig.type
alongside the existing startup checks, accepting only "http", "socks5", or
"socks5h" and rejecting any other value with a clear configuration error before
the instance starts. Keep the default and valid proxy behavior unchanged.
- Line 40: Connect the quality_sabr configuration property to SABR codec
selection by ensuring SABRPlayer.loadVideo reads and applies codecPreference,
using quality_sabr as the configured default. Preserve the existing preference
options and default behavior while making changes to the SABR loading path
rather than only the configuration declaration.

In `@src/invidious/routes/before_all.cr`:
- Around line 44-51: Update the CSP setup in before_all so script-src does not
globally include unsafe-inline or unsafe-eval. Prefer generating and applying a
per-request nonce for BotGuard interpreter injection while retaining unsafe-eval
only for SABR deciphering; otherwise, scope each relaxed directive to
SABR-enabled watch/embed routes rather than every response.
- Around line 55-57: Update the CSP construction in before_all to derive the
BotGuard helper origin from configuration rather than hardcoding
http://127.0.0.1:4416, and append it to connect-src only when the helper is
enabled. Ensure the configured origin matches the URL used by
assets/js/sabr_player.js and omit the permission entirely when disabled.

In `@src/invidious/routes/proxy.cr`:
- Around line 101-104: In the __headers parsing rescue block, replace the direct
puts call with the established LOGGER warning method, preserving the existing
warning message and exception details while routing output through configured
logging.
- Around line 107-120: Validate the __host value and the target_path derived
from __path or env.request.path before constructing target_url, rejecting any
value containing CR, LF, spaces, or other control characters outside the
expected set. Apply the validation in the proxy route before URI.new so invalid
input cannot reach target_url.request_target, while preserving the existing
fallback and "/" handling for valid paths.
- Around line 34-51: Update every regex in ALLOWED_HOST_PATTERNS to use strict
\A and \z anchors instead of the current partial anchoring, ensuring
is_host_allowed? only accepts complete host strings without trailing newlines or
other injected content.
- Around line 53-65: Update the CORS handling in self.options and the
corresponding proxy response path to remove Access-Control-Allow-Credentials and
stop reflecting the request Origin. Set Access-Control-Allow-Origin to a
same-origin-safe value, such as "*", while preserving the existing allowed
methods, headers, and preflight behavior.
- Around line 179-241: Update the proxy handler around HTTP::Client usage to
always close the client in an ensure path, including success and exceptions;
declare the client outside the begin block so the cleanup clause can access it.
Bound the POST body read in the method branch around body_io.getb_to_end,
rejecting or terminating requests that exceed the established maximum instead of
buffering an unbounded payload.
- Around line 87-105: Restrict the __headers handling in the proxy route:
validate each header name for control characters and only copy names present in
the existing ALLOWED_HEADERS set into custom_headers. Ensure disallowed or
malformed entries are ignored, and apply the same restriction to the outbound
request_headers construction near configure_proxy; additionally add the route’s
required access restriction or rate limit if established mechanisms are
available.

In `@src/invidious/routing.cr`:
- Line 49: Gate the self.register_proxy_routes call behind a new sabr_enabled
configuration flag, following the existing CONFIG.invidious_companion.present?
pattern used by register_companion_routes. Add the corresponding config option
with an appropriate default, and ensure proxy routes are registered only when
SABR is enabled while preserving current route registration for enabled
deployments.

In `@src/invidious/videos/parser.cr`:
- Around line 61-67: Update Video#error_video_info to include empty genreUcid
and music fields alongside version, reason, and subreason, ensuring
Video#genre_url and Video#music can use required hash access when rendering
unavailable videos.
- Around line 79-96: Update the player_response.nil? fallback in the video
parsing flow after parse_video_info to mark the absence of server-side streams
with the established non-reason substate consumed by player.ecr, while leaving
reason unset so SABR remains playable. Ensure non-SABR qualities do not render
the standard player or follow the default dash-to-medium path when no
streamingData is available.

In `@src/invidious/yt_backend/extractors_utils.cr`:
- Around line 71-75: Update has_unlisted_badge? to rescue the conversion error
raised by badges.try &.as_a when the badge container is not an array, following
the existing pattern in has_verified_badge?. Ensure malformed or missing badge
data returns false so parse_video_info can continue.

In `@src/invidious/yt_backend/socks_proxy.cr`:
- Around line 166-188: Update ipv6_bytes to support embedded-IPv4 literals by
recognizing a dotted-decimal final component, converting it into two IPv6 16-bit
groups before encoding; strip any scope-id suffix such as “%eth0” before
parsing. Ensure malformed IPv6 or embedded-IPv4 input is caught and re-raised as
SOCKS5::Error so parse failures from ipv6_bytes cannot escape open as
ArgumentError.
- Around line 212-226: Update the HTTP::Client SOCKS integration around
socks_proxy= so every lazy socket creation after close, including exec retries,
calls SOCKS5::ProxyClient#open instead of falling back to a direct TCPSocket.
Apply the equivalent fix to the http_proxy shard’s proxy= integration, retaining
the proxy configuration for subsequent connections. Ensure
Invidious::Routes::Proxy.proxy applies this integration to fresh clients, while
pool checkout only reapplies the proxy between requests.
🪄 Autofix

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7b5a0def-757d-4174-b188-90886334cba0

📥 Commits

Reviewing files that changed from the base of the PR and between 7118170 and ce6d2d6.

⛔ Files ignored due to path filters (4)
  • assets/js/sabr/bgutils-js/bgutils.bundle.min.js is excluded by !**/*.min.js
  • assets/js/sabr/googlevideo/googlevideo.bundle.min.js is excluded by !**/*.min.js
  • assets/js/sabr/youtubei.js/youtubei.bundle.min.js is excluded by !**/*.min.js
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (46)
  • .gitignore
  • assets/css/sabr_player.css
  • assets/js/sabr/bgutils-js/versions.yml
  • assets/js/sabr/googlevideo/versions.yml
  • assets/js/sabr/shaka-player/controls.css
  • assets/js/sabr/shaka-player/shaka-player.ui.js
  • assets/js/sabr/shaka-player/versions.yml
  • assets/js/sabr/youtubei.js/versions.yml
  • assets/js/sabr_ebml_parser.js
  • assets/js/sabr_helpers.js
  • assets/js/sabr_init.js
  • assets/js/sabr_loader.js
  • assets/js/sabr_manifest_parser.js
  • assets/js/sabr_mp4_index.js
  • assets/js/sabr_onesie.js
  • assets/js/sabr_player.js
  • assets/js/sabr_potoken.js
  • assets/js/sabr_scheme_plugin.js
  • assets/js/sabr_webm_index.js
  • config/config.example.yml
  • locales/en-US.json
  • mocks
  • package.json
  • scripts/bundle-sabr-libs.js
  • scripts/fetch-player-dependencies.cr
  • scripts/fetch-sabr-dependencies.cr
  • spec/invidious/socks_proxy_spec.cr
  • spec/invidious/videos/regular_videos_extract_spec.cr
  • src/invidious/config.cr
  • src/invidious/routes/before_all.cr
  • src/invidious/routes/preferences.cr
  • src/invidious/routes/proxy.cr
  • src/invidious/routes/watch.cr
  • src/invidious/routing.cr
  • src/invidious/user/preferences.cr
  • src/invidious/videos.cr
  • src/invidious/videos/parser.cr
  • src/invidious/videos/video_preferences.cr
  • src/invidious/views/components/player.ecr
  • src/invidious/views/embed.ecr
  • src/invidious/views/user/preferences.ecr
  • src/invidious/views/watch.ecr
  • src/invidious/yt_backend/connection_pool.cr
  • src/invidious/yt_backend/extractors_utils.cr
  • src/invidious/yt_backend/socks_proxy.cr
  • src/invidious/yt_backend/youtube_api.cr

Comment on lines +243 to +250
@media (max-width: 768px) {
.shaka-play-button-container .shaka-play-button {
padding: 1em !important;
}

.shaka-controls-button-panel button {
padding: 8px;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep Shaka mobile controls at their declared size.

Line 249 adds 8px padding to buttons that Shaka defines as 48px by 48px. Shaka resets descendants to content-box sizing. Each control therefore renders as 64px by 64px. The control panel can clip controls on narrow screens.

Remove this padding rule, or set box-sizing: border-box for these buttons.

Proposed fix
 `@media` (max-width: 768px) {
     .shaka-play-button-container .shaka-play-button {
         padding: 1em !important;
     }
-    
-    .shaka-controls-button-panel button {
-        padding: 8px;
-    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@media (max-width: 768px) {
.shaka-play-button-container .shaka-play-button {
padding: 1em !important;
}
.shaka-controls-button-panel button {
padding: 8px;
}
`@media` (max-width: 768px) {
.shaka-play-button-container .shaka-play-button {
padding: 1em !important;
}
}
🤖 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 `@assets/css/sabr_player.css` around lines 243 - 250, Update the mobile
.shaka-controls-button-panel button rule in the responsive styles to prevent
padding from expanding Shaka’s declared 48px controls; remove the added padding
rule, or enforce border-box sizing while preserving the existing control
dimensions.

Comment thread assets/js/sabr_helpers.js
Comment on lines +346 to +355
function isGoogleVideoURL(url) {
try {
var urlObj = new URL(url);
return urlObj.hostname.endsWith('.googlevideo.com') ||
urlObj.hostname.endsWith('.youtube.com') ||
urlObj.hostname.includes('googlevideo');
} catch (e) {
return false;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Permissive googlevideo host matching in two places. Both sites classify a host as Google Video when the hostname merely contains the substring googlevideo, so a name such as googlevideo.attacker.example passes and gets routed through the Invidious /proxy route. The shared root cause is substring matching instead of suffix-anchored matching.

  • assets/js/sabr_helpers.js#L346-L355: remove the hostname.includes('googlevideo') clause and match only the exact apex hosts and the .googlevideo.com / .youtube.com suffixes.
  • assets/js/sabr_player.js#L385-L392: delete the local isGoogleVideo and isYouTube checks and call SABRHelpers.isGoogleVideoURL instead.
📍 Affects 2 files
  • assets/js/sabr_helpers.js#L346-L355 (this comment)
  • assets/js/sabr_player.js#L385-L392
🤖 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 `@assets/js/sabr_helpers.js` around lines 346 - 355, Update
assets/js/sabr_helpers.js lines 346-355 in isGoogleVideoURL to remove substring
matching and accept only exact apex hosts or .googlevideo.com/.youtube.com
suffixes. Update assets/js/sabr_player.js lines 385-392 by removing the local
isGoogleVideo and isYouTube checks and using SABRHelpers.isGoogleVideoURL
instead.

Comment thread assets/js/sabr_init.js
Comment on lines +32 to +38
var errorDiv = document.createElement('div');
errorDiv.className = 'sabr-error-display';
errorDiv.innerHTML = '<p>Failed to load video with SABR player.</p>' +
'<p>' + error.message + '</p>' +
'<p><a href="?quality=dash">Try DASH player instead</a></p>';
container.innerHTML = '';
container.appendChild(errorDiv);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not insert error.message with innerHTML.

Line 35 injects dynamic error text into HTML. If SABRPlayer.loadVideo propagates a YouTube or proxy error message containing markup, it can execute script in the watch-page origin.

Create the message element and assign its textContent.

Proposed fix
-            errorDiv.innerHTML = '<p>Failed to load video with SABR player.</p>' +
-                '<p>' + error.message + '</p>' +
-                '<p><a href="?quality=dash">Try DASH player instead</a></p>';
+            var title = document.createElement('p');
+            title.textContent = 'Failed to load video with SABR player.';
+            var message = document.createElement('p');
+            message.textContent = error instanceof Error ? error.message : String(error);
+            var fallback = document.createElement('p');
+            var link = document.createElement('a');
+            link.href = '?quality=dash';
+            link.textContent = 'Try DASH player instead';
+            fallback.appendChild(link);
+            errorDiv.append(title, message, fallback);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var errorDiv = document.createElement('div');
errorDiv.className = 'sabr-error-display';
errorDiv.innerHTML = '<p>Failed to load video with SABR player.</p>' +
'<p>' + error.message + '</p>' +
'<p><a href="?quality=dash">Try DASH player instead</a></p>';
container.innerHTML = '';
container.appendChild(errorDiv);
var errorDiv = document.createElement('div');
errorDiv.className = 'sabr-error-display';
var title = document.createElement('p');
title.textContent = 'Failed to load video with SABR player.';
var message = document.createElement('p');
message.textContent = error instanceof Error ? error.message : String(error);
var fallback = document.createElement('p');
var link = document.createElement('a');
link.href = '?quality=dash';
link.textContent = 'Try DASH player instead';
fallback.appendChild(link);
errorDiv.append(title, message, fallback);
container.innerHTML = '';
container.appendChild(errorDiv);
🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 33-35: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: errorDiv.innerHTML = '

Failed to load video with SABR player.

' +
'

' + error.message + '

' +
'

Try DASH player instead

'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').

(inner-outer-html)

🪛 OpenGrep (1.26.0)

[WARNING] 34-36: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.

(coderabbit.xss.innerhtml-assignment)

🤖 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 `@assets/js/sabr_init.js` around lines 32 - 38, Update the error-display
construction in SABRPlayer.loadVideo’s error handler to avoid inserting dynamic
error.message through innerHTML. Create a separate message element, assign
error.message via textContent, and append it alongside the existing static error
content and DASH fallback link.

Source: Linters/SAST tools

Comment on lines +36 to +38
var buffer = ArrayBuffer.isView(response.data) ? response.data.buffer : response.data;
var initData = buffer.slice(format.initRange.start, format.initRange.end + 1);
var indexData = buffer.slice(format.indexRange.start, format.indexRange.end + 1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Account for byteOffset when slicing the response buffer.

Line 36 discards the view offset and length by taking .buffer directly. Lines 37-38 then slice with format.initRange and format.indexRange offsets that are relative to the segment, not to the underlying ArrayBuffer. This is correct only while response.data is a view whose byteOffset is 0 and whose length equals the buffer length.

Slice the view instead, so the code stays correct if the networking layer ever returns a subarray.

♻️ Proposed change
-    var buffer = ArrayBuffer.isView(response.data) ? response.data.buffer : response.data;
+    var buffer = ArrayBuffer.isView(response.data)
+      ? response.data.buffer.slice(response.data.byteOffset, response.data.byteOffset + response.data.byteLength)
+      : response.data;
     var initData = buffer.slice(format.initRange.start, format.initRange.end + 1);
     var indexData = buffer.slice(format.indexRange.start, format.indexRange.end + 1);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var buffer = ArrayBuffer.isView(response.data) ? response.data.buffer : response.data;
var initData = buffer.slice(format.initRange.start, format.initRange.end + 1);
var indexData = buffer.slice(format.indexRange.start, format.indexRange.end + 1);
var buffer = ArrayBuffer.isView(response.data)
? response.data.buffer.slice(response.data.byteOffset, response.data.byteOffset + response.data.byteLength)
: response.data;
var initData = buffer.slice(format.initRange.start, format.initRange.end + 1);
var indexData = buffer.slice(format.indexRange.start, format.indexRange.end + 1);
🤖 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 `@assets/js/sabr_manifest_parser.js` around lines 36 - 38, Update the buffer
handling before the initData and indexData slices to preserve response.data’s
byteOffset and byteLength when it is an ArrayBuffer view. Ensure
format.initRange and format.indexRange remain relative to the returned segment
while retaining direct handling for full ArrayBuffers.

Comment on lines +154 to +155
mimeType: format.mimeType.split(';', 1)[0],
codecs: format.mimeType.match(CODECS_REGEX)[1],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

format.mimeType.match(CODECS_REGEX)[1] throws when the codecs parameter is missing.

CODECS_REGEX at line 12 requires a codecs= parameter. If a format's mimeType omits it, match returns null and the index access throws a TypeError. The throw happens inside SabrManifestParser.prototype.start, so one malformed format aborts the whole manifest parse and playback fails instead of skipping that format.

Line 208 in createVideoStream has the same problem.

🛡️ Proposed fix
+  function extractCodecs(mimeType) {
+    var match = mimeType.match(CODECS_REGEX);
+    return match ? match[1] : '';
+  }

Then use it at both call sites:

       mimeType: format.mimeType.split(';', 1)[0],
-      codecs: format.mimeType.match(CODECS_REGEX)[1],
+      codecs: extractCodecs(format.mimeType),
🤖 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 `@assets/js/sabr_manifest_parser.js` around lines 154 - 155, Update
SabrManifestParser.prototype.start and createVideoStream to safely handle a null
result from format.mimeType.match(CODECS_REGEX) before accessing the capture
group. Skip formats whose MIME type lacks a codecs parameter, preventing
malformed entries from aborting manifest parsing or playback.

Comment on lines +61 to +67
private def self.error_video_info(reason : String?, subreason : String?) : Hash(String, JSON::Any)
params = {
"version" => JSON::Any.new(Video::SCHEMA_VERSION.to_i64),
} of String => JSON::Any
params["reason"] = JSON::Any.new(reason) if reason
params["subreason"] = JSON::Any.new(subreason) if subreason
params

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Add the fields required by error-page accessors.

error_video_info omits genreUcid and music. src/invidious/views/watch.ecr calls video.genre_url at Line 202 and video.music at Line 276 even when video.reason exists. Video#genre_url and Video#music use required hash access. The watch page can raise instead of rendering the unavailable-video message.

Add empty values to this error hash, or make these Video accessors tolerate absent keys.

🤖 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 `@src/invidious/videos/parser.cr` around lines 61 - 67, Update
Video#error_video_info to include empty genreUcid and music fields alongside
version, reason, and subreason, ensuring Video#genre_url and Video#music can use
required hash access when rendering unavailable videos.

Comment on lines 79 to +96
if player_response.nil?
return nil
# Player endpoint failed (e.g. no companion). Try to get data from /next only.
begin
player_response = YoutubeAPI.next({"videoId": video_id, "params": ""})
rescue ex
LOGGER.debug("extract_video_info: /next also failed for #{video_id}: #{ex.message}")
raise NotFoundException.new("Video unavailable")
end
begin
params = self.parse_video_info(video_id, player_response)
params["version"] = JSON::Any.new(Video::SCHEMA_VERSION.to_i64)
# We have metadata from /next but no server-side streaming data (no
# companion). Don't treat this as a hard error: the watch page still
# renders, and for quality=sabr the browser SABR player fetches the
# stream itself via Innertube + browser-side PoToken. An informational
# subreason is surfaced for non-SABR playback (which needs companion or
# the local streams proxy).
return params

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not render the standard player without stream data.

When Invidious Companion is disabled, /next does not provide streamingData. This branch returns no reason, so src/invidious/views/components/player.ecr renders the normal player for every quality except sabr. That player has no media sources. The watch route then redirects a default dash request to medium, which also has no sources.

Keep SABR playable in this state, but expose an explicit no-server-streams state for non-SABR qualities. Do not use reason for this state because the SABR player is also suppressed when reason is present.

🤖 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 `@src/invidious/videos/parser.cr` around lines 79 - 96, Update the
player_response.nil? fallback in the video parsing flow after parse_video_info
to mark the absence of server-side streams with the established non-reason
substate consumed by player.ecr, while leaving reason unset so SABR remains
playable. Ensure non-SABR qualities do not render the standard player or follow
the default dash-to-medium path when no streamingData is available.

Comment on lines +71 to +75
def has_unlisted_badge?(badges : JSON::Any?)
return badges.try &.as_a.any? { |badge|
badge.dig?("metadataBadgeRenderer", "icon", "iconType").try &.as_s == "PRIVACY_UNLISTED"
} || false
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle malformed badge containers.

badges.try &.as_a raises when YouTube returns a non-array JSON value. parse_video_info calls this helper while handling incomplete /next metadata. A malformed badge field can fail the whole watch-page parse instead of falling back to false.

Rescue the conversion failure, as has_verified_badge? does.

🤖 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 `@src/invidious/yt_backend/extractors_utils.cr` around lines 71 - 75, Update
has_unlisted_badge? to rescue the conversion error raised by badges.try &.as_a
when the badge container is not an array, following the existing pattern in
has_verified_badge?. Ensure malformed or missing badge data returns false so
parse_video_info can continue.

Comment on lines +166 to +188
# Converts an IPv6 address string (possibly using "::" zero-compression)
# into its 16 raw bytes. Embedded-IPv4 forms (e.g. "::ffff:1.2.3.4") are
# not handled — they are rare as connection targets in Invidious.
private def ipv6_bytes(addr : String) : Bytes
head, sep, tail = addr.partition("::")
head_groups = head.empty? ? [] of String : head.split(':')
tail_groups = tail.empty? ? [] of String : tail.split(':')
groups =
if sep.empty?
head_groups
else
head_groups + Array.new(8 - head_groups.size - tail_groups.size, "0") + tail_groups
end
raise Error.new("Malformed IPv6 address: #{addr}") unless groups.size == 8

bytes = Bytes.new(16)
groups.each_with_index do |group, i|
value = group.to_u16(16)
bytes[i * 2] = (value >> 8).to_u8
bytes[i * 2 + 1] = (value & 0xff).to_u8
end
bytes
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle embedded-IPv4 IPv6 literals, and convert parse failures to SOCKS5::Error.

Socket::IPAddress.valid? returns true for "::ffff:142.250.72.174", so parse_ip returns an inet6 address and ipv6_bytes receives that string. The final group is then "142.250.72.174", and group.to_u16(16) raises ArgumentError. ArgumentError is not an IO::Error, so it escapes open and bypasses every caller that rescues transport failures, including the connection pool.

The same applies to a scope-id suffix such as "fe80::1%eth0".

🐛 Proposed fix
     private def ipv6_bytes(addr : String) : Bytes
+      # Strip an optional scope id ("fe80::1%eth0").
+      addr = addr.partition('%')[0]
+
+      # Expand an embedded IPv4 tail ("::ffff:1.2.3.4") into two hex groups.
+      if (last_colon = addr.rindex(':')) && addr[(last_colon + 1)..].includes?('.')
+        v4 = addr[(last_colon + 1)..].split('.').map(&.to_u8)
+        raise Error.new("Malformed IPv6 address: #{addr}") unless v4.size == 4
+        addr = "#{addr[0..last_colon]}#{((v4[0].to_u16 << 8) | v4[1]).to_s(16)}:#{((v4[2].to_u16 << 8) | v4[3]).to_s(16)}"
+      end
+
       head, sep, tail = addr.partition("::")
       head_groups = head.empty? ? [] of String : head.split(':')
       tail_groups = tail.empty? ? [] of String : tail.split(':')
       groups =
         if sep.empty?
           head_groups
         else
           head_groups + Array.new(8 - head_groups.size - tail_groups.size, "0") + tail_groups
         end
       raise Error.new("Malformed IPv6 address: #{addr}") unless groups.size == 8
 
       bytes = Bytes.new(16)
       groups.each_with_index do |group, i|
-        value = group.to_u16(16)
+        value = group.to_u16?(16) || raise Error.new("Malformed IPv6 address: #{addr}")
         bytes[i * 2] = (value >> 8).to_u8
         bytes[i * 2 + 1] = (value & 0xff).to_u8
       end
       bytes
     end
🤖 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 `@src/invidious/yt_backend/socks_proxy.cr` around lines 166 - 188, Update
ipv6_bytes to support embedded-IPv4 literals by recognizing a dotted-decimal
final component, converting it into two IPv6 16-bit groups before encoding;
strip any scope-id suffix such as “%eth0” before parsing. Ensure malformed IPv6
or embedded-IPv4 input is caught and re-raised as SOCKS5::Error so parse
failures from ipv6_bytes cannot escape open as ArgumentError.

Comment on lines +212 to +226
class HTTP::Client
def socks_proxy=(proxy_client : SOCKS5::ProxyClient) : Nil
@io = proxy_client.open(
host: @host,
port: @port,
tls: @tls,
dns_timeout: @dns_timeout,
connect_timeout: @connect_timeout,
read_timeout: @read_timeout,
write_timeout: @write_timeout,
)
rescue ex : IO::Error
raise IO::Error.new("Failed to open SOCKS connection to #{@host}:#{@port} (#{ex.message})", cause: ex)
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm how HTTP::Client re-creates `@io` and whether it retries after IO::Error,
# and how the http_proxy shard installs its proxy.
fd -H -t d 'http_proxy' -d 5
rg -nP -C6 'def (proxy=|io|exec_internal)' --glob '**/http_proxy/**/*.cr'
rg -n 'http_proxy' shard.yml shard.lock 2>/dev/null

Repository: iv-org/invidious

Length of output: 154


🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- tracked files ---'
git ls-files | rg '(^|/)(socks_proxy\.cr|shard\.yml|shard\.lock|http_proxy)(/|$)' || true

printf '%s\n' '--- relevant source ---'
if [ -f src/invidious/yt_backend/socks_proxy.cr ]; then
  cat -n src/invidious/yt_backend/socks_proxy.cr | sed -n '1,280p'
fi

printf '%s\n' '--- dependency declarations ---'
rg -n -C4 'http_proxy|socks5|HTTP::Client|YoutubeConnectionPool|def client|def proxy' \
  shard.yml shard.lock src lib 2>/dev/null | sed -n '1,260p'

printf '%s\n' '--- available dependency files ---'
fd -H -t f -d 6 '(http_proxy|shard\.lock|shard\.yml)' . 2>/dev/null | sed -n '1,200p'

Repository: iv-org/invidious

Length of output: 29159


🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- project Crystal version and HTTP client usage ---'
rg -n -C3 'crystal:|version:|\.exec|HTTP::Client' shard.yml .github src/invidious/yt_backend src/invidious/routes 2>/dev/null | sed -n '1,260p'

printf '%s\n' '--- http_proxy 0.10.3 repository tree ---'
curl -fsSL 'https://api.github.com/repos/mamantoha/http_proxy/git/trees/v0.10.3?recursive=1' |
  jq -r '.tree[] | select(.type == "blob") | .path' | sed -n '1,200p'

printf '%s\n' '--- http_proxy 0.10.3 proxy implementation candidates ---'
for path in src/http_proxy/client.cr src/http_proxy.cr src/http/proxy/client.cr; do
  url="https://raw.githubusercontent.com/mamantoha/http_proxy/v0.10.3/$path"
  body="$(curl -fsSL "$url" 2>/dev/null || true)"
  if [ -n "$body" ]; then
    printf '%s\n' "--- $url ---"
    printf '%s\n' "$body" | nl -ba | sed -n '1,260p'
  fi
done

printf '%s\n' '--- Crystal HTTP client source references ---'
for url in \
  'https://raw.githubusercontent.com/crystal-lang/crystal/1.10.1/src/http/client.cr' \
  'https://raw.githubusercontent.com/crystal-lang/crystal/1.11.2/src/http/client.cr' \
  'https://raw.githubusercontent.com/crystal-lang/crystal/1.12.2/src/http/client.cr'; do
  body="$(curl -fsSL "$url" 2>/dev/null || true)"
  if [ -n "$body" ]; then
    printf '%s\n' "--- $url ---"
    printf '%s\n' "$body" | rg -n -C8 'def (exec|exec_internal|io|close)|`@io`|IO::Error|reconnect|retry' | sed -n '1,260p'
  fi
done

Repository: iv-org/invidious

Length of output: 40858


🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- http_proxy 0.10.3 source candidates ---'
for path in \
  src/http_proxy.cr \
  src/http_proxy/client.cr \
  src/http/proxy/client.cr \
  src/http/proxy.cr; do
  url="https://raw.githubusercontent.com/mamantoha/http_proxy/v0.10.3/$path"
  tmp="$(mktemp)"
  status="$(curl -sS -L -o "$tmp" -w '%{http_code}' "$url" || true)"
  if [ "$status" = 200 ]; then
    printf '%s\n' "--- $path ---"
    awk '{printf "%6d  %s\n", NR, $0}' "$tmp" | sed -n '1,280p'
  else
    printf '%s %s\n' "$status" "$path"
  fi
  rm -f "$tmp"
done

printf '%s\n' '--- HTTP client proxy integration in Crystal source ---'
for version in 1.10.0 1.10.1 1.11.2 1.12.2; do
  url="https://raw.githubusercontent.com/crystal-lang/crystal/$version/src/http/client.cr"
  tmp="$(mktemp)"
  status="$(curl -sS -L -o "$tmp" -w '%{http_code}' "$url" || true)"
  if [ "$status" = 200 ]; then
    printf '%s\n' "--- Crystal $version ---"
    awk '{printf "%6d  %s\n", NR, $0}' "$tmp" |
      sed -n '760,825p'
  fi
  rm -f "$tmp"
done

Repository: iv-org/invidious

Length of output: 13318


🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- http_proxy extension source ---'
for path in \
  src/ext/http/client.cr \
  src/http/ext/client.cr \
  src/ext/http.cr \
  ext/http/client.cr; do
  url="https://raw.githubusercontent.com/mamantoha/http_proxy/v0.10.3/$path"
  tmp="$(mktemp)"
  status="$(curl -sS -L -o "$tmp" -w '%{http_code}' "$url" || true)"
  if [ "$status" = 200 ]; then
    printf '%s\n' "--- $path ---"
    awk '{printf "%6d  %s\n", NR, $0}' "$tmp" | sed -n '1,280p'
  fi
  rm -f "$tmp"
done

printf '%s\n' '--- all proxy-related paths from the repository archive ---'
archive="$(mktemp)"
if curl -sS -L -o "$archive" -w '%{http_code}' \
  'https://github.com/mamantoha/http_proxy/archive/refs/tags/v0.10.3.tar.gz' | grep -qx 200; then
  tar -tzf "$archive" | rg '(^|/)(ext|src)/.*(http|proxy).*\.cr$|\.cr$' | sed -n '1,240p'
fi
rm -f "$archive"

Repository: iv-org/invidious

Length of output: 2620


Preserve SOCKS5 for every HTTP::Client reconnect.

HTTP::Client#close clears @io. The next HTTP::Client#io call creates a direct TCPSocket to @host:@port``. HTTP::Client#exec also closes and retries once after an `IO::Error`, so a dropped tunnel can bypass SOCKS5 and expose the instance address.

The http_proxy shard has the same behavior: proxy= stores @proxy but opens @io only once. Change the client integration so every socket creation, including retries, invokes SOCKS5::ProxyClient#open. Ensure fresh clients in Invidious::Routes::Proxy.proxy retain this integration; pool checkout only reapplies the proxy between requests.

🤖 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 `@src/invidious/yt_backend/socks_proxy.cr` around lines 212 - 226, Update the
HTTP::Client SOCKS integration around socks_proxy= so every lazy socket creation
after close, including exec retries, calls SOCKS5::ProxyClient#open instead of
falling back to a direct TCPSocket. Apply the equivalent fix to the http_proxy
shard’s proxy= integration, retaining the proxy configuration for subsequent
connections. Ensure Invidious::Routes::Proxy.proxy applies this integration to
fresh clients, while pool checkout only reapplies the proxy between requests.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review continued from previous batch...

Comment thread assets/js/sabr_helpers.js
Comment on lines +6 to +25
'use strict';

// Proxy configuration - uses Invidious proxy endpoint
var SABR_PROXY_PROTOCOL = window.location.protocol.replace(':', '');
var SABR_PROXY_HOST = window.location.hostname;
var SABR_PROXY_PORT = window.location.port || (SABR_PROXY_PROTOCOL === 'https' ? '443' : '80');

var REDIRECTOR_STORAGE_KEY = 'googlevideo_redirector';
var CLIENT_CONFIG_STORAGE_KEY = 'yt_client_config';

/**
* Get proxy configuration
*/
function getProxyConfig() {
return {
PROXY_PROTOCOL: SABR_PROXY_PROTOCOL,
PROXY_HOST: SABR_PROXY_HOST,
PROXY_PORT: SABR_PROXY_PORT
};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap the file in an IIFE to avoid global namespace pollution.

The file declares all helpers at top level, so getProxyConfig, encryptRequest, decryptResponse, asMap, makeResponse, proxyUrl, fetchWithProxy, and generateRandomString all become properties of window. Every other SABR file in this PR uses an IIFE and exports one namespace. assets/js/sabr_potoken.js also defines a separate top-level fetchWithProxy, which is a direct collision risk depending on script order. The public surface is already exported as window.SABRHelpers at line 372, so the top-level declarations are not needed.

♻️ Proposed change
 'use strict';
 
+(function () {
 // Proxy configuration - uses Invidious proxy endpoint
 var SABR_PROXY_PROTOCOL = window.location.protocol.replace(':', '');

Then close the IIFE after the window.SABRHelpers assignment:

     CLIENT_CONFIG_STORAGE_KEY: CLIENT_CONFIG_STORAGE_KEY
 };
+})();
🤖 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 `@assets/js/sabr_helpers.js` around lines 6 - 25, Wrap the entire contents of
sabr_helpers.js, including the top-level configuration constants and helper
functions such as getProxyConfig, encryptRequest, fetchWithProxy, and
generateRandomString, in an IIFE. Keep the existing window.SABRHelpers
assignment as the sole public export, and close the IIFE immediately after that
assignment so these declarations no longer pollute the global namespace or
collide with sabr_potoken.js.

Comment thread assets/js/sabr_helpers.js
Comment on lines +286 to +339
function proxyUrl(url, headers) {
var config = getProxyConfig();
var urlObj = typeof url === 'string' ? new URL(url) : new URL(url.toString());
var newUrl = new URL(urlObj.toString());

if (headers) {
var headersArray = [];
if (headers instanceof Headers) {
headers.forEach(function(value, key) {
headersArray.push([key, value]);
});
} else {
for (var key in headers) {
headersArray.push([key, headers[key]]);
}
}
newUrl.searchParams.set('__headers', JSON.stringify(headersArray));
}

newUrl.searchParams.set('__host', urlObj.host);
newUrl.host = config.PROXY_HOST;
newUrl.port = config.PROXY_PORT;
newUrl.protocol = config.PROXY_PROTOCOL + ':';
newUrl.pathname = '/proxy' + urlObj.pathname;

return newUrl;
}

/**
* Fetch through proxy
* @param {string|URL} input - URL to fetch
* @param {RequestInit} init - Fetch init options
* @returns {Promise<Response>}
*/
async function fetchWithProxy(input, init) {
var url = typeof input === 'string' ? new URL(input) : (input instanceof URL ? input : new URL(input.url));
var headers = new Headers(init?.headers || (input instanceof Request ? input.headers : undefined));
var requestInit = Object.assign({}, init, { headers: headers });

var config = getProxyConfig();

var newUrl = new URL(url.toString());
newUrl.searchParams.set('__headers', JSON.stringify(Array.from(headers.entries())));
newUrl.searchParams.set('__host', url.host);
newUrl.host = config.PROXY_HOST;
newUrl.port = config.PROXY_PORT;
newUrl.protocol = config.PROXY_PROTOCOL + ':';
newUrl.pathname = '/proxy' + url.pathname;

var request = new Request(newUrl, input instanceof Request ? input : undefined);
headers.delete('user-agent');

return fetch(request, requestInit);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the duplicated proxy URL construction.

proxyUrl (lines 288-309) and fetchWithProxy (lines 327-333) build the same proxied URL with the same __headers, __host, host, port, protocol, and /proxy pathname logic. assets/js/sabr_potoken.js lines 52-83 contains a third copy with a different shape (__path instead of the path in the URL). Keep one implementation and call it from the others, so a change to the proxy contract only needs one edit.

♻️ Proposed change
 async function fetchWithProxy(input, init) {
     var url = typeof input === 'string' ? new URL(input) : (input instanceof URL ? input : new URL(input.url));
     var headers = new Headers(init?.headers || (input instanceof Request ? input.headers : undefined));
     var requestInit = Object.assign({}, init, { headers: headers });
 
-    var config = getProxyConfig();
-
-    var newUrl = new URL(url.toString());
-    newUrl.searchParams.set('__headers', JSON.stringify(Array.from(headers.entries())));
-    newUrl.searchParams.set('__host', url.host);
-    newUrl.host = config.PROXY_HOST;
-    newUrl.port = config.PROXY_PORT;
-    newUrl.protocol = config.PROXY_PROTOCOL + ':';
-    newUrl.pathname = '/proxy' + url.pathname;
+    var newUrl = proxyUrl(url, headers);
 
     var request = new Request(newUrl, input instanceof Request ? input : undefined);
     headers.delete('user-agent');
 
     return fetch(request, requestInit);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function proxyUrl(url, headers) {
var config = getProxyConfig();
var urlObj = typeof url === 'string' ? new URL(url) : new URL(url.toString());
var newUrl = new URL(urlObj.toString());
if (headers) {
var headersArray = [];
if (headers instanceof Headers) {
headers.forEach(function(value, key) {
headersArray.push([key, value]);
});
} else {
for (var key in headers) {
headersArray.push([key, headers[key]]);
}
}
newUrl.searchParams.set('__headers', JSON.stringify(headersArray));
}
newUrl.searchParams.set('__host', urlObj.host);
newUrl.host = config.PROXY_HOST;
newUrl.port = config.PROXY_PORT;
newUrl.protocol = config.PROXY_PROTOCOL + ':';
newUrl.pathname = '/proxy' + urlObj.pathname;
return newUrl;
}
/**
* Fetch through proxy
* @param {string|URL} input - URL to fetch
* @param {RequestInit} init - Fetch init options
* @returns {Promise<Response>}
*/
async function fetchWithProxy(input, init) {
var url = typeof input === 'string' ? new URL(input) : (input instanceof URL ? input : new URL(input.url));
var headers = new Headers(init?.headers || (input instanceof Request ? input.headers : undefined));
var requestInit = Object.assign({}, init, { headers: headers });
var config = getProxyConfig();
var newUrl = new URL(url.toString());
newUrl.searchParams.set('__headers', JSON.stringify(Array.from(headers.entries())));
newUrl.searchParams.set('__host', url.host);
newUrl.host = config.PROXY_HOST;
newUrl.port = config.PROXY_PORT;
newUrl.protocol = config.PROXY_PROTOCOL + ':';
newUrl.pathname = '/proxy' + url.pathname;
var request = new Request(newUrl, input instanceof Request ? input : undefined);
headers.delete('user-agent');
return fetch(request, requestInit);
}
function proxyUrl(url, headers) {
var config = getProxyConfig();
var urlObj = typeof url === 'string' ? new URL(url) : new URL(url.toString());
var newUrl = new URL(urlObj.toString());
if (headers) {
var headersArray = [];
if (headers instanceof Headers) {
headers.forEach(function(value, key) {
headersArray.push([key, value]);
});
} else {
for (var key in headers) {
headersArray.push([key, headers[key]]);
}
}
newUrl.searchParams.set('__headers', JSON.stringify(headersArray));
}
newUrl.searchParams.set('__host', urlObj.host);
newUrl.host = config.PROXY_HOST;
newUrl.port = config.PROXY_PORT;
newUrl.protocol = config.PROXY_PROTOCOL + ':';
newUrl.pathname = '/proxy' + urlObj.pathname;
return newUrl;
}
/**
* Fetch through proxy
* `@param` {string|URL} input - URL to fetch
* `@param` {RequestInit} init - Fetch init options
* `@returns` {Promise<Response>}
*/
async function fetchWithProxy(input, init) {
var url = typeof input === 'string' ? new URL(input) : (input instanceof URL ? input : new URL(input.url));
var headers = new Headers(init?.headers || (input instanceof Request ? input.headers : undefined));
var requestInit = Object.assign({}, init, { headers: headers });
var newUrl = proxyUrl(url, headers);
var request = new Request(newUrl, input instanceof Request ? input : undefined);
headers.delete('user-agent');
return fetch(request, requestInit);
}
🤖 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 `@assets/js/sabr_helpers.js` around lines 286 - 339, Consolidate the duplicated
proxied-URL construction by making fetchWithProxy reuse proxyUrl for the URL
transformation, preserving its existing header and host parameters and request
behavior. Also update the corresponding construction in sabr_potoken.js to use
the shared implementation where compatible, while retaining its distinct __path
contract; ensure proxy host, port, protocol, and /proxy pathname logic has one
source of truth.

Comment thread assets/js/sabr_helpers.js
Comment on lines +328 to +338
newUrl.searchParams.set('__headers', JSON.stringify(Array.from(headers.entries())));
newUrl.searchParams.set('__host', url.host);
newUrl.host = config.PROXY_HOST;
newUrl.port = config.PROXY_PORT;
newUrl.protocol = config.PROXY_PROTOCOL + ':';
newUrl.pathname = '/proxy' + url.pathname;

var request = new Request(newUrl, input instanceof Request ? input : undefined);
headers.delete('user-agent');

return fetch(request, requestInit);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

headers.delete('user-agent') runs after the headers are serialized, so it has no effect.

Line 328 serializes the full headers object into the __headers query parameter. Line 336 then removes user-agent from the same object. The proxy reads __headers, so the browser user-agent is still forwarded upstream. The Headers.delete call is also a no-op for the outgoing fetch, because user-agent is a forbidden header name that the browser strips anyway.

If the intent is to hide the browser user agent from YouTube, delete it before line 328. If the intent is to forward it, remove line 336.

🐛 Proposed fix
     var config = getProxyConfig();
 
+    headers.delete('user-agent');
+
     var newUrl = new URL(url.toString());
     newUrl.searchParams.set('__headers', JSON.stringify(Array.from(headers.entries())));
     newUrl.searchParams.set('__host', url.host);
     newUrl.host = config.PROXY_HOST;
     newUrl.port = config.PROXY_PORT;
     newUrl.protocol = config.PROXY_PROTOCOL + ':';
     newUrl.pathname = '/proxy' + url.pathname;
 
     var request = new Request(newUrl, input instanceof Request ? input : undefined);
-    headers.delete('user-agent');
 
     return fetch(request, requestInit);
🤖 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 `@assets/js/sabr_helpers.js` around lines 328 - 338, Update the request
preparation flow around the `headers` serialization and `fetch` call: choose the
intended behavior for `user-agent` and make it consistent. To hide it from
YouTube, move `headers.delete('user-agent')` before the `__headers` value is
created; otherwise remove that deletion. Keep the serialized headers and
outgoing request behavior aligned.

Comment thread assets/js/sabr_loader.js
Comment on lines +26 to +32
window.googlevideo = googlevideo;

// BotGuard
window.BG = BG;

console.info('[SABR Loader]', 'All SABR libraries loaded');
window.dispatchEvent(new Event('sabr-libs-loaded')); No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm how consumers wait for the SABR libraries and whether a flag already exists.
rg -n -C3 'sabr-libs-loaded|sabrLibsLoaded|sabr_loader' assets src

Repository: iv-org/invidious

Length of output: 1592


🏁 Script executed:

#!/bin/bash
printf '%s\n' '--- assets/js/sabr_loader.js ---'
cat -n assets/js/sabr_loader.js
printf '%s\n' '--- assets/js/sabr_init.js ---'
cat -n assets/js/sabr_init.js
printf '%s\n' '--- player script context ---'
sed -n '105,140p' src/invidious/views/components/player.ecr
printf '%s\n' '--- SABR-related references ---'
rg -n -C3 'sabr-libs-loaded|sabrLibsLoaded|window\.(youtubei|googlevideo|BG)|from .*(youtubei|googlevideo|bgutils)|import\(' assets src

Repository: iv-org/invidious

Length of output: 50373


🏁 Script executed:

#!/bin/bash
printf '%s\n' '--- loader ---'
cat -n assets/js/sabr_loader.js
printf '%s\n' '--- initializer ---'
cat -n assets/js/sabr_init.js
printf '%s\n' '--- player template ---'
sed -n '120,136p' src/invidious/views/components/player.ecr
printf '%s\n' '--- focused references ---'
rg -n -C2 --glob 'sabr_*.js' --glob 'player.ecr' 'sabr-libs-loaded|sabrLibsLoaded|window\.(youtubei|googlevideo|BG)' assets src

Repository: iv-org/invidious

Length of output: 7457


🏁 Script executed:

#!/bin/bash
python3 - <<'PY'
from pathlib import Path
import re

loader = Path("assets/js/sabr_loader.js").read_text()
init = Path("assets/js/sabr_init.js").read_text()
template = Path("src/invidious/views/components/player.ecr").read_text()

imports = re.findall(r"""import\s+.*?from\s+['"]([^'"]+)['"]""", loader)
print("static_imports:", imports)
print("event_dispatches:", loader.count("dispatchEvent(new Event('sabr-libs-loaded'))"))
print("readiness_flag_in_loader:", "sabrLibsLoaded" in loader)
print("init_event_listeners:", init.count("addEventListener('sabr-libs-loaded'"))
print("init_checks_readiness_flag:", "sabrLibsLoaded" in init)

for url in imports:
    if url.startswith("/js/"):
        path = Path("assets") / url.removeprefix("/js/")
        print(f"{url} -> {path}: {'present' if path.exists() else 'absent'}")

sabr_init = template.find('src="/js/sabr_init.js')
sabr_loader = template.find('src="/js/sabr_loader.js')
print("template_order_init_before_loader:", 0 <= sabr_init < sabr_loader)
PY

printf '%s\n' '--- tracked SABR bundle paths ---'
git ls-files assets/js/sabr | sed -n '1,80p'

Repository: iv-org/invidious

Length of output: 1204


🌐 Web query:

HTML specification ordering and execution semantics for classic deferscripts versustype="module" scripts when both appear in one document

💡 Result:

According to the HTML specification, both classic scripts with the defer attribute and module scripts (type="module") are executed after the document has finished parsing [1][2]. When both appear in the same document, they are executed in the order in which they appear in the document [1][3][4]. The technical details of this behavior are as follows: 1. Unified Execution Queue: Both deferred classic scripts and module scripts are added to a list of scripts that execute when the document has finished parsing [1][4]. Because they are part of the same conceptual queue, the browser maintains their relative document order [4]. 2. defer Attribute: For classic scripts, the defer attribute instructs the browser to download the script in parallel and execute it only after the document parsing is complete [1][3]. 3. Module Scripts: Module scripts have defer semantics by default [4][2]. The defer attribute has no additional effect on module scripts [1][5]. Regardless of whether a module script is external or inline, it is treated as deferred and will be fetched and executed in order with other deferred classic scripts [1][2]. 4. async Exception: If the async attribute is present, both classic and module scripts will execute as soon as they are available, bypassing the ordered, deferred execution queue [1][3][5]. In this case, execution order is not guaranteed [3][4]. In summary, as long as the async attribute is not used, the browser preserves the original order of the script tags in the document for both classic defer scripts and type="module" scripts [1][3][4].

Citations:


🏁 Script executed:

#!/bin/bash
printf '%s\n' '--- all SABR script references ---'
rg -n --glob '!assets/js/sabr/*.min.js' 'sabr_init\.js|sabr_loader\.js|sabr-libs-loaded|sabrLibsLoaded' .
printf '%s\n' '--- script attributes in the player template ---'
rg -n -C1 '<script[^>]+sabr_(init|loader)\.js|async|defer' src/invidious/views/components/player.ecr

Repository: iv-org/invidious

Length of output: 2143


🌐 Web query:

MDN or WHATWG authoritative execution order between a classic script with defer and an external type=module script without async when the classic script appears first

💡 Result:

When a classic script with the defer attribute and an external module script (type="module") without async appear in the same document, they are both placed in the same execution queue [1]. According to the HTML specification, both script types—deferred classic scripts and module scripts—are executed in the order they appear in the document [2][1]. Because module scripts are deferred by default, they do not block the HTML parser and are executed after the document has finished parsing [3][1][4]. The defer attribute for a classic script instructs the browser to do the same [5][2]. Consequently, when these two script types are present, the browser respects their relative document order, and the classic script appearing first will execute before the module script [2][1]. In summary, the execution order is: 1. The classic script with defer (first in document order). 2. The external module script (second in document order). This behavior is maintained because module scripts share the same "execution queue" as classic scripts that have the defer attribute [1].

Citations:


Handle SABR library import failures.

assets/js/sabr_init.js executes before assets/js/sabr_loader.js, so the readiness flag does not fix an ordering race. If a static import fails, the loader never dispatches sabr-libs-loaded, and the initializer waits indefinitely. Expose the import failure and let the initializer render the DASH fallback.

🤖 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 `@assets/js/sabr_loader.js` around lines 26 - 32, Update the SABR loading flow
in the loader and its consumer so static import failures are captured and
exposed instead of leaving initialization waiting indefinitely. Ensure
sabr-libs-loaded is dispatched with failure state or an equivalent error signal,
and update sabr_init.js to detect that failure and render the DASH fallback
while preserving the successful-loading path.

Comment on lines +506 to +511
if (!this._config.disableVideo) {
videoStreams.sort(function (a, b) {
return VIDEO_CODEC_PRIORITIES.findIndex(function (codec) { return a.codecs.indexOf(codec) === 0; }) -
VIDEO_CODEC_PRIORITIES.findIndex(function (codec) { return b.codecs.indexOf(codec) === 0; });
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The video codec sort ranks unknown codecs above av01.

Array.prototype.findIndex returns -1 when no entry in VIDEO_CODEC_PRIORITIES matches. A stream with an unrecognised codec therefore gets rank -1, which sorts before av01 at rank 0. The unknown codec becomes the most preferred variant.

Map a missing match to a rank after the known list.

🐛 Proposed fix
+  function codecRank(codecs) {
+    var index = VIDEO_CODEC_PRIORITIES.findIndex(function (codec) { return codecs.indexOf(codec) === 0; });
+    return index === -1 ? VIDEO_CODEC_PRIORITIES.length : index;
+  }
       videoStreams.sort(function (a, b) {
-        return VIDEO_CODEC_PRIORITIES.findIndex(function (codec) { return a.codecs.indexOf(codec) === 0; }) -
-          VIDEO_CODEC_PRIORITIES.findIndex(function (codec) { return b.codecs.indexOf(codec) === 0; });
+        return codecRank(a.codecs) - codecRank(b.codecs);
       });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!this._config.disableVideo) {
videoStreams.sort(function (a, b) {
return VIDEO_CODEC_PRIORITIES.findIndex(function (codec) { return a.codecs.indexOf(codec) === 0; }) -
VIDEO_CODEC_PRIORITIES.findIndex(function (codec) { return b.codecs.indexOf(codec) === 0; });
});
}
function codecRank(codecs) {
var index = VIDEO_CODEC_PRIORITIES.findIndex(function (codec) { return codecs.indexOf(codec) === 0; });
return index === -1 ? VIDEO_CODEC_PRIORITIES.length : index;
}
if (!this._config.disableVideo) {
videoStreams.sort(function (a, b) {
return codecRank(a.codecs) - codecRank(b.codecs);
});
}
🤖 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 `@assets/js/sabr_manifest_parser.js` around lines 506 - 511, Update the
comparator in the videoStreams sort block to map each findIndex result of -1 to
a rank after VIDEO_CODEC_PRIORITIES.length, ensuring unrecognised codecs sort
below all known codecs while preserving the existing priority order.

Comment thread assets/js/sabr_potoken.js
Comment on lines +89 to +114
async function init(context) {
if (context) sessionContext = context;
if (initializationPromise) {
return await initializationPromise;
}
return setup();
}

/**
* Internal setup function
* @returns {Promise<Object|undefined>}
*/
async function setup() {
if (initializationPromise) {
return await initializationPromise;
}

initializationPromise = _initBotguard();

try {
botguardClient = await initializationPromise;
return botguardClient;
} finally {
initializationPromise = null;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

init repeats the full BotGuard flow after the first call completes.

setup clears initializationPromise in its finally block at line 112. After initialization succeeds, initializationPromise is null and botguardClient is set. A later init() call therefore falls through to setup() and re-runs _initBotguard, which re-fetches the challenge, re-injects the interpreter script, and issues another GenerateIT request. The comment at lines 136-141 of assets/js/sabr_player.js states that a redundant GenerateIT request per video is exactly the behavior this design intends to avoid.

Return the existing client when one is already available.

♻️ Proposed change
     async function init(context) {
         if (context) sessionContext = context;
         if (initializationPromise) {
             return await initializationPromise;
         }
+        if (botguardClient && integrityTokenBasedMinter) {
+            return botguardClient;
+        }
         return setup();
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function init(context) {
if (context) sessionContext = context;
if (initializationPromise) {
return await initializationPromise;
}
return setup();
}
/**
* Internal setup function
* @returns {Promise<Object|undefined>}
*/
async function setup() {
if (initializationPromise) {
return await initializationPromise;
}
initializationPromise = _initBotguard();
try {
botguardClient = await initializationPromise;
return botguardClient;
} finally {
initializationPromise = null;
}
}
async function init(context) {
if (context) sessionContext = context;
if (initializationPromise) {
return await initializationPromise;
}
if (botguardClient && integrityTokenBasedMinter) {
return botguardClient;
}
return setup();
}
/**
* Internal setup function
* `@returns` {Promise<Object|undefined>}
*/
async function setup() {
if (initializationPromise) {
return await initializationPromise;
}
initializationPromise = _initBotguard();
try {
botguardClient = await initializationPromise;
return botguardClient;
} finally {
initializationPromise = null;
}
}
🤖 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 `@assets/js/sabr_potoken.js` around lines 89 - 114, Update init and/or setup so
a previously initialized botguardClient is returned immediately before starting
_initBotguard again. Preserve the existing initializationPromise sharing for
concurrent calls, while ensuring later init calls reuse the stored client and do
not re-run initialization.

Comment on lines +232 to +254
if ((currentState.sabrStreamState.nextRequestPolicy?.backoffTimeMs || 0) > 0) {
var currentBackoffTimeMs = currentState.sabrStreamState.nextRequestPolicy.backoffTimeMs;
currentState.eventEmitter.emit('backoff-requested', { backoffMs: currentBackoffTimeMs });
await new Promise(function (resolve, reject) {
setTimeout(resolve, currentBackoffTimeMs);
currentState.abortController.signal.addEventListener('abort', reject);
});
currentState.timeoutController?.resetTimeoutOnce();

currentState.cumulativeBackOffTimeMs += currentState.sabrStreamState.nextRequestPolicy.backoffTimeMs;
currentState.cumulativeBackOffRequested += 1;
var timeoutMs = operationInputs.request.retryParameters.timeout;
if (currentState.cumulativeBackOffRequested >= 3 || (timeoutMs > 0 && timeoutMs <= (currentState.cumulativeBackOffTimeMs + currentBackoffTimeMs))) {
shouldReloadDueToBackoffLoop = true;
}
}
if (shouldReloadDueToBackoffLoop || currentState.cumulativeRetryDueToNextRequestPolicy >= 100) {
currentState.sabrStreamState.playerReloadRequested = true;
if (!currentState.abortController.signal.aborted) {
currentState.abortController.abort();
currentState.eventEmitter.emit('reload');
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

The backoff wait leaks abort listeners and ignores an already-aborted signal.

Lines 235-238 create a backoff promise with two defects:

  • The abort listener is added without { once: true } and is never removed. doRequest recurses at line 542 with the same currentState.abortController, and line 248 permits up to 100 retries, so listeners accumulate on one signal across the whole request chain.
  • If currentState.abortController.signal is already aborted, addEventListener never fires. The promise then waits the full currentBackoffTimeMs before continuing, instead of rejecting at once.
  • The setTimeout is never cleared when the abort path wins, so the timer stays pending.

After lines 249-253 set playerReloadRequested and abort, control still falls through to the fetch at line 259. Returning early there would make the reload path explicit.

🛡️ Proposed fix
         await new Promise(function (resolve, reject) {
-          setTimeout(resolve, currentBackoffTimeMs);
-          currentState.abortController.signal.addEventListener('abort', reject);
+          var signal = currentState.abortController.signal;
+          if (signal.aborted) { reject(new Error('aborted')); return; }
+          var timer = setTimeout(function () {
+            signal.removeEventListener('abort', onAbort);
+            resolve();
+          }, currentBackoffTimeMs);
+          function onAbort() { clearTimeout(timer); reject(new Error('aborted')); }
+          signal.addEventListener('abort', onAbort, { once: true });
         });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ((currentState.sabrStreamState.nextRequestPolicy?.backoffTimeMs || 0) > 0) {
var currentBackoffTimeMs = currentState.sabrStreamState.nextRequestPolicy.backoffTimeMs;
currentState.eventEmitter.emit('backoff-requested', { backoffMs: currentBackoffTimeMs });
await new Promise(function (resolve, reject) {
setTimeout(resolve, currentBackoffTimeMs);
currentState.abortController.signal.addEventListener('abort', reject);
});
currentState.timeoutController?.resetTimeoutOnce();
currentState.cumulativeBackOffTimeMs += currentState.sabrStreamState.nextRequestPolicy.backoffTimeMs;
currentState.cumulativeBackOffRequested += 1;
var timeoutMs = operationInputs.request.retryParameters.timeout;
if (currentState.cumulativeBackOffRequested >= 3 || (timeoutMs > 0 && timeoutMs <= (currentState.cumulativeBackOffTimeMs + currentBackoffTimeMs))) {
shouldReloadDueToBackoffLoop = true;
}
}
if (shouldReloadDueToBackoffLoop || currentState.cumulativeRetryDueToNextRequestPolicy >= 100) {
currentState.sabrStreamState.playerReloadRequested = true;
if (!currentState.abortController.signal.aborted) {
currentState.abortController.abort();
currentState.eventEmitter.emit('reload');
}
}
if ((currentState.sabrStreamState.nextRequestPolicy?.backoffTimeMs || 0) > 0) {
var currentBackoffTimeMs = currentState.sabrStreamState.nextRequestPolicy.backoffTimeMs;
currentState.eventEmitter.emit('backoff-requested', { backoffMs: currentBackoffTimeMs });
await new Promise(function (resolve, reject) {
var signal = currentState.abortController.signal;
if (signal.aborted) { reject(new Error('aborted')); return; }
var timer = setTimeout(function () {
signal.removeEventListener('abort', onAbort);
resolve();
}, currentBackoffTimeMs);
function onAbort() { clearTimeout(timer); reject(new Error('aborted')); }
signal.addEventListener('abort', onAbort, { once: true });
});
currentState.timeoutController?.resetTimeoutOnce();
currentState.cumulativeBackOffTimeMs += currentState.sabrStreamState.nextRequestPolicy.backoffTimeMs;
currentState.cumulativeBackOffRequested += 1;
var timeoutMs = operationInputs.request.retryParameters.timeout;
if (currentState.cumulativeBackOffRequested >= 3 || (timeoutMs > 0 && timeoutMs <= (currentState.cumulativeBackOffTimeMs + currentBackoffTimeMs))) {
shouldReloadDueToBackoffLoop = true;
}
}
if (shouldReloadDueToBackoffLoop || currentState.cumulativeRetryDueToNextRequestPolicy >= 100) {
currentState.sabrStreamState.playerReloadRequested = true;
if (!currentState.abortController.signal.aborted) {
currentState.abortController.abort();
currentState.eventEmitter.emit('reload');
}
}
🧰 Tools
🪛 ast-grep (0.45.0)

[error] 235-235: React's useState should not be directly called
Context: setTimeout(resolve, currentBackoffTimeMs)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.

(usestate-direct-usage)


[warning] 235-235: Avoid using the initial state variable in setState
Context: setTimeout(resolve, currentBackoffTimeMs)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.

(setstate-same-var)

🤖 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 `@assets/js/sabr_scheme_plugin.js` around lines 232 - 254, Update the backoff
wait in the request flow around currentState.abortController to handle an
already-aborted signal immediately, register the abort listener with one-shot
cleanup, and clear the pending timeout when abort wins; preserve normal timeout
resolution. After setting playerReloadRequested and aborting in the
shouldReloadDueToBackoffLoop branch, return before the fetch path so reload does
not fall through to the request.

Comment on lines +298 to +311
case UMPPartId.SABR_REDIRECT: {
var sabrRedirect = decodePart(part, SabrRedirect);
if (!sabrRedirect) break;
// Match FreeTube EXACTLY: it writes currentState.sabrUrl (a field that does
// NOT exist on currentState), so the redirect URL is effectively ignored and
// every request keeps POSTing to the ORIGINAL server_abr_streaming_url
// (sabrStreamState.sabrUrl). Earlier we "fixed" this to follow the redirect,
// which pins the session to a specific CDN host (rrN---snXXX) that maintains a
// sequential cursor and returns 0 media when the client jumps/seeks. Following
// the redirect breaks seeking; ignoring it (as FreeTube does) keeps seeks working.
currentState.sabrUrl = sabrRedirect.url;
shouldRetry = true;
break;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The file header contradicts the redirect handling.

Line 7 states that this port "Fixes the latent SABR_REDIRECT bug (write sabrStreamState.sabrUrl, not currentState.sabrUrl)". The code at line 308 does the opposite: it writes currentState.sabrUrl, a field that nothing reads, and the comment at lines 301-307 explains that following the redirect is deliberately avoided because it breaks seeking.

A maintainer who trusts the header will "fix" line 308 and reintroduce the seek failure. Correct line 7 to match the implemented behavior. Also make the discard explicit instead of assigning to a dead field.

🐛 Proposed fix
-// Fixes the latent SABR_REDIRECT bug (write sabrStreamState.sabrUrl, not currentState.sabrUrl).
+// SABR_REDIRECT is intentionally NOT followed; see the note in the SABR_REDIRECT case.
-              currentState.sabrUrl = sabrRedirect.url;
+              // Intentionally discarded; see the note above.
               shouldRetry = true;
🤖 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 `@assets/js/sabr_scheme_plugin.js` around lines 298 - 311, Update the file
header to describe that SABR_REDIRECT URLs are intentionally ignored to preserve
seeking, rather than claiming the redirect bug is fixed. In the SABR_REDIRECT
case within the redirect decoder, remove the assignment to the unused
currentState.sabrUrl field and explicitly discard sabrRedirect.url while
retaining shouldRetry behavior.

Comment on lines +485 to +542
} else if (shouldRetry || invalidPoToken) {
// StreamProtectionStatus 3 = ATTESTATION_REQUIRED: the server is refusing to
// send media until we re-attest. Retrying with the same PO token just loops on
// the 2s backoff forever (the symptom: endless "SABR throttled" toasts), so
// mint a fresh token before retrying and give up loudly once maxRetries is hit.
if (invalidPoToken) {
currentState.attestationRetries = (currentState.attestationRetries || 0) + 1;
var attestationLimit = currentState.attestationMaxRetries || 10;
if (currentState.attestationRetries > attestationLimit) {
throw new ShakaError(
ShakaError.Severity.CRITICAL,
ShakaError.Category.NETWORK,
ShakaError.Code.HTTP_ERROR,
operationInputs.uri,
new Error('SABR attestation required and PO token re-minting did not satisfy it'),
operationInputs.requestType
);
}
var freshPoToken = null;
try {
freshPoToken = currentState.mintPoToken ? await currentState.mintPoToken() : null;
} catch (e) {
console.warn('[SabrScheme] PO token re-mint failed', e);
}
if (freshPoToken) {
currentState.abrRequest.streamerContext.poToken = utils.base64ToU8(freshPoToken);
}
}

if (shouldRetryDueToNextRequestPolicy) {
currentState.cumulativeRetryDueToNextRequestPolicy += 1;
}

var prepared = prepareSabrContexts(currentState.sabrStreamState);
currentState.abrRequest.streamerContext.sabrContexts = prepared.sabrContexts;
currentState.abrRequest.streamerContext.unsentSabrContexts = prepared.unsentSabrContexts;

var body;
try {
body = VideoPlaybackAbrRequest.encode(currentState.abrRequest).finish();
} catch (e) {
console.error('Invalid VideoPlaybackAbrRequest data', currentState.abrRequest);
throw e;
}

currentState.requestInit = {
body: body,
method: 'POST',
headers: {
'content-type': 'application/x-protobuf',
'accept-encoding': 'identity',
'accept': 'application/vnd.yt-ump'
},
signal: currentState.abortController.signal
};
currentState.abortStatus.timedOut = false;
currentState.abortStatus.finished = false;
return doRequest(operationInputs, currentState);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

A repeated SABR_REDIRECT produces an unbounded retry loop.

Line 309 sets shouldRetry = true for a SABR_REDIRECT part without incrementing any counter. The retry branch at line 485 then recurses at line 542.

Every other retry path is bounded:

  • invalidPoToken is capped by attestationMaxRetries at lines 491-502.
  • shouldRetryDueToNextRequestPolicy is capped at 100 by lines 248 and 515.
  • Backoff loops are capped at 3 by line 244.

The redirect path has no cap and no backoff. Because line 308 deliberately discards the redirect URL, the retry re-posts to the same original URL. A server that answers every request with only a redirect part causes an immediate, endless request loop against the Invidious proxy.

Add a redirect retry counter with the same escape hatch that the other paths use.

🐛 Proposed fix
             case UMPPartId.SABR_REDIRECT: {
               var sabrRedirect = decodePart(part, SabrRedirect);
               if (!sabrRedirect) break;
+              currentState.cumulativeRedirects = (currentState.cumulativeRedirects || 0) + 1;
               currentState.sabrUrl = sabrRedirect.url;
               shouldRetry = true;
               break;
             }
     } else if (shouldRetry || invalidPoToken) {
+      if ((currentState.cumulativeRedirects || 0) > 5) {
+        throw createRecoverableNetworkError(
+          ShakaError.Code.HTTP_ERROR,
+          operationInputs.uri,
+          new Error('SABR redirect loop'),
+          operationInputs.requestType
+        );
+      }
🤖 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 `@assets/js/sabr_scheme_plugin.js` around lines 485 - 542, Bound the
SABR_REDIRECT retry path by adding a dedicated counter and maximum retry limit,
incrementing it before the recursive retry in the branch controlled by
shouldRetry. When the limit is exceeded, terminate through the same ShakaError
escape path used for exhausted attestation retries; preserve existing handling
for invalidPoToken, next-request policy retries, and other retry conditions.

Comment on lines +641 to +649
} else {
var candidates = variantTracks.filter(function (track) {
return track.audioRoles.indexOf('main') !== -1;
});
var probableAudioFormat = candidates.reduce(function (previous, current) {
return current.audioBandwidth >= previous.audioBandwidth ? current : previous;
}, candidates[0]);
audioFormatId = formatIdFromString(probableAudioFormat.originalAudioId);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

probableAudioFormat can be undefined.

Line 642 filters variantTracks for tracks whose audioRoles contain main. If no track qualifies, candidates is empty. Line 645 then calls reduce with an initial value of candidates[0], which is undefined, and the callback never runs. Line 648 dereferences undefined.originalAudioId and throws inside the registered sabr scheme handler.

assets/js/sabr_manifest_parser.js sets roles from format flags at lines 113-129, and a format with none of those flags produces an empty roles array. A video whose audio formats carry no isOriginal flag therefore reaches this path.

Line 635 has the related risk that variantTracks[0] is undefined when the track list is empty.

🛡️ Proposed fix
         if (activeVariant) {
           audioFormatId = formatIdFromString(activeVariant.originalAudioId);
         } else {
           var candidates = variantTracks.filter(function (track) {
             return track.audioRoles.indexOf('main') !== -1;
           });
+          if (candidates.length === 0) candidates = variantTracks;
+          if (candidates.length === 0) {
+            return new ShakaAbortableOperation(Promise.reject(
+              createRecoverableNetworkError(ShakaError.Code.HTTP_ERROR, uri, new Error('No audio variant available'), requestType)
+            ), function () { return Promise.resolve(); });
+          }
           var probableAudioFormat = candidates.reduce(function (previous, current) {
             return current.audioBandwidth >= previous.audioBandwidth ? current : previous;
           }, candidates[0]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} else {
var candidates = variantTracks.filter(function (track) {
return track.audioRoles.indexOf('main') !== -1;
});
var probableAudioFormat = candidates.reduce(function (previous, current) {
return current.audioBandwidth >= previous.audioBandwidth ? current : previous;
}, candidates[0]);
audioFormatId = formatIdFromString(probableAudioFormat.originalAudioId);
}
} else {
var candidates = variantTracks.filter(function (track) {
return track.audioRoles.indexOf('main') !== -1;
});
if (candidates.length === 0) candidates = variantTracks;
if (candidates.length === 0) {
return new ShakaAbortableOperation(Promise.reject(
createRecoverableNetworkError(ShakaError.Code.HTTP_ERROR, uri, new Error('No audio variant available'), requestType)
), function () { return Promise.resolve(); });
}
var probableAudioFormat = candidates.reduce(function (previous, current) {
return current.audioBandwidth >= previous.audioBandwidth ? current : previous;
}, candidates[0]);
audioFormatId = formatIdFromString(probableAudioFormat.originalAudioId);
}
🤖 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 `@assets/js/sabr_scheme_plugin.js` around lines 641 - 649, Guard the
audio-format selection in the SABR scheme handler before dereferencing
candidates[0], and handle an empty variantTracks list at the related selection
near line 635. Preserve the existing preferred-track behavior when tracks exist,
but avoid calling formatIdFromString or accessing originalAudioId unless a valid
probableAudioFormat was found; use the handler’s existing fallback behavior for
empty or roleless track lists.

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.

[Enhancement] SABR support

3 participants