Page MenuHomePhorge

Fix PHP 8.1 "rawurlencode(null)" exception which blocks rendering a project page
ClosedPublic

Authored by aklapper on May 1 2023, 13:28.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 7, 13:52
Unknown Object (File)
Wed, Apr 3, 10:35
Unknown Object (File)
Mon, Apr 1, 05:39
Unknown Object (File)
Mon, Apr 1, 01:30
Unknown Object (File)
Sun, Mar 31, 19:42
Unknown Object (File)
Sun, Mar 31, 19:11
Unknown Object (File)
Fri, Mar 29, 21:13
Unknown Object (File)
Fri, Mar 29, 18:01
Tokens
"Like" token, awarded by valerio.bozzolan.

Details

Summary

After PHP 8.1 the function rawurlencode() does not accept anymore the null value.

Thus return an empty string when the input parameter is null instead of passing the input parameter to rawurlencode().

Closes T15263

Test Plan

Applied this change on top of D25144, D25145, D25146, D25147, D25151,
D25152, D25153 and D25163 and already existing Workboard located at
/project/view/1/ finally rendered in web browser.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aklapper requested review of this revision.May 1 2023, 13:28
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan added inline comments.
src/utils/utils.php
1890–1895

Since this is a low-level function it's probably better to just check the exact null value and exclude that.

Also, since this function always returned a string, it's probably better to just return a string.

This revision now requires changes to proceed.May 1 2023, 13:54

I would like to see a full stack trace for this thought, since the function is annotated to only receive a string, and null is not a string, so we should not pass null to it if possible.

I would like to see a full stack trace for this thought, since the function is annotated to only receive a string, and null is not a string, so we should not pass null to it if possible.

Makes sense! (This ticket was filed before I managed to get good traces.) Stacktrace added to T15263.

Since my inline edit is probably too much confusing,

In short I just suggest this short-circuit that replicates the behavior before PHP 8.1:

function phutil_escape_uri($string) {
+ if($string === null) {
+   return '';
+ }
  return str_replace('%2F', '/', rawurlencode($string));
}

If you want easy extra points, maybe this can be even more nice, using phlog():

function phutil_escape_uri($string) {
+ if($string === null) {
+   phlog("Received phutil_escape_uri(NULL). Please share this stack trace in https://we.phorge.it/T15263");
+   return '';
+ }
  return str_replace('%2F', '/', rawurlencode($string));
}

Since my inline edit is probably too much confusing,

In short I just suggest this short-circuit that replicates the behavior before PHP 8.1:

function phutil_escape_uri($string) {
+ if($string === null) {
+   return '';
+ }
  return str_replace('%2F', '/', rawurlencode($string));
}

If you want easy extra points, maybe this can be even more nice, using phlog():

function phutil_escape_uri($string) {
+ if($string === null) {
+   phlog("Received phutil_escape_uri(NULL). Please share this stack trace in https://we.phorge.it/T15263");
+   return '';
+ }
  return str_replace('%2F', '/', rawurlencode($string));
}

Also in this patch @aklapper feel free to update this, to maintain authorship

Or, feel free to say "yeah whatever, just update this with the proposed fix". In that case I will probably also "Commander" the revision in order to be the author since my "tip" here is too much invasive compared with the original (but in this case I will not be able to approve myself and see this landed quickly)

If you want easy extra points, maybe this can be even more nice, using phlog():

As I reported in T15263, $string can be null and in my understanding being null is a valid situation. Thus I would not add phlog.

valerio.bozzolan added a subscriber: avivey.

Thanks aklapper for this change!

$string can be null and in my understanding being null is a valid situation.

Yep I 100% agree with your assertion but I mean that some code may use this part in "unexpected ways", for example passing a PhutilURI object or something else.

So,

I accept this as myself since I honestly think this is 100% OK, as this 100% replicates the pre-deprecation expectations.

but, I cannot accept as behalf of O1 since very probably @avivey would like to suggest to put a nice phlog() (ihih) here to monitor incoming unexpected objects (like is happening in D25157).

@valerio.bozzolan: Ah, thanks. If there is an assumption that string passed to phutil_escape_uri($string) shall never ever be null and in that case there is always an underlying bug to fix (that wasn't clear to me), then I'd say adding a phlog() makes sense if it automagically includes a stacktrace and has clear instructions what a random Phorge admin seeing that log entry is supposed to do (like "Unexpected null string passed to phutil_escape_uri. Please report a bug at https://we.phorge.it/maniphest/ and mention T15263 in your task.") Okay.

Thanks for this patch and your semplification

Since this does not use phutil_nonempty_string() or phutil_nonempty_stringlike(), since this is just a reasonable strict NULL check, since this is the very minimal fix required for PHP 8.1 deprecation, since this has 100% backward compatibility, and since I tested this locally without nuclear implosions, even if it would be nice to understand who is calling this with NULL values, I think this is really too much to ask here from this low-level function used here and there.

Any person that would like to improve the calls is free to do that, with follow-up patches.

In the meanwhile, thanks.

Green light.

lgtm

This revision is now accepted and ready to land.May 3 2023, 13:33