GravityKit / GravityView

The best and easiest way to display Gravity Forms entries on your website.
https://www.gravitykit.com/products/gravityview/
245 stars 62 forks source link

Security fixes #1983

Closed doekenorg closed 3 months ago

doekenorg commented 5 months ago

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

doekenorg commented 5 months ago

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

doekenorg commented 5 months ago

@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 commented 5 months ago

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 );
    }
);
mrcasual commented 3 months ago

@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

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

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

mrcasual commented 3 months ago

@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 commented 3 months ago

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

mrcasual commented 3 months ago

@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 commented 3 months ago

@doekenorg, copying to clipboard works great. Let's implement https://github.com/GravityKit/GravityView/pull/1983#issuecomment-1994699377 and we can then merge this.

doekenorg commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

This was a styling issue; cached CSS.