This change prevents a malformed filename from breaking things.
Due to the code in AphrontHTTPSink->writeHeaders.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers
If you attach a file to a maniphest task, and that file has a
newline or null in the filename, you get the following error
when attempting to download the file:
"Declining to emit response with unsafe HTTP header"
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- FixFilenameHeader
- Lint
Lint Errors Severity Location Code Message Error src/applications/diffusion/controller/DiffusionServeController.php:1116 PHL1 Unknown Symbol Error src/applications/diffusion/controller/DiffusionServeController.php:1118 PHL1 Unknown Symbol Error src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php:26 PHL1 Unknown Symbol Error src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php:26 PHL1 Unknown Symbol Error src/applications/nuance/cursor/NuanceGitHubImportCursor.php:83 PHL1 Unknown Symbol Error src/applications/nuance/cursor/NuanceGitHubImportCursor.php:201 PHL1 Unknown Symbol Error src/applications/nuance/cursor/NuanceGitHubRepositoryImportCursor.php:28 PHL1 Unknown Symbol Error src/applications/nuance/github/NuanceGitHubRawEvent.php:100 PHL1 Unknown Symbol Error src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php:51 PHL1 Unknown Symbol Error src/applications/transactions/editengine/PhabricatorEditEngine.php:1421 PHL1 Unknown Symbol Error src/infrastructure/cluster/PhabricatorDatabaseRef.php:296 PHL1 Unknown Symbol Error src/view/layout/PHUICurtainPanelView.php:27 PHL1 Unknown Symbol Error src/view/layout/PHUICurtainView.php:57 PHL1 Unknown Symbol - Unit
Tests Passed - Build Status
Buildable 12039 Build 15158: Run Core Tests Build 15157: arc lint + arc unit
Event Timeline
Comment Actions
Here's some effort to muddle through the RFC's on this:
I think this is defined here:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec19.html#sec19.5.1
And "quoted-string" is defined here:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2
And that basically says that escape sequences like \n and \r and such don't mean anything special: for example, you can not encode newline with \n or \x0A or anything fancy like that.
So the "correct" way to emit this header for "file\nwith\newlines" (with newlines) appears (???) to be:
Content-Disposition: attachment; filename="file with newlines"
I trust approximately zero software in existence to parse that according to my reading of the spec since the defacto way you separate HTTP headers is by exploding them on newlines, so simply stripping these characters out seems like the most reasonable approach to me.
Technically, I think we're supposed to put a backslash before all octets in the range 0-31 and 127, too, but it's hard to imagine anything parsing that wrong.
Comment Actions
rfc2616 is obsolete. You'll want to look at RFC7230 (low level details) and RFC7231 (headers).
Comment Actions
Ah, thanks. RFC7230 appears to allow these characters to appear verbatim (the omission of %x22 is double quote, and %x5C is backslash):
%x09 (tab) / %x20 (space) / %x21 / %x23-5B / %x5D-7E / %x80-%xFF
And these to appear prefixed by backslash:
%x09 (tab) / %x20 (space) / %x21-7E / %x80-%xFF
So it looks like it's impossible (?) to encode \x00-\x08, \x0A-\x1F, and \x7F, and we should presumably strip all of these, then quote " and \\ (or vice-versa, quote/strip order shouldn't matter since the two sets are non-overlapping).
The most correct change is probably:
// Strip characters forbidden by RFC7230. $filename = preg_replace('/[\x00-\x08\x0A-\x1F\x7f]/', '', $filename); $filename = addcslashes($filename, '"\\');
But with testing, since I could easily have that preg_replace() syntax wrong.