Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 54 additions & 29 deletions server.py
Original file line number Diff line number Diff line change
Expand Up @@ -12941,53 +12941,57 @@ def _extract():
_extract_cache_lock = threading.Lock()


@app.get("/api/sloppak/{filename:path}/file/{rel_path:path}")
def serve_sloppak_file(filename: str, rel_path: str):
"""Serve a file from inside a sloppak (stems, cover, etc.)."""
def _resolve_sloppak_local_file(filename: str, rel_path: str):
"""Resolve a file inside a sloppak to its on-disk path.

Applies the same containment guards as ``serve_sloppak_file``. Returns the
resolved ``Path`` on success, or an ``(error, status)`` tuple on failure so
callers can produce their endpoint-appropriate response.
"""
dlc = _get_dlc_dir()
if not dlc:
return JSONResponse({"error": "not configured"}, 404)
# `filename` is an attacker-controlled `:path` param. Contain it under
# DLC_DIR before it reaches the resolver, which does a bare
# `dlc_root / filename`. Without this, `../../../etc` escapes the root
# and the rel_path guard below validates `target` against the already-
# escaped `src`, which trivially passes — yielding arbitrary file reads
# (e.g. /api/sloppak/../../../../etc/file/passwd). Mirrors the guard
# `get_song_art` applies to the same filename param.
return ("not configured", 404)
# `filename` is caller-controlled. Contain it under DLC_DIR before it
# reaches the resolver (see serve_sloppak_file for the traversal rationale).
resolved = _resolve_dlc_path(dlc, filename)
if resolved is None:
return JSONResponse({"error": "forbidden"}, 403)
# Confine the endpoint to actual sloppak bundles. Without this, a
# contained-but-non-sloppak `filename` (e.g. `.` → DLC_DIR itself, or
# any plain subdirectory) would make `resolve_source_dir` hand back a
# directory and turn this into a read-any-file-under-DLC_DIR endpoint.
# Mirrors get_song_art's `is_sloppak` dispatch.
return ("forbidden", 403)
# Confine to actual sloppak bundles — otherwise any plain subdirectory
# would become a read-any-file-under-DLC_DIR source.
if not sloppak_mod.is_sloppak(resolved):
return JSONResponse({"error": "not found"}, 404)
# Canonicalise the cache key against the resolved path so equivalent
# URL forms of the same sloppak (e.g. `A/../B/x.sloppak` vs
# `B/x.sloppak`) converge on one `_source_cache` entry instead of
# fragmenting / re-unpacking — mirrors get_song_info's keying.
return ("not found", 404)
# Canonicalise the cache key against the resolved path so equivalent URL
# forms of the same sloppak converge on one _source_cache entry.
try:
filename = resolved.relative_to(dlc.resolve()).as_posix()
except ValueError:
# safe_join already proved containment, so this is unreachable in
# practice; fail closed rather than fall back to the raw param.
return JSONResponse({"error": "forbidden"}, 403)
# safe_join already proved containment; fail closed regardless.
return ("forbidden", 403)
src = sloppak_mod.get_cached_source_dir(filename)
if src is None:
try:
src = sloppak_mod.resolve_source_dir(filename, dlc, SLOPPAK_CACHE_DIR)
except Exception:
return JSONResponse({"error": "not found"}, 404)
return ("not found", 404)
Comment on lines 12965 to +12975

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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the resolver/cache contracts without executing repository code.
rg -n -C4 'def _resolve_dlc_path|def _resolve_sloppak_local_file|def resolve_source_dir|def get_cached_source_dir|test_junction_subfolder_allowed' server.py lib tests

Repository: got-feedBack/feedBack

Length of output: 2738


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== server.py around _resolve_sloppak_local_file =="
sed -n '12944,12995p' server.py | cat -n

echo
echo "== lib/sloppak.py resolve_source_dir and cache helper =="
sed -n '136,205p' lib/sloppak.py | cat -n

