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

ConnectivityChecker: Remove double // in aliveCheckUrl causing Invalid Redirect URL with CloudFlare Access. #535

Merged
merged 1 commit into from Nov 5, 2022

Conversation

bchavez
Copy link
Contributor

@bchavez bchavez commented Oct 24, 2022

Description

Bug Fix

When Homer is protected behind CloudFlare Access, the ConnectivityChecker builds an aliveCheckUrl with something like:

https://homer.domain.net//index.html?t=1666582254399

and makes an HTTP HEAD to the homer.domain.net to verify connectedness.

The problem arises when the cookie protecting homer.domain.net is expired; and results in an HTTP 302 redirect to a login page.

The browser follows the HTTP 302 redirect and the user lands on a site like:

https://__team__.cloudflareaccess.com/cdn-cgi/access/login/....redirect_url=%2F%2Findex.html%3Ft%3D1666582258096....

Notice the URL encode redirect_url=%2F%2Findex.html. Decoded, the redirect_url looks something like //index.html. The double // in the redirect_url causes CloudFlare Access to display the following error page (instead of a proper Login Page or Redirect):

image

If we remove the leading %2F from the redirect_url so that we have: redirect_url=%2Findex.html...; we get the expected login page and/or a proper redirect with the necessary authorization cookies.

Feel free to push modifications or changes to this PR.

Thanks,
Brian

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've read & comply with the contributing guidelines
  • I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • I have made corresponding changes to the documentation (README.md).
  • I've checked my modifications for any breaking changes, especially in the config.yml file

…ing Invalid Redirect URL with CloudFlare Access.
@netlify
Copy link

netlify bot commented Oct 24, 2022

Deploy Preview for homer-demo-content ready!

Name Link
🔨 Latest commit 3452087
🔍 Latest deploy log https://app.netlify.com/sites/homer-demo-content/deploys/635610a7b04fae0008da821a
😎 Deploy Preview https://deploy-preview-535--homer-demo-content.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 settings.

Copy link
Owner

@bastienwirtz bastienwirtz left a comment

Choose a reason for hiding this comment

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

Hi @bchavez,

Indeed, the location.pathname can contain a trailing slash and cause a double slash. Thanks for this fix!

@bastienwirtz bastienwirtz merged commit d141a69 into bastienwirtz:main Nov 5, 2022
@XxAcielxX
Copy link

XxAcielxX commented Nov 6, 2022

Same issue is happening if you use Authelia as an authentication method. After the login, it redirects to https://mydomain.ltd//index.html?t=1667722753069

Even if the ConnectivityChecker is set to false this happens randomly. On true, it always happens.

@XxAcielxX
Copy link

I'm having a side affect of this fix with the redirecting. Now if I'm on https://mydomain.ltd//index.html, the redirect adds another
/index.html?t=xxxxxxxxxxx and I end up with https://mydomain.ltd/index.html/index.html?t=xxxxxxxxxxx

@martin-micimo
Copy link

Yea. Same as #468

Why was the index.html hardcoded here? Works fine if I remove "/index.html" from aliveCheckUrl in the compiled js.

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.

None yet

4 participants