(feat:) in the REST API controllers support passing all request params to WP_Query

[colin-yard-nl]

Ik heb het doorgeven van de params uit het request nu net zo gedaan als in de OpenPub-plugin gebeurt (in \OWC\OpenPub\Base\RestAPI\Controllers\BaseController::getPaginatorParams()).

Sommige van de controllers doen na die aanroep nog andere dingen om de AbstractRepository::$queryArgs vervolgens nog te manipuleren/overschrijven (bijv. in ItemController::convertParameters()), maar door deze wijziging kunnen in principe alle params uit het request (zoals tax_query en meta_query) aan WP_Query doorgegeven worden.

[SimonvanWijhe]

Aha. Ik kon het al niet zo snel vinden in openpub. Niet geheel logisch om de method dan nog getPaginatorParams te noemen. En moeten we de query parameters niet eerst escapen voor we die doorgeven aan de WP_Query?

[colin-yard-nl]

Aha. Ik kon het al niet zo snel vinden in openpub. Niet geheel logisch om de method dan nog getPaginatorParams te noemen. En moeten we de query parameters niet eerst escapen voor we die doorgeven aan de WP_Query?

Nee, dat gaat zo te zien allemaal al goed: de call vanuit portal naar PDC wordt nu dan bijv. zoiets: https://buren-pdc.lndo.site/wp-json/owc/pdc/v1/items/?limit=-1&include-connected=true&tax_query%5B0%5D%5Btaxonomy%5D=pdc-type&tax_query%5B0%5D%5Bfield%5D=slug&tax_query%5B0%5D%5Bterms%5D=melding, en in \OWC\PDC\Base\Repositories\AbstractRepository::all() zijn de $args dan dit:

[colin-yard-nl]

Aha. Ik kon het al niet zo snel vinden in openpub. Niet geheel logisch om de method dan nog getPaginatorParams te noemen. En moeten we de query parameters niet eerst escapen voor we die doorgeven aan de WP_Query?

Method hernoemen: ja, willen we dat wel? In OpenPub heet ie dus ook zo — en dan moeten we het waarschijnlijk wel in alle branches wijzigen, om te voorkomen dat er verwarrende verschillen ontstaan?

[SimonvanWijhe]

Je hebt gelijk. Let's keep it simple.

[mvdhoek1]

@SimonvanWijhe had je de query al getest op gevoelige data als output?

[colin-yard-nl]

@SimonvanWijhe had je de query al getest op gevoelige data als output?

@SimonvanWijhe @mvdhoek1 De zorg was of door deze wijziging niet ongewenste queries uitgevoerd / ongewenste data opgehaald kan worden, niet waar?

Volgens mij niet (maar misschien zie ik iets over het hoofd): want door deze wijziging worden weliswaar alle ge-poste query-vars doorgegeven aan WP-query, maar die leiden volgens mij altijd tot additionale AND-clauses (zeg maar), dus je kunt hierdoor je query enkel verder beperken, niet uitbreiden?

En ook het overschrijven van bijv. het filter op _owc_pdc_active=true (uit \OWC\PDC\Base\Support\Traits\QueryHelpers::excludeInactiveItemsQuery) lukt niet, omdat die later toegepast wordt: weliswaar krijg je met metaquery%5B0%5D%5Bkey%5D=_owc_pdc_active&metaquery%5B0%5D%5Bvalue%5D=0&metaquery%5B0%5D%5Bcompare%5D=%3D in de URL dan wel twee meta_query-elementen in de query-vars:

maar die latere overschrijft de eerdere en in de SQL die uitgevoerd wordt, staat dan enkel de "juiste": … AND ( 0wc_pdc_postmeta.meta_key = '_owc_pdc_active' AND 0wc_pdc_postmeta.meta_value = '1' ) ….

[sanderdekroon]

Zonder goed door de code te spitten: kun je de post_status, post_type, etc. wijzigen via de URL parameters? Want dat is wellicht wel problematisch.

[colin-yard-nl]

Zonder goed door de code te spitten: kun je de post_status, post_type, etc. wijzigen via de URL parameters? Want dat is wellicht wel problematisch.

@sanderdekroon Ha, inderdaad, dat had ik niet gezien, de post_status-conditie kan zo wèl overschreven worden! post_type niet, want die wordt hard-coded altijd toegepast, maar dat gebeurt bij post_status dus blijkbaar niet. Dank — wordt vervolgd : )

[mvdhoek1]

Interne chat:

Mike:
Ik zou een draft status alleen toestaan wanneer iemand is ingelogd of de api aanroept met een application-password.

if (! is_user_logged_in()) {
    // force post status to be 'published'
}

Simon:
Goed idee. Ik zou dan applicatie password verplichten voor het gebruik van de query parameter. Dan zitten we goed. We hebben ook nog pdc items met het taxonomy type "internal" en die moeten ook niet zonder wachtwoord kunnen worden opgehaald.

[sanderdekroon]

Kunnen we niet een allowlist/blocklist implementeren? Dingen als order, orderby, limit, etc. zijn allemaal queryable, maar niet op elke endpoint. De naamgeving wijkt soms ook af t.o.v. de query parameters in WordPress.

Je kan dan een lijst van toegestane parameters bijhouden voor niet-ingelogde requests. Als iemand geauthenticeerd is sta je (nagenoeg) alles toe.

Of rek ik deze PR dan weer heel erg op? 😬

[mvdhoek1]

@colin-yard-nl ik heb vast een opzet/voorbeeld gemaakt met de whitelisting en wat extra validaties. Kijk maar even wat jullie hiervan vinden. -> 9b0fc04