echo
echo "== tests/test_dlc_junction.py relevant test =="
sed -n '1,140p' tests/test_dlc_junction.py | cat -n

Repository: got-feedBack/feedBack

Length of output: 9860


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== all sloppak cache callers =="
rg -n -C3 '_source_cache|get_cached_source_dir|resolve_source_dir\(' lib server.py tests

echo
echo "== sloppak-related file serving paths in server.py =="
rg -n -C3 'sloppak_mod\.(is_sloppak|get_cached_source_dir|resolve_source_dir|_source_cache)' server.py

Repository: got-feedBack/feedBack

Length of output: 13371


Avoid caching past the resolver

resolved.relative_to(dlc.resolve()) rejects valid junctioned DLC subfolders, and get_cached_source_dir() can return an unpacked directory without rechecking mtime/size, so replaced or removed sloppaks can serve stale files. Keep the cache key DLC-relative and go through resolve_source_dir() on each request; narrow the broad except while here.

🧰 Tools
🪛 Ruff (0.15.20)

[warning] 12974-12974: Do not catch blind exception: Exception

(BLE001)

🤖 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 `@server.py` around lines 12965 - 12975, The cache and path validation in the
sloppak serving flow is too strict on junctioned subfolders and can return stale
content from get_cached_source_dir(). Update the request path logic around
resolved.relative_to(dlc.resolve()) and the src lookup so it stays DLC-relative,
always routes through resolve_source_dir() on each request, and avoids serving
an unpacked directory without revalidating it. Also narrow the broad except in
this block to only the expected resolver failure cases.

Source: Linters/SAST tools

# Prevent path traversal within the sloppak.
target = (src / rel_path).resolve()
try:
target.relative_to(src.resolve())
except ValueError:
return JSONResponse({"error": "forbidden"}, 403)
return ("forbidden", 403)
if not target.exists() or not target.is_file():
return JSONResponse({"error": "not found"}, 404)
return ("not found", 404)
return target


