Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031603002:
In PopularSites, parse (not sanitize) JSON safely. (Closed)
|
Created:
4 years, 6 months ago by sfiera Modified:
4 years, 6 months ago Reviewers:
Marc Treib CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIn PopularSites, parse (not sanitize) JSON safely.
All that JSON sanitization does is to parse it safely and then
reserialize it. For supporting unsafe JSON on iOS, it makes more sense
to me to inject a JSON parser (as we do in ntp_snippets), rather than a
JSON sanitizer. Then, we do the reserialization ourselves.
(this doesn't yet make the parser injectable)
BUG=603026
Committed: https://crrev.com/da97c63efbc08e4056c686e9ad6db8393749d9f7
Cr-Commit-Position: refs/heads/master@{#397394}
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 29 (11 generated)
Description was changed from ========== In PopularSites, parse (not sanitize) JSON safely. All that JSON sanitization does is to parse it safely and then reserialize it. For supporting unsafe JSON on iOS, it makes more sense to me to inject a JSON parser (as we do in ntp_snippets), rather than a JSON sanitizer. Then, we do the reserialization ourselves. BUG=603026 ========== to ========== In PopularSites, parse (not sanitize) JSON safely. All that JSON sanitization does is to parse it safely and then reserialize it. For supporting unsafe JSON on iOS, it makes more sense to me to inject a JSON parser (as we do in ntp_snippets), rather than a JSON sanitizer. Then, we do the reserialization ourselves. (this doesn't yet make the parser injectable) BUG=603026 ==========
sfiera@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:129: std::unique_ptr<std::vector<PopularSites::Site>> UnmarshalJson( IMO "unmarshal" isn't the right term here - we're not converting from a byte stream. Also, a base::Value isn't actually json (even if it's closely related). But, see below: Probably this helper isn't even required, so maybe the naming problem will just go away :) https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:361: std::string json_string; Shouldn't this be inside the "if (write_to_file)"? https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:363: // DO_NOT_SUBMIT: fail :) https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:401: base::Bind(&UnmarshalJson, base::Passed(std::move(json))), Any reason why this happens asynchronously? I think the conversion could just happen inline here..?
Not about the review, but... https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:411: callback_.Run(!!sites); Whoa, wait, shouldn't this be !!sites_ (note the underscore).
https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:129: std::unique_ptr<std::vector<PopularSites::Site>> UnmarshalJson( On 2016/06/02 08:32:11, Marc Treib wrote: > IMO "unmarshal" isn't the right term here - we're not converting from a byte > stream. Also, a base::Value isn't actually json (even if it's closely related). > But, see below: Probably this helper isn't even required, so maybe the naming > problem will just go away :) Yep, inlined. https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:361: std::string json_string; On 2016/06/02 08:32:11, Marc Treib wrote: > Shouldn't this be inside the "if (write_to_file)"? Done. (actually, moved into the blocking thread pool) https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:363: // DO_NOT_SUBMIT: fail On 2016/06/02 08:32:11, Marc Treib wrote: > :) It was indeed a DO_NOT_SUBMIT-fail. https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:401: base::Bind(&UnmarshalJson, base::Passed(std::move(json))), On 2016/06/02 08:32:11, Marc Treib wrote: > Any reason why this happens asynchronously? I think the conversion could just > happen inline here..? It was to keep ParseJson off the UI thread, but we now ensure that elsewhere (although we do *serialize* JSON on the UI thread now; I've fixed that). https://codereview.chromium.org/2031603002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:411: callback_.Run(!!sites); On 2016/06/02 08:49:28, sfiera wrote: > Whoa, wait, shouldn't this be !!sites_ (note the underscore). Fixed to be less confusing.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031603002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2031603002/20001
https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:130: bool WriteJson(const base::FilePath& local_path, const base::Value* json) { nit: WriteJsonToFile? https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:293: safe_json::SafeJsonParser::Parse( I might be missing something, but why do we need the expensive "safe" json parsing here? IIRC, the point of the Sanitizer thing was that we do the expensive out-of-process parsing only once after we first download the json, and then store the known-good json to a file, so on subsequent loads, we can use regular in-process parsing. https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:370: base::ListValue* list; nit: initialize to nullptr? https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.h:141: void OnJsonUnmarshaled(std::unique_ptr<std::vector<Site>> sites); I think this doesn't exist anymore
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:130: bool WriteJson(const base::FilePath& local_path, const base::Value* json) { On 2016/06/02 11:39:55, Marc Treib wrote: > nit: WriteJsonToFile? Done. https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:293: safe_json::SafeJsonParser::Parse( On 2016/06/02 11:39:54, Marc Treib wrote: > I might be missing something, but why do we need the expensive "safe" json > parsing here? IIRC, the point of the Sanitizer thing was that we do the > expensive out-of-process parsing only once after we first download the json, and > then store the known-good json to a file, so on subsequent loads, we can use > regular in-process parsing. We don't have to, but it was easier. I changed it. I still don't really understand what threat safe_json is supposed to mitigate, so I don't know when we should skip it. https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:370: base::ListValue* list; On 2016/06/02 11:39:55, Marc Treib wrote: > nit: initialize to nullptr? Sure, it's your code :) I just moved it. https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.h:141: void OnJsonUnmarshaled(std::unique_ptr<std::vector<Site>> sites); On 2016/06/02 11:39:55, Marc Treib wrote: > I think this doesn't exist anymore Done.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2031603002/40001
https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:293: safe_json::SafeJsonParser::Parse( On 2016/06/02 12:44:19, sfiera wrote: > On 2016/06/02 11:39:54, Marc Treib wrote: > > I might be missing something, but why do we need the expensive "safe" json > > parsing here? IIRC, the point of the Sanitizer thing was that we do the > > expensive out-of-process parsing only once after we first download the json, > and > > then store the known-good json to a file, so on subsequent loads, we can use > > regular in-process parsing. > > We don't have to, but it was easier. I changed it. > > I still don't really understand what threat safe_json is supposed to mitigate, > so I don't know when we should skip it. Well, the threat of malicious json, which might trigger a vulnerability in the json parser. So after the json has been sanitized once, there's no point in going through the exercise again. (IIUC - as I said, I might be missing something. Maybe it'd be best if Bernhard took a look too.) https://codereview.chromium.org/2031603002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:370: base::ListValue* list; On 2016/06/02 12:44:19, sfiera wrote: > On 2016/06/02 11:39:55, Marc Treib wrote: > > nit: initialize to nullptr? > > Sure, it's your code :) I just moved it. Haha, alright. I guess my reviewer was lazy (or less picky) :D https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:297: base::PostTaskAndReplyWithResult( The goal here is to keep the parsing off the UI thread for performance reasons? I don't think that's necessary; lots of other places use JSONReader on the UI thread, and this is not a huge file. Then again, just because other people are doing it doesn't mean it's right, so... I don't know. https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:311: OnJsonParsed(false, std::move(json)); Directly call ParseSiteList here, and remove the write_to_file param from OnJsonParsed?
https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:297: base::PostTaskAndReplyWithResult( On 2016/06/02 13:00:39, Marc Treib wrote: > The goal here is to keep the parsing off the UI thread for performance reasons? > I don't think that's necessary; lots of other places use JSONReader on the UI > thread, and this is not a huge file. > Then again, just because other people are doing it doesn't mean it's right, > so... I don't know. ParseJson() was called on the blocking pool before this CL, so I'm preserving that behavior. If you prefer the simpler code, say the word and I'll change it. (or, ask me to refactor it into a helper--this is basically the UnsafeJsonParser::Parse() that iOS needs) https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:311: OnJsonParsed(false, std::move(json)); On 2016/06/02 13:00:39, Marc Treib wrote: > Directly call ParseSiteList here, and remove the write_to_file param from > OnJsonParsed? Done.
LGTM, thanks! (I'll leave it up to you whether to move ParseJson to the UI thread.) https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:297: base::PostTaskAndReplyWithResult( On 2016/06/02 13:12:11, sfiera wrote: > On 2016/06/02 13:00:39, Marc Treib wrote: > > The goal here is to keep the parsing off the UI thread for performance > reasons? > > I don't think that's necessary; lots of other places use JSONReader on the UI > > thread, and this is not a huge file. > > Then again, just because other people are doing it doesn't mean it's right, > > so... I don't know. > > ParseJson() was called on the blocking pool before this CL, so I'm preserving > that behavior. If you prefer the simpler code, say the word and I'll change it. > > (or, ask me to refactor it into a helper--this is basically the > UnsafeJsonParser::Parse() that iOS needs) That seems to be a historical artifact - before https://codereview.chromium.org/1957313003, the function read the file *and* parsed the json, so it needed to be off-thread. Now that the file handling is elsewhere, IMO we don't need the extra thread-hopping anymore.
https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2031603002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:297: base::PostTaskAndReplyWithResult( On 2016/06/02 13:25:20, Marc Treib wrote: > On 2016/06/02 13:12:11, sfiera wrote: > > On 2016/06/02 13:00:39, Marc Treib wrote: > > > The goal here is to keep the parsing off the UI thread for performance > > reasons? > > > I don't think that's necessary; lots of other places use JSONReader on the > UI > > > thread, and this is not a huge file. > > > Then again, just because other people are doing it doesn't mean it's right, > > > so... I don't know. > > > > ParseJson() was called on the blocking pool before this CL, so I'm preserving > > that behavior. If you prefer the simpler code, say the word and I'll change > it. > > > > (or, ask me to refactor it into a helper--this is basically the > > UnsafeJsonParser::Parse() that iOS needs) > > That seems to be a historical artifact - before > https://codereview.chromium.org/1957313003, the function read the file *and* > parsed the json, so it needed to be off-thread. Now that the file handling is > elsewhere, IMO we don't need the extra thread-hopping anymore. Ah, OK, UI thread it is. Starting a dry run now.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031603002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2031603002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2031603002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031603002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2031603002/80001
Message was sent while issue was closed.
Description was changed from ========== In PopularSites, parse (not sanitize) JSON safely. All that JSON sanitization does is to parse it safely and then reserialize it. For supporting unsafe JSON on iOS, it makes more sense to me to inject a JSON parser (as we do in ntp_snippets), rather than a JSON sanitizer. Then, we do the reserialization ourselves. (this doesn't yet make the parser injectable) BUG=603026 ========== to ========== In PopularSites, parse (not sanitize) JSON safely. All that JSON sanitization does is to parse it safely and then reserialize it. For supporting unsafe JSON on iOS, it makes more sense to me to inject a JSON parser (as we do in ntp_snippets), rather than a JSON sanitizer. Then, we do the reserialization ourselves. (this doesn't yet make the parser injectable) BUG=603026 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== In PopularSites, parse (not sanitize) JSON safely. All that JSON sanitization does is to parse it safely and then reserialize it. For supporting unsafe JSON on iOS, it makes more sense to me to inject a JSON parser (as we do in ntp_snippets), rather than a JSON sanitizer. Then, we do the reserialization ourselves. (this doesn't yet make the parser injectable) BUG=603026 ========== to ========== In PopularSites, parse (not sanitize) JSON safely. All that JSON sanitization does is to parse it safely and then reserialize it. For supporting unsafe JSON on iOS, it makes more sense to me to inject a JSON parser (as we do in ntp_snippets), rather than a JSON sanitizer. Then, we do the reserialization ourselves. (this doesn't yet make the parser injectable) BUG=603026 Committed: https://crrev.com/da97c63efbc08e4056c686e9ad6db8393749d9f7 Cr-Commit-Position: refs/heads/master@{#397394} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/da97c63efbc08e4056c686e9ad6db8393749d9f7 Cr-Commit-Position: refs/heads/master@{#397394} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
