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.

Sign up for GitHub

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

Jump to bottom

fix(oauth): start php session if no session available #3469

Merged
merged 4 commits into from
Oct 11, 2024
Merged

fix(oauth): start php session if no session available #3469

merged 4 commits into from
Oct 11, 2024

Conversation

adekbadek
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

In WordPress, the PHP session might not be always started. This change ensures that it always will be.

How to test the changes in this Pull Request:

I was not able to reproduce the conditions under which a session would not be started. A smoke test should be enough.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@adekbadek adekbadek added the [Status] Needs Review The issue or pull request needs to be reviewed label Oct 9, 2024
@adekbadek adekbadek requested a review from a team as a code owner October 9, 2024 12:55
@leogermani
Copy link
Contributor

leogermani commented Oct 9, 2024 •
edited
Loading

Interesting! If this solves the empty ID issues it will be amazing!

I think it's cleaner if you do just

if ( session_status() !== PHP_SESSION_ACTIVE ) {
session_start();
}
$id = session_id()

I've seen some weird behaviors sometimes when you call session_id() before session_start that it actually prevents a session from being started.. ( I think this happens on CLI though).. but anyway, I think that's the most PHPy way of doing it

@adekbadek
Copy link
Member Author

@leogermani – done! Thanks for the tip

@adekbadek adekbadek requested a review from leogermani October 10, 2024 07:32
@adekbadek
Copy link
Member Author

ca27f36 adds the uid logging to the "Nonce verification failed…" message. This way we'll be able to see if nonces fail in that no-uid case.

leogermani
leogermani requested changes Oct 10, 2024
View reviewed changes
includes/oauth/class-google-login.php Outdated
@@ -122,7 +122,8 @@ public static function oauth_callback() {
}

if ( ! wp_verify_nonce( sanitize_text_field( $_GET[ self::AUTH_CALLBACK ] ), self::AUTH_CALLBACK ) ) {
self::handle_error( __( 'Nonce verification failed.', 'newspack-plugin' ) );
/* translators: %s is a unique user id */
sprintf( __( 'Nonce verification failed for id: %s', 'newspack-plugin' ), OAuth::get_unique_id() );
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you accidentally removed the call to `self::handle_error

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Added back in 2b3a8b9

@adekbadek adekbadek requested a review from leogermani October 11, 2024 07:19
@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Oct 11, 2024
@leogermani leogermani merged commit d3e8265 into release Oct 11, 2024
10 checks passed
matticbot pushed a commit that referenced this pull request Oct 11, 2024
@semantic-release-bot
chore(release): 5.5.1 [skip ci]
cc0c713 
## [5.5.1](v5.5.0...v5.5.1) (2024-10-11)

### Bug Fixes

* **oauth:** start php session if no session available ([#3469](#3469)) ([d3e8265](d3e8265))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 5.5.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leogermani leogermani leogermani approved these changes

Assignees
No one assigned
Labels
released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adekbadek @leogermani @matticbot