[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.

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

Look at master! :)

General Observations

• Positive
  • URL scheme is validated to be http or https and username and password are disallowed
  • Allowed domains default to only local server name if domain list is left unconfigured
  • Ports are limited to 80 and 443 by default
  • Code implements throttling of URL creation for both site and API use
  • Client-side and server-side validation are implemented
• Negative
  • Attacker may create circular redirects, leading to temporary client-side DoS, absent client-side limits on following redirects
    • LIVE EXAMPLE
      • http://us.wmflabs.org/2F (redirects to http://urlshortener.wmflabs.org/wiki/Special:UrlRedirector/2F) redirects to http://urlshortener.wmflabs.org/wiki/Special:UrlRedirector/2E
      • http://us.wmflabs.org/2E (redirects to http://urlshortener.wmflabs.org/wiki/Special:UrlRedirector/2E) redirects to http://urlshortener.wmflabs.org/wiki/Special:UrlRedirector/2F
    • The impact of this attack on WMF infrastructure seems minimal, since requests for canonicalization will be cached and served by Varnish.

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

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).