Page MenuHomePhabricator

Security review for UrlShortener extension
Closed, ResolvedPublic

Description

A security review is needed for the UrlShortener extension The code isn't finalized yet (there are 2 blockers left), but it's mostly complete.

Event Timeline

Legoktm created this task.Sep 17 2015, 7:57 PM
Legoktm raised the priority of this task from to Normal.
Legoktm updated the task description. (Show Details)
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 17 2015, 7:57 PM

[17:15:34] <csteipp> legoktm: What's the timeline for UrlShortener? This quarter? Next quarter?

I'd like to see the extension deployed before the end of the year. We're blocked on a few other bugs, most importantly getting an SSL cert for the domain (T108649), which won't happen until mid-October-November. Ideally the MW extension will also be ready at that point.

UrlShortener.utils.php line 130: * @param $shortCode String base64 shortcode to generate URL For.

base64 is case sensitiv, so it is difficult for Windows and specially for mobile users, where c&p is too complicate to handle.

dpatrick set Security to None.
dpatrick added a project: Security-Team.
dpatrick moved this task from Backlog to In Progress on the Security-Team board.

I'm about to start on this review. Should I look at master, or some other branch?

Look at master! :)

dpatrick added a comment.EditedOct 13 2015, 12:21 AM
General Observations

Overall, no issues were observed that need be addressed before use. Thank you for providing a demo instance of the extension.

Files

./ApiShortenUrl.php

OK

./modules/ext.urlShortener.popup.less

OK

./modules/ext.urlShortener.special.js

OK

./modules/ext.urlShortener.toolbar.js

OK

./redirect.htaccess

OK

./schemas/urlshortcodes.sql

OK

./SpecialUrlRedirector.php

OK

./SpecialUrlShortener.php

OK

./UrlShortener.alias.php

OK

./UrlShortener.hooks.php

OK

./UrlShortener.notranslate-alias.php

OK

./UrlShortener.php

OK

./UrlShortener.utils.php

OK
dpatrick moved this task from In Progress to Done on the Security-Team board.Oct 13 2015, 12:23 AM

Thanks for the review! I filed the circular redirects issue as T115802. If that's not a blocker to deployment, can we mark this bug as resolved?

Are the redirects cached enough, that if someone put a circular redirect loaded from a moderately popular website, it's not going to kill mediawiki? If so, I agree, not a blocker and we should close.

My quick test on labs showed that the 301's were emitting Cache-Control: "private, must-revalidate, max-age=0" headers. I assume that means varnish won't cache it...I'll look into that.

rEUSH02dae289e9c8: Make sure the 301 redirect from Special:UrlRedirector is cached sets a maxage of 30 days (same as the API).

http://urlshortener.wmflabs.org/wiki/Special:UrlRedirector/2F now outputs a Cache-control: s-maxage=2592000, must-revalidate, max-age=0 header. (urlshortener.wmflabs.org/2F outputs 1200, but that's because our labs setup is weird - shouldn't happen in prod).

csteipp closed this task as Resolved.Oct 22 2015, 7:46 PM

Cool. Thanks!