New issue
Advanced search Search tips

Issue 834245 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Chromad: Policy JSON doesn't accept single quotes

Project Member Reported by ljusten@chromium.org, Apr 18 2018

Issue description

Bad JSON still shows up on chrome://policy page. There's no user-visible error messaging, only a log.

For instance NativePrinters:

Ignoring invalid printer.  Invalid JSON object: {'display_name': 'printer', 'uuid': '', 'description': 'printer', 'uri': 'ipp://...', 'model': 'Printer', 'ppd_resource': {'effective_model': 'printer'}, 'manufacturer': 'printer'}
 

Comment 1 by olsen@chromium.org, Jun 19 2018

The JSON parser we are using doesn't allow for single-quoted strings (neither does the JSON specification). We can add support for them with a flag (there is a TODO in json_reader.h to do so) but we would need to talk to the designer of the parser - since parsers are complicated and must be designed by someone with experience.

However, the fixing of  bug 853719  and bug 834804 (both in progress) will at least show user-visible errors for all JSON parsing errors.
Correction - NativePrinters is a somewhat unique policy, and hasn't been fixed by any of the changes so far. Will investigate further.
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48898fe7be280b5a8395e152ed097803bb007e80

commit 48898fe7be280b5a8395e152ed097803bb007e80
Author: A Olsen <olsen@chromium.org>
Date: Fri Jul 06 09:49:33 2018

Add validation for NativePrinters policy

NativePrinters policy is unique in that is a 'list' policy, but it
has a schema that is more complicated than "list of string". There
are two problems with this schema - one, it is never used for
validation, and two, it is actually wrong. The true schema of the
value that we store in the pref store needs to be "list of string",
otherwise the code that applies this policy will stop working. See
this code at go/nativestringprinters

This change makes NativePrinters officially a list of strings, as it
needs to be, but adds a validation_schema that will warn if the JSON
embedded in each string does not parse or does not match the native
printer schema.

Reason for this change:
1. Show useful warnings in chrome://policy
2. Having the wrong schema is actually dangerous, since certain code
paths might automatically convert a policy value to one that matches
the schema - in this case, it would be by automatically parsing each
string as JSON so that the value becomes a list of dicts. See the
code at go/registry_dict_json. If this code path is ever used (for
instance, if support for this policy is added to Chrome for windows)
then the policy will have a different schema in Windows than in
Chrome OS, for non-obvious reasons.

Note that this policy predates LEGACY_EMBEDDED_JSON_WHITELIST so can
be added to it, since it contains embedded JSON. New policies should
not contain embedded JSON and should not be added there.

Bug:  834245 
Change-Id: Ic3beea00811ac6194c1ed05aa1bbbfb66c9ce546
Reviewed-on: https://chromium-review.googlesource.com/1125819
Commit-Queue: A Olsen <olsen@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572929}
[modify] https://crrev.com/48898fe7be280b5a8395e152ed097803bb007e80/chrome/browser/policy/configuration_policy_handler_list_factory.cc
[modify] https://crrev.com/48898fe7be280b5a8395e152ed097803bb007e80/components/policy/resources/policy_templates.json
[modify] https://crrev.com/48898fe7be280b5a8395e152ed097803bb007e80/components/policy/tools/syntax_check_policy_template_json.py

Status: Fixed (was: Assigned)
Fixed - still doesn't accept single quotes (will not fix this), but at least shows proper errors now.
Hi Andrew,

In the order to verify this bug, could you please confirm that this is the expected (proper) errors on chrome://policy page (see attached screenshot).

Thanks,
Ivan
Screenshot 2018-08-17 at 3.14.31 PM.png
138 KB View Download
From what I can see, it looks like a good error (at least more helpful than "OK").

Unfortunately that page has another problem - it is hard to read the entire error, especially from a screenshot.

If you like, you can triple-click on the error to highlight it, then copy and paste it into this bug, so I can verify the entire thing.
The entire error:

Key "items[0]": Error while parsing JSON value: Line: 1, column: 1, Unexpected token.

Also if ONC policy is not correctly configured I see the following error:

Network configuration failed to be parsed.

Could you please confirm that these errors are expected?
Cc: hendrich@chromium.org
I verified if the ONC is valid JSON, there's a reasonable error message. However, if the JSON is invalid, you get "Network configuration failed to be parsed." Maybe it should say something like "Network configuration failed to be parsed. Invalid JSON"?

In other cases we set the pref to the bad JSON string if the string is invalid IIRC. Would that be possible here as well?

I can easily update the error message to include "Invalid JSON". (I'll create a CL right away)

I think we don't display the malformed JSON string for network policies on purpose as those could contain credentials. Those are usually masked in chrome://policy, but if the value is invalid JSON we can't guarantee to properly mask those values anymore.
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1feffde9fb554c3b1c76f9146dc5518554d378f

commit d1feffde9fb554c3b1c76f9146dc5518554d378f
Author: Alexander Hendrich <hendrich@chromium.org>
Date: Tue Nov 13 12:20:14 2018

Extend ONC JSON parse error message

Add more detailed reason "invalid JSON" why the parsing failed.

Bug:  834245 
Change-Id: I9a81f425a2d877fecab7a9da1176e043e154d3f6
Reviewed-on: https://chromium-review.googlesource.com/c/1329977
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Commit-Queue: Alexander Hendrich <hendrich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607565}
[modify] https://crrev.com/d1feffde9fb554c3b1c76f9146dc5518554d378f/components/policy_strings.grdp

Sign in to add a comment