Download plugin: a few observations while aligning with upstream

[Jimmi08]

Hi,

while aligning our download plugin with upstream (checked against aa368363), I noticed a few things. Sharing them as suggestions - please correct me if I misread something.

---

1. Suggestion: require the SEF slug for file requests

Background. On one of our sites downloads are for registered users only. Someone created an account, then used a script to guess download IDs and fetched all files at once. Since then we require the SEF slug together with the ID, and users are used to it. Long time ago, I know that there is count limitation pref now.

Current state. The get route already builds URLs with the slug (download/get/{download_id}/{download_sef}), but the redirect drops it (request.php?id=$1), so request.php serves the file by ID alone. Legacy request.php?123 works the same way.

Idea (maybe as an opt-in pref, to keep old links working):

• pass the slug on: request.php?id=$1&sef=$2
• in request.php, serve the file only when the ID and the stored download_sef match (exact comparison in PHP, not in SQL)
• return the same 404 for a missing row and for a wrong slug, so the response does not reveal which IDs exist

Things we ran into:

• once the slug is part of e_QUERY, the strpos(e_QUERY, "mirror") check and the file-extension regex can match the slug (slugs may contain dots), so they need to be skipped when $_GET['id'] is set
• the router decodes the query with parse_str(), so + in a slug arrives as a space
• {DOWNLOAD_LIST_NAME=request} builds request.php?ID directly instead of e107::url('download', 'get', ...)

We have this marked as LITE modification for now. Nothing else was needed to change for now.

---

2. e_rss.php - undefined $e107

In data(), $e107->base_path is used for link, category_link and enc_url, but $e107 is not defined in that scope. On PHP 8 this gives Attempt to read property "base_path" on null. The links still work, because they end up relative and rss.php prefixes SITEURLBASE.e_PLUGIN_ABS.

enc_url is never printed, since e_rss.php sets enc_type to an empty string and rss.php requires all three enclosure values.

---

3. download_shortcodes.php - author shortcodes (minor)

The same field is read in two different ways:

function sc_download_list_author()   // line 444
{
    return $this->var['download_author'];
}

function sc_download_view_author()   // line 707
{
    return !empty($this->var['download_author']) ? $this->var['download_author'] : "";
}

With the core queries (d.*) the key is always present, so this only warns when the shortcode gets a partial row (e.g. a custom menu or template). sc_download_view_authoremail_lan() and sc_download_view_authorwebsite_lan() (lines 1026, 1032) also have no empty() check.
Just a consistency note.

---

[e107help[bot]]

A defect, a consistency note and a design question, @Jimmi08. Points 2 and 3 are confirmed and going into one fix, master plus a release/v2.3.x twin. Point 1 changes how a file request is authorised, so it is @Deltik's to rule on rather than mine to agree to. I traced all of it at b949097 rather than running the feed, and I am answering out of your order, because the middle one can bite you while you patch it.

2. e_rss.php and the undefined $e107

Confirmed, on master and on release/v2.3.x.

The trap when you fix it locally: do not define $e107. The moment base_path returns / those three strings stop being relative, and rss.php prefixes any link that does not contain http with SITEURLBASE.e_PLUGIN_ABS regardless:

e107/e107_plugins/rss_menu/rss.php

Lines 308 to 318 in b949097

	if($row['link'])
	{
	if(stripos($row['link'], 'http') !== FALSE)
	{
	$this -> rssItems[$k]['link'] = $row['link'];
	}
	else
	{
	$this -> rssItems[$k]['link'] = SITEURLBASE.e_PLUGIN_ABS.$row['link'];
	}
	}

so you would get a doubled path where you had a working link. The fix is to delete the property read and leave the relative string as it stands: identical output, warning gone. That is going into a PR against master with a release/v2.3.x twin.

Two neighbours in the same code path, same PR:

• rss.php appends $row['item_id'] to the enclosure URL, and a v2 data() never sets item_id, so the download feed warns there too:

  e107/e107_plugins/rss_menu/rss.php

  Lines 322 to 325 in b949097

  	if($row['enc_url'])
  	{
  	$this -> rssItems[$k]['enc_url'] = SITEURLBASE.e_PLUGIN_ABS.$row['enc_url'].$row['item_id'];
  	}

• your reading of enc_type is right. The addon sets it to '' and the enclosure element needs all three values, so nothing is ever emitted:

  e107/e107_plugins/rss_menu/rss.php

  Lines 749 to 752 in b949097

  	if($value['enc_url'] && $value['enc_leng'] && $value['enc_type'])
  	{
  	echo "<enclosure url=\"".$value['enc_url']."\" length=\"".$value['enc_leng']."\" type=\"".$value['enc_type']."\" />\n";
  	}

  . Bringing enclosures back needs a real mime lookup ($this->getmime() is commented out in the addon because download_rss has no such method), which is a feature rather than a fix, so it stays out of this one.

3. the author shortcodes

