Make `additonalProperties` jsonschema property required when properties are specified

ASSIGNED
Assigned to

Status

Taskcluster
Platform and Services
ASSIGNED
a month ago
23 days ago

People

(Reporter: pmoore, Assigned: pmoore)

Tracking

Details

Attachments

(7 attachments, 3 obsolete attachments)

57 bytes, text/x-github-pull-request
Details | Review | Splinter Review
56 bytes, text/x-github-pull-request
Details | Review | Splinter Review
55 bytes, text/x-github-pull-request
Details | Review | Splinter Review
56 bytes, text/x-github-pull-request
Details | Review | Splinter Review
56 bytes, text/x-github-pull-request
Details | Review | Splinter Review
61 bytes, text/x-github-pull-request
Details | Review | Splinter Review
52 bytes, text/x-github-pull-request
Details | Review | Splinter Review
(Assignee)

Description

a month ago
Sometimes we neglected to explicitly declare `additionalProperties` property in jsonschema subschemas which have type `object`, which means it will default to the value `true`. This is often not what we want - so this bug is about making it a required property, so that it is always explicitly set. This should help us to ensure it has the correct value.

This is important, as it fundamentally affects how unmarshal the data into native go types, for example. If additionalProperties is false, we can safely generate a struct to unmarshal into. However, if it isn't set, it is effectively true, and then we'd need to marshal into something more generic, like json.RawMessage or map[string]interface{}. This fundamentally impacts the usability of our generated libraries.

Security wise, it also helps keep things tight, to reduce surface area of attacks that inject additional properties into data, since disallowed additional properties will not make it through schema validation.

The parts of this bug are:

1) Add `"additionalProperties": false` to subschemas where it is currently needed and missing, across all our service schemas.
2) Add `"additionalProperties": true` where it is currently not explicitly included (this is a no-op, since the default is true).
3) Update and make a new release of our schema validation library to ensure additionalProperties is always present when type is object.
4) Update our services to use the updated schema validation library.
(Assignee)

Comment 1

a month ago
Created attachment 8943975 [details] [review]
Github Pull Request for taskcluster-queue

This should be all schema changes required for the queue.
Assignee: nobody → pmoore
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a month ago
Created attachment 8943978 [details] [review]
Github Pull Request for taskcluster-auth
(Assignee)

Comment 3

a month ago
Created attachment 8943980 [details] [review]
Github Pull Request for taskcluster-github
(Assignee)

Comment 4

a month ago
Created attachment 8943989 [details] [review]
Github Pull Request for aws-provisioner
(Assignee)

Comment 5

a month ago
Created attachment 8943996 [details] [review]
Github Pull Request for taskcluster-hooks
(Assignee)

Comment 6

a month ago
Created attachment 8943997 [details] [review]
Github Pull Request for taskcluster-index
(Assignee)

Comment 7

a month ago
I'm going to change this bug - to make `additionalProperties` mandatory iff `properties` is specified. This reduces a lot of noise, since when no explicit properties and additionalProperties are both not specified, it is clear that additionalProperties should have the default value: true.
(Assignee)

Updated

a month ago
Summary: Make `additonalProperties` jsonschema property required when subschema is of type object → Make `additonalProperties` jsonschema property required when properties are specified
(Assignee)

Comment 8

a month ago
(In reply to Pete Moore [:pmoore][:pete] from comment #0)

> 2) Add `"additionalProperties": true` where it is currently not explicitly
> included (this is a no-op, since the default is true).

In light of comment 6, this will only be added if `properties` field is specified.
(Assignee)

Updated

a month ago
Attachment #8943980 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8943997 - Attachment is obsolete: true
(Assignee)

Comment 9

a month ago
Created attachment 8944032 [details] [review]
Github Pull Request for taskcluster-login
(Assignee)

Comment 10

a month ago
Created attachment 8944037 [details] [review]
Github Pull Request for taskcluster-treeherder
(Assignee)

Comment 11

a month ago
****************************************************

That should complete changes to all affected schemas

****************************************************


Please note in addition, I made this temporary branch[1] of jsonschema2go to help diagnose problems - although I don't intend to merge it to master, it is just useful for troubleshooting issues.

--
[1] https://github.com/taskcluster/jsonschema2go/commits/bug1431750

Comment 13

a month ago
Commits pushed to master at https://github.com/taskcluster/taskcluster-auth

https://github.com/taskcluster/taskcluster-auth/commit/df068b1f6ec4ff67c95263666f295f12c528a079
Bug 1431750 - add additionalProperties to schemas where missing

https://github.com/taskcluster/taskcluster-auth/commit/d402bb8d6be2041c4ddd7c14c3606b2ef3631a98
Merge pull request #123 from taskcluster/bug1431750

Bug 1431750 - add additionalProperties to schemas where missing

Comment 14

a month ago
Commits pushed to master at https://github.com/taskcluster/taskcluster-queue

https://github.com/taskcluster/taskcluster-queue/commit/181bb23fe0a3890f989c1c88e208b8a971f61324
Bug 1431750 - add additionalProperties to schemas where missing

https://github.com/taskcluster/taskcluster-queue/commit/fcd0931e4be512113949c3ec25da66cdf1174fda
Merge pull request #243 from taskcluster/bug1431750

Bug 1431750 - add additionalProperties to schemas where missing

Comment 16

24 days ago
Commits pushed to master at https://github.com/taskcluster/jsonschema2go

https://github.com/taskcluster/jsonschema2go/commit/a8de4f2562590b344ac181160b2699c4a0351676
Bug 1431750 - don't generate struct for json subschema if additionalProperties is not false

https://github.com/taskcluster/jsonschema2go/commit/0ed473e77675724b47fc80278a0843d6f588d867
Merge pull request #11 from taskcluster/bug1431750

Bug 1431750 - correctly handle case when additionalProperties is not false
(Assignee)

Comment 17

24 days ago
Created attachment 8946698 [details]
Github Pull Request for jsonschema2go

(In reply to Pete Moore [:pmoore][:pete] from comment #11)
> Please note in addition, I made this temporary branch[1] of jsonschema2go to
> help diagnose problems - although I don't intend to merge it to master, it
> is just useful for troubleshooting issues.
> 
> --
> [1] https://github.com/taskcluster/jsonschema2go/commits/bug1431750

So in the end, I did merge a modified form of it to master, since it was useful to provide the extra context in generated code comments.
(Assignee)

Comment 18

23 days ago
Created attachment 8947012 [details] [review]
Github Pull Request for jsonschema2go

Whoops wrong link before. :)
Attachment #8946698 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.