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

outposts/proxy: Fix invalid redirect on external hosts containing path components #8915

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

GermanCoding
Copy link
Contributor

@GermanCoding GermanCoding commented Mar 14, 2024

Details

I was trying to setup a proxy provider (proxy mode, not forwarding) with an outpost. I'm using an external host that contains a path component, like this:

External Host: https://example.com/myApp

This generally works nicely with authentik, except for the login. Assume my webbrowser is currently on https://example.com/myApp/resourceA and the proxy detects that the user needs to (re-)login. Now, what happens is that after the login has completed, you get redirected back to https://example.com/resourceA. The path component myApp is simply cut off, which then causes 404 errors in my app (since that URL doesn't exist).

The reason for this misbehavior is the urlPathSet() function, which completely removes any path components set by the external host setting. This is apparently only done in one place (redirectToStart()) and no where else. It also seems to be unnecessary - why can't we simply join the base (the external host) and the target URL? This works just as before, but without the issue created by cutting off parts of the URL.

I also modernized the urlJoin function to use modern golang functions. The benefit is that the url.JoinPath() is smarter than path.Join() - for example, path.Join cuts off trailing slashes, which might be problematic for URLs. url.JoinPath() knows this and doesn't do this.


Checklist

  • Local tests pass (ak test authentik/)
  • The code has been formatted (make lint-fix)

If an API change has been made

  • The API schema has been updated (make gen-build)

If changes to the frontend have been made

  • The code has been formatted (make web)

If applicable

  • The documentation has been updated
  • The documentation has been formatted (make website)

…h components

Signed-off-by: Max <github@germancoding.com>
@GermanCoding GermanCoding requested a review from a team as a code owner March 14, 2024 20:44
Copy link

netlify bot commented Mar 14, 2024

Deploy Preview for authentik-docs ready!

Name Link
🔨 Latest commit 346f004
🔍 Latest deploy log https://app.netlify.com/sites/authentik-docs/deploys/65f9e30822972900088b519c
😎 Deploy Preview https://deploy-preview-8915--authentik-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Mar 14, 2024

Deploy Preview for authentik-storybook ready!

Name Link
🔨 Latest commit 346f004
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/65f9e308f8ad9e0008d1d580
😎 Deploy Preview https://deploy-preview-8915--authentik-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@BeryJu BeryJu added the status/reviewing thanks for opening, we're taking a look label Mar 15, 2024
@BeryJu BeryJu self-assigned this Mar 15, 2024
Signed-off-by: Max <github@germancoding.com>
@GermanCoding
Copy link
Contributor Author

Sorry, looks like I missed that test case. I changed it to match the new logic.

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.33%. Comparing base (cef1d2d) to head (346f004).
Report is 369 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #8915       +/-   ##
===========================================
+ Coverage   46.62%   92.33%   +45.70%     
===========================================
  Files         626      640       +14     
  Lines       30996    31552      +556     
===========================================
+ Hits        14451    29132    +14681     
+ Misses      16545     2420    -14125     
Flag Coverage Δ
e2e 50.48% <ø> (+5.76%) ⬆️
integration 26.09% <ø> (+0.10%) ⬆️
unit 89.67% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BeryJu BeryJu merged commit 1b81973 into goauthentik:main Mar 19, 2024
58 of 64 checks passed
@GermanCoding GermanCoding deleted the fix-redirects branch March 19, 2024 20:36
kensternberg-authentik added a commit that referenced this pull request Mar 21, 2024
* main:
  outposts/proxy: Fix invalid redirect on external hosts containing path components (#8915)
  core: cache user application list under policies (#8895)
  web: bump the eslint group in /web with 2 updates (#8959)
  web: bump core-js from 3.36.0 to 3.36.1 in /web (#8960)
  website: bump @types/react from 18.2.66 to 18.2.67 in /website (#8962)
  web: bump the eslint group in /tests/wdio with 2 updates (#8963)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/reviewing thanks for opening, we're taking a look
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants