[amuleapi] Add an endpoint to download a shared file's contents over HTTP

[ngosang]

Feature request

Today the amuleapi REST surface exposes plenty of metadata about files
(GET /api/v0/shared, GET /api/v0/shared/{hash}, GET /api/v0/downloads, …)
but there is no way to retrieve the actual file contents over HTTP.

I would like an endpoint that serves the bytes of a completed file that aMule
already has on disk, addressed by hash — something like:

GET /api/v0/shared/{hash}/content

Scope, deliberately small:

• Completed files only. No partial/.part files, no range-into-an-
  incomplete-file logic. If it isn't complete, it isn't downloadable.
• Shared Files list only. Since finished downloads are moved into the
  incoming directory, which is shared, they already show up in
  GET /api/v0/shared — so a single endpoint under /shared covers both the
  "I finished this download" case and the "this is a file I share" case. No need
  for a second endpoint under /downloads.

Why

The main motivation is the Web UI: with this endpoint I could add a simple
"Download" button next to each row in the Shared Files view (and in the Downloads
view for finished entries, resolving to the same hash), so a user running aMule on
a headless box / NAS / seedbox can grab a finished file straight from the browser,
without SSH/SFTP/Samba or a second file server in front of it.

Right now the Web UI can tell you a file is finished, but you still have to leave
it and use another tool to actually get the file. This closes that gap and makes
the Web UI self-sufficient for remote-aMule users.

Implementation left open

I'm deliberately leaving the implementation to the backend maintainers — you
know the EC/amuleapi internals far better than I do. Anything that ends up as a
plain HTTP GET returning the file body works for me. As a starting point only:

• URL shape consistent with the existing {hash}-keyed sub-resources
  (/shared/{hash}/verify, /downloads/{hash}/comments, …).
• Ideally Content-Disposition: attachment; filename="...", Content-Length,
  and Range support so browsers show a real progress bar and can resume — but a
  plain unseekable stream would already be useful.
• HEAD handled like the other read-only endpoints.

Open questions

1. amuleapi runs as a separate process talking to amuled/amule over EC,
   so it may not share a filesystem with the core in remote-EC setups. Is it
   acceptable to support only the common same-host case (open the file directly,
   return a clear error otherwise), or should the bytes be streamed through EC?
2. Security: this turns the API into a file server. It should honour the
   existing session/auth requirements, and must resolve files strictly from the
   shared-files list by hash — never from a user-supplied path.

Happy to do the Web UI side (button + wiring) once the endpoint exists, and to
adapt to whatever URL shape and semantics you settle on.

[danim7]

For the open question I would say: 1. Route via EC, for 2 reasons: you will be able to run amuleapi in a different host, and other EC clients (amulegui, amulecmd...) will also benefit from it 2. Yep, we must take special care about security. At first sight, if we address a file by its hash and check it is a fully shared file, it shall be enough. I hope we are not missing any specific corner case. And one additional open question: - perfs and bandwidth: shall the transfer of a big file be accounted for the upload limits of aMule? An EC client retrieving a file can issue other commands at the same time? Other simultaneous EC clients won't be affected by a running transfer?

[ngosang]

We can postpone this feature to v3.2.0 if there are risks. The bittorrent clients I know neither have this feature. I was thinking it will be nice to stream video directly into your smarphone/tv.

[got3nks]

Some notes on feasibility, mostly on the two open questions.

1. Routing the bytes through EC: I'd rule this out. EC requests are processed synchronously on amuled's main thread — the same thread running all eD2k/Kad socket I/O, upload scheduling and hashing (see the comment block in ExternalConn.cpp::WriteDoneAndQueueEmpty). Packets are fully buffered, anything above EC_MAX_UNCOMPRESSED (1 KB) gets zlib-deflated, and there's a 256 MB receiver gate — so a file can't be one packet, and we'd need new chunk/offset/flow-control opcodes, i.e. reimplementing TCP inside EC. Worse, CECSocket::WritePacket busy-loops when the peer doesn't drain, which is exactly what the wedged-amuleweb report in amule-project#666 hit. A browser pausing a video is a slow consumer, so "user paused the stream" becomes "amuled pegs a core and stops uploading". The same-host local-open path is also nearly free: GET /shared/{hash} already returns path (on-disk dir) and name, so no new EC work at all. Remote-EC setups should get a clear 501.

