A Lookup Answer Could Name Any Record - edmozley/freeitsm GitHub Wiki

Security: a lookup answer could name any record

Fixed in 4.6.1 · Found in a code review while designing financial approvals · Present in every release from 1.0.0 to 4.6.0 · Modules: Forms, Self-service portal

The short version

A lookup question lets somebody pick a record - an asset, say - instead of typing its name. In the portal, the search behind that question was careful: it only offered records from the requester's own company, and only if the form builder had opened that question to customers.

The checks sat on the search. A request is sent by a separate submit, and a crafted submit never goes through the search. The submit checked that the record existed, but not the rest. So a signed-in portal user who tampered with their request could:

name a record from another company on a multi-company install;
name a record the question should not have offered, including from a question not opened to customers, or from the staff directory, which is never open to customers;
change the name stored with their answer, so a request for one thing could read as another to whoever approved it. This one works on a single-company install too.

Who could do it: a signed-in portal user, on a form with a lookup question that they can open. They could not read any data this way: the post carried the name they had typed themselves, so no record's real name came back to them. The most they could learn was whether a given record number existed, because a number that does not exist was refused.

Upgrading is the fix. Nothing else needs doing.

What was wrong

The submit asked one question about a lookup answer: is this record inside the caller's scope?

if (!self::lookupValueAllowed($conn, $source, $lid, $ctx->companyScope)) {

For a member of staff that is right, because their context holds the companies they can reach. A portal context has no company scope, so companyScope is null, and for lookupValueAllowed() null means every company. The comment beside the line said so, as a feature: null there already means "every company". It is true of a background job and wrong for a requester.

The other two problems came from the same assumption, that an answer reaching the submit had come through the search:

The portal gates were not repeated. lookupPortalAllowed() ran in api/forms/lookup_search.php only.
The label was stored as posted. A lookup answer is an id and a label. The id was checked; the label went straight into the stored answer, and the label is what an approver reads.

The fix

All in the lookup branch of FormsService::submitForm():

if ($portalUserId !== null && !self::lookupPortalAllowed($field)) {
    throw new ServiceError(...);
}
$scope = $portalUserId !== null ? self::lookupPortalScope($conn, $portalUserId) : $ctx->companyScope;
$recordLabel = self::lookupValueAllowed($conn, $source, $lid, $scope)
    ? self::lookupRecordLabel($conn, $source, $lid)
    : null;

The scope comes from the requester. lookupPortalScope() returns their own company, never null. It is the same call the search now makes, so the two cannot disagree. A portal user with no company falls back to the Default company rather than to unrestricted.
The portal gates run at submit, as well as at search.
The label comes from the record. lookupRecordLabel() reads the record's own name, and the posted label is ignored. A record with no name is refused, because the search would never have offered it.

Staff submits behave as before, apart from the stored label, which now comes from the record for them too.

📁 Files changed

	File	Change
🟦	`includes/services/forms.php`	`lookupPortalScope()`, `lookupRecordLabel()`, and the checks in `submitForm()`; a `TRAP:` comment on the null scope
🟩	`api/forms/lookup_search.php`	uses `lookupPortalScope()` instead of its own copy of the logic
🟪	`tests/forms-lookup-portal-submit.php`	new, 15 checks through the real submit path

Tested

Reproduced first. On a test install, the unfixed code accepted another company's record, a field not opened to customers, the staff directory and a forged label.
tests/forms-lookup-portal-submit.php passes all 15 checks on the fix. Its controls prove a refusal means refused and not broken: a requester's own record is accepted, and a member of staff keeps every company they can reach.
tests/forms-logic/run.php (85), tests/catalogue-group-approvals.php (38) and tests/task-forms.php (87) pass.

Failures not caused by this fix: tests/forms-lookup/run.php reports its AI whitelist and prompt as missing the grid and image field types, and tests/catalogue-approvals-grid.php fails "table's answers reach the card" twice. Both fail identically without the fix.

Do I need to do anything?

Upgrade. Nothing to configure. Requests already in the system are not changed, so an answer stored before the upgrade keeps the label it was stored with.

For developers

A check on a search protects the search. Whatever a picker refuses to offer, the submit has to refuse too, because a post does not have to come from the picker.
null scope means every company. A portal context carries null, so never pass $ctx->companyScope for a requester; use lookupPortalScope().
Never store a name the client sent when the server can read it. Whoever reads the stored answer trusts it.