@app.get("/api/sloppak/{filename:path}/file/{rel_path:path}")
def serve_sloppak_file(filename: str, rel_path: str):
"""Serve a file from inside a sloppak (stems, cover, etc.)."""
result = _resolve_sloppak_local_file(filename, rel_path)
if isinstance(result, tuple):
error, status = result
return JSONResponse({"error": error}, status)
target = result
ext = target.suffix.lower()
mt = {
".ogg": "audio/ogg", ".opus": "audio/ogg", ".oga": "audio/ogg",
Expand Down Expand Up @@ -13910,13 +13914,19 @@ def _fill_scale_degree(wire: dict, n, t: float) -> None:

@app.get("/api/audio-local-path")
def audio_local_path(url: str, request: Request):
"""Return absolute local filesystem path for an /audio/… URL (Electron desktop only).
"""Return absolute local filesystem path for a song URL (Electron desktop only).

Accepts ``/audio/<path>`` where ``<path>`` may include subdirectory segments —
no scheme, no host, no query string, no fragment. The resolved path must stay
inside AUDIO_CACHE_DIR or STATIC_DIR; ``..`` traversal, backslashes, and
absolute ``filename`` values are rejected.

Also accepts ``/api/sloppak/<filename>/file/<rel>`` (percent-encoded, as
emitted by the highway song payload) and resolves it to the unpacked
sloppak cache file via the same containment guards as
``serve_sloppak_file`` — this lets the desktop engine play a feedpak
full-mix natively under WASAPI-exclusive output.

This endpoint returns a raw filesystem path and is intended exclusively for
the Electron desktop process (which runs on loopback). Requests from non-
loopback clients are rejected with 403.
Expand All @@ -13929,6 +13939,21 @@ def audio_local_path(url: str, request: Request):
is_loopback = client_host == "localhost"
if not is_loopback:
return JSONResponse({"error": "forbidden"}, status_code=403)
# Sloppak in-pack file (feedpak full-mix): /api/sloppak/<fn>/file/<rel>.
# Both segments arrive percent-encoded (built with urllib quote() in the
# highway payload); decode before handing to the shared resolver, which
# re-applies all containment guards on the decoded values.
slop_match = re.fullmatch(r"/api/sloppak/([^?#]+)/file/([^?#]+)", url)
if slop_match:
from urllib.parse import unquote

result = _resolve_sloppak_local_file(
unquote(slop_match.group(1)), unquote(slop_match.group(2))
)
if isinstance(result, tuple):
error, status = result
return JSONResponse({"error": error}, status_code=status)
return JSONResponse({"path": str(result)})
Comment on lines +13946 to +13956

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.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Split sloppak URLs at the first /file/ delimiter.

Line 13946’s greedy first capture misparses valid in-pack paths such as stems/file/full.ogg as part of the sloppak filename, returning 404 for an existing file.

Proposed fix and regression test
-    slop_match = re.fullmatch(r"/api/sloppak/([^?#]+)/file/([^?#]+)", url)
+    slop_match = re.fullmatch(r"/api/sloppak/([^?#]+?)/file/([^?#]+)", url)
def test_sloppak_url_rel_path_can_contain_file_segment(dlc_client):
    tc, _server, dlc = dlc_client
    pak = _make_sloppak(dlc)
    nested = pak / "stems" / "file"
    nested.mkdir()
    (nested / "full.ogg").write_bytes(b"OggS-fake")

    r = tc.get(
        "/api/audio-local-path",
        params={"url": "/api/sloppak/song.sloppak/file/stems/file/full.ogg"},
    )

    assert r.status_code == 200, r.text
    assert r.json()["path"] == str((nested / "full.ogg").resolve())
📝 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
slop_match = re.fullmatch(r"/api/sloppak/([^?#]+)/file/([^?#]+)", url)
if slop_match:
from urllib.parse import unquote
result = _resolve_sloppak_local_file(
unquote(slop_match.group(1)), unquote(slop_match.group(2))
)
if isinstance(result, tuple):
error, status = result
return JSONResponse({"error": error}, status_code=status)
return JSONResponse({"path": str(result)})
slop_match = re.fullmatch(r"/api/sloppak/([^?#]+?)/file/([^?#]+)", url)
if slop_match:
from urllib.parse import unquote
result = _resolve_sloppak_local_file(
unquote(slop_match.group(1)), unquote(slop_match.group(2))
)
if isinstance(result, tuple):
error, status = result
return JSONResponse({"error": error}, status_code=status)
return JSONResponse({"path": str(result)})
🤖 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 `@server.py` around lines 13946 - 13956, Update the sloppak URL parsing in the
`/api/audio-local-path` handling so it splits on the first `/file/` delimiter
instead of using the current greedy `re.fullmatch` capture in the `slop_match`
block. Adjust the regex or parsing logic so `_resolve_sloppak_local_file`
receives the correct pak name and relative path even when the in-pack path
contains `file/`, and add a regression test like
`test_sloppak_url_rel_path_can_contain_file_segment` to verify
`/api/sloppak/song.sloppak/file/stems/file/full.ogg` resolves successfully.

# Accept only simple /audio/<filename> — no scheme, no host, no query/fragment
if not re.fullmatch(r"/audio/[^?#]+", url):
return JSONResponse({"error": "invalid url"}, status_code=400)
Expand Down
87 changes: 77 additions & 10 deletions static/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -1498,7 +1498,7 @@
// Normalize any stale 'Combo' tokens left from legacy-mode sessions.
if (_getArrangementNamingMode() === 'smart') {
filters.arrHas = _toSmartArrs(filters.arrHas);
filters.arrLacks = _toSmartArrs(filters.arrLacks);

Check warning on line 1501 in static/app.js

View workflow job for this annotation

GitHub Actions / ci / lint

File has too many lines (11919). Maximum allowed is 1500
}
return filters;
} catch {
Expand Down Expand Up @@ -4864,6 +4864,47 @@
// (a network blip on /api/audio-local-path, an isAudioRunning() race
// during a device restart) are deliberately NOT memoised so they retry.
let _rerouteRejectedUrl = null;
// Exclusive-style output backends silence every other client on the
// endpoint — including our own <audio> element. The share mode IS the
// JUCE output device type: "Windows Audio (Exclusive Mode)" is a
// hardcoded, unlocalised JUCE type name; ASIO drivers typically hold
// the endpoint exclusively too. "Windows Audio (Low Latency Mode)" is
// shared and must NOT match.
function _isExclusiveOutputType(t) {
return t === 'Windows Audio (Exclusive Mode)' || t === 'ASIO';
}
// [feedpak-route] diagnostics: log the raw outputType string once per
// value change (this runs on a 350ms poll — logging every tick would
// flood the diagnostics buffer).
let _loggedOutputType;
async function _outputIsExclusive() {
if (typeof juceApi.getCurrentDevice !== 'function') {
if (_loggedOutputType !== '<no-getCurrentDevice>') {
_loggedOutputType = '<no-getCurrentDevice>';
console.warn('[feedpak-route] juceApi.getCurrentDevice missing — cannot detect exclusive output');
}
return false;
}
try {
const dev = await juceApi.getCurrentDevice();
const t = dev?.outputType || dev?.type || '';
const excl = _isExclusiveOutputType(t);
if (t !== _loggedOutputType) {
_loggedOutputType = t;
console.log('[feedpak-route] outputType=', JSON.stringify(t), '→ exclusive=', excl);
}
return excl;
} catch (e) {
if (_loggedOutputType !== '<getCurrentDevice-failed>') {
_loggedOutputType = '<getCurrentDevice-failed>';
console.warn('[feedpak-route] getCurrentDevice failed:', e);
}
return false;
}
}
// highway.js's initial song-load routing consults this for the same
// feedpak-under-exclusive decision the watcher makes below.
window._juceOutputIsExclusive = _outputIsExclusive;
// Returns true when window._currentSongAudio no longer references the exact
// snapshot object captured at reroute entry — i.e. the song was swapped (or
// cleared) mid-flight. Staleness is detected by object-reference identity,
Expand Down Expand Up @@ -4904,8 +4945,12 @@
audio.pause();
try {
const res = await fetch(`/api/audio-local-path?url=${encodeURIComponent(url)}`);
if (!res.ok) throw new Error('HTTP ' + res.status);
if (!res.ok) {
console.warn('[feedpak-route] audio-local-path HTTP', res.status, 'for', url);
throw new Error('HTTP ' + res.status);
}
const { path } = await res.json();
console.log('[feedpak-route] audio-local-path resolved:', (typeof path === 'string' && path.split(/[\\/]/).pop()) || '<missing>');
Comment on lines +4948 to +4953

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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Avoid retrying persistent local-path lookup failures every poll.

Line 4950 turns every non-OK /api/audio-local-path response into a transient throw. Since only 'rejected' is memoized later, a persistent 400/403/404 will pause/resume HTML5 and log again every 350ms. Mark deterministic 4xx lookup failures as a hard reject after restoring HTML5 playback.

Proposed direction
             if (!res.ok) {
                 console.warn('[feedpak-route] audio-local-path HTTP', res.status, 'for', url);
-                throw new Error('HTTP ' + res.status);
+                const err = new Error('HTTP ' + res.status);
+                err.hardReject = res.status >= 400 && res.status < 500;
+                throw err;
             }

Then in the existing catch, after restoring/reporting the browser route:

-            throw err;
+            if (err && err.hardReject) return 'rejected';
+            throw err;
🤖 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 `@static/app.js` around lines 4948 - 4953, The audio-local-path fetch path in
the browser route treats every non-OK response as transient, so persistent 4xx
lookup failures keep retrying on every poll. Update the logic around the
`/api/audio-local-path` handling so deterministic client errors are marked as a
hard reject after HTML5 playback is restored, and only true transient failures
continue to be retried. Use the existing `catch` and the `rejected` memoization
flow in `static/app.js` to locate and wire the fix.

if (_isStale(songAudio)) return 'stale'; // song changed mid-fetch
const ok = await juceApi.loadBackingTrack(path);
if (ok === false) {
Expand Down Expand Up @@ -5123,8 +5168,12 @@
async function _reevaluateJuceRouting() {
if (_rerouteInFlight) return;
const songAudio = window._currentSongAudio;
// Only /audio/ songs are JUCE-routable; sloppak stems stay on HTML5.
if (!songAudio || !songAudio.juceEligible) return;
// /audio/ songs are always JUCE-routable. A feedpak full-mix
// (single-mix pack, no stems) is routable ONLY under an
// exclusive-style output — in shared mode it must stay on HTML5 so
// the stem mixer / WebAudio path keeps working. Sloppak stem URLs
// are never routable (per-stem mix can't ride a single transport).
if (!songAudio || (!songAudio.juceEligible && !songAudio.feedpakFullMix)) return;
// Don't race highway.js's own initial song-load routing: it owns
// _juceMode until _juceRoutingPromise settles. Re-running our switch
// concurrently would double-call loadBackingTrack for the same URL.
Expand All @@ -5142,13 +5191,30 @@
try { running = await juceApi.isAudioRunning(); }
catch (_) { return; }
if (_isStale(songAudio)) return; // song changed during IPC
if (!!running === !!window._juceMode) return; // routing already consistent

const wantJuce = running && !window._juceMode;
// Eligibility is evaluated per tick, not snapshotted at song load:
// the output share mode can change mid-song (device switch in the
// Audio Engine panel), and a feedpak full-mix must follow it —
// exclusive → ride the engine; back to shared → return to HTML5.
let eligible = !!songAudio.juceEligible;
if (!eligible && songAudio.feedpakFullMix && running) {
eligible = await _outputIsExclusive();
if (_isStale(songAudio)) return; // song changed during IPC
}
const wantJuce = !!(running && eligible);
// [feedpak-route] diagnostics: one line per decision change (the
// watcher polls at 350ms; steady state must not spam the buffer).
const _decision = 'running=' + running + ' eligible=' + eligible
+ ' feedpakFullMix=' + !!songAudio.feedpakFullMix
+ ' juceMode=' + !!window._juceMode + ' url=' + songAudio.url;
if (_decision !== window._lastFeedpakRouteDecision) {
window._lastFeedpakRouteDecision = _decision;
console.log('[feedpak-route] watcher:', _decision);
}
if (wantJuce === !!window._juceMode) return; // routing already consistent
// Don't keep retrying a track JUCE explicitly rejected.
if (wantJuce && songAudio.url === _rerouteRejectedUrl) return;

if (running) {
if (wantJuce) {
const outcome = await _switchHtml5ToJuce(songAudio);
// Memoise ONLY an explicit hard JUCE reject. A successful
// switch clears the memo; a 'stale' abort (song changed
Expand All @@ -5163,9 +5229,10 @@
// outcome === 'stale': leave _rerouteRejectedUrl as-is.
} else {
await _switchJuceToHtml5(songAudio);
// The engine just stopped. Clear any hard-reject memo so a
// later engine restart re-evaluates the track at least once —
// the rejection may have been a transient device/decoder state.
// The engine stopped (or a feedpak's output left exclusive
// mode). Clear any hard-reject memo so a later engine restart
// or mode change re-evaluates the track at least once — the
// rejection may have been a transient device/decoder state.
_rerouteRejectedUrl = null;
}
} catch (e) {
Expand Down
Loading
Loading