2. Security: hash-scoping is necessary but not sufficient. The bigger problem is that amuleapi serves the Web UI from the same origin as the API, and there is currently no CSP, no X-Content-Type-Options and no X-Frame-Options anywhere in src/webapi/. The shared list isn't only "your" files: completed downloads land in Incoming, which is shared, so both the bytes and the filenames come from strangers on the network. Someone seeds movie.html or a scripted .svg, a user downloads it, and it becomes addressable on the Web UI's origin — script there runs same-origin and can drive the whole API with the user's session (the HttpOnly; SameSite=Strict cookie stops CSRF but not same-origin fetch), including PUT /shared/directories. The existing static handler already guards this by mapping unknown extensions to application/octet-stream; a content endpoint needs the same plus Content-Disposition: attachment, nosniff and a CSP. Three smaller ones: resolve on_disk_dir + name through a realpath containment check against the share roots (ResolveWithinRoot in StaticFs.cpp already does this) so a symlink in a shared directory can't become arbitrary file read; accept only a single Range and reject multi-range and overflow/suffix forms (the Apache Killer / CVE-2011-3192 shape); and note this widens a stolen token from "metadata + control" to "every byte of everything shared", which matters because amuleapi has no TLS.

3. The XSS mitigation directly conflicts with streaming. attachment + octet-stream makes a Download button safe; <video> needs inline and a real MIME type. You can't have both on the same origin without a separate, CSP-sandboxed one.

4. Streaming is a much bigger feature than a GET, and mostly wouldn't work on real ed2k content. CHttpServer::Response::body is a std::string — fully buffered, so a 4 GB file is 4 GB of RSS (the static handler caps at 16 MB for exactly this reason). There's no Range/206/Accept-Ranges, no keep-alive (the session model is read-one-request-write-one-response, so every seek is a fresh TCP connection), gzip would be applied to media bodies by default, and the handler pool is 16 threads total — sixteen concurrent streams would wedge the API and the Web UI with it. Even with all of that fixed, browsers play MP4/H.264+AAC, WebM and Ogg, while typical content here is AVI/XviD or MKV with H.264+AC3/DTS, and MP4s with a trailing moov won't start without seeking the tail. Subtitles are separate .srt files with no muxing. Making "stream to your phone/TV" real means transcoding, and casting devices can't send Authorization headers or cookies, so it also means signed expiring URLs with tokens in proxy logs and history. That's a Jellyfin-shaped feature, not an aMule one.

On danim7's bandwidth question: I wouldn't account these bytes against the eD2k upload limit — they're a local/LAN operator action, not sharing — but there should be a cap on concurrent content connections independent of the 16-thread handler pool, because the transfer does compete with hashing and uploads for the same disk and NIC. Other EC clients are unaffected as long as the bytes never touch EC, which is the main argument for the local-open design.

Suggested scope, agreeing with ngosang that this can slip past 3.1.0: same-host only, hash-resolved with realpath containment, octet-stream + attachment + nosniff + CSP, strict single-range, a chunked writer so memory stays flat, and a separate concurrency cap. Explicitly out of scope: inline playback, transcoding, and unauthenticated or signed URLs. That gets the headless/NAS user off SFTP, which is the concrete win here.

[ngosang]

I know video streaming in the web is really complex. I meant just download the file from the smartphone and open the file in the default player when it finishes. With VLC you can even start playing before download is complete.

Suggested scope, agreeing with ngosang that this can slip past 3.1.0: same-host only, hash-resolved with realpath containment, octet-stream + attachment + nosniff + CSP, strict single-range, a chunked writer so memory stays flat, and a separate concurrency cap. Explicitly out of scope: inline playback, transcoding, and unauthenticated or signed URLs. That gets the headless/NAS user off SFTP, which is the concrete win here.

