Closed Bug 1467593 Opened 6 years ago Closed 6 years ago

Invalid URL in PopupBlocking (or any policy) shouldn't cause entire policy not to apply

Categories

(Firefox :: Enterprise Policies, enhancement, P3)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox62 + wontfix
firefox63 --- fixed

People

(Reporter: mkaply, Assigned: Felipe)

References

Details

Attachments

(2 files)

Bug 1467593 - Support non-strict arrays in the JsonSchemaValidator.
59 bytes, text/x-review-board-request
mythmon
: review+
Details
Bug 1467593 - Add strict=false to the schema of some policies.
59 bytes, text/x-review-board-request
mkaply
: review+
Details 
If you have an invalid URL in PopupBlocking (additional data on the end for instance), all of PopupBlocking fails to apply.

We should try to be better and just apply the things that are correct.

We should also consider trying to have a better error when this happens.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Note: i didn't include better error logging on this patch because I'll be improving errors in general on a separate bug.
Comment on attachment 8988637 [details]
Bug 1467593 - Support non-strict arrays in the JsonSchemaValidator.

mythmon, are you OK with this change?
Attachment #8988637 - Flags: feedback?(mcooper)
I'm uncomfortable with this change. Normandy also uses this sort of validation, for example when validating preferences to set in a rollout. In that case taking only part of an array could in some cases be very bad.

I'd be more comfortable if this behavior was optional, much like how JSON schema can optionally allow extra keys in an object. Normandy would definitely use the all-or-nothing approach for most of its schemas. I don't have a strong opinion about which behavior is default though.
Comment on attachment 8988637 [details]
Bug 1467593 - Support non-strict arrays in the JsonSchemaValidator.

Ok no prob, I'll make this opt-in
Attachment #8988637 - Flags: review?(mozilla)
Attachment #8988637 - Flags: feedback?(mcooper)
I was torn between adding this new param, or adding a new type, "array-non-strict". Both would be equal work, so if someone strongly prefers the latter, let me know and I can make the change.
Comment on attachment 8988637 [details]
Bug 1467593 - Support non-strict arrays in the JsonSchemaValidator.

https://reviewboard.mozilla.org/r/253874/#review260734

LGTM, thanks for adding the strict option
Attachment #8988637 - Flags: review?(mcooper) → review+
Comment on attachment 8988849 [details]
Bug 1467593 - Add strict=false to the schema of some policies.

https://reviewboard.mozilla.org/r/254006/#review260736
Attachment #8988849 - Flags: review?(mozilla) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/62a95cb4f231
Support non-strict arrays in the JsonSchemaValidator. r=mythmon
https://hg.mozilla.org/integration/autoland/rev/7e5805e33c2f
Add strict=false to the schema of some policies. r=mkaply
https://hg.mozilla.org/mozilla-central/rev/62a95cb4f231
https://hg.mozilla.org/mozilla-central/rev/7e5805e33c2f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Blocks: 1479870
Is this something which needs backport consideration?
Flags: needinfo?(felipc)
Yeah.. It probably would be nice to uplift, but it requires the changes in the validator code, so let's see if mkaply thinks it's worth it
Flags: needinfo?(felipc) → needinfo?(mozilla)
I don't think it's worth it. We'll let it ride the trains.
Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: