1452707 - Build site patch for ib.absa.co.za

Status: RESOLVED FIXED

(Web Compatibility :: Interventions, enhancement, P1)

Description

[Dennis Schubert [:denschub]]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1448045#c4, ib.absa.co.za ises user agent matching, which results in Firefox getting a "Browser not supported" warning, although there is no technical reason for it.

Adding "webkit" to the UA string should already be enough to make the site work. This bug is to track the minimal override and the rollout of said patch.

Comment 1

[Dennis Schubert [:denschub]]

Attachment: Pull Request #21, Adding the UA override

PR is against the `release` branch, as this is the latest version approved and landed in mozilla-central, which means we could roll this out via GoFaster at any time, and not add any new features, or potential perf regressions.

r? Tom, f? Mike, as Bugzilla does not like having two r?s on one patch.

Comment 2

[Dennis Schubert [:denschub]]

Attachment: WebCompat v.1.1.1, Unsigned XPI

Attaching an unsigned XPI for testing purposes.

Comment 3

[Dennis Schubert [:denschub]]

Only adding "AppleWebKit" to the User Agent without any version number did not work, as they try to parse the version number and their source throws if it's not there, so we need to append "AppleWebKit/605.1.15". With the XPI installed (and Firefox restarted), I can verify that I no longer see the "Unsupported" message, and the site *seems* to work.

That being said, I do not have a testing account there, so the internal impact may be higher. ni? Mike, as I do not feel 100% comfortable with this for some reasons:

* We cannot verify that the application works when signed in.
* Spoofing the UA actually breaks mobile, but since the GoFaster add-on is not on Mobile yet, this is fine. With 2.0, we have to possibility to only target Desktop, so we shouldn't have any issues here.
* There are a lot of user-agent specific cases, where they check for the browser and do different things. I first tried to spoof as opera, but it did break keyboard event handling, as they have an exception for that. I checked all the instances of WebKit-specific logic in the sources I can see, but I cannot test the site to its full extend. The JS does not feel complete enough to be used for the logged-in part as well, so there are likely more JS files I cannot check.

As the changes in question only break the site in the 61 tree, we should not do a 100% rollout of this override, probably. If anything, it should be limited to the version affected (61+), which would also give us a bit more time to try outreach here.

Mike, what do you think?

Comment 4

[Mike Taylor [:miketaylr]]

> As the changes in question only break the site in the 61 tree, we should not do a 100% rollout of this override, probably. If anything, it should be limited to the version affected (61+), which would also give us a bit more time to try outreach here.

So, we haven't actually removed window.controllers yet, it's just disabled in Nightlies. So if you think it would be safer for us to block on v2, I support that. It seems crappy to break mobile.

Also yeah, it's kinda scary to make these changes on bank without an account. Could you try to PM the user who reported the issue, and see if they could install an XPI and test @ https://support.mozilla.org/en-US/user/johannesstander83? (https://support.mozilla.org/en-US/questions/997702)?

Comment 5

[Dennis Schubert [:denschub]]

Okay, I thought about this again, and as a result, I made this depending on the GoFaster v2 shipping.

With a UA override, the site will treat us as a different browser, which leads to some side effects I'm not in control over. And while I can get in touch with the reporter on SuMo (which I'll do) and have an override tested, I'd still feel much more comfortable with another solution.

Here, they only really check if window.controllers is not falsy. With GoFaster v2, we could easily set window.controllers to a string "window.controllers has been shimmed with a string to get this site's browser detection to work in Firefox 61+", and have the site detecting us as Firefox.

That would make me much more comfortable deploying this.

Comment 6

[Dennis Schubert [:denschub]]

Attachment: Pull Request #22, Adding the site patch

I built a site patch on top of version 2 instead, this is the PR. As Tom is now also a member of the GitHub org, I asked for explicit review there, so I don't have to mess with r? flags here on Bugzilla.

Comment 7

[Dennis Schubert [:denschub]]

Attachment: WebCompat v.2.0.1, Unsigned XPI

And here is an XPI if someone wants to test the site patch without building the addon themselves.

Comment 8

[Dennis Schubert [:denschub]]

Comment on attachment 8967694 [details]
WebCompat v.2.0.1, Unsigned XPI

Marking old XPI as obsolete, as the code got rebased due to changes during the m-c review. Will create a new XPI as part of the patch containing multiple site patches.

Comment 9

[Dennis Schubert [:denschub]]

Site patch is landed in our repo on GitHub, so this can be closed. Landing the site patches (and eventually updating Release users via GoFaster) is tracked in 1481395.