Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(201)

Issue 29377825: Issue 4951 - Restrict request headers in XMLHttpRequest.Also test Accept-Encoding with th… (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 9 months ago by hub
Modified:
2 years, 8 months ago
Reviewers:
Felix Dahlke, sergei
Base URL:
https://hg.adblockplus.org/libadblockplus/
Visibility:
Public.

Description

Issue 4951 - Restrict request headers in XMLHttpRequest. Also test Accept-Encoding with the XHR support.

Patch Set 1 : Initial changes #

Total comments: 14

Patch Set 2 : Reworked the testing. Addressed review comments. #

Total comments: 4

Patch Set 3 : improve tests following feedback. #

Total comments: 9

Patch Set 4 : updated patch with feedback #

Patch Set 5 : Forgot one more change from the review feedback. #

Patch Set 6 : Shorten the "sleep" is the wait loop. #

Total comments: 2

Patch Set 7 : Updated based on the feedback. #

Total comments: 20
Unified diffs Side-by-side diffs Delta from patch set Stats (+200 lines, -30 lines) Patch
M lib/compat.js View 1 2 2 chunks +39 lines, -1 line 0 comments Download
M test/WebRequest.cpp View 1 2 3 4 5 6 2 chunks +161 lines, -29 lines 20 comments Download

Messages

Total messages: 30
hub
2 years, 9 months ago (2017-03-02 21:16:24 UTC) #1
sergei
https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js File lib/compat.js (right): https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js#newcode327 lib/compat.js:327: if (this._forbiddenRequestHeaders[header.toLowerCase()] !== undefined) { Actually it's not according ...
2 years, 9 months ago (2017-03-02 22:25:32 UTC) #2
sergei
BTW, what about a new issue for it?
2 years, 9 months ago (2017-03-02 22:26:33 UTC) #3
hub
https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js File lib/compat.js (right): https://codereview.adblockplus.org/29377825/diff/29377829/lib/compat.js#newcode327 lib/compat.js:327: if (this._forbiddenRequestHeaders[header.toLowerCase()] !== undefined) { On 2017/03/02 22:25:31, sergei ...
2 years, 9 months ago (2017-03-02 23:30:13 UTC) #4
Felix Dahlke
Looks pretty good, I just have a few minor things. Plus, I agree with Sergei ...
2 years, 9 months ago (2017-03-03 08:33:05 UTC) #5
sergei
https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377829/test/WebRequest.cpp#newcode171 test/WebRequest.cpp:171: // The test will override console.warning so that the ...
2 years, 9 months ago (2017-03-03 08:38:59 UTC) #6
hub
https://codereview.adblockplus.org/29377825/diff/29377832/lib/compat.js File lib/compat.js (right): https://codereview.adblockplus.org/29377825/diff/29377832/lib/compat.js#newcode302 lib/compat.js:302: "accept-charset": true, On 2017/03/03 08:33:05, Felix Dahlke wrote: > ...
2 years, 9 months ago (2017-03-03 13:44:16 UTC) #7
hub
On 2017/03/03 08:33:05, Felix Dahlke wrote: > Looks pretty good, I just have a few ...
2 years, 9 months ago (2017-03-03 13:45:18 UTC) #8
hub
On 2017/03/03 08:38:59, sergei wrote: > What about check in WebRequest::GET? This new patch does ...
2 years, 9 months ago (2017-03-03 19:05:55 UTC) #9
Felix Dahlke
https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp#newcode40 test/WebRequest.cpp:40: class XHRTestWebRequest : public AdblockPlus::WebRequest IMHO it'd make more ...
2 years, 9 months ago (2017-03-06 16:51:10 UTC) #10
hub
will update the patch https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp#newcode40 test/WebRequest.cpp:40: class XHRTestWebRequest : public AdblockPlus::WebRequest ...
2 years, 9 months ago (2017-03-06 17:26:05 UTC) #11
Felix Dahlke
https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp#newcode237 test/WebRequest.cpp:237: } while (jsEngine->Evaluate("result")->IsUndefined()) On 2017/03/06 17:26:05, hub wrote: > ...
2 years, 9 months ago (2017-03-07 07:44:23 UTC) #12
hub
https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp#newcode237 test/WebRequest.cpp:237: } while (jsEngine->Evaluate("result")->IsUndefined()) On 2017/03/07 07:44:22, Felix Dahlke wrote: ...
2 years, 9 months ago (2017-03-07 15:52:32 UTC) #13
Felix Dahlke
https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29377868/test/WebRequest.cpp#newcode237 test/WebRequest.cpp:237: } while (jsEngine->Evaluate("result")->IsUndefined()) On 2017/03/07 15:52:32, hub wrote: > ...
2 years, 9 months ago (2017-03-07 17:03:43 UTC) #14
hub
https://codereview.adblockplus.org/29377825/diff/29378669/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378669/test/WebRequest.cpp#newcode209 test/WebRequest.cpp:209: ResetTestXHR(const AdblockPlus::JsEnginePtr& jsEngine, const CatchLogSystemPtr& logger) On 2017/03/07 17:03:43, ...
2 years, 9 months ago (2017-03-08 04:13:35 UTC) #15
hub
maybe I forgot to comment that I have updated the patch for yesterday for the ...
2 years, 9 months ago (2017-03-09 13:59:14 UTC) #16
Felix Dahlke
LGTM with just one nit :) Moving Sergei to CC since he's not available right ...
2 years, 9 months ago (2017-03-09 15:10:58 UTC) #17
hub
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode69 test/WebRequest.cpp:69: void On 2017/03/09 15:10:58, Felix Dahlke wrote: > Nit: ...
2 years, 9 months ago (2017-03-09 15:33:02 UTC) #18
sergei
Although it's already pushed, just in case, I would like to leave some comments. At ...
2 years, 8 months ago (2017-03-13 09:56:57 UTC) #19
sergei
BTW, the codereview is not closed.
2 years, 8 months ago (2017-03-13 09:57:31 UTC) #20
Felix Dahlke
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode211 test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 09:56:56, sergei ...
2 years, 8 months ago (2017-03-13 10:05:47 UTC) #21
sergei
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode211 test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 10:05:47, Felix ...
2 years, 8 months ago (2017-03-13 10:31:02 UTC) #22
Felix Dahlke
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode211 test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 10:31:02, sergei ...
2 years, 8 months ago (2017-03-13 11:05:42 UTC) #23
sergei
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode211 test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 11:05:42, Felix ...
2 years, 8 months ago (2017-03-13 11:39:16 UTC) #24
Felix Dahlke
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode211 test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 11:39:16, sergei ...
2 years, 8 months ago (2017-03-13 11:55:06 UTC) #25
sergei
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode211 test/WebRequest.cpp:211: auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem); On 2017/03/13 11:55:06, Felix ...
2 years, 8 months ago (2017-03-13 12:19:59 UTC) #26
hub
Posted a new patch in a new review. https://codereview.adblockplus.org/29382567 https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode86 test/WebRequest.cpp:86: ...
2 years, 8 months ago (2017-03-13 14:16:51 UTC) #27
hub
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode86 test/WebRequest.cpp:86: AdblockPlus::Sleep(60); On 2017/03/13 14:16:51, hub wrote: > I'll file ...
2 years, 8 months ago (2017-03-13 14:29:32 UTC) #28
sergei
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp#newcode29 test/WebRequest.cpp:29: lastRequestHeaders.clear(); On 2017/03/13 09:56:57, sergei wrote: > Sorry, I ...
2 years, 8 months ago (2017-03-16 11:08:25 UTC) #29
hub
2 years, 8 months ago (2017-03-16 13:03:53 UTC) #30
On 2017/03/16 11:08:25, sergei wrote:
> https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp
> File test/WebRequest.cpp (right):
> 
>
https://codereview.adblockplus.org/29377825/diff/29378689/test/WebRequest.cpp...
> test/WebRequest.cpp:29: lastRequestHeaders.clear();
> On 2017/03/13 09:56:57, sergei wrote:
> > Sorry, I forgot to say it again when my comments had been removed. I think
we
> > should use some unique URL in test requests to distinguish them from another
> > requests sent from FilterEngine. May be we don't observe it on practice
> > currently but there is a race condition because the instance of
MockWebRequest
> > is shared by all requests. BTW, there is also a data race on
> lastRequestHeaders
> > because the access to it is not synchronized.
> 
> What about this point? Actually I find it very important.

Filed an issue:
https://issues.adblockplus.org/ticket/5002

Will work on it.

Thanks
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5