Confirmed, and agreed that with d.* it only shows up on a partial row. I will make the four read the same way in the same PR:

e107/e107_plugins/download/download_shortcodes.php

Lines 444 to 447 in b949097

	function sc_download_list_author()
	{
	return $this->var['download_author'];
	}

joining the shape sc_download_view_author() uses at

e107/e107_plugins/download/download_shortcodes.php

Lines 707 to 710 in b949097

	function sc_download_view_author()
	{
	return !empty($this->var['download_author']) ? $this->var['download_author'] : "";
	}

, and the two _lan ones with it (you pinned their return lines at 1026 and 1032, I am counting from the function lines at 1023 and 1029). Rendered output does not change with the core queries, so there is nothing for a theme to notice.

1. requiring the SEF slug

This one is @Deltik's call, because it is a new pref and a change to how a file request is authorised.

You already named the limits pref, and it is still core's only answer to the incident you describe: a per-class count over a rolling window, checked before the file is served.

Worth being plain about what the slug does and does not add: it is derived from the download name, so any user who can reach the list page can read it, and the script that walked the IDs can walk the list instead. It raises the cost of blind guessing; it is not an access control. With that said, friction against an automated pull is a real thing to want, and an opt-in pref that keeps old links working is the right shape for it if he wants it in core.

Your three snags all check out:

