Seems like an issue with MediaViewer, not CentralNotice. MediaViewer is creating invalid URIs here. I have confirmed that these malformed URIs are not coming from an end-user typing something or gadget etc. It is MediaViewer itself encoding them this way.

Steps to reproduce without CentralNotice:

1. https://da.wikipedia.org/w/index.php?title=Justin_Bieber&oldid=10541210&debug=true
2. Under "Diskografi" click on the blue padlock image. (Yes, this UI icon could be excluded from mediaviewer, but that's not the problem here.)
3. The URL is now https://da.wikipedia.org/w/index.php?title=Justin_Bieber&oldid=10541210&debug=true&mobileaction=toggle_view_desktop#/media/Fil:Nuvola_apps_download_manager2-70%.svg

There are now two observable issues.

• Issue 1: Running new mw.Uri() in the console at this time yields:

Uncaught URIError: malformed URI sequence

• 2. Issue 2: If you reload this address or share the URL with someone else, they will not see the image in question. Instead, MediaViewer crashes on startup and prints the following error to the console:

TypeError: defaultUri is undefined

This suggests MediaViewer is correctly expecting an encoded URL on startup and trying to decode it, but that it isn't actually encoding it on the way into the address bar.

Since this involves nothing but canonical article URLs and standard UI navigation (no hacks etc.) this should not result in an invalid URI. Seems similar to T268060, but looks like it's not exactly the same, so will not be fixed after this train rolls out.

In T269948#6693151, @Krinkle wrote:

Seems like an issue with MediaViewer, not CentralNotice. MediaViewer is creating invalid URIs here. I have confirmed that these malformed URIs are not coming from an end-user typing something or gadget etc. It is MediaViewer itself encoding them this way.

Thanks so much for digging in, @Krinkle, @Jdlrobson and @Reedy! Would it be possible to get a quick perspective on how much this affects users? I didn't understand there was any significant user impact, apologies. Regardless where the root of the problem lies, the change proposed for CN seems straightforward and the right thing to do, in any case. If it's worth useful, we can definitely take a few minutes to push out a CN change on a backport deploy.

Thanks again!

The impact is that when someone is given a link to a full-screen MediaViewer lighbox for an image that contains a % in its file name, then they will see neither the lightbox nor a centralnotice banner. I expect this to be extremely rare. Unfortunately, we don't know how common ths issue is exactly, because for 95% of browsers we pretty discard all client-side errors without logging them (T266517).

I agree a defense in-depth approach from the CentralNotice side would also make sense, assuming that the code in question can be made optional/graceful. I imagine there are cases where it's required to be able to read/parse or serialise/format a URL string. But if this is not one of those cases then using a try-catch would make sense here, with the catch doing whatever would normally happen if there were no query parameters present.

In T269948#6693286, @Krinkle wrote:

The impact is that when someone is given a link to a full-screen MediaViewer lighbox for an image that contains a % in its file name, then they will see neither the lightbox nor a centralnotice banner. I expect this to be extremely rare.

K thanks so much! Ahh ok so, if I understand correctly, adding the try/catch to CN won't solve any issues outside CN not showing a banner on malformed URIs, right? Mostly the CN error in just alerted to the malformed URIs, correct?

If that's the case, I think we should make a separate task to add a try/catch for the call to mw.Uri() in CN (and in the catch block we probably would just emit a warning or something). Does that sound right?

Hi. Has a new task been made for this as mentioned by @AndyRussG ? I'll like to take this / new task up. Also I agree with the approach suggested by him to add a try{} catch{} block to the call to mw.Uri(). If the author / maintainer agrees with that I can go ahead with it.

@SarthakKundra try and catch could be one way to go here.

The offending line in Central Notice is:
var urlParams = $.extend( state.urlParams, ( new mw.Uri() ).query ),
https://github.com/wikimedia/mediawiki-extensions-CentralNotice/blob/8225a51b13cce067006fa31500621aa41e33e418/resources/ext.centralNotice.display/state.js#L156

@Jdlrobson Thanks. I'll add a block. Sorry for the late reply, seems like I missed the notification.

Hi @Jdlrobson. I can't find the steps to setup the project locally on Github or Gerrit. Can you help me with that? Thanks!