Found some Bugs and fixed it..
- Thread starter ichbinz1337
- Start date
Bug report — v6.1.0: uploads are stored on file servers marked "Read Only" or "Disabled"
Product: Yetishare File Hosting Script Version: 6.1.0 (also present in 5.3.1) Severity: Medium to High — the server status setting is silently ignored; in some configurations files are also recorded against the wrong server, which breaks their downloads.
Summary
Two code paths assign an upload to a file server without checking the server's status. Every regular branch of the server selection correctly filters statusId = 2 (active) — these two shortcuts do not.
As a result, a server set to Read Only or Disabled in the admin area can still receive and be assigned new uploads.
Issue 1 — uploadServerOverride ignores the server status
app/helpers/FileServerHelper.class.php, getAvailableServerPoolIds(), lines 25–38:
if (($Auth->loggedIn() === true) && (int)$Auth->user->uploadServerOverride) {
$uploadServerOverride = (int) $db->getValue('SELECT uploadServerOverride '
. 'FROM users WHERE id = :id LIMIT 1', ['id' => $Auth->id]);
if ($uploadServerOverride) {
return [
$uploadServerOverride, // <-- returned without any status check
];
}
}
A user pinned to a specific server via Edit User → Upload Server Override keeps uploading to that server after it has been set to Read Only or Disabled.
Issue 2 — the fallback in _storeFile() ignores the server status
app/services/Uploader.class.php, _storeFile() (starts line 816), lines 836–861:
// if this is a 'direct' server, and it's active, use it
$uploadServerDetails = FileHelper::getCurrentServerDetails();
if ($uploadServerDetails['serverType'] === 'direct' && (int)$uploadServerDetails['statusId'] === 2) {
$uploadServerId = $uploadServerDetails['id']; // correct: status IS checked
}
if ($uploadServerId === null) {
$uploadServerId = FileServerHelper::getAvailableServerId(false); // excludes direct servers
}
$uploadServerDetails = $db->getRow('SELECT * FROM file_server WHERE id = :id',
['id' => (int) $uploadServerId]);
if (!$uploadServerDetails) {
// if we failed to load any server, fallback on the current server
$uploadServerDetails = FileHelper::getCurrentServerDetails();
$uploadServerId = $uploadServerDetails['id']; // <-- no status check here
}
Note that the server is resolved when the upload finishes, not when it starts.
Why this fires routinely, not rarely
The pool call passes false, which adds AND serverType != "direct" to the query. On any installation whose storage is exclusively made up of direct servers — a common setup — that query can never return one of them. If the remaining non-direct servers (typically a single "Local Default") are not active, the pool returns false, the lookup on id = 0 finds nothing, and the fallback runs.
On our installation this is the permanent state: 11 direct servers plus one local server that is disabled. Every upload completing on a Read Only direct server therefore takes the fallback path and is stored there.
Two different outcomes
Steps to reproduce (Issue 2)
The same happens for any client that keeps using a previously obtained upload endpoint, for example a long-running bulk uploader — which is how we first noticed this. One account had accumulated files on six different Read Only servers over a long period, and the behaviour was never explainable from the admin UI.
Suggested fixes
app/helpers/FileServerHelper.class.php
if ($uploadServerOverride) {
- return [
- $uploadServerOverride,
- ];
+ // Only honour the override when that server actually accepts
+ // uploads; otherwise fall through to normal pool selection.
+ $overrideAcceptsUploads = (int) $db->getValue('SELECT COUNT(*) '
+ . 'FROM file_server '
+ . 'WHERE id = :id AND statusId = 2 '
+ . 'LIMIT 1', [
+ 'id' => $uploadServerOverride,
+ ]);
+ if ($overrideAcceptsUploads) {
+ return [
+ $uploadServerOverride,
+ ];
+ }
}
app/services/Uploader.class.php
_storeFile() reports errors via $fileUpload->error and return $fileUpload;, so the failure can be surfaced the same way:
if (!$uploadServerDetails) {
- // if we failed to load any server, fallback on the current server
- $uploadServerDetails = FileHelper::getCurrentServerDetails();
- $uploadServerId = $uploadServerDetails['id'];
+ // Fall back on the receiving server only when it actually accepts
+ // uploads. Otherwise the file would either be recorded against a
+ // Read Only / Disabled server, or - worse - against a different
+ // server than the one holding the bytes, breaking its download.
+ $currentServerDetails = FileHelper::getCurrentServerDetails();
+ if ((int) $currentServerDetails['statusId'] !== 2) {
+ $fileUpload->error = TranslateHelper::t(
+ 'classuploader_no_upload_server_available',
+ 'No file server is currently available to accept uploads. Please try again later.'
+ );
+ return $fileUpload;
+ }
+ $uploadServerDetails = $currentServerDetails;
+ $uploadServerId = $uploadServerDetails['id'];
}
This changes the behaviour from "store it somewhere" to "fail the upload", which we believe is correct, but it is a behavioural change and worth your judgement. A new translation key would be needed, otherwise the English default text is used.
Related observation (not a bug, but worth knowing)
The accountUploadTypes filter is applied only in the pool query, never in the direct-server shortcut. We used that deliberately during maintenance: setting accountUploadTypes to a value matching no account type removes a server from new upload selection, while uploads already in flight still complete correctly on it, because statusId stays 2.
That worked well and was in fact the only safe way we found to drain a server. It does, however, illustrate that the two paths have drifted apart — one honours the filter and the status, the other honours neither.
If the fixes above are applied, a documented "drain" mechanism would be a useful addition: a way to stop new uploads being directed to a server while letting in-flight ones finish and be recorded correctly.
Environment
Product: Yetishare File Hosting Script Version: 6.1.0 (also present in 5.3.1) Severity: Medium to High — the server status setting is silently ignored; in some configurations files are also recorded against the wrong server, which breaks their downloads.
Summary
Two code paths assign an upload to a file server without checking the server's status. Every regular branch of the server selection correctly filters statusId = 2 (active) — these two shortcuts do not.
As a result, a server set to Read Only or Disabled in the admin area can still receive and be assigned new uploads.
Issue 1 — uploadServerOverride ignores the server status
app/helpers/FileServerHelper.class.php, getAvailableServerPoolIds(), lines 25–38:
if (($Auth->loggedIn() === true) && (int)$Auth->user->uploadServerOverride) {
$uploadServerOverride = (int) $db->getValue('SELECT uploadServerOverride '
. 'FROM users WHERE id = :id LIMIT 1', ['id' => $Auth->id]);
if ($uploadServerOverride) {
return [
$uploadServerOverride, // <-- returned without any status check
];
}
}
A user pinned to a specific server via Edit User → Upload Server Override keeps uploading to that server after it has been set to Read Only or Disabled.
Issue 2 — the fallback in _storeFile() ignores the server status
app/services/Uploader.class.php, _storeFile() (starts line 816), lines 836–861:
// if this is a 'direct' server, and it's active, use it
$uploadServerDetails = FileHelper::getCurrentServerDetails();
if ($uploadServerDetails['serverType'] === 'direct' && (int)$uploadServerDetails['statusId'] === 2) {
$uploadServerId = $uploadServerDetails['id']; // correct: status IS checked
}
if ($uploadServerId === null) {
$uploadServerId = FileServerHelper::getAvailableServerId(false); // excludes direct servers
}
$uploadServerDetails = $db->getRow('SELECT * FROM file_server WHERE id = :id',
['id' => (int) $uploadServerId]);
if (!$uploadServerDetails) {
// if we failed to load any server, fallback on the current server
$uploadServerDetails = FileHelper::getCurrentServerDetails();
$uploadServerId = $uploadServerDetails['id']; // <-- no status check here
}
Note that the server is resolved when the upload finishes, not when it starts.
Why this fires routinely, not rarely
The pool call passes false, which adds AND serverType != "direct" to the query. On any installation whose storage is exclusively made up of direct servers — a common setup — that query can never return one of them. If the remaining non-direct servers (typically a single "Local Default") are not active, the pool returns false, the lookup on id = 0 finds nothing, and the fallback runs.
On our installation this is the permanent state: 11 direct servers plus one local server that is disabled. Every upload completing on a Read Only direct server therefore takes the fallback path and is stored there.
Two different outcomes
- Pool returns false (our case): the file is recorded against the server that physically received it. The data stays consistent, but the Read Only / Disabled setting is silently ignored — an administrator taking a server out of rotation has no way to tell it is still being written to.
- Pool returns a different, valid server (installations with an active non-direct server): the bytes are written to the receiving server, but the database records a different server. The file then cannot be downloaded, because the application looks for it in the wrong place.
Steps to reproduce (Issue 2)
- Set up at least one direct file server and make sure no non-direct server is active.
- Start a large upload to that server, so the browser holds its upload endpoint.
- While the upload is running, set the server to Read Only in the admin area.
- Let the upload finish.
The same happens for any client that keeps using a previously obtained upload endpoint, for example a long-running bulk uploader — which is how we first noticed this. One account had accumulated files on six different Read Only servers over a long period, and the behaviour was never explainable from the admin UI.
Suggested fixes
app/helpers/FileServerHelper.class.php
if ($uploadServerOverride) {
- return [
- $uploadServerOverride,
- ];
+ // Only honour the override when that server actually accepts
+ // uploads; otherwise fall through to normal pool selection.
+ $overrideAcceptsUploads = (int) $db->getValue('SELECT COUNT(*) '
+ . 'FROM file_server '
+ . 'WHERE id = :id AND statusId = 2 '
+ . 'LIMIT 1', [
+ 'id' => $uploadServerOverride,
+ ]);
+ if ($overrideAcceptsUploads) {
+ return [
+ $uploadServerOverride,
+ ];
+ }
}
app/services/Uploader.class.php
_storeFile() reports errors via $fileUpload->error and return $fileUpload;, so the failure can be surfaced the same way:
if (!$uploadServerDetails) {
- // if we failed to load any server, fallback on the current server
- $uploadServerDetails = FileHelper::getCurrentServerDetails();
- $uploadServerId = $uploadServerDetails['id'];
+ // Fall back on the receiving server only when it actually accepts
+ // uploads. Otherwise the file would either be recorded against a
+ // Read Only / Disabled server, or - worse - against a different
+ // server than the one holding the bytes, breaking its download.
+ $currentServerDetails = FileHelper::getCurrentServerDetails();
+ if ((int) $currentServerDetails['statusId'] !== 2) {
+ $fileUpload->error = TranslateHelper::t(
+ 'classuploader_no_upload_server_available',
+ 'No file server is currently available to accept uploads. Please try again later.'
+ );
+ return $fileUpload;
+ }
+ $uploadServerDetails = $currentServerDetails;
+ $uploadServerId = $uploadServerDetails['id'];
}
This changes the behaviour from "store it somewhere" to "fail the upload", which we believe is correct, but it is a behavioural change and worth your judgement. A new translation key would be needed, otherwise the English default text is used.
Related observation (not a bug, but worth knowing)
The accountUploadTypes filter is applied only in the pool query, never in the direct-server shortcut. We used that deliberately during maintenance: setting accountUploadTypes to a value matching no account type removes a server from new upload selection, while uploads already in flight still complete correctly on it, because statusId stays 2.
That worked well and was in fact the only safe way we found to drain a server. It does, however, illustrate that the two paths have drifted apart — one honours the filter and the status, the other honours neither.
If the fixes above are applied, a documented "drain" mechanism would be a useful addition: a way to stop new uploads being directed to a server while letting in-flight ones finish and be recorded correctly.
Environment
- Yetishare 6.1.0, upgrade from 5.3.1 tested against a copy of our production database
- 11 direct file servers, 1 local server (disabled)
- Server selection method: Least Used Space
- No geo or account-type restrictions configured on any server
- PHP 8.2, MariaDB 10.3
Bug report — v6.1.0: opening and saving a user in the admin destroys lifetime accounts
Product: Yetishare File Hosting Script Version: 6.1.0 Severity: High — silent data loss on paid customer accountsRegression: yes, does not occur in v5.3.1
Summary
In the admin area, opening Users → Edit User for a paid account that never expires (lifetime) pre-fills the Paid Expiry Date field with today + 1 year. Saving the form — even without changing anything — writes that date to the database and clears the lifetime flag.
The account silently changes from "never expires" to "expires in 1 year". There is no warning and no visible indication that anything changed.
On our installation this puts 835 lifetime accounts at risk. Any routine admin edit (changing an email address, a storage limit, resetting 2FA) is enough to trigger it.
Steps to reproduce
Expected
The field should stay empty and the account should remain lifetime. The field's own help text states: "Leave empty to never expire the account."
Cause
app/views/admin/user_edit.html.twig
checkExpiryDate() is called on page load (line 17, inside the $(function () { ... }) block):
js
AdminForm.initPasswordToggle('[data-password-toggle]');
checkExpiryDate(); // <-- runs on every page load
});
function checkExpiryDate()
{
var levelType = $('#account_type option:selected').data('level-type');
if (levelType === 'paid') {
$('.paid_account_expiry').show();
if (!$('#expiry_date').val()) {
$('#expiry_date').val('{{ defaultExpiryDate }}'); // <-- fills it in
}
} else {
...
defaultExpiryDate is date('d/m/Y', strtotime('+1 year'))(app/controllers/admin/UserController.class.php:565).
The if (!$('#expiry_date').val()) guard was presumably meant to protect existing values, but an empty field is exactly how a lifetime account is represented — so the guard passes precisely in the case that must be left alone.
The controller itself is correct. UserController.class.php:221 renders an empty string when paidExpiryDate is NULL. The value is injected client side after the page has rendered.
On save, UserController.class.php:393 then does:
php
$user->never_expire = ($dbExpiryDate === null && $userLevel->level_type === 'paid') ? 1 : 0;
Since a date is now posted, $dbExpiryDate is no longer null and never_expire is set to 0.
Why this is a regression
In v5.3.1 the same function exists, but it is only called from the account type dropdown's onChange handler — never on page load. Opening and saving a lifetime account was therefore safe.
Suggested fix
Give checkExpiryDate() a parameter so the default is only applied when the admin actively switches the account type, not when the page loads.
diff
--- a/app/views/admin/user_edit.html.twig
+++ b/app/views/admin/user_edit.html.twig
@@
AdminForm.initPasswordToggle('[data-password-toggle]');
- checkExpiryDate();
+ // On page load only show/hide the field - never pre-fill it. An empty
+ // expiry date means "never expires" (lifetime), so pre-filling here
+ // would silently turn a lifetime account into a 1 year account as
+ // soon as the form is saved.
+ checkExpiryDate(false);
});
- function checkExpiryDate()
+ function checkExpiryDate(fillDefault)
{
var levelType = $('#account_type option:selected').data('level-type');
if (levelType === 'paid') {
$('.paid_account_expiry').show();
- if (!$('#expiry_date').val()) {
+ if (fillDefault !== false && !$('#expiry_date').val()) {
$('#expiry_date').val('{{ defaultExpiryDate }}');
}
} else {
@@
- onchange="checkExpiryDate();"
+ onchange="checkExpiryDate(true);"
We have applied this locally and verified it:
Note on user_add.html.twig
The same construct exists in app/views/admin/user_add.html.twig. There the pre-fill is correct behaviour for a new account, so no change is needed — but you may want to keep both files consistent.
Suggested additional hardening
Consider making "never expires" an explicit checkbox bound to never_expire, rather than encoding it as an empty text field. The current representation makes this class of bug easy to reintroduce, and it is invisible to the admin until a customer complains.
Environment
Product: Yetishare File Hosting Script Version: 6.1.0 Severity: High — silent data loss on paid customer accountsRegression: yes, does not occur in v5.3.1
Summary
In the admin area, opening Users → Edit User for a paid account that never expires (lifetime) pre-fills the Paid Expiry Date field with today + 1 year. Saving the form — even without changing anything — writes that date to the database and clears the lifetime flag.
The account silently changes from "never expires" to "expires in 1 year". There is no warning and no visible indication that anything changed.
On our installation this puts 835 lifetime accounts at risk. Any routine admin edit (changing an email address, a storage limit, resetting 2FA) is enough to trigger it.
Steps to reproduce
- Take a user with level_type = 'paid', never_expire = 1 and paidExpiryDate = NULL.
- Open /admin/user-edit/<id>.
- Observe that Paid Expiry Date is pre-filled with today + 1 year, although the database value is NULL.
- Change nothing. Click Update User.
- Check the database.
| Field | Before | After |
|---|---|---|
| users.never_expire | 1 | 0 |
| users.paidExpiryDate | NULL | <today + 1 year> |
The field should stay empty and the account should remain lifetime. The field's own help text states: "Leave empty to never expire the account."
Cause
app/views/admin/user_edit.html.twig
checkExpiryDate() is called on page load (line 17, inside the $(function () { ... }) block):
js
AdminForm.initPasswordToggle('[data-password-toggle]');
checkExpiryDate(); // <-- runs on every page load
});
function checkExpiryDate()
{
var levelType = $('#account_type option:selected').data('level-type');
if (levelType === 'paid') {
$('.paid_account_expiry').show();
if (!$('#expiry_date').val()) {
$('#expiry_date').val('{{ defaultExpiryDate }}'); // <-- fills it in
}
} else {
...
defaultExpiryDate is date('d/m/Y', strtotime('+1 year'))(app/controllers/admin/UserController.class.php:565).
The if (!$('#expiry_date').val()) guard was presumably meant to protect existing values, but an empty field is exactly how a lifetime account is represented — so the guard passes precisely in the case that must be left alone.
The controller itself is correct. UserController.class.php:221 renders an empty string when paidExpiryDate is NULL. The value is injected client side after the page has rendered.
On save, UserController.class.php:393 then does:
php
$user->never_expire = ($dbExpiryDate === null && $userLevel->level_type === 'paid') ? 1 : 0;
Since a date is now posted, $dbExpiryDate is no longer null and never_expire is set to 0.
Why this is a regression
In v5.3.1 the same function exists, but it is only called from the account type dropdown's onChange handler — never on page load. Opening and saving a lifetime account was therefore safe.
Suggested fix
Give checkExpiryDate() a parameter so the default is only applied when the admin actively switches the account type, not when the page loads.
diff
--- a/app/views/admin/user_edit.html.twig
+++ b/app/views/admin/user_edit.html.twig
@@
AdminForm.initPasswordToggle('[data-password-toggle]');
- checkExpiryDate();
+ // On page load only show/hide the field - never pre-fill it. An empty
+ // expiry date means "never expires" (lifetime), so pre-filling here
+ // would silently turn a lifetime account into a 1 year account as
+ // soon as the form is saved.
+ checkExpiryDate(false);
});
- function checkExpiryDate()
+ function checkExpiryDate(fillDefault)
{
var levelType = $('#account_type option:selected').data('level-type');
if (levelType === 'paid') {
$('.paid_account_expiry').show();
- if (!$('#expiry_date').val()) {
+ if (fillDefault !== false && !$('#expiry_date').val()) {
$('#expiry_date').val('{{ defaultExpiryDate }}');
}
} else {
@@
- onchange="checkExpiryDate();"
+ onchange="checkExpiryDate(true);"
We have applied this locally and verified it:
| Test | Result |
|---|---|
| Lifetime account, open + save unchanged | stays never_expire=1, paidExpiryDate=NULL |
| Paid account with an expiry date, open + save unchanged | date preserved |
| Free → Paid via the dropdown | still pre-fills today + 1 year |
| Paid → Free via the dropdown | field cleared and hidden |
The same construct exists in app/views/admin/user_add.html.twig. There the pre-fill is correct behaviour for a new account, so no change is needed — but you may want to keep both files consistent.
Suggested additional hardening
Consider making "never expires" an explicit checkbox bound to never_expire, rather than encoding it as an empty text field. The current representation makes this class of bug easy to reintroduce, and it is invisible to the admin until a customer complains.
Environment
- Yetishare 6.1.0, upgraded from 5.3.1 via the supplied upgrade SQL (v5.4.0 → v5.4.1 → v5.5.0 → v5.6.0 → v6.0.0 → v6.1.0), all applied without errors
- Nova theme
- PHP 8.1, MariaDB
- Verified against a copy of our production database (~206,000 users, 835 of them lifetime paid accounts)