Navigation Menu

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

Basicauth does not facilitate browser re-prompt #3239

Closed
Eadinator opened this issue Apr 7, 2020 · 5 comments
Closed

Basicauth does not facilitate browser re-prompt #3239

Eadinator opened this issue Apr 7, 2020 · 5 comments
Labels
bug 🐞 Something isn't working
Milestone

Comments

@Eadinator
Copy link

With the basicauth directive in a Caddyfile, if I go to the site and enter incorrect credentials I do not get a browser re-prompt as expected. This requires users closing and re-opening the browser or similar as refreshing the page does not offer the prompt again.

It seems initially the server sends 401 with the WWW-Authenticate header but after entering incorrect credentials only 401 is sent without the WWW-Authenticate header.

As a workaround, I have added the following to the Caddyfile which seems to produce the expected behaviour but I really don't want to maintain this and the CEL expression already broke in v2rc1 when upgrading from beta20 (required changing from "401" to 401).

 	@auth_error {
 		expression {http.error.status_code}==401
 	}
 
 	handle_errors {
 		header @auth_error {
 			WWW-Authenticate "Basic realm=\"restricted\""
 		}
 		respond @auth_error 401
 	}

Thanks

@mholt mholt added the bug 🐞 Something isn't working label Apr 7, 2020
@mholt mholt added this to the v2.0.0 milestone Apr 7, 2020
@mholt
Copy link
Member

mholt commented Apr 7, 2020

Oops, I think this is an easy fix! Thanks for the report.

I've pushed a fix to #3240. Please try it out and confirm that it works for you too! (Build artifacts are available via CI.)

and the CEL expression already broke in v2rc1 when upgrading from beta20 (required changing from "401" to 401).

Yes, that's because pre-releases are allowed to make breaking changes. :) (And the CEL matcher specifically is documented as experimental and subject to changes, even after the 2.0 tag, FWIW.) We improved the CEL parsing so quotes weren't needed to compare integers in this case.

@Eadinator
Copy link
Author

Eadinator commented Apr 8, 2020

Hi. Thanks for the quick reply.

Here are some tests I ran and the Caddyfile I used.

Firefox 74.0 Private Window / Opensuse Tumbleweed / caddy_v2_Linux_c88e687.zip

127.0.0.1 {
	basicauth {
		# admin / password
		admin	JDJhJDEwJFpsMDlUdnR4UUMweW0zQmRyZHRLMC5vLndJLzB1aVEyUHplS0x2bjg0cUZ3UWF5WHAuWTl1
	}

	respond "Hello World!"
}

Supply admin / password. Expected: "Hello World!". Result: "Hello World!"

Supply admin / qwerty. Expected: reprompt. Result: reprompt

Click Cancel on initial prompt. Expected: 401 and prompt on page refresh. Result: as expected

Supply no user and no password. Expected: reprompt. Result: no reprompt, 401 without WWW-Authenticate header. No prompt on page refresh.

Supply nonexistentuser / no password. Expected: reprompt. Result: no reprompt, 401 without WWW-Authenticate header. No prompt on page refresh.

As you can see the last two in bold didn't do what I thought it would, but it is progress from before.

Thanks

@mholt
Copy link
Member

mholt commented Apr 8, 2020

@Eadinator Ah, thanks. I missed a particular code path. Your reply made it easy and I was able to get all 4 cases to pass. Please try again with the latest push, thanks!

@Eadinator
Copy link
Author

Yes the last two tests work as I expected now, thanks!

@mholt
Copy link
Member

mholt commented Apr 8, 2020

Thanks for testing and the helpful report!

@mholt mholt closed this as completed in fbd9515 Apr 8, 2020
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

No branches or pull requests

2 participants