Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

The pymsrc option allows users to load arbitrary javascript on the page. #8

Closed
benlk opened this issue Jun 13, 2016 · 4 comments
Closed

Comments

@benlk
Copy link
Collaborator

benlk commented Jun 13, 2016

The content of the pymsrc script is output directly to the page: https://github.com/INN/pym-shortcode/blob/26bbef9e561651da110e6dc77d590bc362736fa5/pym-shortcode.php#L31

    if ( $pym_id == 0 || $atts['pymscr'] ) {
        echo sprintf(
            '<script src="%s"></script>',
            $pymsrc
        );
    }

We should check to make sure that this is at least a URL.

We may want to consider implementing a whitelist in the future, for security reasons.

@benlk
Copy link
Collaborator Author

benlk commented Jun 30, 2017

Thought on this:

  • Create an admin page, accessible to super-admins on multisite and admins on single-site, that allows whitelisting sources for the pymsrc and whitelisting domains for the src attribute.
  • As a default, whitelist the current site's default pymsrc URL and the current domain.
  • At page render time, only output the pymsrc script on the page if it's in the whitelist. Only output the src on the page if it's in the whitelist.
  • For a version before the on-render whitelisting goes active, for each site, set up some sort of cron job or something that looks at posts with the [pym shortcode and adds their pymsrcs and src domains to the whitelist, before we present the whitelist to the users.
  • On post save, throw an error in the admin if the post's pymsrc or pym domain aren't whitelisted. include a link to an explanation why. Include the text of the forbidden shortcode. In the post, leave the shortcode there but change the attribute names so that they're obviously not working.
    • maybe include a list of users with the appropriate permission to do the edit?

Why limit it to those users? They're who can put <script> tags in posts, by default. (This is the unfiltered_html permission). By limiting editing the whitelist to those users, we raise the trust level required to put javascript on the page. Right now it's anyone who can edit who determines what javascript is allowed on the page. This change limits the determination of what javascript is allowed to the people who normally have that permission.

Why collect data for a version before it goes active? Because if we launch whitelisting with an empty whitelist, we have the chance of silently breaking everyone's existing embeds.

Why cause an error? Because we need to provide feedback that the embed isn't permitted. Why provide instructions? So people know how to remedy the error.

@benlk benlk mentioned this issue Feb 14, 2018
10 tasks
@benlk
Copy link
Collaborator Author

benlk commented Apr 27, 2018

Another possible solution:

  • on the settings page Add settings page #32, register a setting that allows admins to enable the pymsrc attribute, which would be disabled by default.
  • If the attribute is enabled, allow the custom pymsrc.

@benlk
Copy link
Collaborator Author

benlk commented Aug 29, 2018

Like, at minimum, we should be using https://developer.wordpress.org/reference/functions/wp_http_validate_url/ or something to make sure that the URL is valid.

@benlk
Copy link
Collaborator Author

benlk commented Sep 6, 2018

register a setting that allows admins to enable the pymsrc attribute, which would be disabled by default.

This would be a breaking change for sites that have used an alternate pymsrc.

However, we can encourage admins in the upgrade notes and in the general readme and in the install notes to check the box and set the pymsrc to the CDN. As part of this, link to the "how to test upgrading this plugin on your site" docs, which should be written as part of #32.

benlk added a commit that referenced this issue Sep 6, 2018
- INN\PymShortcode is now INN\PymEmbeds
- Adds settings page
	- addresses #32
	- helper function INN\PymEmbeds\Settings\option_group() to get the option group name
	- helper function INN\PymEmbeds\Settings\option_key() to get the option_key for wp_options
	- helper function INN\PymEmbeds\Settings\settings_section() to get the settings section ID for the settings page
	- helper function INN\PymEmbeds\Settings\settings_page() to get the settings page ID
	- option default_pymsrc to change away from that provided by pym_pymsrc_local_url(), which is run through wp_http_validate_url to address #8 (comment)
	- option override_pymsrc to force use of default_pymsrc in embed output, addressing #8
@benlk benlk mentioned this issue Sep 6, 2018
6 tasks
@benlk benlk closed this as completed Sep 6, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

1 participant