Page MenuHomePhorge
Differential D25418

Catch RuntimeException: mb_convert_encoding(): Illegal character encoding specified at PhabricatorTextDocumentEngine.php:73
ClosedPublic
Actions

Authored by aklapper on Aug 22 2023, 17:24.
Tags
None
Referenced Files
F2690076: D25418.1734743445.diff
Fri, Dec 20, 01:10
F2689292: D25418.1734731603.diff
Thu, Dec 19, 21:53
F2682982: D25418.1734675897.diff
Thu, Dec 19, 06:24
F2682980: D25418.1734675819.diff
Thu, Dec 19, 06:23
F2682860: D25418.1734674558.diff
Thu, Dec 19, 06:02
F399763: image.png
Nov 16 2023, 14:29
Subscribers
Cigaryno
Matthew
Sten
tobiaswiese
valerio.bozzolan

Details

Reviewers
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15624: RuntimeException: mb_convert_encoding(): Illegal character encoding specified at PhabricatorTextDocumentEngine.php:73
Commits
rPfe1122bd4d6d: Catch RuntimeException: mb_convert_encoding(): Illegal character encoding…
Summary

When given $encoding is invalid, catch the exception to show a proper error message and make the server logs provide more hints.

EXCEPTION: (RuntimeException) mb_convert_encoding(): Illegal character encoding specified at [<arcanist>/src/error/PhutilErrorHandler.php:261]
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
#1 <#2> mb_convert_encoding(string, string, string) called at [<phabricator>/src/applications/files/document/PhabricatorTextDocumentEngine.php:73]

Closes T15624

Test Plan

Open a URL which passes a bogus encoding value as parameter, like /source/somerepository/browse/master/README.md?as=source&encode=TROLOLOL

Diff Detail

Repository
rP Phorge
Branch
T15624charenc (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 802
Build 802: arc lint + arc unit

Event Timeline

aklapper created this revision.Aug 22 2023, 17:24
Owners added a reviewer: O1: Blessed Committers.Aug 22 2023, 17:24
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptAug 22 2023, 17:24
aklapper requested review of this revision.Aug 22 2023, 17:24
Harbormaster completed remote builds in B801: Diff 1325.Aug 22 2023, 17:24
aklapper added a comment.Aug 22 2023, 17:25

Please do note that this patch is untested as I am unaware of reproduction steps.

aklapper updated this revision to Diff 1326.Aug 22 2023, 17:27

fix a typo

Harbormaster completed remote builds in B802: Diff 1326.Aug 22 2023, 17:27
Sten subscribed.Sep 8 2023, 08:35
Sten added inline comments.
src/applications/files/document/PhabricatorTextDocumentEngine.php
79

We get $message, but don't make use of it. Shouldn't we include $message in the log rather than. assuming it's an Illegal character encoding?

Also, should we set $this->encodingMessage to the error message?

valerio.bozzolan added a comment.Nov 14 2023, 08:27

(I share the same question from Sten)

aklapper updated this revision to Diff 1506.Nov 14 2023, 10:34

Good points. This patch puts also the exception message into phlog.

Harbormaster completed remote builds in B929: Diff 1506.Nov 14 2023, 10:34
valerio.bozzolan added a comment.EditedNov 14 2023, 11:12

OK the situation is a bit more clear now.

We can maybe limit the Test Plan to people requesting nonsense encoding. Example:

/source/myrepo/browse/master/README.md?as=source&encode=TROLOLOL

And, I totally agree that the try ... catch would be useful, but we can introduce something like this:

$this->encodingMessage = pht(
  'Unable to convert from the requested encoding to UTF8.');

(Note that this message does not mention the weird input parameter by design, so we can avoid to generate silly pages if the user visits the page with TROLOLOL as encoding etc.)

And, at this point, since we probably triaged the root problem, and since the error is already supposed to be reported to the corresponding user, and since system administrators already have the access logs to see weird URLs like these, feel free to evaluate whenever you want to suppress the new phlog() call to just keep the user message. Probably in your use case you want less logs to be inspected, so feel free to omit, but it's up to you.

valerio.bozzolan added a comment.Nov 14 2023, 11:14

(I will immediately hard-approve this lovely change as soon as we share whatever ACK like "Yeah let's log the planet" or "Yeah let's just show a user message")

aklapper edited the test plan for this revision. (Show Details)Nov 16 2023, 10:48
aklapper marked an inline comment as done.Nov 16 2023, 10:53
aklapper updated this revision to Diff 1527.Nov 16 2023, 11:01

Thanks for the additional debugging and finding steps to reproduce! In this case, I'd prefer not to log the issue and to only show a message to the user explaining what's the issue, including their requested bogus encoding.

Harbormaster completed remote builds in B946: Diff 1527.Nov 16 2023, 11:01
valerio.bozzolan accepted this revision as: valerio.bozzolan.EditedNov 16 2023, 14:29

Side note, we are introducing the possibility to share this kind of very creative and confusing URLs (that are safe from the point of view of XSS but) that could be an attracting point for lamers to generate confusing user messages inside Phorge itself, like:

/source/reponame/browse/master/README.md?as=source&encode=AHAHAH%20YOU%20ARE%20A%20LUSER%20AHAHAHAH%20%3Cscript%3Ealert(1)%3C/script%3E%20AHAHAHHAHAHA

image.png (60×594 px, 8 KB)

I'm totally not aware of potential consequences, I'm totally not aware of similar cases. I'm just somehow sure that this is somehow safe, so, I soft-approve since this works. Thanks!

Let's wait for additional eyes for extra feedback. I will hard-approve this in a week approx if we receive just silence.

(I can also surely hard-approve if we remove that user-generated output since that indeed would be 100000% blindly super-safe).

valerio.bozzolan added a comment.EditedNov 20 2023, 14:42

Interestingly, Gerrit does similar things, and does not mention the problematic URI value in case it's somehow already clear:

https://gerrit.wikimedia.org/g/phabricator/translations?format=TEXT

The specified format type is not supported

valerio.bozzolan accepted this revision.Nov 23 2023, 14:47

Nobody seems interested in my concern, so, hard approve :) Thanks

This revision is now accepted and ready to land.Nov 23 2023, 14:47
Closed by commit rPfe1122bd4d6d: Catch RuntimeException: mb_convert_encoding(): Illegal character encoding…. · Explain WhyDec 17 2023, 10:42
This revision was automatically updated to reflect the committed changes.
aklapper added a commit: rPfe1122bd4d6d: Catch RuntimeException: mb_convert_encoding(): Illegal character encoding….

Revision Contents
Changeset List

PathSizePackages
src/
applications/
files/
document/
16 linesO1: Blessed Committers

Diff 1326

View Options

src/applications/files/document/PhabricatorTextDocumentEngine.php

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0