Skip to content

Conversation

@Nelwhix
Copy link
Contributor

@Nelwhix Nelwhix commented Dec 3, 2024

What does this PR do?

Fixing issue #11313 where ForwardAuth middleware modifys the Location header instead of sending it as is.

Fixes #11313

More

  • Added/updated tests
  • Added/updated documentation

@Nelwhix Nelwhix changed the base branch from master to v3.2 December 3, 2024 21:39
@Nelwhix Nelwhix changed the title fix: fixing issue #11313 where ForwardAuth middleware modifys the Location header instead of sending it as is. fix: fixing issue where ForwardAuth middleware modifys the Location header instead of sending it as is. Dec 3, 2024
@kevinpollet kevinpollet changed the title fix: fixing issue where ForwardAuth middleware modifys the Location header instead of sending it as is. Fix issue where ForwardAuth middleware modifys the Location header instead of sending it as is. Dec 5, 2024
@kevinpollet kevinpollet added this to the next milestone Dec 5, 2024
@kevinpollet
Copy link
Member

Hello @Nelwhix and thanks for this contribution,

The way you address the issue is breaking because some users may rely on the current behavior of the ForwardAuth middleware. We may have not been clear enough, but the issue should be addressed in a non-breaking way. This means that an option should be added to the middleware to control whether the location header should be left as is (the new behavior) or resolved against the auth server address.

Also, as this is an enhancement, could you please rebase it on the master branch?

@Nelwhix Nelwhix changed the base branch from v3.2 to master December 5, 2024 17:11
@Nelwhix Nelwhix force-pushed the forward-auth-fix-2 branch 3 times, most recently from ed848d3 to 4ea9aaa Compare December 5, 2024 17:53
@Nelwhix
Copy link
Contributor Author

Nelwhix commented Dec 5, 2024

Hi @kevinpollet, I understand what you mean. Now to find a new name for this field. What do you think of preserveLocationHeader ?

http:
  middlewares:
    test-auth:
      forwardAuth:
        preserveLocationHeader: true

@kevinpollet
Copy link
Member

kevinpollet commented Dec 9, 2024

Hi @kevinpollet, I understand what you mean. Now to find a new name for this field. What do you think of preserveLocationHeader ?

@Nelwhix Thanks, it looks good to me.

@Nelwhix
Copy link
Contributor Author

Nelwhix commented Dec 9, 2024

@kevinpollet made the changes

@rtribotte
Copy link
Member

Hello @Nelwhix,

Thanks for your changes.
This new option needs to be documented, both in configuration references (you just need to run go generate to update them), and in the middleware specific page.
As well, the option needs to be replicated in our CRDs by adding it to traefikv1alpha1.ForwardAuth, and make use of it in createForwardAuthMiddleware func.

@Nelwhix
Copy link
Contributor Author

Nelwhix commented Dec 12, 2024

Hi @rtribotte, I have made the updates.

@rtribotte
Copy link
Member

Hello @Nelwhix,

Thanks for last changes!
Do you mind if I rebase you branch and push a review commit?

@Nelwhix
Copy link
Contributor Author

Nelwhix commented Dec 12, 2024 via email

@rtribotte rtribotte changed the title Fix issue where ForwardAuth middleware modifys the Location header instead of sending it as is. Add an option to preserve the ForwardAuth Server Location header Dec 12, 2024
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@traefiker traefiker merged commit 2302deb into traefik:master Dec 13, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Middleware of ForwardAuth returns incorrect location header

5 participants