recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

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

epriestley b8bb4d3a bfdc9411

+88 -17
+88 -17
src/applications/phriction/markup/PhrictionRemarkupRule.php
··· 83 83 return; 84 84 } 85 85 86 + $viewer = $engine->getConfig('viewer'); 87 + 86 88 $slugs = ipull($metadata, 'link'); 87 - foreach ($slugs as $key => $slug) { 88 - $slugs[$key] = PhabricatorSlug::normalize($slug); 89 + 90 + $load_map = array(); 91 + foreach ($slugs as $key => $raw_slug) { 92 + $lookup = PhabricatorSlug::normalize($raw_slug); 93 + $load_map[$lookup][] = $key; 94 + 95 + // Also try to lookup the slug with URL decoding applied. The right 96 + // way to link to a page titled "$cash" is to write "[[ $cash ]]" (and 97 + // not the URL encoded form "[[ %24cash ]]"), but users may reasonably 98 + // have copied URL encoded variations out of their browser location 99 + // bar or be skeptical that "[[ $cash ]]" will actually work. 100 + 101 + $lookup = phutil_unescape_uri_path_component($raw_slug); 102 + $lookup = phutil_utf8ize($lookup); 103 + $lookup = PhabricatorSlug::normalize($lookup); 104 + $load_map[$lookup][] = $key; 89 105 } 90 106 91 - // We have to make two queries here to distinguish between 92 - // documents the user can't see, and documents that don't 93 - // exist. 94 107 $visible_documents = id(new PhrictionDocumentQuery()) 95 - ->setViewer($engine->getConfig('viewer')) 96 - ->withSlugs($slugs) 108 + ->setViewer($viewer) 109 + ->withSlugs(array_keys($load_map)) 97 110 ->needContent(true) 98 111 ->execute(); 99 - $existant_documents = id(new PhrictionDocumentQuery()) 100 - ->setViewer(PhabricatorUser::getOmnipotentUser()) 101 - ->withSlugs($slugs) 102 - ->execute(); 112 + $visible_documents = mpull($visible_documents, null, 'getSlug'); 113 + $document_map = array(); 114 + foreach ($load_map as $lookup => $keys) { 115 + $visible = idx($visible_documents, $lookup); 116 + if (!$visible) { 117 + continue; 118 + } 119 + 120 + foreach ($keys as $key) { 121 + $document_map[$key] = array( 122 + 'visible' => true, 123 + 'document' => $visible, 124 + ); 125 + } 126 + 127 + unset($load_map[$lookup]); 128 + } 129 + 130 + // For each document we found, remove all remaining requests for it from 131 + // the load map. If we remove all requests for a slug, remove the slug. 132 + // This stops us from doing unnecessary lookups on alternate names for 133 + // documents we already found. 134 + foreach ($load_map as $lookup => $keys) { 135 + foreach ($keys as $lookup_key => $key) { 136 + if (isset($document_map[$key])) { 137 + unset($keys[$lookup_key]); 138 + } 139 + } 103 140 104 - $visible_documents = mpull($visible_documents, null, 'getSlug'); 105 - $existant_documents = mpull($existant_documents, null, 'getSlug'); 141 + if (!$keys) { 142 + unset($load_map[$lookup]); 143 + continue; 144 + } 106 145 107 - foreach ($metadata as $spec) { 146 + $load_map[$lookup] = $keys; 147 + } 148 + 149 + 150 + // If we still have links we haven't found a document for, do another 151 + // query with the omnipotent viewer so we can distinguish between pages 152 + // which do not exist and pages which exist but which the viewer does not 153 + // have permission to see. 154 + if ($load_map) { 155 + $existent_documents = id(new PhrictionDocumentQuery()) 156 + ->setViewer(PhabricatorUser::getOmnipotentUser()) 157 + ->withSlugs(array_keys($load_map)) 158 + ->execute(); 159 + $existent_documents = mpull($existent_documents, null, 'getSlug'); 160 + 161 + foreach ($load_map as $lookup => $keys) { 162 + $existent = idx($existent_documents, $lookup); 163 + if (!$existent) { 164 + continue; 165 + } 166 + 167 + foreach ($keys as $key) { 168 + $document_map[$key] = array( 169 + 'visible' => false, 170 + 'document' => null, 171 + ); 172 + } 173 + } 174 + } 175 + 176 + foreach ($metadata as $key => $spec) { 108 177 $link = $spec['link']; 109 178 $slug = PhabricatorSlug::normalize($link); 110 179 $name = $spec['explicitName']; ··· 114 183 // in text as: "Title" <link>. Otherwise, we'll just render: <link>. 115 184 $is_interesting_name = (bool)strlen($name); 116 185 117 - if (idx($existant_documents, $slug) === null) { 186 + $target = idx($document_map, $key, null); 187 + 188 + if ($target === null) { 118 189 // The target document doesn't exist. 119 190 if ($name === null) { 120 191 $name = explode('/', trim($link, '/')); 121 192 $name = end($name); 122 193 } 123 194 $class = 'phriction-link-missing'; 124 - } else if (idx($visible_documents, $slug) === null) { 195 + } else if (!$target['visible']) { 125 196 // The document exists, but the user can't see it. 126 197 if ($name === null) { 127 198 $name = explode('/', trim($link, '/')); ··· 131 202 } else { 132 203 if ($name === null) { 133 204 // Use the title of the document if no name is set. 134 - $name = $visible_documents[$slug] 205 + $name = $target['document'] 135 206 ->getContent() 136 207 ->getTitle(); 137 208