Good enough

[kno]

Implemented, following the scope agreed above, plus the Web UI half that motivated the request. Everything below is measured against a running daemon, not asserted.

Branch: kno/amule@feat/webui-download-button — eight commits off 2e2c837, nothing from my fork in it.

Up front, because it should not be discovered in review: all of this code, its tests and this comment were generated with AI. Happy to be judged on the diff.

Against the agreed scope

Agreed	Status
Same-host only, clear error otherwise	503 ec_content_unreachable when the path does not resolve on amuleapi's filesystem; 503 ec_content_mismatch when a different file of another size sits at that path
Hash-resolved, realpath containment	Resolved only from the shared list; the joined path is canonicalised and contained against each configured share root, so a symlink in a shared directory cannot become an arbitrary file read. Any resolution failure collapses to an opaque 404 so it cannot probe the share layout
octet-stream + attachment + nosniff + CSP	All four, on this response only — no global CSP, that is a separate change
Strict single range	206 + Content-Range; 416 + bytes */size when unsatisfiable
Chunked writer, flat memory	Flat, but not chunked — see below
Separate concurrency cap	6 concurrent file responses, independent of the 16-thread handler pool and the 32 SSE slots; over the cap, 503 file_responses_exhausted + Retry-After
Out of scope: inline playback, transcoding, unauthenticated/signed URLs	None present

Three deviations, deliberate

Content-Length instead of chunked. The SSE writer is chunked-only, so it cannot carry a Content-Length — no progress bar, no resume. Instead the transport grew a file-response path, and the bytes go out asynchronously on the io_context, so a file response holds no handler-pool thread at all. That is what makes the concurrency affordable rather than merely bounded.

It needed a small Beast Body of its own: basic_file_body::value_type::seek() sets the remaining size to size minus pos, so it always runs to EOF and a first-last range cannot stop early. The replacement caps each read by what remains and streams through a fixed 64 KiB buffer.

Multi-range is ignored, not refused. RFC 7233 §3.1 permits ignoring Range, and never emitting multipart/byteranges is what makes the CVE-2011-3192 shape a no-op: a request naming hundreds of overlapping windows costs one linear read, the same as no header at all. A wrong unit and a 64-bit overflow are treated the same way. Refusing is the other defensible choice — happy to switch.

503, not 501, for the not-reachable-here case, matching the ec_unsupported convention Api.cpp states explicitly rather than being the eighth site to disagree.

Beyond the scope

• 304 conditional GET. The ETag is mtime-size, because hashing a multi-gigabyte body per request is not a slow path but an unusable one. Evaluated before Range per RFC 9110 §13.2.2, so a conditional request carrying a Range answers 304, never 206.
• HEAD, reporting what a GET would, without reading the file.
• Content-Disposition header-injection hardening. These filenames come off the network: one carrying CR or LF would otherwise inject response headers. The quoted form drops controls and neutralises quote and backslash; the extended form percent-encodes everything outside RFC 6266's attr-char. Regression test uses a name with a quote and a semicolon.

A partfile is refused with the same 409 partfile_unsupported that /shared/{hash}/verify uses — a partfile's on-disk layout is gapped, so a byte range out of it would be silently wrong, not merely unavailable.

The Web UI

The Download button, since it is the motivation stated here: a plain anchor in the Shared Files panel and in the panel of a finished download. Deliberately not a fetch() — the endpoint sends attachment, so a navigation hands the response to the browser's own downloader, the page stays mounted, and the bytes never enter JS. A fetch()+Blob would buffer the whole file in browser memory and undo the entire point. The session cookie is HttpOnly and same-origin, so no token goes in the URL. A partfile renders as a disabled button with a title saying why, rather than an anchor that would navigate to a 409.

