469760 - blocklist url %LOCALE% replaced with general.useragent.locale value, but without resolving complex value on ubuntu

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect)

Description

[Alexander Sack]

Attachment: hacky patch used for initial verify

1. start http live headers
2. reset app.update.lastUpdateTime.blocklist-background-update-timer
3. open error console
4. force blocklist update by running:
  Components.classes['@mozilla.org/extensions/blocklist;1'].getService(Components.interfaces.nsITimerCallback).notify(null)
5. check the GET url invoked

result:
 https://addons.mozilla.org/blocklist/2/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/3.0.4/Firefox/2008111319/Linux_x86_64-gcc3/chrome://glo
bal/locale/intl.properties/default/Linux%202.6.27-7-generic%20(GTK%202.14.4)/canonical/1.0/

expected result:
https://addons.mozilla.org/blocklist/2/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/3.0.4/Firefox/2008111319/Linux_x86_64-gcc3/en-US/default/Linux%202.6.27-7-generic%20(GTK%202.14.4)/canonical/1.0/


difference: %LOCALE% is replaced by chrome://global/locale/intl.properties ... but should be en-US

Note: this probably busts "OS" stats if they are based on blocklist runs.

Evaluation: PREF_GENERAL_USERAGENT_LOCALE seems to be a complex pref; fix would first check for complex pref and then fall back to not complex pref.

attaching a hacky patch i did for initial testing ... will attach a clean one

Comment 1

[Alexander Sack]

sorry ... the result url had a line break:

https://addons.mozilla.org/blocklist/2/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/3.0.4/Firefox/2008111319/Linux_x86_64-gcc3/chrome://global/locale/intl.properties/default/Linux%202.6.27-7-generic%20(GTK%202.14.4)/canonical/1.0/

Comment 2

[Alexander Sack]

fwiw, its an ubuntu only issue. we ship a special pref since i took over the package ... we can drop that if you dont want to fix it in blocklist code.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Why is general.useragent.locale a localized pref in your builds? browser/app/profile/firefox.js and browser/locales/en-US/firefox-l10n.js explicitly set it to a non-localized value, and this isn't the only piece of code that assumes it's a normal charpref. In fact all.js seems to be the only one who thinks it can end up localized, so the easiest fix is probably to just fix it to use @AB_CD@ as well.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Ah, sorry, missed your comment. I think you should just remove that pref, yeah (and we should fix all.js too).

Comment 5

[Alexander Sack]

remove blocking from stable branches in turn of comment 2

Comment 6

[Alexander Sack]

