Manifest V3 update.

[kellenmurphy]

I took a stab at Issue #79. For the most part this was a straightforward, following the Chrome Migration Path.

Changes

• browserAction has been replaced by a more generic action.
• there are some permissions changes, namely that '' is now a host permission and that we don't need webRequestBlocking (see note below re: the reviseRedirectedRequestMethod issue) raised in #79 by @khlr.
• we now refer the background script as a "service worker" instead of a background script

Notes

• We do not want to merge this now to main, as Manifest V3 is unsupported by Firefox at the moment, though will be out later this year. I think it may make sense to keep this around in a feature branch until then, however, so I'm contributing the code I have now.

  • I checked and even my Firefox Dev Edition (104.0b3), which has an about:config option (extensions.manifestV3.enabled) to "support" MV3, doesn't yet support service workers... so we're kind of dead in the water with MV3 development at the moment.

  • Re: the necessity of blocking permissions for reviseRedirectedRequestMethod

  • I took a look at where the requirement for this code came from in the original issue cited within the code, and referencing this StackOverflow post and I have to be completely honest here, try as a I might I was unable to replicate. Obviously, I'm not leveraging the versions of Chrome and FF from back then, but despite throwing many test cases of POSTs followed by GETs I did not see untracked requests within SAML-Tracer.

  • That said, that does not prove that we don't have an issue. The absence of evidence is not the evidence of absence. In particular, either (a) webRequest has changed since that time, or (b) I'm not replicating the issue correctly.

  • Re: (a), I'm not in a position to comment since I've not done this research. :wink:

  • Re: (b), I'm not in a position to comment since I'm the cause of that issue.

  • For what it's worth, I think there's a strong likelihood that I'm not replicating the previous issue correctly... but I will expound on what I did:

    • I tested with the https://wiki.surfnet.nl/ SP, as described in the original issue: https://github.com/SimpleSAMLphp/SAML-tracer/pull/23#issuecomment-344970423

    • I tested with my own Shibboleth and SimpleSAMLphp dev environment and forcing POSTs followed by GETs.

    • I tested with two Shibboleth IDPs that are configured as proxied instances to upstream IDPs... one a "production" quality system I maintain that upstreams to Okta, and another my development proxy instance running in Azure that's got a selectable selection of upstreams (Shibboleth, WSO2 IS, Azure, and Keycloak).

    • At no point with ANY of these was I able to "break" things insofar as non-tracked SAML assertions being logged.

  • So while I do not want to say with 100% certainty that "it's working", I would be relatively comfortable stating that I have some degree of confidence that it's working, and that I think the next step is for some others to test to confirm my findings / validate that they can't replicate.

  • And lastly, by "replicate" I should also clarify to mean that I cannot get these lines of code to activate under any of the test circumstances described above.

[khlr]

Thank you so much for this PR, @kellenmurphy! I really like your detailed explanations!

Regarding the reviseRedirectedRequestMethod-issue:

IIRC case 1 affected Chrome and Firefox. At least at that time. Since case 2 is/was a FF-only special treatment we can ignore it for the moment. So I focused on case 1. And yep, I can't reproduce it either! But have a try using Firefox (without your MV3-changes). Then you can activate the mentioned lines! Maybe have a try, too. Go here and then select Academisch Medisch Centrum (AMC). If you set a breakpoint within the if, you'll reach that line! But only in FF and not in Chrome...

Possibly Chrome changed "something" in the meantime... So maybe both cases are FF-only speacial treatments by now ¯\_(ツ)_/¯

Hence I think at the moment we can't clarify if altering the method will break something in the future...

[kellenmurphy]

@khlr great insights! I will check this out later this week to see if I can replicate, and I will also update the keyboard-shortcut code.

Thanks!!! I'll have more soon... (my week is very front-heavy).

[kellenmurphy]

Just an update... I'm still planning to work on this, I've just had to be away for a bit due to a death in the family. I'll be circling back to this soon!

[tvdijen]

My sincerest condolences @kellenmurphy ! First things first!

[kellenmurphy]

Thanks @tvdijen...

FWIW I just fixed the keyboard shortcut, but I do still need to do some digging in line with @khlr's comment above. We also obviously need to wait for Firefox to start supporting MV3.

[khlr]

Hello @kellenmurphy 👋

How are you? Do you think you'll find the time picking up this issue again?

[kellenmurphy]

Sure! I can probably get to that next week. 😀

Kellen Murphy @.***

---

