Skip to content

Enable optional POST redirects#408

Open
andreasonny83 wants to merge 3 commits intoeinaregilsson:masterfrom
andreasonny83:enable-post
Open

Enable optional POST redirects#408
andreasonny83 wants to merge 3 commits intoeinaregilsson:masterfrom
andreasonny83:enable-post

Conversation

@andreasonny83
Copy link

@andreasonny83 andreasonny83 commented Nov 6, 2024

Allow users to optionally enable redirects to POST requests.

Comparing_einaregilsson_master___andreasonny83_enable-post_·_einaregilsson_Redirector

@Gitoffthelawn
Copy link
Collaborator

This looks fantastic! Thank you! How well have you tested it so far?

@andreasonny83
Copy link
Author

This looks fantastic! Thank you! How well have you tested it so far?

I'm actually using it at work for mocking the backend API when running the project locally. Since I need to mock both GET and POST requests, I can say this is perfectly working as expected. By default, the POST requests don't get mocked, but as soon as I enable the POST checkbox, everything start working as expected

@Gitoffthelawn
Copy link
Collaborator

Gitoffthelawn commented Nov 7, 2024

This looks fantastic! Thank you! How well have you tested it so far?

I'm actually using it at work for mocking the backend API when running the project locally. Since I need to mock both GET and POST requests, I can say this is perfectly working as expected. By default, the POST requests don't get mocked, but as soon as I enable the POST checkbox, everything start working as expected

Great! Which browser(s) and OS(es) have you tested it on? If you will, please include version numbers. Thanks.

@andreasonny83
Copy link
Author

Tested on MacOS against Microsoft Edge v130.0.2849.46 and Chrome v130.0.6723.117

@Gitoffthelawn
Copy link
Collaborator

Thanks. Do you have any other browsers and/or OSes (perhaps via VM) on which you can test? In addition to your existing tests, I'm thinking Firefox, Windows, and Linux tests are needed to cover a good chunk of our user base.

Copy link
Collaborator

@Gitoffthelawn Gitoffthelawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This looks good and can be merged once #408 is merged and released.

Edit: GitHub UI makes it a bit confusing.. the above code review comment was for a change to the README in relation to POST functionality.

@andreasonny83
Copy link
Author

Sorry @Gitoffthelawn but I did not get your last comment. This is the PR #408. Are you asking me to take out the Readme update from this PR? That was just added to provide customer's documentation about the added functionality

@Gitoffthelawn
Copy link
Collaborator

Sorry @Gitoffthelawn but I did not get your last comment. This is the PR #408. Are you asking me to take out the Readme update from this PR? That was just added to provide customer's documentation about the added functionality

No problem. The GitHub UI can be confusing to others for this, and I was trying to clarify (and I clearly failed!). My comment "looks good..." was created when I performed the "code review" for your edit the readme. I was saying it looks good and can be merged once the corresponding code is merged is and release. But the GitHub UI doesn't make that clear, so I tried to clear that up, and I obviously failed in a spectacular fashion! 🤣

But, now that you mention it, maybe my comedic mishap resulted in some good. It may actually be better to separate the PR for the README. Why? Because we don't want to change the README until the code is actually released, not just when it's merged. I may be mistaken (please tell me if I am), but I think if I merge the code now, the updated README will be available now (before the code is released), which will confuse people.

This reverts commit 7c39f6f.
@andreasonny83
Copy link
Author

Thanks for the clarification @Gitoffthelawn .
I have now reverted the Readme update commit as suggested

@andreasonny83
Copy link
Author

Any update on this one @Gitoffthelawn? Is this PR ready for merge now?

@Gitoffthelawn
Copy link
Collaborator

Any update on this one @Gitoffthelawn? Is this PR ready for merge now?

It just needs testing on Linux & Windows, and on Firefox. Ideally, it will work perfectly.

@andreasonny83
Copy link
Author

Any update on this one @Gitoffthelawn? Is this PR ready for merge now?

It just needs testing on Linux & Windows, and on Firefox. Ideally, it will work perfectly.

Will you cover that testing? I cannot test that at my end unfortunately

@gldrk
Copy link

gldrk commented Mar 14, 2025

Chiming in to confirm this works flawlessly in Firefox ESR 115 packaged with Debian. Tested on Wikipedia previews.

@SmartManoj
Copy link

It just needs testing on Linux & Windows, and on Firefox. Ideally, it will work perfectly.

How about merging into a dev branch?

@JamesClonk
Copy link

Any news on this? Would really love to have officially in the extension, can confirm it works on Linux with Firefox Developer Edition (147.0b3) 👍

@Gitoffthelawn
Copy link
Collaborator

@JamesClonk I really wanted POST support for a long time, but I haven't had a use for it in a number of years. Are you (or others) able to publicly post about how you are using POST (minor pun intended)? If it's on private sites, no problem with not providing details.

@JamesClonk
Copy link

@Gitoffthelawn We have a bunch of internal web applications that are hosted behind (rather dumb/simplistic) reverse-proxies. And these all have integration with various IdPs (which are also behind said reverse-proxies). So usually we use Redirector to on-the-fly exchange the redirectURIs of the OAuth/OIDC login flow within the browser to URLs pointing to the hostnames of the reverse-proxies instead, as the actual original URLs aren't reachable directly. So lets say for a common web app it's thus 2 Redirector entries, 1 for the web app itself, and 1 for the OIDC IdP.
However we have one case where the IdP has been integrated with SAML instead, and the browser login-and-callback flow there is doing POST requests as callback to the original web app instead of GET with query params / paths. 🤦
I have no other way of telling the browser where to point the POST request instead, and unfortunately currently Redirector would not hand over the payload. I was even considering to maybe have to script some automation that runs locally with mechanize or selenium or something similar, but that seems an extreme solution for just this particular web app.

TLDR; SAML IdP doing POST callbacks. 😅

@Gitoffthelawn
Copy link
Collaborator

@JamesClonk Got it. That's a great use of Redirector. Your testing of this PR was as described (2 Redirector entries: 1 for the web app and 1 for the OIDC IdP), and all was good?

@JamesClonk
Copy link

@Gitoffthelawn Yes, I'm using a build of this PR in my Firefox dev. When enabling POST in Redirector config then the login flow works with the 2 Redirector entries, and it doesn't when not having enabled POST support.

@huyz
Copy link

huyz commented Dec 21, 2025

Possibly dumb question: would a POST redirect by Redirector also hand over the payload? I thought this would be a browser security violation and cannot happen in any case. How would this PR help in the use case "SAML IdP doing POST callbacks"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants