* FIX Drag and drop of a file reports a wrong error, or none at all
The error handler of dragAndDropFileUpload() had three defects that all end on
the user believing the file was attached when it was not.
- The first assignment of the message was dead code, there was no return after
the test on the http code 403, so a refusal was reported as a generic error.
- The key ErrorUploadPermissionDenied it used exists in no language file, so
the raw key was shown.
- JSON.parse() was called with no try/catch on an answer that is not always a
json, a fatal error of the endpoint or a request over post_max_size for
example. The exception left the user on a page with no message at all.
- An empty list of files was treated as a success, while it means the endpoint
stored nothing.
The value of PHP_SELF is also escaped before it is written into the 6 generated
javascript strings. It holds the path info of the request on a server that
accepts it, so it is a user input. dol_escape_js() is called with the mode that
escapes a double quote by a double quote, the strings being delimited by double
quotes, and the sequence '</' is escaped too because the function does not do it
and the path info could otherwise close the script tag and open one of its own.
Adds the key ErrorOnAtLeastOneFileUpload to en_US, used when some files of a
batch failed and some did not.
* FIX getMultidirOutput returns a directory the Documents tab does not read
Four defects of the same function, all ending on a file stored where the user
will never see it, or written outside the documents directory.
- The ref of the project of a task was only passed through dol_sanitizePathName(),
which keeps a slash, a colon and the accented chars, while projet/tasks/document.php
sanitizes it with dol_sanitizeFileName(). A project ref holding a slash even
created an extra level of directory. Measured on real databases: 304 projects
over 330 hold a slash on one of them, carrying 701 tasks over 824.
- The same case calls $object->fetchProject() with no guard, while the signature
of the function accepts an object that is not a CommonObject, and even a null
when a module is given. Such a caller gets a fatal error where it expects the
error string. No core caller is in that case today, an external module or a
hook can be.
- The entity of the object may have no declared directory, an object shared by
another entity for example. The undefined index returned a relative path, so
the caller read or wrote under the web root. The current entity is used
instead, and the fallback is logged because the directory is then not the one
of the entity of the object, which matters for a caller that deletes files.
- When the current entity has no declared directory either, the fallback
returned the sub directory alone, again a relative path. The same error than
for a module that declares no directory at all is now returned.
Adds the sub directory of a partnership and of a stock transfer, which their own
document tabs already read.
* FIX getElementProperties answers wrong properties for 9 elements
A customer payment, a supplier payment, a various payment, a stock transfer and
the 4 objects of the hrm module had no properties at all, or wrong ones, so any
caller that resolves a class, a table or a document directory from an element
failed on them.
- payment, payment_supplier and payment_various had no branch. The branch of a
customer payment tests $elementType and not $element, because the rule on the
elements named myobject_mysubobject rewrites $element to 'payment' for
'payment_salary' too, which is stored somewhere else.
- job, position, skill and evaluation answered a wrong table (hrm_job_user for
a position, and so on) and no sub directory, while their document tabs read
one named after the element.
- stocktransfer answered an empty classname, because it is not the ucfirst() of
the element, so a caller doing new $classname($db) ended on a fatal error.
- The sub directory was concatenated even when the module is disabled and the
directory is empty, which answered a path at the root of the file system.
- A contact and a conference are stored into a sub directory their tab reads.
isModEnabled('invoice') is tested for a customer payment: there is no module
named 'compta', so testing it was always false, while $conf->compta->payment is
set unconditionally by Conf::setValues() and could not be used as a proxy.
* FIX Access refused to everyone on 11 objects of the core
restrictedArea() and checkUserAccessToObject() refuse the access to objects that
no permission and no rule can match, whatever the user, an administrator
included.
- The hrm module declares no permission at its first level, only 'all', and the
stocktransfer module only 'stocktransfer'. A check on the module itself
therefore tests a permission that does not exist. The mapping is the same one
as into User::hasRight().
- The module of an event organization declares no permission at all, its whole
permission block being commented out on purpose, and its cards check the
parent project instead. The feature is mapped onto that project, with the two
guards the card has: an external user is refused, and so is a conference with
no parent project, whose id of 0 would otherwise grant an access with no check
on the record at all.
- The default rule of checkUserAccessToObject() builds its sql on the columns
entity and fk_soc of the table. llx_asset, llx_paiement, llx_paiementfourn and
llx_workstation_workstation have no fk_soc, and llx_hrm_job, llx_hrm_job_user
and llx_hrm_skill have neither. The sql failed, so the access was refused to
every user this rule applies to. These tables are now checked on their entity
only, which is what the $check rule already does for the same class of tables,
and the 3 tables of hrm can be checked on nothing at all. The rule is selected
on the table and not on the element, because $object is an id and not an
object for most of the callers, the cards of an asset and of a workstation
included, which are broken today for any user without the permission to see
all third parties.
- An external user is refused explicitly on those tables: none of these objects
is linked to a third party, so the default rule refused him through a link
that does not exist, and the rules that replace it do not look at the third
party of the user at all.
Measured on a vanilla instance with 5 profiles, an administrator, an internal
user with every right, one without the permission to see all third parties, one
that is not a sales representative of the third party of the object, and an
external user: the 11 objects go from refused to granted for the internal users
and stay refused for the external one, and the 26 other elements answer exactly
the same for the 5 profiles.
* FIX A file dropped on a card is lost, or reported as refused when it was stored
FileUpload stores the file into a directory that the "Attached files" tab of the
object never reads, so the user attaches a file that no screen will ever show,
and nothing is indexed in database to find it back. Measured on real databases:
216487 thirdparties over 216887 and 157852 products over 280319 are in that case
on the cards that already enable the drag and drop.
- The directory of the object is now forged with get_exdir(), the way the tabs
do: it always uses the id for a thirdparty, whose ref is a company name and is
not unique, and it falls back on the id when the ref is empty. The sub
directory of the module is read with getMultidirOutput(), which knows the
elements that store their documents into one. That function does not return
an empty string when it fails but a string starting with 'error-', so only an
absolute path is accepted: writing into that string would create the files
under the web root.
- fetchObjectByElement() returns an object even when fetch() returned 0. The
object was then not loaded, and the file was stored at the root of the
directory of the module, out of any object. The constructor now throws, and
the endpoint answers the error with the same json contract than a successful
call so that the caller can show it, instead of a fatal error and an http 500.
- An attachment of the same name was silently overwritten, dol_move_uploaded_file()
being called with $allowoverwrite = 1 while the name was checked before the
ref of the object was added as a prefix. The check is done again on the final
name, and on the .noexe suffixed name too, which that function appends to an
executable file.
- An executable file was renamed with that .noexe suffix and then reported as an
error, while it was correctly stored.
- The endpoint called restrictedArea() with an empty feature when the element is
unknown, and the loop of that function then takes no branch at all and grants
the access with no check. It refuses before, with the same http code and the
same message than a refusal, so that a user cannot tell an object that exists
but is not allowed from an object that does not exist. The refusal of an
external user on an object of another third party answered a message of its
own, which allowed the same enumeration.
Comes with the tests of the path resolution, of the fallback of get_exdir(), of
the rejection of the error string of getMultidirOutput(), and of the file name
deduplication.
* FIX Remove the drop area from 13 cards where dropping a file is harmful
Two distinct groups, both of them removing a drop area that only produces a
result the user does not want.
7 pages have no "Attached files" tab at all, so a file dropped on them can be
reached by no screen: a fiscal year and its info page, an accounting model, a
webhook trigger history, an intracomm report, a bookcal calendar and its booking
list. Measured: 4 of them accepted the upload and wrote an orphan file, the
accounting model wrote it at the root of the directory of the module, out of any
object, and the 2 bookcal ones answered an http 500.
6 cards print their tabs inside their edit form, so the drop area covered that
form: a product, an expense report, a VAT payment, a social contribution, a loan
and a salary. Dropping a file there reloads the page, which discards what the
user is typing. A product and an expense report have a second call for the read
only view, so the drop area is only removed from the call of the edit branch;
the 4 others share a single call between both views, so the parameter is now
conditional.
* NEW Drag and drop a file on the 23 remaining cards
Every card of the core that owns an "Attached files" tab can now receive a file
by drag and drop, which was the case of 32 of them only. The 23 added here are
an asset, an event, a various payment, a customer payment, a contact, a donation,
a conference or booth, a shipment, a supplier payment, a leave request, an
evaluation, a job, a position, a skill, a knowledge record, a manufacturing
order, a partnership, a lot, a stock transfer, a task, a reception, a resource
and a workstation.
The tab bar of a leave request and of a resource is printed inside their edit
form, so the drop area is not enabled there: dropping a file reloads the page,
which would discard what the user is typing. The card of a supplier payment
prints its tab bar even when the object was not loaded, so the drop area is only
enabled when it is.
* FIX getMultidirOutput refuses an entity with no directory instead of falling back [skip-claudemd]
The previous revision of this PR fell back on the directory of the current
entity when the entity of the object had none, with a LOG_WARNING. On a
multicompany install that made a caller read, write and above all delete
files in the directory of another entity. Refuse instead: the function
already answers 'error-diroutput-not-defined-for-this-object' when the
module declares no directory at all, so the caller has one behaviour to
handle, not two.
The entity is cast to int, which is what the array index needs and what
silences the four PhanTypeMismatchDimFetchNullable this function reported.
---------
Co-authored-by: Laurent Destailleur <eldy@destailleur.fr>
Continuing the audit of commonly-used but untested functions in
functions.lib.php.
- testDolPrintSize: the 10240-byte threshold that decides whether
$shortvalue actually converts to Kilobytes, and the short vs long
unit label.
- testDolGetFirstLastname: all 6 documented $nameorder values (0-5),
including the "if defined else the other" fallback behavior of
3 and 5, and the -1 auto mode.
- testJsonOrUnserialize: valid JSON (object and array forms), a
legacy PHP-serialized string falling back to unserialize(), and a
string that is neither (returns false; the resulting unserialize()
warning is expected and suppressed in the test).
- testPictoFromLangcode: empty input, the 'auto' special case, a
hardcoded special-cased language code (fr_CA), the generic
'xx_YY' -> country-part flag resolution, a bare country code, the
$notitlealt flag, and how a 'class="..."' in $moreatt merges into
the span's own class instead of becoming a separate attribute.
All expected values were verified empirically against the real
function outputs before being written into the test.
getElementProperties() is the central registry used across the
codebase (fetchObjectByElement(), generic links/extrafields, document
generation...) to resolve an element type string into its module,
classpath, classfile, classname, table and parent element, but it had
no test coverage despite being a ~480 line function with about 60
special-case branches plus several generic fallback paths.
Covers the representative paths rather than every branch:
- the generic completion of classfile/classname from subelement
(project)
- the 'myobject@mymodule' external-module syntax, including the
surprising fact that table_element keeps the raw '@'-string
- the 'myobject_mysubobject' syntax combined with a dedicated case
branch overriding module (project_task)
- the generic '...det' object-line fallback for an unknown module,
including the non-capitalized classname it produces (myobjectdet)
- a real '...det' case where the generic fallback runs first and a
dedicated branch only adds parent_element on top (contratdet)
- a real '...det' case where the dedicated branch instead overrides
classpath and classname set by the generic fallback (facturedet)
- the action/actioncomm special case, where table_element differs
from the raw input element
These are two of the most heavily used display functions in the whole
codebase (every page showing an amount or a date calls one of them),
yet neither had any test coverage.
testPrice covers: default rounding (min of MAIN_MAX_DECIMALS_UNIT/TOT),
keeping extra decimals present in the input, en_US vs fr_FR separators,
HTML mode ( ), international separators via $outlangs='none',
forcerounding (explicit decimal count and 'MU'), and currency symbol
placement (before for USD, after for EUR).
testDolPrintDate covers: empty input returning '', timestamp 0 being a
valid date, the language-independent format shortcuts (dayrfc,
standard, dayhourrfc, dayhourlog), a literal strftime-style format
string, and tzoutput=true (GMT) vs tzoutput=false (server TZ, forced
to UTC for the comparison) producing the same result.
testIsValidMXRecord() does a real live DNS lookup via
isValidMXRecord(), which is inherently CI-environment dependent. In a
recent Travis run, the assertion for usace.army.mil (result expected
1) failed while the other three domains in the same test resolved
correctly, indicating the .mil zone specifically was filtered/
unreachable from that CI network rather than any bug in
isValidMXRecord() itself - .mil DNS is a known target for network
filtering on shared CI infrastructure.
Replace it with microsoft.com, which has the same "domain with a
valid MX record" semantics but a far more globally reachable, stable
DNS setup less likely to be blocked by CI network policies.
The substr offsets in dol_print_phone did not line up with the segment
lengths, so formatted numbers repeated one digit and lost another.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
get_default_tva() VATRULE 2 selects the VAT rate attached to the buyer
department (c_tva.fk_department_buyer) but filters only on the department id.
Unlike get_product_vat_for_country(), it omits t.active and t.entity, so a
disabled rate (or a rate from another entity) can be returned as the default,
and because of the taux DESC ordering an inactive higher rate wins over the
active one.
Add "AND t.active > 0" and "AND t.entity IN (getEntity('c_tva'))" to the query.
The ordering is unchanged, so the documented behaviour is preserved: the rate
flagged Default (use_default) wins, otherwise the highest active rate.
Add testGetDefaultTvaForBuyerState() covering VATRULE 2, which was previously
untested: it asserts the active department rate is selected and the inactive
one is ignored.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* Qual: Add testcase for price2num to convert '21.500,00' in FR
* NEW: Accept numbers like 1.213,00 in locales with whitespace/empty thousands separator
# NEW: Accept numbers like 1.213,00 in locales with whitespace/empty thousands separator
- Graceful handling of comma and dot as decimal points and thousands separators in the `price2num` function.
- Improved the `getNonEmptyString` function to include proper indentation and spacing (automatic).
* Add testcases for price2num
* en_US tests for invalid prices, add doc for price2num regex
* Restore lost test to validate compatibility
* Update FunctionsLibTest.php
---------
Co-authored-by: Laurent Destailleur <eldy@destailleur.fr>