From: Jan Köhler @.> Sent: Thursday, September 28, 2023 9:51:45 AM To: simplesamlphp/SAML-tracer @.> Cc: Kellen Murphy @.>; Mention @.> Subject: Re: [simplesamlphp/SAML-tracer] Manifest V3 update. (PR #80)

Hello @kellenmurphyhttps://github.com/kellenmurphy 👋

How are you? Do you think you'll find the time picking up this issue again?

— Reply to this email directly, view it on GitHubhttps://github.com/simplesamlphp/SAML-tracer/pull/80#issuecomment-1739251224, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHHM43YIFPNKCURY4FIFFVLX4V6HDANCNFSM55AWDX5Q. You are receiving this because you were mentioned.Message ID: @.***>

[kellenmurphy]

@khlr I'm picking this back up this week. I've rebased onto simplesamlphp/main to pick up the recently approved PC, and squashed the previous commits to clean up the PR (and amended the commit message text).

I'm planning to carve out some time this week to thoroughly test this... it's been over a year (!) since I last looked at this so I want to vet this thoroughly.

[kellenmurphy]

@khlr I inadvertently requested review. Disregard. Mis-click. See my last message this isn't ready to review yet.

[rileyw]

Any thoughts on next steps for this PR recognizing that this extension will stop working in Chrome with manifest-v2 extensions will stop working "soon?"

[thijskh]

I've given it a test on Chrome and my scenarios seem to work just fine, including several different SAML flows, a WSFED flow and import/export. So that's already looking great 👍

When trying to load the zip built from this PR into Firefox 129 I get an error that the extension is damaged (without meaningful information to me what is exactly wrong).

It might be nice to rebase this PR since there's a (simple) merge conflict now in manifest.json.

[tvdijen]

I've taken care of the rebase.. I've also figured out what is wrong with the extension to be able to use it in Firefox. Mozilla provides a web-ext npm-package that can be used to verify your extension and sign it. The tools reports the following issues:

Also see: https://extensionworkshop.com/documentation/develop/getting-started-with-web-ext/#Automatic_extension_reloading

[tvdijen]

OK, so apparently Chrome stopped using the scripts in v3 and only supports service workers, and Firefox never adopted the service workers and still requires scripts. Firefox will refuse any manifest file that has content it doesn't recognize.. Long story short: I think there's no other solution than to create separate builds for Chrome & Firefox with their own specific manifest.json ...

Update: It is possible to use both! See https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/background#browser_support

I can however still not load it in Firefox.. Maybe this is because xpinstall.signatures.required can only be set to false in the developer- & nightly editions of Firefox. I will try this later tonight.

[tvdijen]

Ok, second update: I can install the addon in Firefox developer edition. Apparently Firefox doesn't allow disabling signature-checking on regular versions.. Everything works fine, so I think we can just merge this and get it released

[tvdijen]

@khlr With your permissions I'm merging this and tagging a new 1.8 release. I understand you have the ability to upload this release to the app stores? I think we should just try it and see what next bump in the road we hit, but I'm fairly confident with it now.

[khlr]

I could check out the code tomorrow and test it a bit. Is there any interest? Wouldn't hurt imho. 🤓

[tvdijen]

Sure, take your time and test. Just make sure to use Firefox Developer edition.. Chrome is fine with loading unsigned unpacked add-ons.

[khlr]

I can't download the Firefox Developer Edition right now. That's why I only looked at it with Chrome by now.

Chrome lists 2 warnings (no errors, “just” warnings"):

1. 'background.scripts' requires manifest version of 2 or lower.
2. Unrecognized manifest key 'browser_specific_settings'.

Warning 2 can probably be ignored as Chrome just seems to be unaware of this option. It's not particularly “nice” that it becomes a warning. But we can certainly live with it if you can't somehow teach Chrome not to complain here?

Regarding warning 1: Can't we just drop the line "scripts": ["bootstrap.js"] since it's not required for mv3 extensions anyway?

[tvdijen]

Yes it is required.. Firefox never implemented service workers in their implementation of manifest v3. That's the entire point I made in my comments yesterday and the day before.. Also see my screenshot above and this comment.

[khlr]

Oh boy... Sorry, I didn't reall take note of that 🫣

[khlr]

I just tested it using FF. Looks good, everything seems to run as expected :)

As expected, however, FF ‘warns’ of the unknown background.service_worker... 😉 At some point, FF will surely catch up with Chrome, and hopefully we will no longer have to use the different properties.

From my point of view, there is now nothing to be said against merging this PR 👍