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

Fix/issue 2768 #2845

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

JuanchoPestana
Copy link
Contributor

@JuanchoPestana JuanchoPestana commented Jan 17, 2020

Fixes #2768

Changes proposed in this Pull Request:

  • This PR sends the http referer in the register form in a hidden value, and then creates the redirect link from that value.

What I found to be causing this issue was that the wp_get_referer() function in line 1768, class-sensei-frontend.php file, always returned false (thus, the $return variable was always being set to the home url value). The wp_get_referer() function checks that the http referer is different than the request uri, and in the register process case, they are the same since the form action calls itself.

Testing instructions:

  1. Enable 'Anyone can register' in Settings > General, and 'Access Permissions' in Sensei LMS > Settings.
  2. View a course page as a logged out user.
  3. Click the Register button and complete the registration form
  4. You should be redirected to the course you were viewing

@JuanchoPestana
Copy link
Contributor Author

Hey @donnapep! I suspect you deleted your comment because I can't find it... Anyways, I fixed the Travis CI issues... Let me know your thoughts on this PR's approach and if there's anything else I could do to help! Thanks!

Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

Added a few comments about escaping.

includes/class-sensei-frontend.php Outdated Show resolved Hide resolved
@@ -1705,6 +1707,10 @@ public function sensei_process_registration() {
$new_user_email = $_POST['sensei_reg_email'];
$new_user_password = $_POST['sensei_reg_password'];

if ( isset( $_POST['sensei_reg_http_referer'] ) && '' !== $_POST['sensei_reg_http_referer'] ) {
$new_user_http_referer = esc_url_raw( wp_unslash( $_POST['sensei_reg_http_referer'] ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is wp_unslash necessary?

The URL is also being double escaped. It's being escaped here and on line 1775. It's always better to escape late, so I would do the escaping further down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add wp_unslash and the escaping here to pass the Travis CI build. Those were the issues that blocked my first attempt...
I found this article that explains why we should use the wp_unslash for $_POST, so I think that's why the sniffer marked that as a violation.
So, what is your suggestion? Leave it like this? Add the sniffer ignore comment? Or something else?

includes/class-sensei-frontend.php Outdated Show resolved Hide resolved
includes/class-sensei-frontend.php Outdated Show resolved Hide resolved
includes/class-sensei-frontend.php Outdated Show resolved Hide resolved
@donnapep donnapep removed their request for review September 2, 2022 11:49
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.

Registering from course page does not redirect back to the course
2 participants