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 fixes #1983

Merged
merged 23 commits into from
Mar 18, 2024
Merged

Security fixes #1983

merged 23 commits into from
Mar 18, 2024

Conversation

doekenorg
Copy link
Contributor

@doekenorg doekenorg commented Feb 6, 2024

  • Add secret to short code use
  • Add copy option to short code field
  • Make copy popup accessible
  • Add secret to view block
  • Test gventry tag short code.
  • Add secret to entry block

This PR fixes the Security issue of enumerating on views based on their ID.

  • It adds a setting to enable this for a view, with the default for new views being set to on
    image

  • It the possibility to copy the short code instead of just selecting it.
    copied

  • It also fixes a bug where the GravityView capabilities for a role were overwritten on every admin request.

@doekenorg
Copy link
Contributor Author

The ClipboardJS plugin is accessible. Didn't add more, because this doubles up the amount of info for the screenreader.

@doekenorg doekenorg force-pushed the issue/security-fixes branch from f85ccb2 to 870c61a Compare February 8, 2024 12:38
@doekenorg doekenorg requested a review from zackkatz February 8, 2024 12:42
@doekenorg
Copy link
Contributor Author

doekenorg commented Feb 8, 2024

@zackkatz I think this is it for GravityView. Unfortunately since short codes are so specific per plugin, I don't really see an abstraction to extract. At least for gravityview I've consolidated it on the Shortcode base class.

This doesn't include a filter for returning views visible only for the current user. Not sure if that should be a fix on its own.

@doekenorg doekenorg changed the title WIP: Security fixes Security fixes Feb 8, 2024
@doekenorg
Copy link
Contributor Author

doekenorg commented Feb 8, 2024

I'm not sure how we should filter the views from the list. We can only do this based on a capability I guess, but those are not settable per post per user.

This is what I came up with, but gravityview_edit_entry should then be set for that specific view. I'm not sure how to do that.

Maybe someone has a good idea.

// in `GravityKit\GravityView\Gutenberg\Blocks::get_views()`

// Remove views the current user cannot edit.
$views = array_filter(
	$views,
	static function ( $view ) {
		return GravityView_Roles_Capabilities::has_cap( 'gravityview_edit_entry', $view->ID );
	}
);

@zackkatz zackkatz added this to the 2.21 milestone Feb 21, 2024
@zackkatz zackkatz requested a review from mrcasual February 28, 2024 03:09
@doekenorg doekenorg self-assigned this Feb 28, 2024
@mrcasual
Copy link
Collaborator

mrcasual commented Mar 13, 2024

@doekenorg, could you please:

  1. Remove "Click to copy" from the column name and move it to individual inputs, ensuring accessibility

CleanShot 2024-03-13 at 09 40 34@2x

  1. Allow copying using keyboard (e.g., by pressing Enter; currently it copies, but the page gets refreshed)

  2. Add unit tests for the secret attribute (this can wait until after the release if we're gunning for March 14)

@doekenorg doekenorg force-pushed the issue/security-fixes branch from f477b0d to e6f33ea Compare March 13, 2024 15:04
@mrcasual
Copy link
Collaborator

mrcasual commented Mar 13, 2024

@doekenorg, in GravityExport Lite and GravityCalendar we display a notice to admin users when there is a missing (but expected) secret key:

Gravity Forms Calendar: Invalid feed secret provided. Update with the new shortcode: [gravitycalendar id="2" secret="22c8d575a29e" /]

For consistency purposes, let's do the same here.

@doekenorg
Copy link
Contributor Author

@mrcasual only the unit test thing remains; but I think this can be released.

@mrcasual
Copy link
Collaborator

@doekenorg, please hide the "click to copy" completely :)

CleanShot 2024-03-13 at 11 43 52@2x

Instead, add aria-label,aria-describedby and title.

@mrcasual
Copy link
Collaborator

@doekenorg, copying to clipboard works great. Let's implement #1983 (comment) and we can then merge this.

@doekenorg
Copy link
Contributor Author

@mrcasual allright, that's in there. I used a WP_Error to be consistent; and not introduce an Exception, although that would have been my first solution ;-)

The notice is generic because it is re-used for different short codes ([gravityview, [gventry and [gvfield); and It doesn't provide the original shortcode for context; so this was the easiest solution.

@crbdev
Copy link

crbdev commented Mar 18, 2024

@doekenorg I just noticed that, on the "All Views" page, in the shortcode column, it says "Copied!" even before clicking on it.

CleanShot 2024-03-18 at 16 23 14@2x

@doekenorg
Copy link
Contributor Author

This was a styling issue; cached CSS.

@mrcasual mrcasual merged commit a291d4d into develop Mar 18, 2024
1 check passed
@mrcasual mrcasual deleted the issue/security-fixes branch March 18, 2024 13:17
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.

4 participants