search

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

Bugfix/fix 302 redirection #46

Closed khlr closed 6 years ago

khlr commented 6 years ago

This PR fixes a problem with redirected 302 requests, which I noticed while working on the PR for the Chrome Compatibility (#45):

For redirected requests, it is known that to be necessary to manually adjust the redirected request (see here). However, during this manually adjustment it's additionally necessary to include the HTTP method in the UniqueRequestId. This is necessary in order to determine the redirected request from the traced requests (collected in httpRequests). If the HTTP method is not taken into account in the UniqueRequestId, the first request (and not the subsequent second one) will always be found and thus displayed twice in the UI.

Please note: To avoid potential merge conflicts I branched directly from #45. So this PR may seem bigger than it is. So it'd be advisable to first merge #45 and take a look at the then probably smaller diff again ;-)

jaimeperez commented 6 years ago

Should we merge this directly then and close #45?

khlr commented 6 years ago

In the meantime @thijskh has told me about another problem with redirects. This time it's about 303 redirects. RFC 2616 says that these 303 redirects should be followed by using a GET. So the method of the originating request should not be preserved (more or less the opposite of 302. "However, most existing user agent implementations treat 302 as if it were a 303 response").

Long story short: Since SAML-tracer should act the same way as the browsers do, we need to manipulate the way 302 und 303 redirects are. This way SAML-tracer conforms to the browsers' behaviour but ignores (in case of 302) the spec (because the browsers ignore it, too). In both cases (302 and 303) the method of the request following the redirect will be simply revised to a GET.

I'll put another commit on top of this PR in a sec, which contains the aforementioned changes.

khlr commented 6 years ago

This PR now fixes three bugs:

  1. The 302 redirection bug, mentioned in the description of this PR
  2. The 303 redirection bug, that @thijskh noticed
  3. And an incorrect way of decoding application/x-www-form-urlencoded values as noticed by @tvdijen in #49

@jaimeperez, do you think you'll find some time in the near future to merge this PR and release a v1.5.1 of SAML-tracer to get rid of these bugs? Thank you in advance!

jaimeperez commented 6 years ago

Thanks a lot for the bugfixes! Shouldn’t be a problem to have a new release with them on Monday 😉

-- Jaime

On 21 Jul 2018, at 14:21, Jan Köhler notifications@github.com<mailto:notifications@github.com> wrote:

This PR now fixes three bugs:

  1. The 302 redirection bug, mentioned in the description of this PR
  2. The 303 redirection bug, that @thijskhhttps://github.com/thijskh noticedhttps://github.com/Uninett/SAML-tracer/pull/46#issuecomment-406792244
  3. And an incorrect way of decoding application/x-www-form-urlencoded valueshttps://github.com/Uninett/SAML-tracer/issues/49#issuecomment-396059828 as noticed by @tvdijenhttps://github.com/tvdijen in #49https://github.com/Uninett/SAML-tracer/issues/49

@jaimeperezhttps://github.com/jaimeperez, do you think you'll find some time in the near future to merge this PR and release a v1.5.1 of SAML-tracer to get rid of these bugs? Thank you in advance!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/Uninett/SAML-tracer/pull/46#issuecomment-406793289, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AB2kyEmnnhHF3YDG0yzaCfT4ud6hZk9bks5uIxy2gaJpZM4S6P45.

jaimeperez commented 6 years ago

Merged, released and published to both Firefox and Chrome web stores! Thanks a lot @khlr!