Page MenuHomePhabricator

Escape thumbnail URL before rendering it in CSS context
Closed, ResolvedPublic

Description

Per @csteipp:

In card.hogan the thumbnail URL isn't sanitised, escaped, or enclosed in single quotes.

So:

  • while processing the result of the API call, the thumbnail URL should be escaped (see CSS.escape for inspiration)
  • the thumbnail URL must start with http[s]://
  • surround the thumbnail URL with single quotes

Event Timeline

phuedx created this task.Nov 13 2015, 9:48 AM
phuedx raised the priority of this task from to Needs Triage.
phuedx updated the task description. (Show Details)
phuedx added subscribers: phuedx, csteipp.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptNov 13 2015, 9:48 AM
phuedx updated the task description. (Show Details)Nov 13 2015, 9:58 AM
phuedx set Security to None.
phuedx claimed this task.Nov 13 2015, 12:04 PM
phuedx triaged this task as High priority.

Change 253328 had a related patch set uploaded (by Phuedx):
Sanitize/escape thumbnail URLs

https://gerrit.wikimedia.org/r/253328

Change 253328 merged by jenkins-bot:
Sanitize/escape thumbnail URLs

https://gerrit.wikimedia.org/r/253328

It's hard to test because mediawiki doesn't allow uploading an image with invalid characters in the title.

phuedx added a comment.EditedNov 19 2015, 3:31 PM

There's a bug in 253328 around – I think – escaping encoded entities in thumbnail URLs. I caught it while testing 253318 (T117108) locally on a page with a test image from T111609: http://en.m.wikipedia.beta.wmflabs.org/wiki/File:Lady_von_Büsum_(ship,_1980)_001.jpg.

Change 254149 had a related patch set uploaded (by Phuedx):
Don't encode the thumbnail URL before escaping it

https://gerrit.wikimedia.org/r/254149

Change 254149 merged by jenkins-bot:
Don't encode the thumbnail URL before escaping it

https://gerrit.wikimedia.org/r/254149

There's a bug in 253328 https://gerrit.wikimedia.org/r/253328 around –
I think – double escaping entities in thumbnail URLs. I caught it while
testing 253318 https://gerrit.wikimedia.org/r/253318 (
https://phabricator.wikimedia.org/T117108) locally on a page with a test image from https://phabricator.wikimedia.org/T111609
.

yes, ' " ) & are all legal in filenames, so testing with a filename that
contains those should identify any issues.

bmansurov added a comment.EditedNov 19 2015, 5:24 PM

For me, file upload fails when trying to upload a file with any of those characters. The error is about not being able to create some directory. It works fine when the file name is alphanumeric. I guess I need to enable some php extensions? Fixed the issue by changing the images directory permissions to be more permissive.

Tested locally, looks good.

Fixed the issue by changing the images directory permissions to be more permissive.

If you're ending up with directory names that aren't 0-9 a-f, let me know... that would be a different issue that I would want to look at! But glad it worked.

Are you guys backporting that, or is this issue now resolved?

Directories (and a file) inside the images dir:

1	README	archive	c	deleted	e	f	lockdir	temp	thumb

So I think it looks right.

Are you guys backporting that, or is this issue now resolved?

Backporting the patch committed above? To where? We haven't released any version of Cards yet.

Are you guys backporting that, or is this issue now resolved?

Backporting the patch committed above? To where? We haven't released any
version of Cards yet.

Ok, I'll just let you guys close this whenever you're ready. Thanks for
fixing it.

bmansurov closed this task as Resolved.Nov 25 2015, 2:29 AM