Repository: koajs/koa
Pull request #897 (2017-02-09 11:56:21)

Fix malformed content-type header causing exception on charset get
A malformed content-type header may cause an exception when the charset getter is triggered. This is unexpected as the returning value should always be a string. This can be exploited by bad actors.

codecov-io: # [Codecov](https://codecov.io/gh/koajs/koa/pull/897?src=pr&el=h1) Report
> Merging [#897](https://codecov.io/gh/koajs/koa/pull/897?src=pr&el=desc) into [master](https://codecov.io/gh/koajs/koa/commit/948d3efaf6ef9520692112fe2a9a707c313468b6?src=pr&el=desc) will **decrease** coverage by `-0.24%`.


```diff
@@            Coverage Diff             @@
##           master     #897      +/-   ##
==========================================
- Coverage     100%   99.76%   -0.24%     
==========================================
  Files           4        4              
  Lines         417      421       +4     
  Branches      102      103       +1     
==========================================
+ Hits          417      420       +3     
- Misses          0        1       +1
```


| [Impacted Files](https://codecov.io/gh/koajs/koa/pull/897?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [lib/request.js](https://codecov.io/gh/koajs/koa/compare/948d3efaf6ef9520692112fe2a9a707c313468b6...09e3993736dfb0c5dde5a39d92c26b531af7a277?src=pr&el=tree#diff-bGliL3JlcXVlc3QuanM=) | `99.28% <80%> (-0.72%)` | :x: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/koajs/koa/pull/897?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/koajs/koa/pull/897?src=pr&el=footer). Last update [948d3ef...09e3993](https://codecov.io/gh/koajs/koa/compare/948d3efaf6ef9520692112fe2a9a707c313468b6...09e3993736dfb0c5dde5a39d92c26b531af7a277?el=footer&src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
ruimarinho: I've updated the PR based on @dead-horse's feedback, but I still think it should not swallow the errors.
jonathanong: i personally like throwing, but i would want the error to be a `4xx` error so that my error handler could catch it and tell the client that something is malformed. i wonder if we should have a "strict" app mode that throws instead of silently failing, but i'm -1 on adding new features.

i'm okay with this PR though if everyone else agrees

Patch:
diff --git a/lib/request.js b/lib/request.js
index d1a73ff..33c4b34 100644
--- a/lib/request.js
+++ b/lib/request.js
@@ -319,7 +319,13 @@ module.exports = {
     var type = this.get('Content-Type');
     if (!type) return '';
 
-    return contentType.parse(type).parameters.charset || '';
+    try {
+      type = contentType.parse(type);
+    } catch (e) {
+      return '';
+    }
+
+    return type.parameters.charset || '';
   },
 
   /**


Test patch:
diff --git a/test/request/charset.js b/test/request/charset.js
index 12eb09b..2d7a582 100644
--- a/test/request/charset.js
+++ b/test/request/charset.js
@@ -26,5 +26,11 @@ describe('req.charset', function(){
       req.header['content-type'] = 'text/plain; charset=utf-8';
       req.charset.should.equal('utf-8');
     })
+
+    it('should return "" if content-type is invalid', function(){
+      var req = request();
+      req.header['content-type'] = 'application/json; application/text; charset=utf-8';
+      req.charset.should.equal('');
+    })
   })
 })