fix: Authorizer "remote" throws exception

ory / oathkeeper

A cloud native Identity & Access Proxy / API (IAP) and Access Control Decision API that authenticates, authorizes, and mutates incoming HTTP(s) requests. Inspired by the BeyondCorp / Zero Trust white paper. Written in Go.

https://www.ory.sh/?utm_source=github&utm_medium=banner&utm_campaign=hydra

Apache License 2.0

3.2k stars 349 forks source link

fix: Authorizer "remote" throws exception #1138

Open timthornton-avid opened 9 months ago

timthornton-avid commented 9 months ago

…ed Body" if request body is present in request

https://github.com/ory/oathkeeper/issues/1136

Related issue(s)

Checklist

[x] I have read the contributing guidelines.
[x] I have referenced an issue containing the design document if my change introduces a new feature.
[x] I am following the contributing code guidelines.
[x] I have read the security policy.
[x] I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security vulnerability, I confirm that I got the approval (please contact security@ory.sh) from the maintainers to push the changes.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] I have added or changed the documentation.

Further Comments

CLAassistant commented 9 months ago

All committers have signed the CLA.

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.60%. Comparing base (8fc9b7a) to head (6eaedba). Report is 2 commits behind head on master.

:exclamation: Current head 6eaedba differs from pull request most recent head febbe7d. Consider uploading reports for the commit febbe7d to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1138 +/- ## ========================================== - Coverage 77.90% 77.60% -0.31% ========================================== Files 80 79 -1 Lines 3929 4014 +85 ========================================== + Hits 3061 3115 +54 - Misses 595 618 +23 - Partials 273 281 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alnr commented 9 months ago

Thanks for the PR! Would you mind adding a failing test case as a first step? That way we're sure to catch any regressions in the future. Also, it makes it easier to understand the problem and convince ourselves the fix is correct.

ouranders commented 3 months ago

@timthornton-avid Really nice of you to fix this. Could you get the branch up-to-date with the base branch and try to resolve the failing "Docker Image Scanners"? We would really love to have this issue fixed. Thanks!

ralfkrakowski commented 3 months ago

Any progress on this pull? We would really love to have this issue fixed. Thank you!

timthornton-avid commented 3 months ago

Hi, I'll try to get this branch up to date and retest this week or next. As per the docker image failure, there was nothing I added that would impact docker image. There was a 1 line code change + comments and a unit test added. I don't recall the docker image scan failure but they would have existed prior to my changes. Lets see what it looks like after I rebase.

timthornton-avid commented 3 months ago

Ok, had a few minutes to rebase code and test. Looks like everything in pipeline has passed. Should be good to go :)

ralfkrakowski commented 3 months ago

Hi, is there any progress with the review of this issue @taisph and @aeneasr ? This fix would be really valuable for us. Thanks for releasing such a great product!

timthornton-avid commented 2 months ago

Hey Guys, Any update on this ? Updated branch a month ago.

keyur-scogo commented 1 month ago

Hi any plans on releasing patch for this issue?

ouranders commented 1 month ago

As others has previously mentioned, we are also very eager to have this issue fixed and merged :) @taisph @aeneasr any news on when this can be looked on? Kudos for a great product!

mwahlhuetter commented 23 hours ago

Also wanted to chime in, would be great if this could be resolved and merged :) Just had a very long troubleshooting session to find out that there is a bug in oathkeeper and not in my setup...