simplesamlphp / SAML-tracer

Browser extension for examining SAML messages
https://addons.mozilla.org/nl/firefox/addon/saml-tracer/
BSD 2-Clause "Simplified" License
141 stars 39 forks source link

changes for other browsers #45

Closed khlr closed 6 years ago

khlr commented 6 years ago

This is a pull request that @alanbuxey originally proposed in my fork of SAML-tracer. Thanks again, Alan!

The modification to SAML-tracer, contained in this pull request, allow the extension to be used in other browsers besides Firefox as well; namely Chrome and Opera. I think that's a great enhancement and intruduces a bunch of new possibilites! (For example, just the other day I was talking to a customer who is constrained to use IE or Chrome; so this'd be an ideal solution for him to benefit from SAML-tracer as well!)

@jaimeperez, could you please submit SAML-tracer to the Chrome web store (https://chrome.google.com/webstore/category/extensions), too? This would be the only "downside" that I can think of: The extension would have to be committed to two extension stores (Firefox + Chrome). Are you ok with that?

thijskh commented 6 years ago

A quick review says it's a bit hard to see the changes due to mixed whitespace/formatting changes. Will filter them out and review later.

khlr commented 6 years ago

Thank you, Thijs! Yep, it's unfortunately a bit unclear because I adjusted the indentation in those files.

khlr commented 6 years ago

@jaimeperez, If you come across this PR soon, please don't merge it yet. I noticed a bug that occurs in Chrome that appears when POSTs are issued that exceed 4096 bytes.

I think I'll be able to fix this at the weekend. I'll then elaborate on this issue a bit more detailed.

alanbuxey commented 6 years ago

good catch :)

alan

thijskh commented 6 years ago

Note that my approval is from a code review and testing in FF; not from testing in any other browser.

khlr commented 6 years ago

Commit c5da896 fixes the bug I mentioned recently:

This issue affected requests larger than 4096 bytes (which is pretty much every request containing a token). Chrome behaves differently here than Firefox: While Firefox also parses large requests as formData, Chrome only offers access to the raw bytes in this case. The raw request bytes must therefore be parsed manually.

khlr commented 6 years ago

@thijskh, if you'd be so kind to do a second review, I'd like to ask you, to directly take a closer look at #46, because it contains the changes from this PR plus an additional bugfix.

To everyone else: If someone could test these changes in Chrome, that would be very helpful! And a parallel Firefox review wouldn't hurt, either :-) Thanks!

jaimeperez commented 6 years ago

At first sight, this looks good to me! 👍

(although I have neither tested it nor looked deeply into the code)

Has anyone tested with both Firefox and Chrome?

Regarding the Chrome Web Store, I can do that once I find out how I can get the registration fee sorted out 😄

jaimeperez commented 6 years ago

I've just tested it in Chrome and it works like a charm! 👍 🍾

jaimeperez commented 6 years ago

... aaaand now we have SAML-tracer 1.5 published 😄

Thanks a lot again @khlr! Also, looking at the contributions stats you are now by far the biggest contributor to SAML-tracer, so adding you as an author in the manifest is the least that I could do. After all, this new extension is basically your work. Hope you don't mind 🙂

khlr commented 6 years ago

Wow, thanks a lot 😊

khlr commented 6 years ago

Regarding the Chrome Web Store, I can do that once I find out how I can get the registration fee sorted out 😄

You didn't find a way to get around that fee, did you, @jaimeperez?

jaimeperez commented 6 years ago

No, I'm afraid there's no way around it except for paying it 😉

Shouldn't be a problem though, I don't see a reason why we shouldn't just cover the expense.

jaimeperez commented 6 years ago

There we go!