• {DOWNLOAD_LIST_NAME=request} does build the URL by hand:

  e107/e107_plugins/download/download_shortcodes.php

  Lines 404 to 441 in b949097

  	function sc_download_list_name($parm = '')
  	{
  	$tp = e107::getParser();
  	if ($parm == "nolink")
  	{
  	return $tp->toHTML($this->var['download_name'], true, 'LINKTEXT');
  	}
  	if ($parm == "request")
  	{
  	$agreeTextJs = $this->getAgreeTextAsHtmlEventAttribute();
  	if ($this->var['download_mirror_type'])
  	{
  	$href = e_PLUGIN_ABS . "download/download.php?mirror." . $this->var['download_id'];
  	}
  	else
  	{
  	$href = e_PLUGIN_ABS . "download/request.php?" . $this->var['download_id'];
  	}
  	$text = "<a" . $tp->toAttributes([
  	"href" => $href,
  	"onclick" => $agreeTextJs,
  	"title" => defined("LAN_DOWNLOAD") ? LAN_DOWNLOAD : null,
  	]) . ">";
  	$text .= $tp->toHTML($this->var['download_name'], false, 'TITLE') . "</a>";
  	return $text;
  	}
  	$url = e107::url('download', 'item', $this->var);
  	return "<a href='" . $url . "'>" . $tp->toHTML($this->var['download_name'], true, 'LINKTEXT') . "</a>";
  	// return "<a href='".e_PLUGIN_ABS."download/download.php?action=view&id=".$this->var['download_id']."'>".$tp->toHTML($this->var['download_name'],TRUE,'LINKTEXT')."</a>";

  . A hundred lines below it, sc_download_request_url() already goes through the router:

  e107/e107_plugins/download/download_shortcodes.php

  Lines 546 to 550 in b949097

  	function sc_download_request_url($parm = null)
  	{
  	return e107::url('download', 'get', $this->var); // /request/item',array('id'=>$this->var['download_id'], 'name'=>$this->var['download_sef']));
  	}

  and that is what the core download button uses. So the get route is wired up, just not by that parm. Aligning it is a few lines, and it is the piece that would have to move first before a slug-aware route mattered at all. It changes the rendered href, so it waits for @Deltik rather than riding along in a bug-fix PR.
• In request.php, the by-name lookup already stands aside when $_GET['id'] is set (line 88) and the file-extension check is guarded by $resolved (line 202), but the strpos(e_QUERY, "mirror") test is not:

  e107/e107_plugins/download/request.php

  Lines 110 to 114 in b949097

  	if(strpos(e_QUERY, "mirror") !== false)
  	{ // Download from mirror
  	list($action, $download_id, $mirror_id) = explode(".", e_QUERY);
  	$download_id = intval($download_id);
  	$mirror_id = intval($mirror_id);

  . Today that costs a couple of undefined offsets when a file name happens to contain "mirror" and then falls through, which is why nobody has noticed. With a slug in e_QUERY it is your case exactly. !$resolved on that condition goes in the same PR.
• The + arriving as a space is parse_str() doing ordinary query-string decoding, so that one has to be handled where the URL is built rather than where it is read.

I will link the PR here when it is up.

Thanks for writing all three up with the line numbers, it made the trace quick.

[e107help[bot]]

Both PRs are up, @Jimmi08: #6510 against master and #6511 against release/v2.3.x. They carry your points 2 and 3, plus the !$resolved guard on the mirror test from the snags under point 1. Point 1 itself is untouched: nothing in either PR changes how a file request is authorised.

Two things went in beyond what you reported, and they are the two worth knowing if you are patching your own copy:

• The feed builder copies an addon's row key by key, and comment and content_encoded were not among the keys it copied. The news addon has been building a comment URL for every commentable item and having it dropped on the way through. Both are copied now, and every item leaves the constructor carrying the keys the four writers read, so an addon that leaves one of them out no longer puts a warning in the middle of a body already sent as application/xml.
• request.php?mirror and request.php?mirror.5 reach a three-element list() over a shorter array. What that costs today is two undefined-offset warnings and a pointless query for download id 0 before the request falls through, so it is a diagnostics fix and not a hole; it needs no login only because nothing behind it does anything. Padded.

One thing I could not fold in: the download feed has never emitted an <enclosure> at all. Your reading was right, and it needs a real MIME lookup to fix, which is a feature rather than a warning fix, so it is written up separately as #6509 for @Deltik to decide rather than smuggled into a bug-fix PR. The commented-out line that hinted at it is deleted in #6510, so the issue is now the only record of it.

If you take the e_rss.php change into your fork, take the relative strings rather than a defined $e107, for the reason in my last post: the builder prefixes anything without a scheme, so an absolute string there gets the site path twice.

[Jimmi08]

1. requiring the SEF slug

• it doesn't have to be SEF-URL (easy to get from the title, you are right), but any string - hash of title + salt string set in preferences. I would suggest site_hash as the salt, but it is known on the public site from the code.

[e107help[bot]]

The salt wants to be random rather than derived from anything, @Jimmi08. On a 2.3.4 tree that means minting one once from a CSPRNG, keeping it in a pref of your own, and never printing it anywhere: a value nobody can recompute, whatever else they can read off the page.

Your instinct about site_hash is right, and it is worse than "known from the code". It is ten characters of an md5 over the database name and the table prefix:

e107/e107_handlers/e107_class.php

Lines 686 to 692 in b949097

	/**
	* Create a unique hash for each database configuration (multi-site support).
	*/
	function makeSiteHash($db, $prefix) // also used by install.
	{
	return substr(md5($db . "." . $prefix), 0, 10);
	}

Both inputs are guessable, so that table can be generated offline without reading the site at all. And the same string is the media directory name, so every avatar and every image on a public page spells it out anyway:

e107/e107_handlers/e107_class.php

Lines 727 to 730 in b949097

	// $this->site_path is appended to MEDIA_DIRECTORY in defaultDirs(), which is called above.
	if(strpos($this->e107_dirs['MEDIA_DIRECTORY'],$this->site_path) === false)
	{
	$this->e107_dirs['MEDIA_DIRECTORY'] .= $this->site_path."/"; // multisite support.

Master has since grown the same thing, for whenever you track it. The secret is a core pref, token_secret, 128 hexadecimal characters:

e107/e107_handlers/Security/SealedToken.php

Lines 79 to 87 in b949097

	/**
	* Core preference holding the master secret.
	*/
	const PREF_SECRET = 'token_secret';
	/**
	* Length of that preference in hexadecimal characters.
	*/
	const SECRET_LENGTH = 128;

and a caller never touches it directly. e107::getSealedToken('download') hands back a handler whose key is derived from that secret with HKDF-SHA512 under a purpose string, so a value minted for downloads cannot be opened as anything else:

e107/e107_handlers/Security/SealedToken.php

Lines 440 to 459 in b949097

	protected function key()
	{
	if($this->key !== null)
	{
	return $this->key;
	}
	$secret = $this->secret();
	if(!self::isSecret($secret))
	{
	return false;
	}
	$key = Hkdf::derive(
	self::HKDF_HASH,
	pack('H*', $secret),
	AesCbcHmacSha2::keyLength(Compact::ENC),
	self::KEY_INFO . $this->purpose
	);

e107_handlers/Security/ does not exist on release/v2.3.x at all, which is why a 2.3.4 tree wants a secret of its own rather than a backport of that.

Two shapes, if @Deltik does want this in core. A keyed digest of the id, truncated, gives a stable link that survives a bookmark and needs no new column. A sealed token gives a link that expires and can be tied to the user, at the price of a long URL and a dead link an hour later. The first is the one your users would not notice; the second is the one that limits what a harvested link is worth.

[e107help[bot]]

Ahoj @Jimmi08, a correction to something I told you in this thread. In my post announcing the PRs I said the news feed had been building a comments link for every item open to comments. It was the other way round: it built the link only for items closed to comments, and those links lead to the home page, because comment.php shows comments only for an item that is open to them. The news_allow_comments column is backwards (1 means closed), and the feed read it forwards. Nothing showed it until #6510 and #6511 started printing the link, and no release has printed it yet.

The fix is up: #6802 against master and #6803 against release/v2.3.x. If you took the <comments> change from #6510 or #6511 into your fork, take this pair's whole change to e107_plugins/news/e_rss.php with it. It is three lines, and this is the one that turns the link the right way round:

if(empty($value['news_allow_comments']) && $pref['comments_disabled'] != 1)