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 coming soon unintentionally expose the rest of the site #1529

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

chihsuan
Copy link
Member

@chihsuan chihsuan commented Oct 23, 2024

Changes proposed in this Pull Request:

Closes #1528.

This PR modifies the handling of WooCommerce's coming soon initialization endpoint to respect WordPress.com's site-wide Coming Soon status. Key changes:

  • Added a check to detect if WordPress.com Coming Soon mode is active
  • If active, we return true immediately to prevent WooCommerce from modifying the site's visibility
  • Only set the woocommerce_store_pages_only option when the site is already live
  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

Test with Coming Soon mode enabled

  1. Set up a WordPress.com business site with Coming Soon mode enabled
  2. Install WooCommerce through the plugins menu
  3. Go through the core profiler (either skip or complete)
  4. Verify that the site remains in Coming Soon mode after the process

Test with Coming Soon mode disabled

  1. Set up a WordPress.com business site with Coming Soon mode disabled
  2. Install WooCommerce through the plugins menu
  3. Go through the core profiler (either skip or complete)
  4. Verify that site is in Coming Soon mode with Store Pages only

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?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@chihsuan chihsuan self-assigned this Oct 23, 2024
@chihsuan chihsuan requested review from a team, psealock and ilyasfoo October 23, 2024 09:56
Copy link

Size Change: 0 B

Total Size: 201 kB

ℹ️ View Unchanged
Filename Size
./build/53.js 1.08 kB
./build/index.css 731 B
./build/index.js 126 kB
./build/marketing.js 58.3 kB
./build/payment-gateway-suggestions.css 1.24 kB
./build/payment-gateway-suggestions.js 6.57 kB
./build/plugins.js 3.93 kB
./build/style-index.css 2.15 kB
./build/style-marketing.css 800 B

compressed-size-action

@adrianduffell
Copy link
Contributor

This looks like a good solution, @chihsuan. I could see other hosts having a similar problem in the future if they integrate with coming soon mode. Perhaps we should move the logic to the core rest api and solve it generally? 🤔

@chihsuan
Copy link
Member Author

This looks like a good solution, @chihsuan. I could see other hosts having a similar problem in the future if they integrate with coming soon mode. Perhaps we should move the logic to the core rest api and solve it generally? 🤔

Good idea! I think we can move the coming soon mode check and return 200 logic to the core reset api. But we probably need to keep the logic for setting woocommerce_store_pages_only option in the host, since only they know how to determine if the site is already live for their environment. cc @ilyasfoo if you have any thoughts on this. 🙏

Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

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

Tests well, nice work @chihsuan!

@ilyasfoo
Copy link
Contributor

ilyasfoo commented Oct 24, 2024

Perhaps we should move the logic to the core rest api and solve it generally? 🤔

@adrianduffell Good point, I think so too.

@chihsuan I think to provide the most flexibility, we could add 2 filters for both of these just before we set them. It should allow for hosts to determine the default coming soon state and store-only mode after going through core profiler

@chihsuan
Copy link
Member Author

I think to provide the most flexibility, we could add 2 filters for both of these just before we set them. It should allow for hosts to determine the default coming soon state and store-only mode after going through core profile

Sounds good! I'll create a follow-up issue for this.

Copy link
Contributor

@adrianduffell adrianduffell left a comment

Choose a reason for hiding this comment

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

Code change makes sense and LGTM! Thanks @chihsuan 🚀

@chihsuan chihsuan merged commit c65407d into master Oct 24, 2024
5 checks passed
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.

WooCommerce coming soon unintentionally expose the rest of the site
3 participants