Avoid HTML in WebRestrictionsContentProvider interface

Avoid HTML in WebRestrictionsContentProvider interface

Replace the HTML error message with list of custom error parameters to avoid
potential security problems with passing HTML between apps.

BUG=594973
Committed: https://crrev.com/2fb7c77d7c638883ae4fcddff407ae6a4c02e438
Cr-Commit-Position: refs/heads/master@{#384308}

[aberent, 2016-03-30 11:56:10]

aberent@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2016-03-30 12:54:00]

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:58:
private Pair<Boolean, ErrorData> mResult;
Would it make sense to just combine these into a single class now?

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:95:
mResult = new Pair<Boolean, ErrorData>(false, null);
I think we can use just Pair<> now?

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:108:
// see
Please revert these. I'm okay with having a smaller column limit for comments
(although that's not in the style guide!), but if we reformat, we should remove
the existing line breaks beforehand.

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:288:
"Profile image URL", "Profile image URL2", "Custodian", "Custodian email",
"Second profile image URL"? It's a bit weird to have a "2" suffix here, but
"second" below. (Which I realize is an issue in existing code as well, but let's
start here with cleaning this up.)

https://codereview.chromium.org/1847523002/diff/1/components/web_restrictions...
File
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/1/components/web_restrictions...
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java:101:
if (column >= errorIntCount + 1 && column < errorIntCount + errorStringCount +
1) {
This probably deserves a comment. It might also be helpful to extract the
conversion from column to index in the strings list into a separate method, both
to avoid duplicating the logic, and for documentation purposes.

https://codereview.chromium.org/1847523002/diff/1/components/web_restrictions...
File
components/web_restrictions/browser/junit/src/org/chromium/components/webrestrictions/WebRestrictionsContentProviderTest.java
(right):

https://codereview.chromium.org/1847523002/diff/1/components/web_restrictions...
components/web_restrictions/browser/junit/src/org/chromium/components/webrestrictions/WebRestrictionsContentProviderTest.java:71:
// TODO Auto-generated method stub
😃

[aberent, 2016-03-30 14:28:20]

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:58:
private Pair<Boolean, ErrorData> mResult;
On 2016/03/30 12:54:00, Bernhard Bauer wrote:
> Would it make sense to just combine these into a single class now?

Done.

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:95:
mResult = new Pair<Boolean, ErrorData>(false, null);
On 2016/03/30 12:54:00, Bernhard Bauer wrote:
> I think we can use just Pair<> now?

N/A; it isn't a Pair any more.

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:108:
// see
On 2016/03/30 12:54:00, Bernhard Bauer wrote:
> Please revert these. I'm okay with having a smaller column limit for comments
> (although that's not in the style guide!), but if we reformat, we should
remove
> the existing line breaks beforehand.

Done.

https://codereview.chromium.org/1847523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:288:
"Profile image URL", "Profile image URL2", "Custodian", "Custodian email",
On 2016/03/30 12:54:00, Bernhard Bauer wrote:
> "Second profile image URL"? It's a bit weird to have a "2" suffix here, but
> "second" below. (Which I realize is an issue in existing code as well, but
let's
> start here with cleaning this up.)

Done.

https://codereview.chromium.org/1847523002/diff/1/components/web_restrictions...
File
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/1/components/web_restrictions...
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java:101:
if (column >= errorIntCount + 1 && column < errorIntCount + errorStringCount +
1) {
On 2016/03/30 12:54:00, Bernhard Bauer wrote:
> This probably deserves a comment. It might also be helpful to extract the
> conversion from column to index in the strings list into a separate method,
both
> to avoid duplicating the logic, and for documentation purposes.

Commented, and refactored calculation into local variable.

[aberent, 2016-03-30 14:35:55]

https://codereview.chromium.org/1847523002/diff/1/components/web_restrictions...
File
components/web_restrictions/browser/junit/src/org/chromium/components/webrestrictions/WebRestrictionsContentProviderTest.java
(right):

https://codereview.chromium.org/1847523002/diff/1/components/web_restrictions...
components/web_restrictions/browser/junit/src/org/chromium/components/webrestrictions/WebRestrictionsContentProviderTest.java:71:
// TODO Auto-generated method stub
On 2016/03/30 12:54:00, Bernhard Bauer wrote:
> 😃

Done.

[aberent, 2016-03-30 14:36:32]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-30 14:36:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847523002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847523002/20001

[commit-bot: I haz the power, 2016-03-30 15:17:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-30 15:17:39]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[aberent, 2016-03-31 09:39:53]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-31 09:40:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847523002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847523002/40001

[aberent, 2016-03-31 09:41:25]

PTAL - new files added to CL for changes to Web Restrictions client.

[Bernhard Bauer, 2016-03-31 10:09:07]

https://codereview.chromium.org/1847523002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:75:
mResult = new WebRestrictionsResult(false, 3, 6);
Actually, why don't we just pass in the error ints and strings as array
parameters? Then we could make WebRestrictionsResult completely immutable.

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
File
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java:41:
private int mNumErrorInts;
These could be final. Or you could derive the value from the array.

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java:47:
public WebRestrictionsResult(boolean shouldProceed, int numErrorInts, int
numErrorStrings) {
You could assert that |numErrorInts| and |numErrorString| are positive only if
|shouldProceed| is false.

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
components/web_restrictions/browser/web_restrictions_client.cc:136: return 0;
How can you return an int in a method that is supposed to return a std::string?

[commit-bot: I haz the power, 2016-03-31 10:21:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 10:21:01]

Dry run: This issue passed the CQ dry run.

[aberent, 2016-03-31 11:53:50]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[aberent, 2016-03-31 11:53:59]

https://codereview.chromium.org/1847523002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:75:
mResult = new WebRestrictionsResult(false, 3, 6);
On 2016/03/31 10:09:07, Bernhard Bauer wrote:
> Actually, why don't we just pass in the error ints and strings as array
> parameters? Then we could make WebRestrictionsResult completely immutable.

Done.

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
File
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java:41:
private int mNumErrorInts;
On 2016/03/31 10:09:07, Bernhard Bauer wrote:
> These could be final. Or you could derive the value from the array.

Done.

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
components/web_restrictions/browser/java/src/org/chromium/components/webrestrictions/WebRestrictionsContentProvider.java:47:
public WebRestrictionsResult(boolean shouldProceed, int numErrorInts, int
numErrorStrings) {
On 2016/03/31 10:09:07, Bernhard Bauer wrote:
> You could assert that |numErrorInts| and |numErrorString| are positive only if
> |shouldProceed| is false.

Done.

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1847523002/diff/40001/components/web_restrict...
components/web_restrictions/browser/web_restrictions_client.cc:136: return 0;
On 2016/03/31 10:09:07, Bernhard Bauer wrote:
> How can you return an int in a method that is supposed to return a
std::string?

Done.

[commit-bot: I haz the power, 2016-03-31 11:54:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847523002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847523002/60001

[commit-bot: I haz the power, 2016-03-31 12:37:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 12:38:00]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-03-31 13:06:03]

https://codereview.chromium.org/1847523002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:75:
int errorInt[] = new int[3];
You can construct the array inline: `new int[]{reason, allowAccessRequests,
isChildAccount}`.

[aberent, 2016-03-31 13:39:59]

https://codereview.chromium.org/1847523002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1847523002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:75:
int errorInt[] = new int[3];
On 2016/03/31 13:06:03, Bernhard Bauer wrote:
> You can construct the array inline: `new int[]{reason, allowAccessRequests,
> isChildAccount}`.

Done.

[Bernhard Bauer, 2016-03-31 14:36:34]

lgtm

[aberent, 2016-03-31 16:12:10]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2016-03-31 16:12:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847523002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847523002/80001

[commit-bot: I haz the power, 2016-03-31 16:52:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-31 16:54:02]

Description was changed from

==========
Avoid HTML in WebRestrictionsContentProvider interface

Replace the HTML error message with list of custom error parameters to avoid
potential security problems with passing HTML between apps.

BUG=594973
==========

to

==========
Avoid HTML in WebRestrictionsContentProvider interface

Replace the HTML error message with list of custom error parameters to avoid
potential security problems with passing HTML between apps.

BUG=594973

Committed: https://crrev.com/2fb7c77d7c638883ae4fcddff407ae6a4c02e438
Cr-Commit-Position: refs/heads/master@{#384308}
==========

[commit-bot: I haz the power, 2016-03-31 16:54:03]

Patchset 5 (id:??) landed as
https://crrev.com/2fb7c77d7c638883ae4fcddff407ae6a4c02e438
Cr-Commit-Position: refs/heads/master@{#384308}