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

Stabilize WAF Compatibility #5059

Closed
swissspidy opened this issue Oct 29, 2020 · 2 comments · Fixed by #5364
Closed

Stabilize WAF Compatibility #5059

swissspidy opened this issue Oct 29, 2020 · 2 comments · Fixed by #5364
Assignees
@swissspidy
Labels
Group: Integration Integration with other platforms and plugins Group: WordPress Changes related to WordPress or Gutenberg integration P1 High priority, must do soon Type: Enhancement New feature or improvement of an existing feature

Comments

@swissspidy
Copy link
Collaborator

swissspidy commented Oct 29, 2020 •
edited
Loading

Feature Description

This ticket is two-fold:

Preventing issues with HTML markup in REST API requests

On one hand, This is a follow-up to #4805 to make the feature stable and remove the experimental flag.

One reason why the current method is experimental is because it uses mb_convert_encoding with UTF-16, which might not be available on all sites (the polyfill only handles UTF-8 well).

I think we can make this much, much simpler by not using Base64 and instead simply replace all > and < characters with placeholders over the wire. This should help prevent WAF issues.

For example, < could be replaced with __WEB_STORIES_LT__, and > with __WEB_STORIES_GT__

Example code:

function encodeMarkup(string) {
  return (
    '__WEB_STORIES_ENCODED__' +
    string
      .replaceAll('<', '__WEB_STORIES_LT__')
      .replaceAll('>', '__WEB_STORIES_GT__')
  );
}

Result:

// Markup generated by editor
<html amp><head>

// Sent via REST API
__WEB_STORIES_LT__html amp__WEB_STORIES_GT____WEB_STORIES_LT__head__WEB_STORIES_GT__

// Reconstructed in PHP
<html amp><head>

Of course there could be other ways too.

Preventing other issues with WAFs

Prevent conflicts in other areas, like HTTP request methods or other blocks, as done in #5120.

Alternatives Considered

Additional Context

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance Criteria

Implementation Brief
@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature P1 High priority, must do soon Group: Integration Integration with other platforms and plugins Group: WordPress Changes related to WordPress or Gutenberg integration Pod: WP & Infra labels Oct 29, 2020
This was referenced Oct 29, 2020
Closed
Merged
@bmattb bmattb added this to the Sprint 41 milestone Nov 3, 2020
@spacedmonkey spacedmonkey self-assigned this Nov 10, 2020
@bmattb bmattb modified the milestones: Sprint 41, Sprint 42 Nov 16, 2020
@bmattb
Copy link

bmattb commented Nov 19, 2020

@spacedmonkey will take first crack at the IB for this.

@spacedmonkey
Copy link
Contributor

Instead of a IB here, I worked on a small POC PR found here #5364.

In short, using uriencoding, to side step the utf-16 issue. More details in this stack overflow issue.

This seems like a simple workaround that works. @swissspidy can you look at the PR and seem what you think. If you are happy, I will add some more tests, remove the experiment flag.

@spacedmonkey spacedmonkey mentioned this issue Nov 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@swissspidy swissspidy

Labels
Group: Integration Integration with other platforms and plugins Group: WordPress Changes related to WordPress or Gutenberg integration P1 High priority, must do soon Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stabilize WAF Compatibility GoogleForCreators/web-stories-wp
3 participants
@spacedmonkey @swissspidy @bmattb