(In reply to comment #4)
> Ah, sorry, missed your comment. I think you should just remove that pref, yeah
> (and we should fix all.js too).

ok, will prepare a patch for that.

Comment 7

[Axel Hecht [:Pike]]

I would expect this to take a patch like in bug 446527 to really work independent of user settings.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/urlformatter/src/nsURLFormatter.js is worth a look, too. Sadly, one can't pass in extra variables. Not sure if some of the blocklist variables would be worth adding.

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to comment #7)
> I would expect this to take a patch like in bug 446527 to really work
> independent of user settings.
I believe that the difference between this and bug 446527 is that we want the currently running locale vs. the installation locale.

Comment 9

[Axel Hecht [:Pike]]

I wonder if we really mind in the end. In which scenario would be blacklist an extension for just the currently selected locale?

Maybe just making sure that the actual char prefs are properly escaped so that "/" chars don't make it through would be already good enough.

Comment 10

[Alexander Sack]

(In reply to comment #9)
> I wonder if we really mind in the end. In which scenario would be blacklist an
> extension for just the currently selected locale?
> 

I agree that its not obvious as of why to pass a %LOCALE% to the blocklist server at all ...

If there is no use-case for that we can just drop that component from the blocklist URL completely. Not sure how many backends/services rely on that component for parsing though.

> Maybe just making sure that the actual char prefs are properly escaped so that
> "/" chars don't make it through would be already good enough.

Agreed, properly escaping the components would make this more fail-safe ... not sure if its really worth the effort though. Thoughts?

Comment 11

[Dave Townsend [:mossop]]

I don't see any particular reason to block 3.1 on this, given that it works for the standard case. I don't see why we wouldn't take a patch though.

(In reply to comment #10)
> (In reply to comment #9)
> > I wonder if we really mind in the end. In which scenario would be blacklist an
> > extension for just the currently selected locale?
> > 
> 
> I agree that its not obvious as of why to pass a %LOCALE% to the blocklist
> server at all ...
> 
> If there is no use-case for that we can just drop that component from the
> blocklist URL completely. Not sure how many backends/services rely on that
> component for parsing though.

bug 430120

> > Maybe just making sure that the actual char prefs are properly escaped so that
> > "/" chars don't make it through would be already good enough.
> 
> Agreed, properly escaping the components would make this more fail-safe ... not
> sure if its really worth the effort though. Thoughts?

bug 468527

Comment 12

[Axel Hecht [:Pike]]

As the rationale for bug 430120 is to retrieve the same data as with update pings, we should feed the same locale info, i.e., the installed locale code, as it determines in http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/src/nsUpdateService.js.in#519.

Not sure what info linux distro's have there, and what we do about that.

Comment 13

[Daniel Einspanjer [:dre] [:deinspanjer]]

Confirming that a patch for this problem should interpolate the installation locale to match the spec.  As comment #12 mentions, bug 430120 contains the rational for having these extra fields in the URL.

The parser expects a legitimate @AB_CD@ code, but it special cases the chrome:// value using a gawd awful complicated regex.  That workaround would break if you tried to do something like adding escaping to the URL instead, so if we do end up adding any new escaping to the URL, we need to open a separate bug for Metrics to track that.

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> Confirming that a patch for this problem should interpolate the installation
> locale to match the spec.  As comment #12 mentions, bug 430120 contains the
> rational for having these extra fields in the URL.
> 
> The parser expects a legitimate @AB_CD@ code, but it special cases the
> chrome:// value using a gawd awful complicated regex.  That workaround would
> break if you tried to do something like adding escaping to the URL instead, so
> if we do end up adding any new escaping to the URL, we need to open a separate
> bug for Metrics to track that.

Presumably if we take a patch for this bug (getting rid of the chrome url from the locale and using a proper locale code) then adding escaping shouldn't actually affect anything.

Comment 15

[Axel Hecht [:Pike]]

... right, escaping should just kick in when you'd get garbage anyway. I'd suggest to drop the locale to something "None" or "undefined" in the metrics backend whenever you get a non-good locale code. No idea what you currently do if the regexp isn't matched.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #7)
> I would expect this to take a patch like in bug 446527 to really work
> independent of user settings.
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/urlformatter/src/nsURLFormatter.js
> is worth a look, too. Sadly, one can't pass in extra variables. Not sure if
> some of the blocklist variables would be worth adding.

Ideally we should just get bug 430235 fixed

Comment 17

[Daniel Einspanjer [:dre] [:deinspanjer]]

replacing the chrome URL with a properly formatted locale would be transparent on metrics.

I just went back and looked at the regex.  It would properly handle an escaped bogus value as well without a problem, but I'd still very much like to get mindset that any time a change happens with the structure of one of these URLs, a bug gets opened with metrics to review it for potential impact.  I've opened Bug 469806 to that effect.

FYI, here is the piece of the regex that deals with the locale segment:

   /(?>                        # locale raw_string
     (?>
        chrome://(?:.{0,108})\.properties
          # Note: some Linuxes are messed up and report chrome URL instead of locale
     )|(?>
       (                       # locale code
        (?>[a-z]{2,3})         # language
        (?>-[A-Z]{2})?       # region
        (?>-[\w]{2,8})?      # dialect
       )
     )|(?>
        [^/]*
         # Failsafe to eat unacceptable values
     )
   )

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> I just went back and looked at the regex.  It would properly handle an escaped
> bogus value as well without a problem, but I'd still very much like to get
> mindset that any time a change happens with the structure of one of these URLs,
> a bug gets opened with metrics to review it for potential impact.  I've opened
> Bug 469806 to that effect.

I'm not entirely sure what the benefit is in opening a bug to evaluate the potential impact of another bug. Surely it would be better for all the discussion to go in the single bug?

Comment 19

[Axel Hecht [:Pike]]

(In reply to comment #17):

There are more variants to locale codes than that regexp matches. Like script, at least. We don't have any of those right now, but the real monty looks more complex still. Not this bug, though.

Comment 20

[Alexander Sack]

(In reply to comment #17)
> FYI, here is the piece of the regex that deals with the locale segment:
> 
>    /(?>                        # locale raw_string
>      (?>
>         chrome://(?:.{0,108})\.properties
>           # Note: some Linuxes are messed up and report chrome URL instead of
> locale
>      )|(?>

Thanks for the insight! So I guess this means that the url i posted in the bug description is properly matched as of now? can you double check that by running your complete regexp against what i posted? Its quite important for us to know, so we can assign a proper prio to this issue in ubuntu.

Thanks again!

Comment 21

[Daniel Einspanjer [:dre] [:deinspanjer]]

Yes, the "expected result" URL you pasted in the description is properly matched and the locale is captured.

@Axel Hecht :Pike -- Regarding comment #19, I replied via e-mail asking for more details.  Did you get that?  I figured that tangent wasn't very related to the actual bug so I didn't want to junk up the comments here.

Comment 22

[Axel Hecht [:Pike]]

Yeah, got that. I should update the docs to say that http://tools.ietf.org/html/rfc4646 is what we use, while trying to be as short as possible conforming to that spec.

Comment 23

[Andy McKay]

Due to a long period of inactivity on this bug (5.57 years), I am intending to close this bug within a month or so in accordance with: https://wiki.mozilla.org/Add-ons/OldBugs Please remove [intent-to-close] from the whiteboard and comment on this bug if you would like to keep it open.