Skip to content

Consider "returnUrl" in the OpenId actions#7558

Merged
sebastienros merged 3 commits intoOrchardCMS:devfrom
ThaerAlAjlouni:issue/7530
Mar 2, 2017
Merged

Consider "returnUrl" in the OpenId actions#7558
sebastienros merged 3 commits intoOrchardCMS:devfrom
ThaerAlAjlouni:issue/7530

Conversation

@ThaerAlAjlouni
Copy link
Copy Markdown
Contributor

Consider returnUrl in the OpenId actions. Fixes issue #7530


if (Request.IsAuthenticated) {
Redirect(Url.Content("~/"));
Redirect(returnUrl);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't use returnUrl anywhere in a redirect without checking the destination is allowed, like RedirectLocal does or if it needs to go to a provider's domain, then check it too.

This comment applies to every change in this PR.

Copy link
Copy Markdown
Contributor Author

@ThaerAlAjlouni ThaerAlAjlouni Feb 17, 2017

Choose a reason for hiding this comment

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

The code is updated to use the RedierctLocal extension method instead, as we never redirect to providers from the AccountController

@Html.TextBoxFor(m => m.GraphApiUrl, new { @class = "text large" })
<span class="hint">@T("Typically https://graph.windows.net")</span>

@Html.LabelFor(m => m.GraphApiKey, T("Graph API Key"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will merge the PR, but can you create a new one that removes the usage of LabelFor and instead use a <label> tag? Otherwise there can be issues with double-encoding (try to use an accent for instance, or < in the text)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure :)

}

public string GraphApiKey
{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Format

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK :)

@sebastienros sebastienros merged commit e0e8dae into OrchardCMS:dev Mar 2, 2017
@ThaerAlAjlouni ThaerAlAjlouni deleted the issue/7530 branch March 3, 2017 21:32
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.

3 participants