Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking change missing in changelog between v7.4.0 and v7.5.0 #2271

Closed
Matroxt opened this issue Oct 13, 2023 · 5 comments
Closed

Breaking change missing in changelog between v7.4.0 and v7.5.0 #2271

Matroxt opened this issue Oct 13, 2023 · 5 comments
Labels
breaking A change that will cause a major version bump bug

Comments

@Matroxt
Copy link

Matroxt commented Oct 13, 2023

This fix introduced a different behavior with regards to the skip-auth-route/skip-auth-regex arguments.

Expected Behavior

In v7.4.0, if you had skip_auth_routes = ["^/get$"] in your config, you'd be able to reach endpoints with a URL parameter.

  • localhost:4180/get => HTTP 200 (auth bypassed)
  • localhost:4180/get?test=true => HTTP 200 (auth bypassed)

Current Behavior

In v7.5.0, you'll get

  • localhost:4180/get => HTTP 200 (auth bypassed)
  • localhost:4180/get?test=true => ! HTTP 403 (auth required) !

Possible Solution

This is kind of a big deal, it took us digging into the source code to figure out why our skips weren't working anymore.
Turns out it was the URL parameters.

I think you guys can keep this breaking change if you want, but it needs to be mentioned as one in the changelog.

Steps to Reproduce (for bugs)

  1. Clone this repo
  2. In contrib/local-environment, add this skip_auth_routes =["^/get$"] to oauth2-proxy.cfg
  3. docker compose up and navigate to http://localhost:4180/get?test=true
    1. See that it asks for authentication on v7.5.0
  4. Change docker-compose.yaml to use v7.4.0
  5. docker compose up and navigate to http://localhost:4180/get?test=true
    1. See that it let's you through

Temp fix

I now have to allow URL parameters explicitly with this regex: "^/get(\\?.*)?$"

Context

We were affected because some skipped endpoints had URL params and that was working fine before, but not anymore.

@tuunit
Copy link
Member

tuunit commented Oct 15, 2023

Hi @Matroxt,

thank you for bringing this up. I'll talk to the other maintainers regarding an update to the CHANGELOG.

@tuunit tuunit added bug breaking A change that will cause a major version bump labels Oct 15, 2023
@Matroxt
Copy link
Author

Matroxt commented Oct 16, 2023

Personally, I preferred when skip-auth-route/skip-auth-regex didn't factor in the URL parameters.
So if there's a possibility to adjust the behavior to just not have a breaking change instead, that would be ideal IMO.

Of course, the final decision is up to you guys.

@Matroxt
Copy link
Author

Matroxt commented Nov 28, 2023

Hi @tuunit

Not sure if you guys came to an agreement, but I really think this should be added to the breaking change list as soon as possible.

Whether you want to revert that change or not is another discussion, but users really need to know about this when upgrading, as there are still no notice on the 7.5.0 release.

@tuunit
Copy link
Member

tuunit commented Dec 26, 2023

Merry Christmas 🎄

Thanks for the reminder as we are already working on newer releases and it has been out for a while. We decided to just add the breaking change notice. We will refer to this issue in the CHANGELOG and on the release page as the detailed description.

@kvanzuijlen
Copy link
Member

The breaking change was added to the changelog. Thanks for noticing/bringing it up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A change that will cause a major version bump bug
Projects
None yet
Development

No branches or pull requests

3 participants