Fix PHP 8.1 "base64_decode(null)" exception calling Conduit's file.upload with no data passed
ClosedPublic
Actions

Authored by aklapper on May 29 2023, 11:31.

Details

Reviewers

speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15426: PHP 8.1 "base64_decode(null)" exception calling Conduit's file.upload with no data passed

Commits

rP108cbcd09bd3: Fix PHP 8.1 "base64_decode(null)" exception calling Conduit's file.upload with…

Summary

Since PHP 8.1, base64_decode() does not accept passing null as a parameter. Thus first check that data !== null before calling decodeBase64($data) (which then calls PHP's base64_decode()), and throw an exception if it is.

EXCEPTION: (RuntimeException) base64_decode(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=18554ea76ceb), phorge(head=conduitDashboardPanelEdit, ref.master=0d81da590923, ref.conduitDashboardPanelEdit=ab4391b30465)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
  #1 <#2> base64_decode(NULL, boolean) called at [<phorge>/src/applications/files/conduit/FileConduitAPIMethod.php:84]

Closes T15426

Test Plan

Applied this change; afterwards /api/file.upload under "Method Result", "error_info" shows "Unable to decode base64 data!" instead of "base64_decode(): Passing null to parameter #1 ($string) of type string is deprecated" which is more descriptive.

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25258

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 534
Build 534: arc lint + arc unit

Event Timeline

aklapper created this revision.May 29 2023, 11:31

Owners added a reviewer: O1: Blessed Committers.May 29 2023, 11:31

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 29 2023, 11:31

aklapper requested review of this revision.May 29 2023, 11:31

Harbormaster completed remote builds in B477: Diff 836.May 29 2023, 11:31

aklapper edited the summary of this revision. (Show Details)May 30 2023, 14:57

speck requested changes to this revision.Jun 8 2023, 00:49

speck added inline comments.

src/applications/files/conduit/FileConduitAPIMethod.php
84–86 ↗	(On Diff #836)	The offending caller should not be passing null to this method. Instead of this change update `FileUploadConduitAPIMethod::execute()` to check for a null argument, similar to how the other arguments are validated before proceeding

This revision now requires changes to proceed.Jun 8 2023, 00:49

Instead of this change update FileUploadConduitAPIMethod::execute() to check for a null argument, similar to how the other arguments are validated before proceeding

Just to make sure we're talking about the same thing: If I added

if ($data !== null) {
      $data = $this->decodeBase64($data);
}

in FileUploadConduitAPIMethod::execute() similar to how the other arguments are validated before proceeding, that would trigger an exception a few lines later in $file = PhabricatorFile::newFromFileData($data, $params) passing $data being null:
EXCEPTION: (RuntimeException) hash(): Passing null to parameter #2 ($data) of type string is deprecated

So I'd rather throw the exception error message defined in FileConduitAPIMethod.php::decodeBase64() one call earlier in FileUploadConduitAPIMethod::execute() by adding a check if ($data === null).

Harbormaster completed remote builds in B529: Diff 919.Jun 10 2023, 15:37

aklapper edited the summary of this revision. (Show Details)Jun 10 2023, 15:40

aklapper edited the test plan for this revision. (Show Details)

Correct, this change is what I was suggesting and not trying to continue if there’s no file data. If someone is calling the api to upload a file but doesn’t give any file data that’s a user/caller error and the server-side api execution has no sensible path forward.

src/applications/files/conduit/FileUploadConduitAPIMethod.php
35	Could you update this to better indicate the user/caller error? Field “data_base64” must be non-empty. Or similar

speck added inline comments.Jun 10 2023, 19:43

src/applications/files/conduit/FileUploadConduitAPIMethod.php
34	This is probably fine to use phutil non empty string, or also check if strlen. This exception should be hit for both cases of the field being omitted and also if the field is present but has the value of empty string.