Opened 12 months ago

Last modified 9 months ago

#7153 closed defect

setUninstallURL: String must not be more than 255 characters long. — at Version 5

Reported by: greiner Assignee:
Priority: P2 Milestone: Adblock-Plus-3.5-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, kzar, sporz, Kirill Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/21/

Description (last modified by kzar)

Environment

Chrome 70
Adblock Plus 3.4.2.2206

How to reproduce

Open background page console.

Observed behaviour

The following error is shown:

Uncaught (in promise) Error: Invalid value for argument 1. String must not be more than 255 characters long.
    at validate (extensions::schemaUtils:34)
    at Object.normalizeArgumentsAndValidate (extensions::schemaUtils:119)
    at Object.<anonymous> (extensions::binding:363)
    at Object.value [as setUninstallURL] (polyfill.js:108)
    at Object.exports.setUninstallURL (uninstall.js:60)
    at Promise.all.then.then.then.then (subscriptionInit.js:311)

Expected behaviour

No such error is shown.

Notes

The uninstall URL it tries to set has 256 characters:

https://adblockplus.org/redirect
  ?link=uninstalled
  &lang=en-US
  &addonName=adblockpluschrome
  &addonVersion=3.4.2.2206
  &application=chrome
  &applicationVersion=70.0.3538.110
  &platform=chromium
  &platformVersion=70.0.3538.110
  &notificationDownloadCount=30-89
  &corrupted=0

What to change

Since we need to keep the URL under 255 characters, we need to keep the key names as short as possible. For #6655 where we ended up shortening the "dataCorrupted" parameter to "corrupted", but this time we need to do more.

Of the submitted query parameters, we're stuck with (for now) the two which are used to determine the page being viewed; link and lang.

But the others, we can do something about. Let's use these abbreviations:

Current name Abbreviation
addonNameaN
addonVersionaV
applicationap
applicationVersionapV
platformp
platformVersionpV
notificationDownloadCountnDC
corruptedc

Change History (5)

comment:1 Changed 12 months ago by greiner

Looking through the code, I came up with the following values:
assuming application and platform versions follow the pattern /.{2,4}(\.{1,4}){0,3}/

Separators Key Value {min,max}
https://adblockplus.org/redirect 0 32 0
link 2 4 9
lang 2 4 {2,6}
addonName 2 9 {15,18}
addonVersion 2 12 {1,23}
application 2 11 {5,7+}
applicationVersion 2 18 {2?,19?}
platform 2 8 {5,8}
platformVersion 2 15 {2?,19?}
notificationDownloadCount 2 25 {1,6}
corrupted 2 9 1
total 20 147 {43?,116?+} {210?,283?+}

Example of a string with 283 characters:

https://adblockplus.org/redirect
  ?link=uninstall
  &lang=es_419
  &addonName=adblockplusfirefox
  &addonVersion=10000.10000.10000.10000
  &application=firefox
  &applicationVersion=1000.1000.1000.1000
  &platform=chromium
  &platformVersion=1000.1000.1000.1000
  &notificationDownloadCount=90-179
  &corrupted=0

That leads me to think that compressing each of the keys to only three characters could be sufficient for now and would still leave us with a bit of room to expand.

comment:2 Changed 11 months ago by kzar

That leads me to think that compressing each of the keys to only three characters could be sufficient for now and would still leave us with a bit of room to expand.

I wouldn't mind doing that as long as the code still makes it clear what the values we submit are, for example with comments or otherwise making it obvious. E.g. // dc = dataCorrupted?. I mention this because AdBlock doesn't, I remember reading through their code to figure out which bits of data they were submitting. I don't want to give the impression that we're hiding something, or doing something shady.

Last edited 11 months ago by kzar (previous) (diff)

comment:3 Changed 11 months ago by kzar

  • Cc sporz Kirill added

Copying in some data folks. What do you think, would it be acceptable to replace the key names with one/two letter abbreviations? If so, any preferences what they should be?

comment:4 Changed 11 months ago by sporz

Acknowledged - we can work with any abbreviation you come up with. Please give us a heads up before it goes live, so we can adjust our processing.

comment:5 Changed 11 months ago by kzar

  • Description modified (diff)
  • Review URL(s) modified (diff)

Alright, how about this?

Note: See TracTickets for help on using tickets.