Skip to content

Add a filename-terminated agent download URL - #2442

Open
urbanadventurer wants to merge 1 commit into
hashtopolis:masterfrom
urbanadventurer:fix/agent-download-filename-url
Open

Add a filename-terminated agent download URL#2442
urbanadventurer wants to merge 1 commit into
hashtopolis:masterfrom
urbanadventurer:fix/agent-download-filename-url

Conversation

@urbanadventurer

Copy link
Copy Markdown

Problem

wget http://host/agents.php?download=1 saves the file as
agents.php?download=1 instead of hashtopolis.zip.

The Content-Disposition: attachment; filename="hashtopolis.zip" header is
already sent, but clients that name the file from the URL rather than the header
still get it wrong, because the last path segment is agents.php, not the
filename:

  • wget (without --content-disposition) appends the query string too, saving
    agents.php?download=1.
  • curl -O drops the query string and saves agents.php.

Neither yields hashtopolis.zip.

Fix

Accept an additional, filename-terminated URL:

agents.php/download/<id>/<filename>     e.g. agents.php/download/1/hashtopolis.zip

The id is read from PATH_INFO via a new pure helper
Util::getAgentDownloadId(); the trailing <filename> is the last path segment
and there is no query string, so wget/curl save hashtopolis.zip directly. The
UI's download link on the "new agent" page is switched to this form. The old
?download=<id> URL still works (the new branch only triggers when ?download
is absent).

AgentHandler::downloadAgent() is unchanged — the served bytes and the
Content-Disposition filename still come from the binary record, so the
<filename> in the URL is cosmetic for the client and can't be used to serve a
different file (the numeric id, matched as [0-9]+, selects the binary).

Testing

  • New PHPUnit tests in ci/phpunit/inc/UtilTest.php cover
    Util::getAgentDownloadId(): the /download/<id>/<file>, /download/<id>
    forms extract the id; /download/abc/x, /other, "", and null return
    null.
  • php -l passes on src/agents.php, src/inc/Util.php, and the test.
  • End-to-end with php -S + a router mimicking mod_php PATH_INFO: wget and
    curl -O on agents.php/download/1/hashtopolis.zip both save
    hashtopolis.zip, and the handler receives id 1.

Deployment note

This relies on the web server passing PATH_INFO to agents.php, which the
shipped php:8.5-apache (mod_php) image does by default — no mod_rewrite or
Apache-config change is needed. A reverse-proxied php-fpm setup that doesn't
forward PATH_INFO would simply not see the new URL and fall back to the
still-working ?download=<id> form, so this is strictly additive.

Alternatives considered

  • A fully clean agents/download/<id>/<filename> URL (no .php): requires a
    mod_rewrite rule in the vhost config (000-default.conf), changing the
    deployment for every install. The PATH_INFO form gives the same
    client-visible result with no config change.
  • Putting the parser inline in agents.php: kept it in Util instead so it is
    covered by PHPUnit and analysed by phpstan (both scan src/inc).
  • Adding Content-Length / switching echo file_get_contents() to readfile()
    in downloadAgent(): doesn't address the filename problem, and for a ~30 KB
    static zip the streaming change makes no practical difference. Out of scope.

The download link was agents.php?download=<id>. Clients that name the
saved file from the URL rather than the Content-Disposition header get it
wrong, because the last path segment is agents.php, not the filename:
wget (without --content-disposition) also appends the query string and
saves 'agents.php?download=1', while curl -O drops the query and saves
'agents.php'. Neither yields 'hashtopolis.zip'.

Accept an additional agents.php/download/<id>/<filename> form via
PATH_INFO and build the UI link that way, so the filename is the final
path segment and no query string is present. The existing ?download=<id>
form still works. The <filename> segment is cosmetic for the client; the
served file and its Content-Disposition come from the binary record, as
before.

The path parsing lives in a pure Util::getAgentDownloadId() helper with
PHPUnit coverage in UtilTest, rather than inline in agents.php.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

1 participant