394245 - if any of our AUS url parameters contain a slash, update will break

Status: RESOLVED INCOMPLETE

(Toolkit :: Application Update, defect)

Description

[(not reading, please use [e-mail] instead)]

if any of our AUS url parameters contain a slash, update will break

http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/update/src/nsUpdateService.js.in#1887

This hasn't happened to us yet (I don't think), but if CHANNEL was "foo/bar" or if OS_VERSION was "cheese/ball" that would result in a null update from aus.

aus gurus:  any quick way if we can look at the aus logs to see if the reason we're serving any null updates is because we have one (or more) "too many" parameters?

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Code went in recently to allow arbitrary parameters in the update url too... I doubt it checks for slashes.

Comment 2

[(not reading, please use [e-mail] instead)]

> Code went in recently to allow arbitrary parameters in the update url too

ben, was that part of bug https://bugzilla.mozilla.org/show_bug.cgi?id=299716?

Comment 3

[Dave Townsend [:mossop]]

I don't believe anything like this has changed for the AUS update url in some time, though it's always been possible for an extension to change it to whatever they like by tinkering with prefs.

The extensions update url on the other hand has had bug 384052, possibly you are thinking of that?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Crap, yeah, I was thinking about bug 384052 and the extension update url.

/me goes back to regressing tbox performance.

Comment 5

[Michael Morgan [:morgamic]]

So if I'm reading right this is not a server issue?  Correct me if I'm wrong and I can take a look.

Comment 6

[Dan Mills [:thunder]]

Correct, this is a client issue.

Comment 7

[(not reading, please use [e-mail] instead)]

mike, yes a client issue.  but is there a way we could check through the aus logs for any requests where we've gotten too many parameters?

For example, instead of:

https://aus2.mozilla.org/update/2/Firefox/3.0a8pre/2007082905/WINNT_x86-msvc/en-US/nightly/Windows_NT%205.1/update.xml?force=1

We got:

https://aus2.mozilla.org/update/2/Firefox/3.0a8pre/2007082905/WINNT_x86-msvc/en-US/nightly/Windows/_NT%205.1/update.xml?force=1

aus will currently silently give us back an empty update xml snippet.

Comment 8

[Dave Townsend [:mossop]]

I'm not sure I understand the nature of the problem here. It reads to me as "if the client uses an invalid update url then AUS returns no updates". That seems to be the correct behaviour.

Comment 9

[Dan Mills [:thunder]]

The client should a) url-escape the individual fields, so that their content does not interfere with the parsing of the url.

e.g., if the channel is set to "cheese/ball", the client should send "cheese%2Fball" as the channel.

Comment 10

[Dan Mills [:thunder]]

I should add, the client currently escapes '+':

http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/update/src/nsUpdateService.js.in#1887

Instead, it should fully url-escape each field before replacing them into the template url.

Comment 11

[Michael Morgan [:morgamic]]

Wanted to add that in the server code we split on slashes for a couple of reasons:
1) that's the schema that was agreed upon, the URL escaping was implied
2) to not have to worry about path traversal since args are split on '/'

(In reply to comment #0)
> aus gurus:  any quick way if we can look at the aus logs to see if the reason
> we're serving any null updates is because we have one (or more) "too many"
> parameters?

Seth, we don't log failures -- there are just access logs but we can't match those up to null updates as far as I know.  There was an old bug about noting failures in bug 351622 -- but even then there wouldn't be aggregate "cause for failure" data anywhere.

We could add logging and take a sample of a couple days, possibly -- would that be worth a shot?

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #11)
> We could add logging and take a sample of a couple days, possibly -- would that
> be worth a shot?

That log would also be useful for the Build Q3 goal "enumerate orphaned versions of FF/TB".

Comment 13

[(not reading, please use [e-mail] instead)]

as morgamic points out in comment #11, this is not an aus server bug at all as the aus server is following the agreed url schema.

as dmills points out in comment #9 and #10, the client should be escaping each field.  as dmills and I discussed over irc, this could also be an issue if some of the new fields that will be getting added to the aus url (for partner distribution) have non-ascii characters.  

as dmills points out, the fix here is to use encodeURIComponent() for each field and remove the code that escapes + to %2B.

my only concern is that we will start escaping something we weren't escaping before, and orphan a build that wasn't orphaned before, so let's first get some aus logging in place.

taking the client side of this bug, for now.

Comment 14

[(not reading, please use [e-mail] instead)]

morgamic / nick:

> We could add logging and take a sample of a couple days, possibly -- 
> would that be worth a shot?

as nick points out, that would definitely be worth it.  thanks mike!   

see also bug #368578, about logging anything that results in an empty snippet (frankenfox or otherwise).  That bug covers this issue, right?

It would be very interesting to know if anyone is actually hitting his particular issue before making a change to the client.