Page MenuHomePhorge

Fix important regression in search engine
ClosedPublic

Authored by valerio.bozzolan on Mar 25 2024, 09:08.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 10:00
Unknown Object (File)
Wed, May 1, 19:22
Unknown Object (File)
Wed, May 1, 19:22
Unknown Object (File)
Wed, May 1, 19:22
Unknown Object (File)
Wed, May 1, 19:22
Unknown Object (File)
Wed, May 1, 00:21
Unknown Object (File)
Sun, Apr 28, 08:48
Unknown Object (File)
Thu, Apr 25, 19:22
Tokens
"Mountain of Wealth" token, awarded by 20after4.

Details

Summary

Interestingly Phorge allows to paste a Phorge URL in the search bar to be redirected there...

Now this interesting feature was crashing the whole search engine for simple queries like "asdasd"
with the following exception message:

Refusing to redirect to local resource "asdasd". This URI is not formatted in a recognizable way.

You are affected by this crash after this commit:

328aee033fbdc704620e2facae4aa68b836217bb

This specific commit is probably legitimate since "#asdasd" and "/asdasd" are indeed internal URIs,
plus, yes, there are some undocumented and untested cases like "asdasd" that may be considered
internal URIs as well. Or not. But this is just an undocumented corner-case.

In short:

PhabricatorDatasourceURIEngineExtension should not be built over undocumented corner-cases and
should require an absolute URI, with at least a domain.

Note that PhutilURI#getDomain() is always a string and never null, so we can do a strict check.

This fix also involves a micro-optimization to avoid duplicate URI parsing. Since the method was calling
the constructor PhutilURI(string) twice (line 13, line 24). Now just once.

Closes T15763

Test Plan

Search "lol", no crash anymore.

Copy a random Phorge URL and paste that in your search bar. It still redirects there.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

speck added inline comments.
src/applications/meta/engineextension/PhabricatorDatasourceURIEngineExtension.php
16

Could this be combined with the next if statement, inverted? Looks like if the next if statement fails then it also returns null, and combining would simplify the logic here.

I prefer to invert the logic and return early, to avoid having the entire function contents inside a nested level - up to you but this looks good.

This revision is now accepted and ready to land.Mar 25 2024, 17:24

swap cases - thanks @speck - please double-accept if this is what was suggested :D