Scrub body_data / data params too (e.g. POSTed JSON) #3
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
bigpresh
force-pushed
the
bigpresh/scrub_deserialised_body_data_params
branch
from
2f85e05
to
91dbba8
Compare
bigpresh
changed the title
bigpresh
force-pushed
the
bigpresh/scrub_deserialised_body_data_params
branch
2 times, most recently
from
96bdee9
to
069310c
Compare
If we have `$c->req->body_data` - for e.g. the request was a POST with a JSON body which Catalyst has decoded into `$c->req->body_data` - then scrub HTML in there too (but applying the same `ignore_params` checks so that you can exempt certain JSON body params from scrubbing). Also moved the ignore_params tests into t/03_params.t, and added the tests for this new feature there too - don't need so many individual test apps, when most features can be tested with a single test app.
Use `is()` not `ok()` so that, if the request status is *not* what we expect, we get to see what it actually was.
If Catalyst::Action::REST / Catalyst::Controller::REST is in use, the
request object will have a `data()` method for deserialised data as added by
the Catalyst::TraitFor::Request::REST role which ought to be scrubbed too.
To support this,
(a) the scrubbing needs to happen later in the request flow - now
`hooking dispatch()` instead of `prepare_parameters()`
(b) to avoid the data not being read if the request body had already
been read by `$c->req->body_data`, the fix in this PR is needed:
perl-catalyst/catalyst-runtime/pull/186
Until such time, dirtily monkey-patch the `seek()` in.
This seems to be the right time to go scrubbing, without the scrubbed data getting accidentally clobbered, and/or happening too late.
bigpresh
force-pushed
the
bigpresh/scrub_deserialised_body_data_params
branch
from
dfd74f9
to
222e5d5
Compare
Our monkey-patch pokes at Catalyst, so Catalyst needs to be loaded first. In the usual way of loading the plugin, e.g. listing the plugins you want on the `use Catalyst` line, that's fine, but here we're testing just that the plugin compiles, so we will need to load Catalyst first.
Yes, we're redefining a sub, intentionally, to monkey-patch, so silence that warning.
3 tasks
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.
If we have
$c->req->body_data- for e.g. the request was a POST with a JSON body which Catalyst has decoded into$c->req->body_data- then scrub HTML in there too (but applying the sameignore_paramschecks so that you can exempt certain JSON body params from scrubbing).Also, if we have
$c->req->dataadded by the role Catalyst::TraitFor::Request::REST which Catalyst::Action::REST / Catalyst::Controller::REST apply to Catalyst::Request to provide RESTful API tools, we need to scrub that too.Also moved the
ignore_paramstests intot/03_params.t, and added the tests for this new feature there too - don't need so many individual test apps, when most features can be tested with a single test app.A new test script and associated test app was added for the tests for scrubbing
$c->req->datathough, as they depend onCatalyst::Controller::RESTbeing available.There is a slightly ugly bit of monkey-patching to handle an issue that causes JSON parsing via the default handler for
application/jsonto fail if the body content filehandle has already been read - that's the fix I submitted in PR perl-catalyst/catalyst-runtime/pull/186 but fixed via monkey-patching in the meantime.