Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use to toMap() in Link::jsonSerialize() could lead to data disclosure #208

Closed
emteknetnz opened this issue Feb 7, 2024 · 0 comments
Closed
Assignees

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Feb 7, 2024

The following code exists in Link::jsonSerialize()

        // TODO: this could lead to data disclosure - we should only return the fields that are actually needed
        $data = $this->toMap();

The code is called as part of LinkFieldController::linkData()

Data disclosure could happen when custom links or extensions to existing links add data columns that are not supposed to be shown in the CMS

Instead of using the 'include everything' approach, we should have an explicit list of fields returned. The list should be extensible.

Acceptance criteria

  • Review the usage of Link::jsonSerialize() and confirm what its used for and if it's returning fields that aren't needed.
  • Re-factor jsonSerialize and/or related method to only returned needed fields.
  • If there's an inherent residual security risk for custom link types, that risk is called out explicitly in the documentation.
  • TODO comment is removed from codebase.
  • Consider adding an extension hooks if there's a probably use case for it.

Affected code snippet

public function jsonSerialize(): mixed
{
$typeKey = LinkTypeService::create()->keyByClassName(static::class);
if (!$typeKey) {
return [];
}
// TODO: this could lead to data disclosure - we should only return the fields that are actually needed
$data = $this->toMap();
$data['typeKey'] = $typeKey;
unset($data['ClassName']);
unset($data['RecordClassName']);
$data['Title'] = $this->getTitle();
return $data;
}

Extra context

  • Being able to JSON serialise Links used to be a big deal because all the data was being passed via JSON string. It's a much smaller concern now.
  • Link used to extend the JsonSerializable interface. JsonSerializable::jsonSerialize() allows you to customise how an object gets represented when json_encode is called on it.
  • I would strongly suggest either:
    • Reimplementing the JsonSerializable interface if there's still a need to be able to json serialise Links ... and using json_encode instead of directly calling jsonSerialize directly.
    • Rename jsonSerialize to a more descriptive name if it's not actually serializing the Link.

PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants