From b8bb4d3ad590a2b01c135302df145d50bc048765 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 16 Feb 2018 06:26:02 -0800 Subject: [PATCH] Accept either "[[ %24doge ]]" or "[[ $doge ]]" as references to the "/w/$doge/" Phriction document Summary: Depends on D19105. Ref T13077. Fixes T12344. The `[[ ... ]]` syntax accepts and handles characters which would require URL encoding if they appeared in URIs. For example, `[[ 100% Natural Cheese Dust ]]` is a legitimate, supported piece of remarkup syntax, and does not need to be written as `... 100%25 Natural ...`. Likewise, `[[ BUY $DOGE ]]` is legitimate and does not need to be written as `[[ BUY %24DOGE ]]`. This piece of syntax creates a link to `/w/buy_$doge/`. This may or may not appear in your browser's URL bar as `/w/buy_%24doge/`, but internally "$" is a valid slug character and you'll see `buy_$doge` over Conduit, etc. However, since users may reasonably copy paths from their browser URL bar, they may have unnecessary URL encoding. The syntax `[[ buy_$doge ]]` is legitimate, but a user copy/pasting may write `[[ buy_%24doge ]]` instead. Currently, this extra URL encoding causes links to break, since `[[ buy_%24doge ]]` is a treated as link to `/w/buy_24doge/`, just like `[[ Fresh 100%AB Blood ]]` is a link to `/w/fresh_100_ab_blood/`. To fix this: - When the target for a link can be URL decoded, try to do lookups on both the un-decoded and decoded variations. - If the un-decoded variation works, great: use it. This preserves behavior for all existing, working links. - If the un-decoded variation fails but the decoded variation works, okay: we'll assume you copy-pasted a URL-encoded version and strip URL encoding. - If both fail, same behavior as before. Also, use a different spelling for "existent". See T13084 for some "attacks" based on this behavior. I think the usability affordance this behavior provides greatly outweighs the very mild threat those attacks represent. Test Plan: - Created links to existing, nonexisting, and existing-but-not-visible documents, all of which worked normally. - Created links to `[[ $doge ]]` and `[[ %24doge ]]`, saw them both go to the right place. - Performed the "attacks" in T13084. Maniphest Tasks: T13077, T12344 Differential Revision: https://secure.phabricator.com/D19106 --- .../markup/PhrictionRemarkupRule.php | 105 +++++++++++++++--- 1 file changed, 88 insertions(+), 17 deletions(-) diff --git a/src/applications/phriction/markup/PhrictionRemarkupRule.php b/src/applications/phriction/markup/PhrictionRemarkupRule.php index c4bc1f7249..a6362d1624 100644 --- a/src/applications/phriction/markup/PhrictionRemarkupRule.php +++ b/src/applications/phriction/markup/PhrictionRemarkupRule.php @@ -83,28 +83,97 @@ final class PhrictionRemarkupRule extends PhutilRemarkupRule { return; } + $viewer = $engine->getConfig('viewer'); + $slugs = ipull($metadata, 'link'); - foreach ($slugs as $key => $slug) { - $slugs[$key] = PhabricatorSlug::normalize($slug); + + $load_map = array(); + foreach ($slugs as $key => $raw_slug) { + $lookup = PhabricatorSlug::normalize($raw_slug); + $load_map[$lookup][] = $key; + + // Also try to lookup the slug with URL decoding applied. The right + // way to link to a page titled "$cash" is to write "[[ $cash ]]" (and + // not the URL encoded form "[[ %24cash ]]"), but users may reasonably + // have copied URL encoded variations out of their browser location + // bar or be skeptical that "[[ $cash ]]" will actually work. + + $lookup = phutil_unescape_uri_path_component($raw_slug); + $lookup = phutil_utf8ize($lookup); + $lookup = PhabricatorSlug::normalize($lookup); + $load_map[$lookup][] = $key; } - // We have to make two queries here to distinguish between - // documents the user can't see, and documents that don't - // exist. $visible_documents = id(new PhrictionDocumentQuery()) - ->setViewer($engine->getConfig('viewer')) - ->withSlugs($slugs) + ->setViewer($viewer) + ->withSlugs(array_keys($load_map)) ->needContent(true) ->execute(); - $existant_documents = id(new PhrictionDocumentQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withSlugs($slugs) - ->execute(); - $visible_documents = mpull($visible_documents, null, 'getSlug'); - $existant_documents = mpull($existant_documents, null, 'getSlug'); + $document_map = array(); + foreach ($load_map as $lookup => $keys) { + $visible = idx($visible_documents, $lookup); + if (!$visible) { + continue; + } - foreach ($metadata as $spec) { + foreach ($keys as $key) { + $document_map[$key] = array( + 'visible' => true, + 'document' => $visible, + ); + } + + unset($load_map[$lookup]); + } + + // For each document we found, remove all remaining requests for it from + // the load map. If we remove all requests for a slug, remove the slug. + // This stops us from doing unnecessary lookups on alternate names for + // documents we already found. + foreach ($load_map as $lookup => $keys) { + foreach ($keys as $lookup_key => $key) { + if (isset($document_map[$key])) { + unset($keys[$lookup_key]); + } + } + + if (!$keys) { + unset($load_map[$lookup]); + continue; + } + + $load_map[$lookup] = $keys; + } + + + // If we still have links we haven't found a document for, do another + // query with the omnipotent viewer so we can distinguish between pages + // which do not exist and pages which exist but which the viewer does not + // have permission to see. + if ($load_map) { + $existent_documents = id(new PhrictionDocumentQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withSlugs(array_keys($load_map)) + ->execute(); + $existent_documents = mpull($existent_documents, null, 'getSlug'); + + foreach ($load_map as $lookup => $keys) { + $existent = idx($existent_documents, $lookup); + if (!$existent) { + continue; + } + + foreach ($keys as $key) { + $document_map[$key] = array( + 'visible' => false, + 'document' => null, + ); + } + } + } + + foreach ($metadata as $key => $spec) { $link = $spec['link']; $slug = PhabricatorSlug::normalize($link); $name = $spec['explicitName']; @@ -114,14 +183,16 @@ final class PhrictionRemarkupRule extends PhutilRemarkupRule { // in text as: "Title" . Otherwise, we'll just render: . $is_interesting_name = (bool)strlen($name); - if (idx($existant_documents, $slug) === null) { + $target = idx($document_map, $key, null); + + if ($target === null) { // The target document doesn't exist. if ($name === null) { $name = explode('/', trim($link, '/')); $name = end($name); } $class = 'phriction-link-missing'; - } else if (idx($visible_documents, $slug) === null) { + } else if (!$target['visible']) { // The document exists, but the user can't see it. if ($name === null) { $name = explode('/', trim($link, '/')); @@ -131,7 +202,7 @@ final class PhrictionRemarkupRule extends PhutilRemarkupRule { } else { if ($name === null) { // Use the title of the document if no name is set. - $name = $visible_documents[$slug] + $name = $target['document'] ->getContent() ->getTitle();