-
Notifications
You must be signed in to change notification settings - Fork 87
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
Bug/password protected posts exposed #804
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… right now but works according to my testing
…better home for them
…items related to password protected models
…fields values were protected
…ere password protected
…lthough Im not sure if youre ok to just add properties like that willy nilly). Assert that featuerd image calculated fields are visible even on protected events (like featured images are on posts)
…ove the fung shway
…ected- an array unless its a belongsTo relation, in which case null was still best
… were set to an int or something
eeteamcodebase
pushed a commit
that referenced
this pull request
Dec 3, 2018
…ord protected posts exposed (#804) Originally developed in private: https://github.com/eventespresso/event-espresso-private/pull/2 <!-- Thanks for your pull request. Comments within these type of tags will be hidden and not show up when you submit your pull request --> <!-- Please answer the following questions in order to expediate acceptance --> ## Problem this Pull Request solves ## The outward effects we want are: * events, venues and attendees with a password set are considered password-protected, meaning for REST API requests their excerpt and body are, by default, replaced with empty strings (when requesting using the default "caps context", which is "read", front-end reads, but not when using "admin_read" admin-end reads, which is what our mobile apps use) * similarly, there are other fields that are password-protected (see each model's `EE_Password_Field` for the list). This means their true values get replaced with defaults (more notes on this later) * similarly, by default, you cannot "read" data related to password-protected posts over the REST API unless you provide the password for the model they're related to (eg the following will exclude datetimes for password-proteted posts: `/wp-json/ee/v4.8.36/datetimes`, `wp-json/ee/v4.8.36/events/1/datetimes` and `wp-json/ee/v4.8.36/events?include=Datetime`) * all REST API requests allow you to pass a `password` query parameter. If this does not match the password that protects that data (eg on events that's the password field, or datetimes its their event's password field, on tickets it's their datetime's event's password field), an error response is returned. If it does, you are permitted to see the password-protected data. Note, the following are valid requests: `wp-json/ee/v4.8.36/events?password=mypassword`, `/wp-json/ee/v4.8.36/datetimes?password=mypassword`, `wp-json/ee/v4.8.36/events/1/datetimes?password=mypassword` and `wp-json/ee/v4.8.36/events?include=Datetime&password=mypassword`. BUT if that password isn't correct any single item returned, the entire REST response will be replaced with an error messsage. So `wp-json/ee/v4.8.36/events?password=mypassword` will return an error if that is NOT the password for any of the events retrieved. This makes brute force attacks more difficult. * full admin can use the `password` when querying, but no one else can. (Because otherwise you could sneakily do `wp-json/ee/v4.8.36/events?where[password]=foo`, which would return all events whose password was "foo", which I'm sure hackers would like to know.) * REST API responses will only contain entities' passwords if the correct password was provided while querying, or when querying using a caps context other than `read` (front-end reads) * REST API responses for entities with passwords (eg events, venues, attendees) have an extra property `_protected`. Here is its schema: ``` "_protected": { "description": "Array of property names whose values were replaced with their default (because they are related to a password-protected entity.)", "type": "array", "items": { "description": "Each name corresponds to a property that is protected by password for this entity and has its default value returned in the response.", "type": "string", "readonly": true }, "readonly": true }, ``` and here is an example of it in a request for events: ``` "_protected": [ "password", "EVT_desc", "EVT_short_desc", "EVT_display_desc", "EVT_display_ticket_selector", "EVT_visible_on", "EVT_additional_limit", "EVT_default_registration_status", "EVT_member_only", "EVT_phone", "EVT_allow_overflow", "EVT_timezone_string", "EVT_external_URL", "EVT_donations" ] }, ``` That indicates each of those fields, which can be password protected, were replaced with their default value on this particular event. So the `EVT_desc` in this event entity is NOT its real value; whereas if it weren't mentioned in `_protected` it would be its real value. * most calculated fields aren't accessible on password-protected entities. Eg, if event 123 has a password, doing `GET .../wp-json/ee/v4.8.36/events/123?calculate=optimum_sales_at_start, image_medium`, the property `optimum_sales_at_start` will be replaced with 0 (instead of its true value, which should be kept hidden for a protected event), but `image_medium` will be visible as usual (WP core allows you to see featured media for password-protected posts). The `_calculated_fields` property also has a `_protected` property which will list which calculated fields were "protected" on the current request. Also, we indicate which calculated fields can be password-protected in the schema, by adding `protected: true` to the property. ### Model-Level Changes * add new class `EE_Password_Field`, which stores a list of the fields on the model to which it protects access * this field is defined on events, venues and attendees. Other EE CPT models could define it too. * you can now pass in a new boolean query parameter to models, `exclude_protected`. If set to true, model objects relating to models with a password set will be excluded. Eg `EEM_Datetime::instance()->get_all(array('exclude_protected' => true));` will return all datetimes for events with no password. * in order to support that new query parameter, the property `EEM_Base::model_chain_to_password` was introduced. This indicates which related model, if any, has a password that protects access to this model's items. Eg on `EEM_Datetime` its just `Event`, meaning the event holds a password field, so that queries for datetimes that set `'exclude_protected' => true` should be excluded if their related event has a password set. On `EEM_Ticket` its `Datetime.Event`. This property doesn't need to be set for models which aren't publicly queryable (eg `EEM_Registration` isn't publicly queryable, so it doesn't need that property.) * note: the REST API makes use of `exclude_protected` in order to hide datetimes and tickets for events with passwords ## Up for Debate or Improvement * the WP API allows you to see a password-protected post's author and terms. Following that logic, in EE REST API, some entities for password-protected entities should be accessible. Eg you should be able to see a password protected event's WP user, and terms and taxonomies. However, each of those models are from WP core models, which we hoped to deprecate in the EE REST API anyway. Do we have a preference? Should we do both approaches in both cases? ## How has this been tested FYI This has been pretty extensively unit tested. <!-- Please describe in detail how you tested your changes and how testing can be reproduced --> <!-- Include details of your testing environment, and tests ran to see how your changes affect other areas of code --> <!-- Include any notes about automated tests you've written for this pull request. Pull requests with automated tests are preferred. --> * [x] Create a password-protected event. Request it via the REST API (eg `/wp-json/ee/v4.8.36/events/{id}`). Its password, description, and excerpt should be blank. Also, the `_protected` property on that event should list all the fields which were replaced with their default values. * [x] Try including related data (eg `/wp-json/ee/v4.8.36/events/{id}?include=Datetime`) the entity's `datetimes` property should be replaced with null, and `datetimes` should be in the `_protected` property, indicating it was protected. * [x] Try fetching its related data (eg `/wp-json/ee/v4.8.36/events/{id}/datetimes`). You should get a 403 * [x] Use the mobile apps to check a registrant into an event. It should work exactly as before. ## Checklist * [x] I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs * [x] User input is adequately validated and sanitized * [x] all publicly displayed strings are internationalized (usually using `esc_html__()`, see https://codex.wordpress.org/I18n_for_WordPress_Developers) * [x] My code is tested. * [x] My code follows the Event Espresso code style. * [x] My code has proper inline documentation. * [x] My code accounts for when the site is Maintenance Mode (MM2 especially disallows usage of models) -> the EE REST API doesn't handle this super great but that's a long-standing issue * [x] Maybe add some more unit tests * [ ] Add documentation "
mnelson4
added
type:bug 🐞
category:security
category:models-and-data-infrastructure
and removed
status:triage
labels
Dec 3, 2018
9 tasks
11 tasks
nerrad
added a commit
that referenced
this pull request
Jan 30, 2019
…API response (#805) <!-- Thanks for your pull request. Comments within these type of tags will be hidden and not show up when you submit your pull request --> <!-- Please answer the following questions in order to expediate acceptance --> ## Problem this Pull Request solves <!-- Please describe your changes in the context of the problem they solve --> In a #804, password protected content is being handled by the Event Espresso REST API. Therefore our model entity work will need to be updated to account for that. In this pull the following is added: - `BaseEntity` instances now have a `protectedFields` property that contains an array of the fields that have protected content replacement for that entity instance. For now, these are the _canonical_ field names (eg. `EVT_desc`). This also includes any protected fields that are part of the `_calculated_fields` property on the response. A field name in this array indicates that the content for that field in the instance is not the actual content because it is protected. - `BaseEntity` instances now have a `isPasswordProtected` getter that returns true if there is any protected field values in the current instance. So true means that client code can expect there to be _defaults_ for protected content as opposed to the actual value (which is protected). - `BaseEntity` instances now have a `isFieldPasswordProtected` getter that returns whether the provided field name has protected content for the current instance. For example `Event.isFieldPasswordProtected( 'EVT_desc' )` would return `true` if the entity instance contains the default value for `EVT_desc` because the correct password wasn't provided for that content or would return `false` when either the provided field is not part of a password protected entity, or the correct password was provided in the request so the value is the _actual_ content. - all tests were updated and new tests added for the new expectations on `BaseEntity` instances. This is a backward-compatible change as it is only added new features. There currently is no _pressing_ need to add handling for password protected content in our stores as its more of an implementation detail in apps. However, I do anticipate we may have to eventually consider whether an entity can be persisted if it has protected values in it. I don't want to pre-emptively add any logic though until its proven necessary. Co-authored-by: Josh Feck <josh@nxnweb.com>
eeteamcodebase
pushed a commit
that referenced
this pull request
Jan 30, 2019
…or new password-protected content functionality in the REST API response (#805) <!-- Thanks for your pull request. Comments within these type of tags will be hidden and not show up when you submit your pull request --> <!-- Please answer the following questions in order to expediate acceptance --> ## Problem this Pull Request solves <!-- Please describe your changes in the context of the problem they solve --> In a #804, password protected content is being handled by the Event Espresso REST API. Therefore our model entity work will need to be updated to account for that. In this pull the following is added: - `BaseEntity` instances now have a `protectedFields` property that contains an array of the fields that have protected content replacement for that entity instance. For now, these are the _canonical_ field names (eg. `EVT_desc`). This also includes any protected fields that are part of the `_calculated_fields` property on the response. A field name in this array indicates that the content for that field in the instance is not the actual content because it is protected. - `BaseEntity` instances now have a `isPasswordProtected` getter that returns true if there is any protected field values in the current instance. So true means that client code can expect there to be _defaults_ for protected content as opposed to the actual value (which is protected). - `BaseEntity` instances now have a `isFieldPasswordProtected` getter that returns whether the provided field name has protected content for the current instance. For example `Event.isFieldPasswordProtected( 'EVT_desc' )` would return `true` if the entity instance contains the default value for `EVT_desc` because the correct password wasn't provided for that content or would return `false` when either the provided field is not part of a password protected entity, or the correct password was provided in the request so the value is the _actual_ content. - all tests were updated and new tests added for the new expectations on `BaseEntity` instances. This is a backward-compatible change as it is only added new features. There currently is no _pressing_ need to add handling for password protected content in our stores as its more of an implementation detail in apps. However, I do anticipate we may have to eventually consider whether an entity can be persisted if it has protected values in it. I don't want to pre-emptively add any logic though until its proven necessary. Co-authored-by: Josh Feck <josh@nxnweb.com>"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally developed in private: https://github.com/eventespresso/event-espresso-private/pull/2
Problem this Pull Request solves
The outward effects we want are:
EE_Password_Field
for the list). This means their true values get replaced with defaults (more notes on this later)/wp-json/ee/v4.8.36/datetimes
,wp-json/ee/v4.8.36/events/1/datetimes
andwp-json/ee/v4.8.36/events?include=Datetime
)password
query parameter. If this does not match the password that protects that data (eg on events that's the password field, or datetimes its their event's password field, on tickets it's their datetime's event's password field), an error response is returned. If it does, you are permitted to see the password-protected data. Note, the following are valid requests:wp-json/ee/v4.8.36/events?password=mypassword
,/wp-json/ee/v4.8.36/datetimes?password=mypassword
,wp-json/ee/v4.8.36/events/1/datetimes?password=mypassword
andwp-json/ee/v4.8.36/events?include=Datetime&password=mypassword
. BUT if that password isn't correct any single item returned, the entire REST response will be replaced with an error messsage. Sowp-json/ee/v4.8.36/events?password=mypassword
will return an error if that is NOT the password for any of the events retrieved. This makes brute force attacks more difficult.password
when querying, but no one else can. (Because otherwise you could sneakily dowp-json/ee/v4.8.36/events?where[password]=foo
, which would return all events whose password was "foo", which I'm sure hackers would like to know.)read
(front-end reads)_protected
. Here is its schema:and here is an example of it in a request for events:
That indicates each of those fields, which can be password protected, were replaced with their default value on this particular event. So the
EVT_desc
in this event entity is NOT its real value; whereas if it weren't mentioned in_protected
it would be its real value.GET .../wp-json/ee/v4.8.36/events/123?calculate=optimum_sales_at_start, image_medium
, the propertyoptimum_sales_at_start
will be replaced with 0 (instead of its true value, which should be kept hidden for a protected event), butimage_medium
will be visible as usual (WP core allows you to see featured media for password-protected posts). The_calculated_fields
property also has a_protected
property which will list which calculated fields were "protected" on the current request. Also, we indicate which calculated fields can be password-protected in the schema, by addingprotected: true
to the property.Model-Level Changes
EE_Password_Field
, which stores a list of the fields on the model to which it protects accessexclude_protected
. If set to true, model objects relating to models with a password set will be excluded. EgEEM_Datetime::instance()->get_all(array('exclude_protected' => true));
will return all datetimes for events with no password.EEM_Base::model_chain_to_password
was introduced. This indicates which related model, if any, has a password that protects access to this model's items. Eg onEEM_Datetime
its justEvent
, meaning the event holds a password field, so that queries for datetimes that set'exclude_protected' => true
should be excluded if their related event has a password set. OnEEM_Ticket
itsDatetime.Event
. This property doesn't need to be set for models which aren't publicly queryable (egEEM_Registration
isn't publicly queryable, so it doesn't need that property.)exclude_protected
in order to hide datetimes and tickets for events with passwordsUp for Debate or Improvement
Do we have a preference? Should we do both approaches in both cases?
How has this been tested
FYI This has been pretty extensively unit tested.
/wp-json/ee/v4.8.36/events/{id}
). Its password, description, and excerpt should be blank. Also, the_protected
property on that event should list all the fields which were replaced with their default values./wp-json/ee/v4.8.36/events/{id}?include=Datetime
) the entity'sdatetimes
property should be replaced with null, anddatetimes
should be in the_protected
property, indicating it was protected./wp-json/ee/v4.8.36/events/{id}/datetimes
). You should get a 403Checklist
esc_html__()
, see https://codex.wordpress.org/I18n_for_WordPress_Developers)