Skip to content

Commit

Permalink
Only call body_data if we have a data handler #4
Browse files Browse the repository at this point in the history
Only call `$c->req->body_data` if the request content type is one
there's a data handler for - otherwise we'll cause an exception e.g.:

```
  [error] Caught exception in engine "multipart/form-data does not have
    an available data handler. Valid data_handlers are application/json,
    application/x-www-form-urlencoded." <at /usr/local/lib/perl5/Catalyst.pm line 2420>
```

Also, add tests for multi-part file uploads, to ensure that the content
of uploaded files is left alone, but any other normal POST parameters
are still scrubbed.
  • Loading branch information
bigpresh committed Sep 19, 2023
1 parent 5fda49e commit dc2ab26
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 4 deletions.
13 changes: 9 additions & 4 deletions lib/Catalyst/Plugin/HTML/Scrubber.pm
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,18 @@ sub html_scrub {
my ($c, $conf) = @_;

# If there's body_data - for e.g. a POSTed JSON body that was decoded -
# then we need to walk through it, scrubbing as appropriate
if (my $body_data = $c->request->body_data) {
$c->_scrub_recurse($conf, $c->request->body_data);
# then we need to walk through it, scrubbing as appropriate; don't call
# body_data unless the content type is one there's a data handler for
# though, otherwise we'll trigger an exception (see GH#4)
if (exists $c->req->data_handlers->{ $c->req->content_type }) {
if (my $body_data = $c->request->body_data) {
$c->_scrub_recurse($conf, $c->request->body_data);
}
}

# And if Catalyst::Controller::REST is in use so we have $req->data,
# then scrub that too
# then scrub that too - but only if the request's content type is one
# there's a data handler for, otherwise it will explode!
if ($c->request->can('data')) {
my $data = $c->request->data;
if ($data) {
Expand Down
29 changes: 29 additions & 0 deletions t/03_params.t
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,34 @@ JSON
);
}

# multi-part file upload testing - ensure that an uploaded file's contents are
# not fiddled with, and that we don't explode trying to process it
# (e.g. by calling $c->req->body_data causing an exception because there's
# no data handler for this content type)
{
my $content = "<p>File content, <b>with HTML</b> in it.</p>";
diag "Uploaded file left alone, but other form fields still scrubbed";
my $req = POST '/upload',
Content_Type => 'form-data',
Content => [
foo => 'Form field <script>window.alert("with XSS!");</script>',
myfile => [
undef, "fake.file", 'Content-Type' => 'text/fake', Content => $content
],
];
my ($res, $c) = ctx_request($req);
is($res->code, RC_OK, 'response ok');
is(
$c->req->param('foo'),
'Form field ',
'XSS still stripped from normal POST body param in multi-part upload',
);
is(
$c->req->upload('myfile')->slurp,
"<p>File content, <b>with HTML</b> in it.</p>",,
"File content left alone",
);

}
done_testing();

5 changes: 5 additions & 0 deletions t/lib/MyApp03/Controller/Root.pm
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,10 @@ sub index : Path : Args(0) {
$c->res->body('index');
}

sub upload : Local : Args(0) {
my ($self, $c) = @_;
$c->res->body("Uploaded file content: " . $c->req->upload('myfile')->slurp);
}

1;

0 comments on commit dc2ab26

Please sign in to comment.