Closed Bug 520346 Opened 16 years ago Closed 16 years ago

Periodic update check for lightweight themes

Categories

(Toolkit :: Add-ons Manager, defect, P2)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 4 obsolete files)

patch v3
14.03 KB, patch
mossop
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
Support for periodic update checks for lightweight themes has been requested by the Labs/Personas team and we initially made it a P1, but I'm not sure that was the right decision. Motivations for the update check are: * Providing updates Most lightweight themes, by design, won't need updates. I expect authors to tweak images and colors based on feedback, but I don't think keeping existing users updated with these changes is a requirement. Installing a lightweight theme is more comparable to saving a desktop wallpaper (where you wouldn't expect updates) than to installing a traditional add-on. * Supporting dynamic themes Definitely useful, although I think Chris mentioned there's no such content yet. Could be supported specifically by adding an isDynamic flag, so it probably shouldn't be part of this discussion. * Revoking themes because of copyright violations or the like If you hosted desktop wallpapers and would be required to remove one, nobody would expect you to remove it from the users' PCs as well. Such a request would only make sense if we did the update check in the first place. So this doesn't seem to work as a primary motivation to support the update check. * Collecting usage data for metrics This also shouldn't be the primary motivation. If it turns out we need the update check for other reasons, we can certainly use that request for metrics as we do elsewhere. But otherwise I think the Personas add-on could continue to do the pingback and getpersonas.com could extrapolate broader statistics by comparing the numbers of add-on and non-add-on installs. (Add-on installs can be identified with the CheckPersonas event.)
Chris, Suneel - you guys have thought about the update process here for longer than we have. As Dao says, this was was marked a P1, but we want to understand what goal it's serving and whether there are other ways to serve that goal, before turning on an automatic ping for every persona user in the product. If it needs to be there, we'll do it, and IT will support it, but if it's primarily aimed at, for instance, enabling dynamic personas, then maybe Chris' idea to mark certain personas as "dynamic" is a better approach?
(In reply to comment #0) > * Providing updates > > Most lightweight themes, by design, won't need updates. I expect authors to > tweak images and colors based on feedback, but I don't think keeping existing > users updated with these changes is a requirement. Installing a lightweight > theme is more comparable to saving a desktop wallpaper (where you wouldn't > expect updates) than to installing a traditional add-on. > This is not true at all, designers are frequently updating their designs to make them better and to respond to user feedback. We've received 4,139 update requests this month. We're also talking about moving the metrics information to this ping so as to help optimize the back end. Metrics are important to power the gallery and to provide everyone with feedback on popularity. We modeled this off of the add-on version check ping, and the back-end us similar in terms of how it interfaces with the metrics infrastructure. > * Supporting dynamic themes > > Definitely useful, although I think Chris mentioned there's no such content > yet. Could be supported specifically by adding an isDynamic flag, so it > probably shouldn't be part of this discussion. My put here would be to leave the update ping equivalent to the verison check for add-ons that exists today. That should be sufficient to support dynamic themes in the future -- although that would require server and client changes beyond a design that updates on a daily basis. > * Revoking themes because of copyright violations or the like > > If you hosted desktop wallpapers and would be required to remove one, nobody > would expect you to remove it from the users' PCs as well. Such a request would > only make sense if we did the update check in the first place. So this doesn't > seem to work as a primary motivation to support the update check. I agree that this is not a priority, but wonder if we want to be consistent. We have and want the ability to blocklist add-ons today which disables them. It would be prudent to ensure we have the same ability, regardless of whether or not it's used. (beltzner?)
s/beltzner/jonath
(In reply to comment #2) > (In reply to comment #0) > > > * Providing updates > > > > Most lightweight themes, by design, won't need updates. I expect authors to > > tweak images and colors based on feedback, but I don't think keeping existing > > users updated with these changes is a requirement. Installing a lightweight > > theme is more comparable to saving a desktop wallpaper (where you wouldn't > > expect updates) than to installing a traditional add-on. > > This is not true at all, designers are frequently updating their designs to > make them better and to respond to user feedback. We've received 4,139 update > requests this month. To me, this is the crux of it. I don't know what I expected in terms of updates, but this is really a surprising amount, and suggests that the ability to revise and improve already-deployed personas is central. That we get a sort of "poor man's dynamic persona" out of this is a significant plus, as are the metrics, but the relatively massive volume of updates is what tips it for me, here. Personas are somewhere in a continuum between wallpaper and themes, and given the frequency with which they are updated, a daily ping to check for updates and support basic levels of dynamism seems worthwhile to me. Dao - how would we implement this? Check the images themselves for updates, or look for the corresponding JSON object somewhere?
(In reply to comment #2) > (In reply to comment #0) > > > * Providing updates > > > > Most lightweight themes, by design, won't need updates. I expect authors to > > tweak images and colors based on feedback, but I don't think keeping existing > > users updated with these changes is a requirement. Installing a lightweight > > theme is more comparable to saving a desktop wallpaper (where you wouldn't > > expect updates) than to installing a traditional add-on. > > > > This is not true at all, designers are frequently updating their designs to > make them better and to respond to user feedback. We've received 4,139 update > requests this month. What we wrote doesn't clash, so I'm not sure what "not true at all" refers to. getpersonas.com absolutely should allow authors to fix/tweak/remove their themes. I don't see the need to push these changes to the users' browsers. Those who selected a theme and kept it did so because they were happy with it, those who weren't wouldn't get the update anyway. I don't think traditional themes are comparable in this regard. Some of them have been under development for years now. They have thousands of little details that can be tweaked, they are rather complex pieces of software that can actually have bugs, they can have conflicts with extensions, and they /always/ need an update when Firefox does a major update. I set up a page with a dozen lightweight themes as a playground for this feature, while getpersonas.com wasn't compatible with Firefox nightlies yet. After three or four days, when the page had already gotten press and quite a few visitors, I decided that one theme looked washed out and that the original motif wasn't really recognizable. So I tweaked the header image. New visitors will see that version, but I didn't have the desire to force this change upon anyone. Again, if somebody selected and kept that theme (well, I doubt it), I don't feel like I should affect the selection that this user made. He probably wouldn't expect that either. Maybe the washed-out background is exactly what he liked about the theme, since that made it less obtrusive. There's probably a bunch of users who can be considered part of the personas community, who frequently send feedback and who would indeed eagerly await updates. That's not our primary audience. It's an audience that the personas extension can better cater for. > > * Revoking themes because of copyright violations or the like > > > > If you hosted desktop wallpapers and would be required to remove one, nobody > > would expect you to remove it from the users' PCs as well. Such a request would > > only make sense if we did the update check in the first place. So this doesn't > > seem to work as a primary motivation to support the update check. > > I agree that this is not a priority, but wonder if we want to be consistent. We > have and want the ability to blocklist add-ons today which disables them. It > would be prudent to ensure we have the same ability, regardless of whether or > not it's used. (beltzner?) The blocklist is there to protect users from harmful extensions and it's not controlled by content providers either. (In reply to comment #4) > Dao - how would we implement this? Check the images themselves for updates, or > look for the corresponding JSON object somewhere? It would have to be the latter.
Do note that any type of new automatic update check would require changes to the Firefox privacy policy, which means talking to Legal, etc.
This is a Firefox 3.6 release blocker, IMO. The entire Personas experiment has always been about changing two things about the way we deliver customizations to our users: allowing lightweight (no restart required, one-click to install) customization of the browser to remove costs and barriers to entry, and creating a mechanism through which the user can "subscribe" to a piece of user interface which can be updated over time without requirement for confirmation or revisitation. These concepts were instrumental to the success and ease of the Personas experiment, and have been carried over in other Mozilla Labs based experiments (cf: Ubiquity, Jetpack) as well. Implementing Personas in the Firefox mainline product has always been about moving towards these UI concepts. <span style="philosophy"> There are specific product reasons for doing this: the lines between cloud-based and desktop-based services are blurring, and it is far easier for users to manage subscriptions to services than to continually need to check for updates to their software. Providing ways to undo and unsubscribe allow us to deliver improved experiences and products without bothering users about updating. Essentially: providers aren't asking the question "Do you want this better thing?" because forcing users to answer that question often ends up with a large number of users just not bothering to answer, and thus missing out on improved experiences. </span> Dao argued that Personas don't require frequent updates, but as Beard noted, by offering this mechanism, over 4,000 personas have been updated to improve their layout and appearance for users. The ability to leverage this mechanism has been extremely valuable both to Persona authors and their users. I do not believe that a privacy policy update is necessary, as we already declare that we check for updates in the section about Add-ons. What I think is important to determine is the frequency of the update checks, and the desired behaviour if the Persona is no longer available (for whatever reason). I think tying this to the standard Add-on update ping frequency is desirable, and has the benefit of not requiring another pref to manage. If a Persona is no longer available, I think it would be virtuous to indicate that to users somehow (probably a separate bug, and would require strings) but I do not think that we should remove the Persona from the local cache. Beard: is that consistent with our responsibilities if we receive a takedown notice?
Flags: blocking1.9.2+
Priority: -- → P2
(In reply to comment #7)gs) Beard: is > that consistent with our responsibilities if we receive a takedown notice? I posed this question to legal (Catherine) last week, while we were considering a bug that would 'indicate that a specific Persona is no longer available'. From that discussion, my understanding is that our legal obligation is not to *distribute* Persona designs after the receipt of a legitimate takedown notice. Therefore, removal of a persona from our server, but not from the local cache, is consistent with our legal responsibilities. Note that legal is conducting an overall audit of the legal process for Personas in anticipation of the scale 3.6 will likely bring. This includes making recommendations on this specific issue, and validating the rule above. CC'ing Julie Martin.
It sounds like there are two legal issues lurking here: --privacy policy --extent of disable after a dmca takedown notice (locally or just on our servers) These are both open issues and have not been resolved within legal. I am working on these with Suneel and Harvey and outside counsel.
Depends on: 522188
Attached patch dynamic update URL (obsolete) — — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #407763 - Flags: review?(dtownsend)
Blocks: 522188
No longer depends on: 522188
(In reply to comment #10) > Created an attachment (id=407763) [details] > patch the intent here was that the update check be similar to the versioncheck call for add-ons, so that we can 1) more effectively scale the back end, and 2) collect performance metrics & characteristics to power the service (like we do with other forms of add-ons) here's how the metrics elements are currently implemented in the personas add-on: http://hg.mozilla.org/labs/personas/file/db02cfdc2f06/client/modules/service.js#l257 also, we were looking at more cleanly separately out the versioncheck from the data load as well (and only load & refresh data when needed), whereas presently this API call to the server returns data in all cases. zandr & toby can provide more detail on the server API, and pedro for the metrics tie-in.
As with other update checks, I think it's pretty clear that we won't send more data than needed for the update.
(In reply to comment #12) > As with other update checks, I think it's pretty clear that we won't send more > data than needed for the update. That's not entirely true... For example, the sole purpose of bug 430120 was to get more information for metrics purposes. However, considering we already have that information being sent both in blocklist and versioncheck pings, I don't see why we need it in the lightweight themes update check. However, I definitely think sending things like duration of use is stepping over a line, as we definitely don't do that with regular add-ons, and it also makes it theoretically easy to map this update information to actual people, violating their privacy.
Yeah I think we should be doing the same kind of %% escaping of values into the url that we do for other add-ons, probably simply the same as the list here: https://developer.mozilla.org/en/Install_Manifests#updateURL. ITEM_MAXAPPVERSION and ITEM_STATUS probably don't make sense though.
(In reply to comment #13) > However, I definitely think sending things like duration of use is stepping > over a line, as we definitely don't do that with regular add-ons, and it also > makes it theoretically easy to map this update information to actual people, > violating their privacy. How about if we sent a low resolution value for duration, maybe the number of days in use. I think that would make it pretty difficult to then track an individual user with this information and yet could still provide useful information, what do you think?
(In reply to comment #14) > Yeah I think we should be doing the same kind of %% escaping of values into the > url that we do for other add-ons, probably simply the same as the list here: > https://developer.mozilla.org/en/Install_Manifests#updateURL. > ITEM_MAXAPPVERSION and ITEM_STATUS probably don't make sense though. I don't see how any of these values are useful for lightweight theme providers. My understanding is that we send them for traditional add-on updates because add-ons depend on them. If it's only for metrics, then the UA string should be used. (In reply to comment #15) > (In reply to comment #13) > > However, I definitely think sending things like duration of use is stepping > > over a line, as we definitely don't do that with regular add-ons, and it also > > makes it theoretically easy to map this update information to actual people, > > violating their privacy. > > How about if we sent a low resolution value for duration, maybe the number of > days in use. I think that would make it pretty difficult to then track an > individual user with this information and yet could still provide useful > information, what do you think? It still wouldn't be covered by the privacy policy, and it shouldn't be.
(In reply to comment #16) > (In reply to comment #14) > > Yeah I think we should be doing the same kind of %% escaping of values into the > > url that we do for other add-ons, probably simply the same as the list here: > > https://developer.mozilla.org/en/Install_Manifests#updateURL. > > ITEM_MAXAPPVERSION and ITEM_STATUS probably don't make sense though. > > I don't see how any of these values are useful for lightweight theme providers. > My understanding is that we send them for traditional add-on updates because > add-ons depend on them. If it's only for metrics, then the UA string should be > used. We send them both for add-ons that rely on them and for add-ons developers to gain a better understanding of their audience. For personas the ID and VERSION may be important for the server to be able to return the appropriate response. The Firefox version, OS and locale seems pretty useful for persona developers to know, not only to make sure their work works in the right places, but it might be possible that we offer different info based on them. ABI is maybe unhelpful but I'd just leave it in for consistency's sake. The UA string is not reliable as it may have been altered.
(In reply to comment #17) > > I don't see how any of these values are useful for lightweight theme providers. > > My understanding is that we send them for traditional add-on updates because > > add-ons depend on them. If it's only for metrics, then the UA string should be > > used. > > We send them both for add-ons that rely on them and for add-ons developers to > gain a better understanding of their audience. Yeah, so we send them for technical reasons, well knowing and accepting that those who get the data can use it differently. That's slightly different from sending data for different use in the first place. > For personas the ID and VERSION > may be important for the server to be able to return the appropriate response. The server may include the version in the updateURL. > The Firefox version, OS and locale seems pretty useful for persona developers > to know, not only to make sure their work works in the right places, but it > might be possible that we offer different info based on them. We should talk about that when there's demand. Without actual demand, this seems like a red herring to me. (As an aside, I'd be pretty careful about allowing lightweight themes to depend on a specific Firefox version. That's a problem we have with traditional themes and that we were going to solve with this new approach, I think.) > The UA string is not reliable as it may have been altered. The fact that the user has control over this is a feature when it comes to site metrics.
> The server may include the version in the updateURL. I just realized that the server could also include a unique id in the updateURL to track the user. I guess this could be done with traditional add-ons too, but it's notably easier here, as you don't have to package an xpi. That makes the update check worrisome to me even if we don't actively include metrics information...
Attachment #407763 - Flags: superreview?(dveditz)
So maybe we should implement the proposed "API to request the latest/up-to-date JSON object for a persona, given an ID, http://server/static/1/0/34510/index_1.json" from https://wiki.mozilla.org/Firefox/Projects/Personas_Uplift_Exploration#Server.2FBackend_Changes Requiring arbitrary sites to use this structure seems suboptimal, but it's the lesser of the two evils, and I don't expect most potential lightweight theme providers to have an interest in delivering updates anyway.
Attached patch dynamic update host, fixed path (obsolete) — — Splinter Review
It's not clear to me why index_1.json has been proposed, this uses index.json instead.
Attachment #407763 - Attachment is obsolete: true
Attachment #408220 - Flags: superreview?(dveditz)
Attachment #408220 - Flags: review?(dtownsend)
Attachment #407763 - Flags: superreview?(dveditz)
Attachment #407763 - Flags: review?(dtownsend)
Attachment #407763 - Attachment description: patch → dynamic update URL
Whiteboard: needs review dtownsend
(In reply to comment #16) > If it's only for metrics, then the UA string should be used. I argued for using the UA string when we first implemented metrics in the extension, but Pedro had specific reasons for wanting the information to be part of the URL. I don't recall what they are, but he should be able to provide details.
(In reply to comment #21) > It's not clear to me why index_1.json has been proposed, this uses index.json > instead. The server appends a number to the filename for versioning, in case we need to incompatibly change the format of the JSON file in the future while continuing to support existing clients. Nevertheless, it isn't necessary, since we can easily add it if/when we actually do make such an incompatible change.
(In reply to comment #22) > I argued for using the UA string when we first implemented metrics in the > extension, but Pedro had specific reasons for wanting the information to be > part of the URL. I don't recall what they are, but he should be able to > provide details. Reliability. Daniel made some studies at the time; Here's a snippet of the conclusions: Out of 2,492,801 blocklist requests: 9012 (0.36%) gave UA strings indicating they were MSIE 423,353 (20.46%) did not have straight forward UA strings that would require some sort of heuristic parsing. We should definitely keep the current approach, if possible
Seems to me there are (at least) a couple of questions here: 1) Should we require a fixed update URL structure, since allowing anything else would potentially let personas authors insert unique IDs (comment 19)? 2) Should we allow %% substitutions in the update URL, or is User-Agent enough? (comment 18, among others) To #1, I think Dao's absolutely right that allowing personas to define arbitrary update URLs could allow them to insert unique IDs, enabling potentially unfortunate user tracking scenarios. Installing a persona with a unique-to-me facebook.com updateURL would let facebook know every day when I turned on my PC. Even though they only get my IP, that's still not a great thing. On the other hand - restricting the structure of the URL really doesn't help that much, imo. No matter how restricted we make the URL, we'll need to include a persona identifier somewhere, for sites that host multiple personas, and that could be generated in a unique-per-user way, too. Even if we shut that option down somehow (e.g. with some super-restricted persona ID syntax) they could just use different subdomains to accomplish the same thing (updateURL: 3df892e44.coolpersonas.com). So, basically, I don't think restricting the update URL will get us what we want here. And if it doesn't improve things, I think we shouldn't do it - letting sites build update URLs that suit their architecture is certainly easier. As to #2, I don't think we should allow any %%-substitution beyond what User-Agent would offer, but I don't have a problem with giving personas the *ability* to use those variables in their update URLs. I agree that User-Agent already exists but, again, if we're not talking about releasing meaningfully different information than User-Agent, I'm all for leaving the flexibility in. AMO and getpersonas may not use it, but it's not a lot of extra code to pass update URLs through the substitution check, right? Dao - are you concerned that sites will get different/more private information through this channel? I hear what you're saying about people who modify their user agent having made that choice deliberately, but I strongly suspect the vast majority have done it to make some IE-specific site behave properly - not because they wanted personas authors to believe they were using the persona in IE. If there are meaningful reasons why %%-substitution would be bad, though, I would say that we shouldn't block on it. Getting the update ping working now without %%-substitution and then filing a follow up to support it if the need arises is an outcome I'm okay with, if doing so unblocks the work on fixing this bug.
(In reply to comment #24) > > I argued for using the UA string when we first implemented metrics in the > > extension, but Pedro had specific reasons for wanting the information to be > > part of the URL. I don't recall what they are, but he should be able to > > provide details. > > Reliability. > > Daniel made some studies at the time; Here's a snippet of the conclusions: > > > Out of 2,492,801 blocklist requests: > 9012 (0.36%) gave UA strings indicating they were MSIE > 423,353 (20.46%) did not have straight forward UA strings that would require > some sort of heuristic parsing. Well, I guess it depends on how you define straightforward, but I can tell that getting these 20% down drastically isn't hard (since I happen to have written a UA string parser). You may have to ignore a measurable rest, but I think it's largely irrelevant whether there are 87 or 88 per cent of the getpersonas.com users on Windows, unless I'm missing an important use case. The use case I know about is popularity ranking, for which you don't need the UA data at all. So if this boils down to ordinary nice-to-have user statistics, I think it's fair to provide lightweight theme providers with the same data that any other site gets. (In reply to comment #25) > On the other hand - restricting the structure of the URL really doesn't help > that much, imo. No matter how restricted we make the URL, we'll need to include > a persona identifier somewhere, for sites that host multiple personas, and that > could be generated in a unique-per-user way, too. Even if we shut that option > down somehow (e.g. with some super-restricted persona ID syntax) they could > just use different subdomains to accomplish the same thing (updateURL: > 3df892e44.coolpersonas.com). We could require the host to be the installation source, at which point the user would have to approve of the installation from 3df892e44.coolpersonas.com. I admit that's a gross limitation. On the other hand, it seems to fit getpersonas.com, which is the only provider I really care about right now with regards to updates. > As to #2, I don't think we should allow any %%-substitution beyond what > User-Agent would offer, but I don't have a problem with giving personas the > *ability* to use those variables in their update URLs. I agree that User-Agent > already exists but, again, if we're not talking about releasing meaningfully > different information than User-Agent, I'm all for leaving the flexibility in. I'm against leaving the flexibility in, as I don't think we want these themes to target specific Firefox versions.
Comment on attachment 408220 [details] [diff] [review] dynamic update host, fixed path As Johnathan says, this doesn't introduce any additional privacy since sites can just use the persona ID as a means of tracking so I don't think using a restrictive URL scheme is the right choice. The %% encoding I am musing on but I generally think we should do it so long as we are not exposing any information not already in the regular user agent string. It gives websites additional flexibility and confidence in their data. Based on the potential privacy risk in update pings we should probably consider in a separate bug whether to allow update pings for personas installed from non-whitelisted sites.
Attachment #408220 - Flags: review?(dtownsend) → review-
Attached patch dynamic update URL, v2 (obsolete) — — Splinter Review
Attachment #408220 - Attachment is obsolete: true
Attachment #409979 - Flags: superreview?(dveditz)
Attachment #409979 - Flags: review?(dtownsend)
Attachment #408220 - Flags: superreview?(dveditz)
Attached patch dynamic update URL, v2 (obsolete) — — Splinter Review
updated to trunk
Attachment #409979 - Attachment is obsolete: true
Attachment #410737 - Flags: superreview?(dveditz)
Attachment #410737 - Flags: review?(dtownsend)
Attachment #409979 - Flags: superreview?(dveditz)
Attachment #409979 - Flags: review?(dtownsend)
For testing: {"id":"update-test","name":"Update Test","headerURL":"about:blank","updateURL":"http://design-noir.de/mozilla/themes/update-test.php"}
Attachment #410737 - Flags: review?(dtownsend) → review+
Whiteboard: needs review dtownsend → needs sr dveditz
Comment on attachment 410737 [details] [diff] [review] dynamic update URL, v2 Have I been asked for sr= as a regular second code review, or more specifically because of security concerns or as some sort of referee for the policy/privacy disputes in this bug?
(In reply to comment #31) > Have I been asked for sr= as a regular second code review, or more specifically > because of security concerns or as some sort of referee for the policy/privacy > disputes in this bug? Specifically for the privacy concern that the updateURL could be used to track users. For instance, it could be used to track the duration of use which getpersonas.com showed interest in, by just appending a date to the updateURL.
(In reply to comment #27) > Based on the potential privacy risk in update pings we should probably consider > in a separate bug whether to allow update pings for personas installed from > non-whitelisted sites. I think tying this to the whitelist makes a lot of sense, and agree that it's a separate bug. (In reply to comment #32) > Specifically for the privacy concern that the updateURL could be used to track > users. For instance, it could be used to track the duration of use which > getpersonas.com showed interest in, by just appending a date to the updateURL. I thought that concern was asked and answered by Johnathan in comment 25 and Dave Townsend in comment 26. I don't see how this privacy concern is any different than one we might already have with Firefox Add-ons, and feel that it's a cost/benefit decision that falls to the value of allowing Personas to be updated periodically. We established that value in comment 7, and indeed it's a fundamental part of the design goals of Personas. Dan: please take that into account when performing your sr.
(In reply to comment #33) > > Specifically for the privacy concern that the updateURL could be used to track > > users. For instance, it could be used to track the duration of use which > > getpersonas.com showed interest in, by just appending a date to the updateURL. > > I thought that concern was asked and answered by Johnathan in comment 25 and > Dave Townsend in comment 26. Nope. It was just agreed that we can't prevent it completely. > I don't see how this privacy concern is any > different than one we might already have with Firefox Add-ons It's fundamentally the same, but it's different in that xpis need to be packaged, so there's a slight burden, while lightweight themes are just strings that can be generated on the fly. The benefit side is also different, as we definitely need add-on updates to keep the add-ons working. >, and feel that > it's a cost/benefit decision that falls to the value of allowing Personas to be > updated periodically. Sure.
Depends on: 527463
(In reply to comment #27) > Based on the potential privacy risk in update pings we should probably consider > in a separate bug whether to allow update pings for personas installed from > non-whitelisted sites. Filed bug 527463 for that. It's not *immediately* clear to me that we need to block the release on it. I sure don't like it, and was going to blocking+ it out of the gate, but beltzner argued that it's only modestly worse than the possibility for addons to do this now. Addons have a higher barrier to entry (and higher ability to be nefarious) but in any event, I've added it to the nomination queue for further discussion. The point is someone moot if we just fix the thing, anyhow. I am certain that a low-risk patch would get approval, if nothing else.
Dan: any ETA on the sr here? Dao: is it still needed, or does bug 527463 comment 5 satisfy the requirement? Shaver is satisfied that we're meeting privacy concerns (and notes that search plugins have the exact same characteristics) with the current design, as do I, as does Johnath.
Bug 527463 comment 5 doesn't help me decide this, and in any case I'd prefer the super-reviewer to say so if sr isn't needed. Why bug 527463 comment 5 doesn't help me: Whether this is not any different from search plugins seems debatable. Do providers have the same interest in tracking search plugin users as they have in tracking theme users? Is getting users to install these things equally easy? /If/ they are not any different, does that mean there's no privacy issue? If there's a privacy leak, the fact that we have a similar leak elsewhere doesn't satisfy me. So the key questions that motivated me to put this up for super-review remain: Is there a potential privacy leak? How severe is it? Can we do something about it?
Dan, over IRC you said that you'd be able to get to this yesterday; what's our ETA, here?
Comment on attachment 410737 [details] [diff] [review] dynamic update URL, v2 The code looks fine (of course). I have a slight worry about the use of "example.com" in the testcase. That's a real domain, and even if some of the URLs return 404s we'll be pinging that machine constantly if the test gets into the tinderbox rotation. If you need a URL you don't actually need a response from use something like "lwttest.invalid" or "theme.test" (both TLDs specifically reserved for such use), and if you do need a response use a real Mozilla server. Is there any way for users to turn off theme updates? Users can turn off addon update checks, they can turn off app update checks, and they can turn off (albeit without UI) search engine update checks. I guess, yes, these are treated just like addons and obey the same pref. I sort of understand the UI simplification of pretending these are the same thing, but they are not the same thing. Users will find it jarring if their UI suddenly changes when they weren't expecting it to. Even if it's unarguably a change for the better, having it change will provoke a startle that is not a good feeling. If someone in the vicinity is running AirPwn they may _really_ startle! This may lead users to turn off updates which will then have a negative security impact for addons (which are generally much less obtrusive), and many times more important to update. Currently there's even some UI around addon updates that doesn't appear to happen with LWT updates, although there are bugs/plans to remove some of the addon UI. I admit this is just my opinion and that I don't have the UI expertise of the people who are pushing for this change, but I would like to see an extra hidden .enable pref checked just in case. Speaking for myself, I would be pissed as hell if my persona changed without me asking it to (e.g. explicitly signing up for a rotating theme). I've seen LWT updates compared to search plugin updates (which in practice don't update) but one difference is that search plugin update URLs are required to be SSL while these don't. Addons are required to have an SSL update url or else have signed update packets. It does not seem unreasonable to require LWT to use an SSL updateURL. At the very least our own themes should use SSL update URLs, because if large numbers of users are pinging getpersonas daily that will be a very tempting target to have some fun with. This is not a hypothetical guess, there are tools out there today you can download that try to mess specifically with Mozilla client updates (trying to exploit a now-patched SSL flaw). How tempting would it be to goatse people's browser chrome? Or even just silently update their themes with a new updateURL pointing at someone who wants to track you while leaving the image URLs alone? Speaking of tracking, in the bug there was talk about allowing %% expansion in the update URLs but I don't think I'm seeing that in the patch. Looks like you added the id and version to the header and footer URLs (which don't get re-loaded unless you change themes) but the updateURL doesn't get any ID or anything unless the site included it when it gave you the theme data originally. It can be made to work, but I'd have thought the id/version would be more useful on the updateURL than the images if we're adding it anywhere. > get currentThemeForDisplay () { > var data = this.currentTheme; > > if (data && PERSIST_ENABLED) { > for (let key in PERSIST_FILES) { > try { > if (data[key] && _prefs.getBoolPref("persisted." + key)) >- data[key] = _getLocalImageURI(PERSIST_FILES[key]).spec; >+ data[key] = _getLocalImageURI(PERSIST_FILES[key]).spec >+ + "?" + data.id + ";" + _version(data); Isn't this the local file:/// spec for the persisted image? Does adding query parameters make sense in that case? If I'm totally misreading this and this is the URL from which to fetch the image what if the headerURL already has a query string? >+ parseTheme: function (aString, aBaseURI) { ... >+ data[prop] = _makeURI(data[prop], _makeURI(aBaseURI)).spec; >+ if (/^https?:/.test(data[prop])) >+ continue; It's good that you're putting limits on this. It looks like this only happens when you first get the JSON data. If someone altered the persisted JSON string we might end up with unexpected and possibly harmful URI schemes. I don't know how much that's worth worrying about, probably not much since I can't think of any legitimate cases but there might be some other addon trying to be clever. >+ updateCurrentTheme: function () { >+ >+ var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] >+ .createInstance(Ci.nsIXMLHttpRequest); What happens if the updateURL is https and there's a certificate error? I'm guessing you'll get a click-through dialog but it'd be better to catch that error and just not do the update. sr-minus primarily because daily update checks over an insecure channel is too tempting a target -- please restrict the updateURLs to https (whether or not we restrict it to whitelisted hosts). I also think the whitelisted host restriction would be a good idea, but I'm OK leaving that argument in the other bug. There seem to be a bunch of helper functions that are passed a theme "id" and operate on the first theme found with that ID. Will ID's be unique? The current getpersonas.com IDs are just numbers and are probably setting a bad example in that regard -- if some other site starts doing the same thing we'll get clashes. When you update, the updating site could change the update URL to be a completely different site. I'm uncomfortable with the number of sites that can be involved in a single theme transaction. recommending site (e.g. Lifehacker "Check out this cool theme") header image site (cool getpersonas gallery image) footer image site (transparent gif for tracking) update site (evil tracking site. Could bounce you to a new tracking site every day)
Attachment #410737 - Flags: superreview?(dveditz) → superreview-
Whiteboard: needs sr dveditz
Whiteboard: needs new patch
Thanks for the review, Dan. I agree with the https restriction for theme updates (we should also file a bug for searchplugins over https only if they aren't already, I guess?). As for the preference, Add-on updates should use the same preference that themes and extensions currently use, in Options > Advanced > Update. Personas *are* themes, just with different behavioural characteristics.
(In reply to comment #39) Very helpful, thanks! > If you need a URL you don't actually need a response > from use something like "lwttest.invalid" or "theme.test" (both TLDs > specifically reserved for such use), and if you do need a response use a real > Mozilla server. Will fix, the test doesn't want a response. > Is there any way for users to turn off theme updates? Users can turn off addon > update checks, they can turn off app update checks, and they can turn off > (albeit without UI) search engine update checks. I guess, yes, these are > treated just like addons and obey the same pref. Yes, the nsExtensionManager.js.in call depends on that pref. > [...] This may lead users to turn off updates which will then have a > negative security impact for addons [...] > I admit this is just my opinion and that I don't have the UI expertise of the > people who are pushing for this change, but I would like to see an extra hidden > .enable pref checked just in case. Will add lightweightThemes.update.enabled > I've seen LWT updates compared to search plugin updates (which in practice > don't update) but one difference is that search plugin update URLs are required > to be SSL while these don't. Addons are required to have an SSL update url or > else have signed update packets. It does not seem unreasonable to require LWT > to use an SSL updateURL. Will do. > Speaking of tracking, in the bug there was talk about allowing %% expansion in > the update URLs but I don't think I'm seeing that in the patch. Looks like you > added the id and version to the header and footer URLs (which don't get > re-loaded unless you change themes) but the updateURL doesn't get any ID or > anything unless the site included it when it gave you the theme data > originally. It can be made to work, but I'd have thought the id/version would > be more useful on the updateURL than the images if we're adding it anywhere. The id and version aren't sent when requesting the remote images. (See below.) Since we don't enforce a pattern on updateURL and it's theme-specific, I don't think it makes sense to substitute anything in there. > > get currentThemeForDisplay () { > > var data = this.currentTheme; > > > > if (data && PERSIST_ENABLED) { > > for (let key in PERSIST_FILES) { > > try { > > if (data[key] && _prefs.getBoolPref("persisted." + key)) > >- data[key] = _getLocalImageURI(PERSIST_FILES[key]).spec; > >+ data[key] = _getLocalImageURI(PERSIST_FILES[key]).spec > >+ + "?" + data.id + ";" + _version(data); > > Isn't this the local file:/// spec for the persisted image? Does adding query > parameters make sense in that case? Yes, this is local, but it's still needed to invalidate the cache for that local image. See bug 522188. > >+ parseTheme: function (aString, aBaseURI) { > ... > >+ data[prop] = _makeURI(data[prop], _makeURI(aBaseURI)).spec; > >+ if (/^https?:/.test(data[prop])) > >+ continue; > > It's good that you're putting limits on this. It looks like this only happens > when you first get the JSON data. If someone altered the persisted JSON string > we might end up with unexpected and possibly harmful URI schemes. I don't know > how much that's worth worrying about, probably not much since I can't think of > any legitimate cases but there might be some other addon trying to be clever. I don't think this is worth worrying about. At the point where something can modify the local JSON string, it can do other harmful things, so this doesn't expose more attack surface. An extension could unintentionally screw it up, but protecting against all such cases seems near to impossible. Not limiting this also makes it possible to ship a product with pre-installed lightweight themes that use local images. > >+ updateCurrentTheme: function () { > >+ > >+ var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > >+ .createInstance(Ci.nsIXMLHttpRequest); > > What happens if the updateURL is https and there's a certificate error? I'm > guessing you'll get a click-through dialog but it'd be better to catch that > error and just not do the update. req.mozBackgroundRequest = true; should already take care of that. > There seem to be a bunch of helper functions that are passed a theme "id" and > operate on the first theme found with that ID. Will ID's be unique? The current > getpersonas.com IDs are just numbers and are probably setting a bad example in > that regard -- if some other site starts doing the same thing we'll get > clashes. Yes, there could be clashes. If two sites use the same ids, those themes will overwrite each other. They are required to be unique locally. > When you update, the updating site could change the update URL to be a > completely different site. We can certainly prevent that. > I'm uncomfortable with the number of sites that can > be involved in a single theme transaction. > recommending site (e.g. Lifehacker "Check out this cool theme") > header image site (cool getpersonas gallery image) > footer image site (transparent gif for tracking) > update site (evil tracking site. Could bounce you to a new tracking > site every day) A transparent gif for tracking isn't going to work, as we'd store it and not request it anymore. However, the server could return a failure code to prevent us from storing something.
Attached patch patch v3 — — Splinter Review
Attachment #410737 - Attachment is obsolete: true
Attachment #412199 - Flags: superreview?(dveditz)
Attachment #412199 - Flags: review?(dtownsend)
Whiteboard: needs new patch → needs review dtownsend / sr dveditz
(In reply to comment #39) > When you update, the updating site could change the update URL to be a > completely different site. I'm uncomfortable with the number of sites that can > be involved in a single theme transaction. > recommending site (e.g. Lifehacker "Check out this cool theme") > header image site (cool getpersonas gallery image) > footer image site (transparent gif for tracking) > update site (evil tracking site. Could bounce you to a new tracking > site every day) Thanks for the thorough review, Dan - I think I agree with most of it and the suggestions are quite helpful, but I think restricting the updateURL in this way is over-restrictive. I can readily imagine cases where changing an updateURL would be beneficial (e.g. personas which change monthly and want update URLs to reflect that, moving from a single host to a CDN, moving from staging/early adopter to publicly available...) whereas I don't see a lot of plausible attacks that could be perpetrated through this mechanism, that aren't already possible by just manipulating which images are returned on a given host (or, if it's a tracking thing, passing around logs on the back end). I think at very least we should remove the restriction from this patch and file a follow up for it, but I really think it creates too heavy a restraint, for too little security win. Dan, did you mean this part more as a "thing to consider", or is it essential to your sr?
My sr requirements were contained in the one paragraph that started "sr-minus". I don't like the loose JSON design, but that's an opinion and I'm not going to be a dick about it by withholding my review when everyone else obviously disagrees. Let me try once more to clarify what I don't like by offering up an alternate design (that would not be much additional work). If you agree you can file a new bug to change things, if you don't at least I tried. Meanwhile I'll review this patch and we can move forward with the improvement to the existing code. Instead of sending a JSON object to start an install, a site should send the URL of a JSON file. This file could have its own MIME type so that personas could be shared from places where firing events can't be done -- e.g. links in twitter or blog comments. Of course the event firing will still work, the content would just be a URL rather than JSON. We could then require that the images referenced be same-origin with the install file itself, and then don't need to worry about the updateURL (other than using SSL). - Blogs can still recommend cool themes - Recommending gets even easier since it's links (the web is made of links) - Theme providers don't have to worry about not getting usage stats since the updateURL is specified under their control - Users don't have to worry about evil random tracking from someone unrelated to the theme provider. Someone who wanted to "steal" a theme would have to actually copy the images or set up a redirect to them which would be more work and probably more detectable.
forgot to add... since the updateURL already points at a file of this type it wouldn't be any extra work for the theme provider and we obviously have the code to deal with it.
(In reply to comment #45) > forgot to add... since the updateURL already points at a file of this type it > wouldn't be any extra work for the theme provider Assuming the theme provider wants to offer updates, yes.
Comment on attachment 412199 [details] [diff] [review] patch v3 >+pref("lightweightThemes.update.enabled", true); Thanks! >+ // We don't allow the update URL to change. However, we allow it to be >+ // unset as a means to stopping future requests. >+ if (newData.updateURL) >+ newData.updateURL = theme.updateURL; Johnathan seemed to think the flexibility of not having this check was important. In my review I thought the updateURL should be the same host, but not that it had to be exactly the same (sites change). Just a comment, I'll leave the resolution up the Fx team. sr=dveditz
Attachment #412199 - Flags: superreview?(dveditz) → superreview+
Whiteboard: needs review dtownsend / sr dveditz → [needs review dtownsend]
Comment on attachment 412199 [details] [diff] [review] patch v3 r+, but remove the part restricting changing the updateURL, I think it provides too many useful capabilities.
Attachment #412199 - Flags: review?(dtownsend) → review+
Whiteboard: [needs review dtownsend]
http://hg.mozilla.org/mozilla-central/rev/13b234ce67cd http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7d24f06f8e68
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
status1.9.2: --- → final-fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(In reply to comment #30) > For testing: > > {"id":"update-test","name":"Update > Test","headerURL":"about:blank","updateURL":"http://design-noir.de/mozilla/themes/update-test.php"} Dao, can i get some instructions on how to utilize this testcase to find updates? I understand how to apply this to error console, and i see the "update test" theme in theme manager, but i still get a grayed out "Find Updates" button. What am i missing to fully test this fix? Thanks.
The update happens automatically, there's no UI. So you can either modify some extension manager prefs to get the automatic update earlier or call LightweightThemeManager.updateCurrentTheme() in the error console.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: