SAMLTracer.js Error in Firefox Developer Edition (103.0b2)

[kellenmurphy]

SAML-tracer appears to have stopped functioning in the latest Firefox Beta (Developer Edition tracks with Beta)

Uncaught Error: Type error for parameter extraInfoSpec (Error processing 2: Invalid enumeration value "extraHeaders") for webRequest.onBeforeSendHeaders.
    init moz-extension://21213b32-20d7-404e-b9f3-25e9c5930f22/src/SAMLTrace.js:943
    <anonymous> moz-extension://21213b32-20d7-404e-b9f3-25e9c5930f22/src/ui.js:15
[SAMLTrace.js:943:42](moz-extension://21213b32-20d7-404e-b9f3-25e9c5930f22/src/SAMLTrace.js)
    makeError resource://gre/modules/Schemas.jsm:539
    throwError resource://gre/modules/Schemas.jsm:2659
    fixedArgs resource://gre/modules/Schemas.jsm:2727
    map self-hosted:180
    checkParameters resource://gre/modules/Schemas.jsm:2720
    addStub resource://gre/modules/Schemas.jsm:2956
    init moz-extension://21213b32-20d7-404e-b9f3-25e9c5930f22/src/SAMLTrace.js:943
    <anonymous> moz-extension://21213b32-20d7-404e-b9f3-25e9c5930f22/src/ui.js:15

I've tried tried disabling all other extensions, etc. This occurs only with FF 103 insofar as I can tell... which will be the "main" version of Firefox come July 22. I don't know precisely that this is an issue with SAML-tracer vs. FF, though a cursory search on Bugzilla doesn't reflect anything related to changes/bugs with webRequest.onBeforeSendHeaders.

Screenshot:

Could any of the maintainers provide some insight? I typically use developer edition just so that I have a completely separate browser for debugging SAML (hate clearing cache on my main browser). I can work around this tentatively, but I'd hate to see FF103 drop in July and break SAML-Tracer.

[thijskh]

I think this is where it goes awry:

SAML-tracer/src/SAMLTrace.js

Lines 907 to 913 in a500371

	let isFirefox = () => {
	return typeof InstallTrigger !== 'undefined';
	}
	let getOnBeforeSendHeadersExtraInfoSpec = () => {
	return isFirefox() ? ["blocking", "requestHeaders"] : ["blocking", "requestHeaders", "extraHeaders"];
	};

Apparently your browser does not satisfy the test in the method "isFirefox()". So a value is sent that Firefox does not understand. Question is why your Firefox is not Firefox.

[thijskh]

This Bugzilla item seems to describe more about the InstallTrigger deprecation and explicitly mentions the use of this construct to test for Firefoxness. And if understand it well, also that in "late beta" they will make using InstallTrigger for this purpose work again?

[tvdijen]

We might consider doing an actual user agent check instead of using this strange InstallTrigger global?

[kellenmurphy]

@thijskh you're spot on. In "about:config" I checked and extensions.InstallTrigger.enabled was set to "false". Setting that to "true" makes SAML-Tracer work once more, though now with a deprecation warning in the console:

InstallTrigger is deprecated and will be removed in the future.

My reading also matches yours... by release they'll make InstallTrigger work.

Thanks so much @thijskh for taking a second look at this. It does seem that perhaps at some point in the future, there may need to be a refactor to remove the InstallTrigger references and -- I guess? -- implement UA checking or something? I think this issue can probably be closed now, though, don't you?

[khlr]

This is a really interesting issue! 🙂👍

I think you have to ask yourself why isFirefox() is needed at all. It was introduced with pull request #61 because Chrome 72+ requires extraHeaders to access some certain HTTP headers. Therefore, "is Firefox" is actually misleading. It is rather the question whether it "is Chrome".

We could easily utilize the user agent string to determine the browser type. By doing something like navigator.userAgent.indexOf("Chrome") !== -1. But user agent detection should be avoided if possible.

Instead we could use feature detection and check if the "feature" "extraHeaders" is available. Probably by doing something like this:

var browser = browser || chrome;
const useExtraHeaders = () => {
  try  {
    const dummyListener = req => console.log(req);
    browser.webRequest.onBeforeSendHeaders.addListener(dummyListener, {urls: ["<all_urls>"]}, ["extraHeaders"]);
    browser.webRequest.onBeforeSendHeaders.removeListener(dummyListener);
    // seems to be Chrome which supports an requires "extraHeaders"
    return true;
  } catch (err) {
    // seems to be Firefox since "extraHeaders" is not supported
    return false;
  }
}

console.log(useExtraHeaders());

But I'm afraid this is a pretty expensive piece of code to check for the feature... Any ideas?

[tvdijen]

Is it actually still a problem to pass the extra parameter in Firefox? Because it is in the Mozilla MDN and nothing on that page indicates it wouldn't work on Firefox.. Is it perhaps something from the past?

If we still need it, it should perhaps be possible to test the amount of parameters onBeforeSendHeaders takes? Like browser.webRequest.onBeforeSendHeaders.length? That would work for methods, but I'm not sure it will also work for events..

[khlr]

The problem isn't addListener's third parameter (extraInfoSpec). It's rather the values that extraInfoSpec can hold.
While the Chrome-webRequest-API requires the value extraHeaders (see here), the Firefox-webRequest-API can't handle that value (the docs don't mention it at all) and FF throws the exception that Kellen reported: Invalid enumeration value "extraHeaders"

[tvdijen]

Ah, my bad.. Forget what I said :)

[thijskh]

Ideally we'd change the detection method. But I would only change it if it's actually a significant improvement. The code suggested by @khlr seems indeed quite a lot of code/effort for the simple case.

They are adding the object back in late beta (even if it's only null) precisely because the way we test for it was "the way to do it" and many extensions did it. So I'm not afraid it will break quickly in the future. It would seem a premature optimization to replace this simple check with complex code if the simple check is not likely to go away for the forseeable future.

So long story short, I'd like to replace it only if there's a similarly straightforward check available.

[tvdijen]

How about the window.chrome var?

[kellenmurphy]

I think there may be a predefined constant can be leveraged here (see: https://stackoverflow.com/questions/69287721/how-to-manipulate-webrequest-cookie-in-a-cross-browser-extension).

Specifically, the answer:

The API exposes all predefined constants in chrome.webRequest.OnXXXXXXXXX objects for each event so only in new Chrome such objects will have EXTRA_HEADERS key with extraHeaders value whereas in Firefox and old Chrome it'll be undefined, which can be filtered out via filter()

chrome.webRequest.onBeforeSendHeaders.addListener(
  listenerFunc,
  { urls: ['*://*/*'] },
  ['blocking', 'requestHeaders', 
   chrome.webRequest.OnBeforeSendHeadersOptions.EXTRA_HEADERS].filter(Boolean)
);

I'll try to test this... would be happy to create a PR with that change if it works.

It seems to me that a simple check that can be applied with initiating the listener seems like the simplest solution long term, and potentially the new "right way" to do things once InstallTrigger is removed.

[khlr]

Thank you for this contribution, Kellen! 🙂
It's good to have this change in the codebase!

However, I think we can do without a release for now, since InstallTrigger is being made available again.

[kellenmurphy]

You're very welcome @khlr! I've been using this valuable tool for so long I figured I shouldn't be afraid to dig into it...

I totally agree with you re: release... likely anyone knowledgeable to notice this problem (using FF dev or beta) is knowledgable to google it and find the solution anyway, but being prepared for the eventual deprecation of InstallTrigger now isn't a bad thing 😀