The branch also carries §4 of #984, and I should flag that rather than let it surface in review. #995 said the Web UI half was left to @ngosang, so this may be duplicated effort — if it is, drop those two commits and take the rest; nothing here depends on them. They are fa4ead4 and cb7647c: the per-file Clients tab reading …/{hash}/clients instead of filtering the global list, the three missing desktop columns, A4AF rows, and the two part indices (next_requested_part, last_downloading_part) that were already decoded but never serialized — without them the source bar can only draw three of the desktop's five states. Three notes for whoever finishes §4: its §5 is already done, its line numbers are all stale, and "every static/i18n/*.json" is exactly two files with a CI gate demanding identical key sets.

Evidence

• 65 curl assertions over a live daemon, plus 2 new ctest suites for the pure logic (path join, containment, range grammar, disposition building) and the range-bounded body. 76/76 suites green.
• Flat memory, measured: a 600 MB transfer moved amuleapi's RSS from 31392 KB to 31632 KB — 240 KB.
• Resume works: cut at 1.5 MB, curl -C - completed it byte-identical.
• 9 simultaneous requests split into exactly 6 × 200 and 3 × 503.
• Running on a real library since this morning: HEAD on a 1.2 GB share returns its exact Content-Length without reading it; bytes=0-9 returns 206 and ten bytes.
• Zero new compiler warnings; clang-format 18 and the i18n gate clean.

How would you like this?

One PR, or split — endpoint first, then the Web UI on top? And is the multi-range decision the one you want?

[got3nks]

Checked this against the code rather than the summary. The scope holds up: same-host with two distinct 503s, realpath containment with a separator boundary check and non-regular files rejected, all four of octet-stream/attachment/nosniff/CSP hard-coded rather than derived from the extension, the 6-slot cap claimed atomically and released from the destructor so a vanished peer cannot leak one. The size-mismatch check for the split-deployment case was not in the agreed scope and is a good addition.

Both deviations are fine as they stand.

Multi-range: keep ignoring it. The security half of "strict single range" is that no multipart/byteranges is ever assembled, which you do, so the CVE-2011-3192 shape has nothing to amplify whichever status you return. Please document the behaviour in REFERENCE.md so a client author is not surprised by a 200 carrying the whole file.

Content-Length rather than chunked: the intent of that scope item was flat memory, and 240 KB for a 600 MB transfer meets it. It is also the better choice for the deployment we care about: amuleapi has no TLS, so it belongs behind a reverse proxy, and nginx proxies with HTTP/1.0 upstream by default where chunked does not apply.

Two things to add, both from that same reverse-proxy angle:

1. The file path needs X-Accel-Buffering: no. The SSE path already sets it (HttpServer.cpp, and EVENTS.md explains why), but the file path does not. With nginx's default proxy_buffering on a large response is spooled to the proxy's disk up to proxy_max_temp_file_size, 1 GB by default, which delays the first byte and moves the cost you removed from amuleapi onto the proxy host. Worth a line in the docs about proxy_buffering off for that location too.

2. Make the concurrency cap configurable. Behind a proxy every client arrives from one address, so 6 is a global budget with no per-user fairness, and X-Forwarded-For is still not honoured. A constant is the right default, not the right ceiling for a NAS with several devices.

On your two questions: split it. 26 files and ~3.7k lines is not reviewable in one pass. Endpoint first, then the Web UI on top. Keep the Web UI work rather than dropping it, but as its own PR so @ngosang can review that half independently against #984, and note that fa4ead4 is amuleapi-side rather than §4, so it stands on its own as a gap in #995. The branch also carries a merge commit, worth flattening when you split it since we squash-merge.

One thing before you split: master has moved 13 commits since 2e2c837 and you will need to rebase. Ten of the files you touch changed there too, so it is not a formality: Api.cpp, State.h, and on the Web UI side client-table.js, download-detail.js, shared-detail.js, api.js, components.js, app.css and both i18n catalogues. #1197 in particular renames the downloads resource, so route strings and anything keyed on it will need updating rather than merging cleanly.

[kno]

Split as you asked, and rebased onto 9722a68 first.

