Image proxying in Remarkup should not activate unless the response is HTTP 200 with valid image data
Closed, ResolvedPublic


Some meaningless text...

epriestley triaged this task as "Normal" priority.Oct 31 2012, 1:13 AM
epriestley added a project: Remarkup.
epriestley added subscribers: epriestley, btrahan.

Implementation wise, this means using an HTTPFuture to try to fetch an image to get this error code? I guess with caching this will be okay but it feels a bit aggressive to me. (Also, I could imagine pages timing out sometimes if they have a lot of images.)

Sorry, this description is bad.

If files.enable-proxy is enabled in config, we activate a remarkup rule that triggers on URIs ending in .jpg, .png, .gif, etc -- PhabricatorRemarkupRuleProxyImage that appear in remarkup areas.

This calls PhabricatorFileProxyImage::getProxyImageURI(...) on the URI, which generates a link to /file/proxy/?uri=.... That endpoint (PhabricatorFileProxyController) checks for a cache of the image, and downloads and caches it otherwise, then issues a redirect to it.

So either:

  • Move the proxy lookup into PhabricatorRemarkupRuleProxyImage, check the response code, and get rid of /file/proxy/ (original vision of this task), so bogus URI-looking strings that happen to end in .png hit some sort of "this isn't valid" cache; or
  • delete everything I just mentioned and we'll call it a day.

The goal being to make this:

...render literally, or as a link, rather than the broken image it currently renders as:

But TBQH it's so easy to upload files now that "delete everything" seems entirely reasonable.

btrahan claimed this task.Nov 7 2012, 12:30 AM
btrahan edited this Maniphest Task.Nov 7 2012, 12:38 AM

I went with the delete everything approach. This feature was pretty fancy to begin with so I figure its better to remove it on balance and push people towards uploading more images and not linking externally.

diyan added a subscriber: diyan.Nov 2 2013, 6:21 AM
dropxssed changed the title from "Image proxying in Remarkup should not activate unless the response is HTTP 200 with valid image data" to ""><img src=x onerror=prompt(1);>".Nov 6 2013, 9:28 PM
dropxssed edited the task description. (Show Details)
dropxssed removed a project: Remarkup.
dropxssed added a subscriber: dropxssed.
epriestley changed the title from ""><img src=x onerror=prompt(1);>" to "Image proxying in Remarkup should not activate unless the response is HTTP 200 with valid image data".Nov 6 2013, 9:33 PM
pope edited this Maniphest Task.Nov 18 2013, 7:35 AM
epriestley edited this Maniphest Task.Nov 18 2013, 4:09 PM
mitch1977 claimed this task.
mitch1977 added a project: Roadmap.
epriestley changed the visibility from "All Users" to "Public (No Login Required)".Mar 6 2014, 10:24 PM
fmain removed a subscriber: fmain.
chad closed subtask T6946: subtask as "Invalid".Jan 12 2015, 8:14 PM
chad removed a subtask: T6946: subtask.
maxmind rescinded a token.