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

Security fix: Security check on email template preview page #220

Merged
merged 2 commits into from
May 8, 2020
Merged

Security fix: Security check on email template preview page #220

merged 2 commits into from
May 8, 2020

Conversation

timbocode
Copy link
Contributor

Backport from woocommerce/woocommerce@60148d7

See also #214

File affected: includes/admin/class-wc-admin.php

@timbocode timbocode added the Security Security fixes including WC backports label Feb 23, 2020
@timbocode timbocode added this to the 1.0.0-beta2 milestone Feb 23, 2020
@timbocode timbocode requested review from nylen and bahiirwa February 23, 2020 14:02
@timbocode timbocode modified the milestones: 1.0.0-beta2, 1.0.0-beta1 Feb 23, 2020
@timbocode timbocode changed the title Security fix: security check on email template preview page Security fix: Security check on email template preview page Feb 23, 2020
@nylen
Copy link
Contributor

nylen commented Feb 23, 2020

@bahiirwa it's a good idea to explain what was tested/reviewed, we shouldn't just check the "approved" box with no context.

See also #214 (comment) - we should probably make sure we agree on the approach we are using to review and approve these changes.

@timbocode
Copy link
Contributor Author

Testing

  • Manually checked the email preview in admin. All seems OK.

@bahiirwa
Copy link
Collaborator

bahiirwa commented Mar 6, 2020

Tested like an ordinary user.

  • Admin preview also works fine for me.
  • Functionality to add templates to the theme all remain working fine. Nothing is broken.

Screenshot 2020-03-06 at 19 18 35

nylen
nylen previously requested changes Mar 6, 2020
Copy link
Contributor

@nylen nylen left a comment

Choose a reason for hiding this comment

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

I compared the changes here against woocommerce/woocommerce@60148d7 and the end result looks good.

The changes themselves are different, because there was another previous commit to this line that we haven't specifically included yet: woocommerce/woocommerce@14d9678

Since this commit also includes escaping changes in other lines I think we should include it in this PR.

@timbocode timbocode requested a review from nylen March 9, 2020 18:53
@timbocode timbocode removed the request for review from nylen May 2, 2020 10:35
@bahiirwa
Copy link
Collaborator

bahiirwa commented May 4, 2020

Whats the way forward on this PR?

@timbocode timbocode requested a review from nylen May 4, 2020 19:16
@timbocode
Copy link
Contributor Author

Awaiting approval from @nylen.

@timbocode timbocode requested review from nylen and removed request for nylen May 7, 2020 09:23
@timbocode timbocode dismissed nylen’s stale review May 8, 2020 14:36

As discussed and agreed with @nylen. An exercise in continuity planning. Reduces single-person dependency.

@timbocode timbocode merged commit 3c97fb8 into ClassicPress:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Security Security fixes including WC backports
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants