Opened 9 days ago

Closed 2 days ago

#7153 closed defect (fixed)

setUninstallURL: String must not be more than 255 characters long.

Reported by: greiner Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-for-Chrome-Opera-Firefox-next
Module: Platform Keywords:
Cc: sebastian, kzar, sporz, Kirill Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
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

Which brings the number of characters used by the key names down to 24, which by Thomas' calculations means the total URL length will now be between 119 and 192 characters.

Hints for testers

Uninstall Adblock Plus and look at the URL for the uninstall page which opens. Ensure that the parameters are all passed correctly, using the abbreviations listed above.

Change History (14)

comment:1 Changed 9 days 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 9 days 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 9 days ago by kzar (previous) (diff)

comment:3 Changed 9 days 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 8 days 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 6 days ago by kzar

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

Alright, how about this?

comment:6 Changed 6 days ago by kzar

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

comment:7 follow-up: Changed 6 days ago by kzar

With #6840 making it impossible to send uppercase query string parameters on Edge, I wonder if the parameter name abbreviations we pick should all be lowercase? So far I used capitalisation in an attempt to make them easier to understand.

comment:8 Changed 5 days ago by kzar

  • Owner set to kzar
  • Priority changed from Unknown to P2
  • Ready set
  • Review URL(s) modified (diff)

comment:9 Changed 5 days ago by kzar

  • Status changed from new to reviewing

comment:10 Changed 5 days ago by kzar

I've gone ahead and triaged this, and implemented it. Please, if you have any problem with the abbreviations I've picked speak now!

comment:11 in reply to: ↑ 7 ; follow-up: Changed 5 days ago by sebastian

Replying to kzar:

With #6840 making it impossible to send uppercase query string parameters on Edge, I wonder if the parameter name abbreviations we pick should all be lowercase? So far I used capitalisation in an attempt to make them easier to understand.

+1. That's the same I was thinking when I read this issue's description.

comment:12 in reply to: ↑ 11 Changed 2 days ago by kzar

  • Description modified (diff)

Replying to sebastian:

+1. That's the same I was thinking when I read this issue's description.

Cool alright, I'll make them all lowercase.

comment:13 Changed 2 days ago by abpbot

A commit referencing this issue has landed:
Issue 7153 - Abbreviate the parameter names in uninstall URL

comment:14 Changed 2 days ago by kzar

  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.