Page MenuHomePhabricator

Security review for UrlShortener extension
Closed, ResolvedPublic


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 raised the priority of this task from to Medium.
Legoktm updated the task description. (Show Details)

[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 Incoming 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?

General Observations

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




























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). now outputs a Cache-control: s-maxage=2592000, must-revalidate, max-age=0 header. ( outputs 1200, but that's because our labs setup is weird - shouldn't happen in prod).