API: Fix redirect issues #22285

papagian · 2020-02-18T23:42:38Z

What this PR does / why we need it:

The 6.6.1 has the following issues:

In this modification I had made the wrong assumption that the appSubUrl does not include the leading slash. As a result the redirect validation is not correct. In addition to this, validation should not consider correct paths like /<appSubUrl>blahblah.
Frontend prepends the subpath to redirects in addition to the backend (/grafana ends up to /grafana/grafana)

This fix addresses the above issues by removing the subpath from redirects in the backend.
However it does not work optimally when redirects occur in the backend before reaching the frontend (like in the OAuth flow) because the session cookie path is set to appSubUrl/ and requests without the appSubUrl can not read it.

This fixes the above issue by removing the trailing slash from the cookie path.

The reason for this is that addresses the causes of the problem instead of the symptoms.

This PR:

Reverts this (Revert "API: Fix redirect issue when configured to use a subpath (#21652)" #22671)
Fixes redirect validation and tests (Fix redirect validation #22675)
Modifies the backend and the frontend to prepend the appSubUrl only if it’s missing from the redirect URL (Fix redirect: prepend subpath only if it's missing #22676)
Validates the redirect URL in OAuth login (Validate redirect in OAuth login #22677)
Fixes login view if redirect_to value is invalid but the user is authenticated so as the user is navigated to the home page instead of rendering login page with error. (Fix invalid redirect for authenticated user #22678)

Which issue(s) this PR fixes:

Fixes #22227
Fixes #22461

Special notes for your reviewer:

I have tested it with the following scenarios:

with subpath /grafana:
- base auth:
- oauth:
without subpath:
- base auth:
- oauth:

pkg/api/login.go

kylebrandt · 2020-02-18T23:48:51Z

pkg/api/login_oauth.go

-		ctx.Redirect(redirectTo)
-		return
+		if err := hs.validateRedirectTo(redirectTo); err == nil {
+			middleware.DeleteCookie(ctx.Resp, "redirect_to", hs.cookieOptionsFromCfg)


happen to understand this enough to add a comment explaining why the cookie is deleted?

the cookie is no longer needed since it held the redirect_to location. this code path is going to send the browser to the new location, and should not retain the redirect_to cookie (could end up in a redirect loop, but mainly it is no longer needed).

exactly; the cookie is deleted because it's has fulfilled its purpose.

kylebrandt · 2020-02-18T23:49:48Z

Don't understand this enough to approve it, but an exemplar pull request message :-)

briangann

looks great!

pkg/api/login.go

briangann · 2020-02-19T02:09:36Z

pkg/api/login_oauth.go

-		ctx.Redirect(redirectTo)
-		return
+		if err := hs.validateRedirectTo(redirectTo); err == nil {
+			middleware.DeleteCookie(ctx.Resp, "redirect_to", hs.cookieOptionsFromCfg)


the cookie is no longer needed since it held the redirect_to location. this code path is going to send the browser to the new location, and should not retain the redirect_to cookie (could end up in a redirect loop, but mainly it is no longer needed).

consideRatio · 2020-02-19T08:56:22Z

I have not fully understood the details here, but I think it makes sense to use the logic from #22265 alongside this PR that allows cookies to be passed along to /grafana/ and /grafana paths and not only the former. My example for why I think so is the following.

Assume I have a grafana_session cookie already, but without Server: Don't include trailing slash in cookie path when hosting Grafana in a sub path #22265's logic, it's path value remains at /grafana/ as I understand it, because I don't think this PR influence the grafana_session cookie's path value.
Assume I visit mydomain.org/grafana, then my grafana_session cookie isn't passed along because I didn't include a trailing / in my host header, so I get redirected to /login instead of directly visiting Grafana's root path even though I'm already logged in.

Thanks for working on this!

papagian · 2020-02-19T11:44:06Z

I have not fully understood the details here, but I think it makes sense to use the logic from #22265 alongside this PR that allows cookies to be passed along to /grafana/ and /grafana paths and not only the former. My example for why I think so is the following.

Assume I have a grafana_session cookie already, but without Server: Don't include trailing slash in cookie path when hosting Grafana in a sub path #22265's logic, it's path value remains at /grafana/ as I understand it, because I don't think this PR influence the grafana_session cookie's path value.

In that case, this validation returns error and the redirect_to cookie is ignored and delete it and the logged in user is navigated to the home page.

Assume I visit mydomain.org/grafana, then my grafana_session cookie isn't passed along because I didn't include a trailing / in my host header, so I get redirected to /login instead of directly visiting Grafana's root path even though I'm already logged in.

This is true.

Thanks for working on this!

@consideRatio
Your concerns are fair and I want also to test your modifications on top of these. I'm only a bit reluctant changing the cookie path from what is used to be until today because we had faced a number of issues related to cookie handling and there can be some cases we can't think of right now.

It's also a bit concerning that two different people have difficulty understanding the modifications in this PR therefore I would love some feedback from anybody with better insight than me.

papagian · 2020-02-19T17:29:09Z

Assume I visit mydomain.org/grafana, then my grafana_session cookie isn't passed along because I didn't include a trailing / in my host header, so I get redirected to /login instead of directly visiting Grafana's root path even though I'm already logged in.

This is true.

I have pushed a modification for fixing this.

consideRatio · 2020-02-19T18:07:49Z

Assume I visit mydomain.org/grafana, then my grafana_session cookie isn't passed along because I didn't include a trailing / in my host header, so I get redirected to /login instead of directly visiting Grafana's root path even though I'm already logged in.

This is true.

I have pushed a modification for fixing this.

The fix will still make /grafana?and=query-params redirect to /grafana/login, but from /grafana/login, it will realize that the redirect_to cookie specifies that it should go to /grafana instead of /grafana/, so it will say "no! this is invalid" and then assume the user wants to go to /grafana/ where the authentication cookie is actually passed along.

It makes a visit to /grafana a bit less invalid, but it still disrupts the user by bounces that truncates potential query parameters.

Your concerns are fair and I want also to test your modifications on top of these. I'm only a bit reluctant changing the cookie path from what is used to be until today because we had faced a number of issues related to cookie handling and there can be some cases we can't think of right now.

This is the key discussion in my mind. Cookie handling is indeed a delicate and complicated matter. I have never really felt confident about them, but after the references you found I've started to feel that I understand them somewhat. That makes me want to tackle the question if the change suggested in #22265 actually would cause issues head on. If #22265 isn't going to cause issues, it would actually solve the issue I described above with the workaround in 4e52076 and remove the need to bounce the user to /home/login once just to get redirected again where the cookie is passed.

marefr

Sorry being late to the party. I probably lack a lot of context, but added some comments/questions.

marefr · 2020-02-19T23:46:39Z

pkg/middleware/auth.go

@@ -47,7 +47,11 @@ func notAuthorized(c *m.ReqContext) {
 		return
 	}

-	WriteCookie(c.Resp, "redirect_to", url.QueryEscape(c.Req.RequestURI), 0, newCookieOptions)
+	redirectTo := c.Req.RequestURI
+	if setting.AppSubUrl != "" && !strings.HasPrefix(redirectTo, setting.AppSubUrl) {


Why doesn't c.Req.RequestURI include AppSubUrl? Would it help to use c.Req.URL instead to skip writing the appSubUrl in cookie?

c.Req.Request.URI: /grafana/d/5SdHCadmz/panel-tests-graph?orgId=1
c.Req.Request.URL.String(): /d/5SdHCadmz/panel-tests-graph?orgId=1

Why doesn't c.Req.RequestURI include AppSubUrl?

As far as I understand, the user can request whatever URL.
For instance, the user can request http://localhost:3000 even though the subpath is /grafana.

In addition to this, we always prepended the subpath before this change which was introduced for fixing this problematic situation.

Probably now that we have validation for the redirects (and if it's working correctly) we no more need to prepend it
because invalid redirect paths are ignored.
But I think it's safer to keep it here, since it appends it only if it's missing.

marefr · 2020-02-20T00:17:01Z

public/app/core/components/Login/LoginCtrl.tsx

@@ -104,9 +104,17 @@ export class LoginCtrl extends PureComponent<Props, State> {
    const params = this.props.routeParams;
    // Use window.location.href to force page reload
    if (params.redirect && params.redirect[0] === '/') {
-      window.location.href = config.appSubUrl + params.redirect;
+      if (config.appSubUrl !== '' && !params.redirect.startsWith(config.appSubUrl)) {


Do we need frontend logic at all for redirect, couldn't the backend make sure to always return the correct url? Wait why are we using redirect query param here - thought we always had redirect in cookie?

)" (#22671) This reverts commit 0e2d874.

* Fix redirect validation * Fix tests * Add removed redirect validation comment * Chore: Add test for parse of app url and app sub url Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

* Prepand subpath in redirect URL only if it's missing * Change frontend to prepend the subpath only if needed

New changes

* Fix login view in case of invalid redirect If the user is authenticated and redirect_to cookie points to an invalid path, the user should be navigated to the home page instead of rendering the login page with error. * Fix TestLoginViewRedirect

marefr

A: Logout scenario with anonymous auth disabled:

Login
Browse dashboard X
Clear cookies
Refresh browser. Should be redirected to login page
Login
Should be redirected to dashboard X

B: Anonymous scenario:

Browse dashboard X
Click on login in sidemenu and redirect to login page
Login
Should be redirected to dashboard X

Root url default without sub path using Grafana auth:
A: Works as expected
B: Works as expected

Root url default without sub path using OAuth:
A: Works as expected
B: No, redirected to home page

Root url http://localhost:3000/grafana with serve_from_sub_path using Grafana auth:
A: Works as expected
B: Works as expected

Root url http://localhost:3000/grafana with serve_from_sub_path using OAuth:
A: Works as expected
B: No, redirected to home page

Root url http://grafana.myproxy.com:10080/grafana without serve_from_sub_path using nginx and Grafana auth:
A: Works as expected
B: Works as expected

Root url http://grafana.myproxy.com:10080/grafana without serve_from_sub_path using nginx and OAuth:
A: Works as expected
B: No, redirected to home page

Also confirmed this resolves #22227

@papagian Not sure if B scenario using OAuth is a new or existing regression? We could create a new issue if there's no existing one and fix that for 6.7 stable maybe?

papagian · 2020-03-11T08:43:16Z

@papagian Not sure if B scenario using OAuth is a new or existing regression? We could create a new issue if there's no existing one and fix that for 6.7 stable maybe?

I was able to reproduce it in 6.5.2 and 6.6.2 and therefore I have filed a new issue

papagian requested review from briangann, a team, marefr, torkelo and mckn and removed request for a team February 18, 2020 23:42

kylebrandt reviewed Feb 18, 2020

View reviewed changes

pkg/api/login.go Show resolved Hide resolved

kylebrandt reviewed Feb 18, 2020

View reviewed changes

briangann previously approved these changes Feb 19, 2020

View reviewed changes

papagian mentioned this pull request Feb 19, 2020

Server: Don't include trailing slash in cookie path when hosting Grafana in a sub path #22265

Merged

marefr reviewed Feb 20, 2020

View reviewed changes

papagian mentioned this pull request Feb 27, 2020

Nginx not redirecting to Dashboards even Auth Proxy is enabled #22461

Closed

papagian force-pushed the 22227_fix branch from 6321109 to f5a9e0d Compare February 28, 2020 12:35

papagian requested a review from marefr February 28, 2020 13:50

papagian mentioned this pull request Mar 10, 2020

Revert "API: Fix redirect issue when configured to use a subpath (#21652)" #22671

Merged

papagian added the type/epic Issue made of smaller issues label Mar 10, 2020

papagian closed this Mar 10, 2020

papagian force-pushed the 22227_fix branch from f5a9e0d to 5aeb367 Compare March 10, 2020 09:49

Revert "API: Fix redirect issue when configured to use a subpath (#21652

7cdf2af

)" (#22671) This reverts commit 0e2d874.

papagian reopened this Mar 10, 2020

papagian requested a review from a team as a code owner March 10, 2020 10:14

This was referenced Mar 10, 2020

Fix redirect validation #22675

Merged

Fix redirect: prepend subpath only if it's missing #22676

Merged

This was referenced Mar 10, 2020

Validate redirect in OAuth login #22677

Merged

Fix invalid redirect for authenticated user #22678

Merged

papagian and others added 3 commits March 10, 2020 17:03

Fix redirect validation (#22675)

dec83d1

* Fix redirect validation * Fix tests * Add removed redirect validation comment * Chore: Add test for parse of app url and app sub url Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

Fix redirect: prepend subpath only if it's missing (#22676)

3c3f858

* Prepand subpath in redirect URL only if it's missing * Change frontend to prepend the subpath only if needed

Validate redirect in login oauth (#22677)

a832a7b

marefr requested review from briangann and removed request for a team March 10, 2020 15:42

papagian and others added 2 commits March 10, 2020 17:45

Login: Use correct path for OAuth logos

cd862f6

marefr approved these changes Mar 10, 2020

View reviewed changes

papagian added this to the 6.7-beta1 milestone Mar 11, 2020

papagian added add to changelog type/bug labels Mar 11, 2020

papagian merged commit be022d4 into master Mar 11, 2020

papagian deleted the 22227_fix branch March 11, 2020 09:04

bergquist mentioned this pull request Mar 12, 2020

302 redirection toward login page even if password is correct #22731

Closed

adberger mentioned this pull request May 26, 2020

[Microsoft Edge only] OAuth redirect issue when using subpath #23422

Closed

marefr mentioned this pull request Mar 25, 2021

API: Redirect after OAuth login when auth.anonymous is enabled #22709

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Fix redirect issues #22285

API: Fix redirect issues #22285

papagian commented Feb 18, 2020 •

edited

Loading

kylebrandt Feb 18, 2020

briangann Feb 19, 2020

papagian Feb 19, 2020

kylebrandt commented Feb 18, 2020

briangann left a comment

briangann Feb 19, 2020

consideRatio commented Feb 19, 2020

papagian commented Feb 19, 2020

papagian commented Feb 19, 2020

consideRatio commented Feb 19, 2020 •

edited

Loading

marefr left a comment

marefr Feb 19, 2020

papagian Feb 20, 2020

marefr Feb 20, 2020

marefr left a comment •

edited

Loading

papagian commented Mar 11, 2020

API: Fix redirect issues #22285

API: Fix redirect issues #22285

Conversation

papagian commented Feb 18, 2020 • edited Loading

kylebrandt Feb 18, 2020

Choose a reason for hiding this comment

briangann Feb 19, 2020

Choose a reason for hiding this comment

papagian Feb 19, 2020

Choose a reason for hiding this comment

kylebrandt commented Feb 18, 2020

briangann left a comment

Choose a reason for hiding this comment

briangann Feb 19, 2020

Choose a reason for hiding this comment

consideRatio commented Feb 19, 2020

papagian commented Feb 19, 2020

papagian commented Feb 19, 2020

consideRatio commented Feb 19, 2020 • edited Loading

marefr left a comment

Choose a reason for hiding this comment

marefr Feb 19, 2020

Choose a reason for hiding this comment

papagian Feb 20, 2020

Choose a reason for hiding this comment

marefr Feb 20, 2020

Choose a reason for hiding this comment

marefr left a comment • edited Loading

Choose a reason for hiding this comment

papagian commented Mar 11, 2020

papagian commented Feb 18, 2020 •

edited

Loading

consideRatio commented Feb 19, 2020 •

edited

Loading

marefr left a comment •

edited

Loading