Skip to content

Confirm Logout and Redirects#5871

Merged
live627 merged 12 commits intoSimpleMachines:release-2.1from
jdarwood007:outsideRedirect
Mar 8, 2021
Merged

Confirm Logout and Redirects#5871
live627 merged 12 commits intoSimpleMachines:release-2.1from
jdarwood007:outsideRedirect

Conversation

@jdarwood007
Copy link
Copy Markdown
Member

This PR has two changes.

The first change is we fix it so Logins can properly redirect back out to 3rd party integrations, as long as they have formatted the URL and supplied a hash via the image proxy check. We use the image proxy check as it seems redundant at this stage to create another secret key for this. If this is good enough to prevent abuse for the proxy, its good enough for a integration.

The second change here is we are allowing logouts to have a prompt. This addresses #5869
This is mostly designed as external integrations may not have the ability to get SMF's session information to perform the automatic logout, and thus allows a proper confirmation page to log the user out properly.
We also in this properly address a logout redirect from a 3rd party integration as well.

Comment thread Themes/default/Login.template.php Outdated
@Sesquipedalian
Copy link
Copy Markdown
Member

FYI, once I merge the auth_secret code from 2.0.16, we'll want to change this to use get_auth_secret() instead of $image_proxy_secret, since that is the appropriate secret key to use for this. In the meantime, though, your choice to use $image_proxy_secret makes sense.

Copy link
Copy Markdown
Member

@Sesquipedalian Sesquipedalian left a comment

Choose a reason for hiding this comment

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

As I said in my comment above, this will also need to be changed to use get_auth_secret() instead of $image_proxy_secret once my forthcoming PR adding get_auth_secret() has been merged.

Comment thread Sources/LogInOut.php Outdated
Comment thread Sources/LogInOut.php Outdated
@jdarwood007
Copy link
Copy Markdown
Member Author

I could add a placeholder for get_auth_secret() so it can be updated once ready. The placeholder can just use the image proxy for now.

@jdarwood007
Copy link
Copy Markdown
Member Author

Any issues with merging this PR?
This PR makes the integration with MediaWiki work seamlessly for login/outs.

@sbulen
Copy link
Copy Markdown
Contributor

sbulen commented Feb 9, 2021

I'd rather see this one in sooner rather than later for broader testing coverage.

@live627
Copy link
Copy Markdown
Contributor

live627 commented Feb 26, 2021

Does this have to use base64? Is there a problem with urlencode?

@live627 live627 linked an issue Feb 26, 2021 that may be closed by this pull request
@jdarwood007
Copy link
Copy Markdown
Member Author

It made it safer and easier to deal with browser manipulation.

@live627
Copy link
Copy Markdown
Contributor

live627 commented Mar 2, 2021

Not sue I quite follow you on the safety bit—you mean that it isn't plain text? Is base64 really the right tool for the job? Seems overkill to me.

I seem to vaguely remember support issues on the forum about zealous malware scanners tripping up and removing base64 calls. (Many years ago)

I also found this Stack Overflew post which states that this is bad to do:

No, you would need to url-encode it, since base64 strings can contain the "+", "=" and "/" characters which could alter the meaning of your data - look like a sub-folder.

Valid base64 characters are below.

ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=

@live627
Copy link
Copy Markdown
Contributor

live627 commented Mar 6, 2021

uhh bump

@jdarwood007
Copy link
Copy Markdown
Member Author

Let me do some testing again. When I was testing between SMF and MediaWiki, things would not return back to MediaWiki properly. When I did it with the base64 encoding, things worked as expected. This PR is over a year old now so I have forgot what I did during my testing to come to the conclusion that base64 was needed other than that the URL would not be correct when we returned to MediaWiki.

# Conflicts:
#	other/buildTools
@jdarwood007
Copy link
Copy Markdown
Member Author

I got something that works.

SMF calls this in QueryString.php

	// Add entities to GET.  This is kinda like the slashes on everything else.
	$_GET = htmlspecialchars__recursive($_GET);

Which completely breaks the URL since it has a & in it. SMF makes it a &

So this has to run the un_htmlspecialchars process because of that. Otherwise the check will never work.

@live627 live627 merged commit d9697b4 into SimpleMachines:release-2.1 Mar 8, 2021
@pr-triage pr-triage Bot added the PR: merged label Mar 8, 2021
@jdarwood007 jdarwood007 deleted the outsideRedirect branch March 17, 2021 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logout should have a prompt screen

5 participants