• feat(amuleapi): add GET /shared/{hash}/content #1203 — the endpoint, with both review follow-ups folded in (X-Accel-Buffering: no on the file path, and [Streaming]/MaxConcurrentFileResponses, still 6 by default). Multi-range stays ignored and is now documented as returning a whole-file 200.
• feat(amuleapi): surface the two part indices the source bar needs #1204 — the two part indices. Filed on its own as you suggested, as a gap in feat(amuleapi): per-file client routes, per-part bitmaps, and peer fields on the list row #995 rather than §4 of WebUI: Clients of a file - per-file routes (/downloads/{hash}/clients, /shared/{hash}/clients), the per-source part bar, and A4AF rows #984.

The Web UI half is held back deliberately: the Download button depends on #1203 and the per-file Clients tab on #1204, so opening either now would show a cumulative diff and undo the split. I will open each once its parent merges. The merge commit is gone — all four branches are flat.

On the rebase, which was not a formality. The renames caught this branch in six places. size → size_bytes broke an assertion in the curl suite, which is how I found the rest. part_count → parts_total_count and available_parts → parts_offered_count merged silently and had to be found by sweeping every key the branch reads. os_info → reported_os and remaining_time → remaining_seconds conflicted cleanly.

The one that mattered was view_shared_disabled → shared_files_browsable, with the value inverted. The Web UI column rendered the negation of the old field; against the new one it would have rendered backwards, and looked entirely plausible doing it. Caught by sweeping rather than by a test, which is worth saying out loud — nothing in the suite would have failed.

Unrelated, found while rebasing: src/webapi/static/js/views/shared-detail.js:171 still reads s.part_count, which #1201 renamed, so that stat row renders empty. Not touched in either PR since neither owns that line — happy to file it separately, or fold it into the Web UI PR when that one opens, whichever you prefer.

Also unrelated, and worth a look: amuleapi aborts on SIGTERM instead of exiting cleanly — Unhandled standard exception of type "NSt3__112bad_weak_ptrE", reproducible with nothing in flight. I confirmed it on upstream/master at 9722a68 built clean with none of my changes, so it is not from this work, but it means the container's supervisor sees a crash on every stop. I will file it separately unless you would rather it went somewhere else.

[got3nks]

Two answers, and thanks for splitting the branches.

shared-detail.js needs nothing. #1212 landed this morning and swept that whole class, not just line 171. I checked every key from your rename list (part_count, available_parts, os_info, remaining_time, view_shared_disabled, and bare size) against the current Web UI and there is nothing stale left, so there is nothing to file or fold in.

The SIGTERM abort: please open a separate issue for it. A crash on every clean stop is worth its own thread rather than a note inside a feature request, and it is unrelated to both PRs. Include the repro steps, the fact that it reproduces on 9722a68 with none of your changes, and a backtrace if you can get one from a debug build. bad_weak_ptr means shared_from_this() ran with no owning shared_ptr, and HttpServer.cpp's Session uses it in a dozen places including the SSE registration, so the frame it aborts in is most of the diagnosis.

On the rename that inverted its value: worth saying out loud, as you did. A sweep caught it and no test would have. That is a gap on our side, not yours.

[kno]

Both crossed off.

shared-detail.js — confirmed, #1212 covers it. I re-checked every key from my rename list against current master and there is nothing stale left, so nothing to fold into either PR.

The SIGTERM abort — filed as #1214, with the repro, the fact that it reproduces on 9722a68 and 91c7434 built clean with none of my changes, and the shared_from_this() reasoning your hint pointed at.

No backtrace, and I would rather say why than let it look like an oversight: lldb -b -o 'process launch' never returns for a daemon that keeps running, so the commands after it never execute, and lldb -b -p <pid> would not complete an attach either — the usual macOS codesigning restriction on an unsigned local build. The repro is deterministic, so the frame should fall straight out of a debug build on Linux. Happy to run a specific incantation and report back if that is quicker than doing it yourselves.

On the inverted rename: taken. The reason it caught me is that nothing in the suite drives the frontend, which is the same reason it survived your rename series — a curl suite cannot see a boolean rendered backwards. Not much comfort in it for either of us, but it does say where the gap is rather than who missed it.