1451909 - contextualIdentities.update() doesn't validate color or icon values before setting

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

There is a bug in the contextualIdentities.update() function. From [1] you can see the "color" and "icon" should be a one of a set of string values. When you try to set something else, a js error is thrown, but the value is still set regardless.

STR:

1. Call the api with something like:

contextualIdentities.update('firefox-container-1',{icon:"NOTREAL", color:"ALSOBAD")} 


Expected:
Error thrown, color and icon not changed.

Actual: 

this is what ends up in containers.json in the profile directory:
"identities": [{
        "userContextId": 1,
        "public": true,
        "icon": "NOTREAL",
        "color": "ALSOBAD",
        "telemetryId": 1,
        "name": "personal"
    }...

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/update


PS Not sure if this is the correct competent (I suspect the bug here might actually be in ContextualIdentity.jsm, though we could probably fix this in the web api.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Ah is this just the update method? I know we had validation for create at one point.

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1451909 - Adding in icon and color validation for contextualIdentities.update web extension method.

Review commit: https://reviewboard.mozilla.org/r/234748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234748/

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

LGTM. NB: there IS actually an error thrown from somewhere when you try to use an invalid color/icon. But the data is set in containers.json regardless of the error. Sorry I should have made that clear.

Comment 4

[Jonathan Kingston [:jkt] he/him]

I think the throw was likely from convertIdentity or convertIdentityFromObserver:
https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/toolkit/components/extensions/parent/ext-contextualIdentities.js#83

Which indeed would be too late to prevent it saving into the ContextualIdentityService

Thanks for catching this!

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8965964 [details]
Bug 1451909 - Adding in icon and color validation for contextualIdentities.update web extension method.

https://reviewboard.mozilla.org/r/234748/#review240734

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c318860e5179
Adding in icon and color validation for contextualIdentities.update web extension method. r=aswan

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c318860e5179

Comment 8

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!

Comment 9

[Will Bamberg [:wbamberg]]

On discussion with :cmills, it doesn't look like this needs a docs update.

Comment 10

[Jonathan Kingston [:jkt] he/him]

Removing ni as this isn't required sorry for the delay.