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

caddyhttp: Fix edgecase with HTTP->HTTPS logic #4243

Merged
merged 1 commit into from
Jul 14, 2021
Merged

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Jul 11, 2021

Followup to #4033, see https://caddy.community/t/how-to-set-priority-in-caddyfile/13002/8 for context

As it turns out, my earlier change for HTTP->HTTPS redirect was not enough, because it left one edgecase.

If none of the routes in the HTTP server had host matchers, then it would return the index of the last route, even if the only route was a catch-all (which is defined as having no host matcher).

This means that a config like this would not work correctly, and Foo would be returned on requests to http://bar.localhost/

http:// {
	respond "Foo"
}
bar.localhost {
	respond "Bar"
}

What we want is for http://bar.localhost/ to return a redirect to https://bar.localhost/ instead (and this can be overridden by using auto_https disable_redirects if you want it otherwise).

So the fix I went with is to keep track of whether we actually found a host matcher during the loop, and if we didn't, then return 0 as the index to insert the redirect routes, i.e. before any user-defined catch-all.

P.S. Integration tests don't actually run in CI because they're flaky and slow, but I ran them all again with my change and the new test to cover this case, and it passes.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Jul 11, 2021
@francislavoie francislavoie added this to the v2.4.4 milestone Jul 11, 2021
@francislavoie francislavoie requested a review from mholt July 11, 2021 15:08
@mohammed90
Copy link
Member

P.S. Integration tests don't actually run in CI because they're flaky and slow, but I ran them all again with my change and the new test to cover this case, and it passes.

Except they do run on the IBM Z machine 😃

PASS
ok  	github.com/caddyserver/caddy/v2/caddytest/integration	4.493s

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Let's give this a try - thank you Francis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants