1431750 - Make `additonalProperties` jsonschema property required when properties are specified

Status: RESOLVED FIXED

(Taskcluster :: Services, enhancement)

Description

[Pete Moore [:pmoore][:pete]]

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.

Comment 1

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for taskcluster-queue

This should be all schema changes required for the queue.

Comment 2

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for taskcluster-auth

Comment 3

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for taskcluster-github

Comment 4

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for aws-provisioner

Comment 5

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for taskcluster-hooks

Comment 6

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for taskcluster-index

Comment 7

[Pete Moore [:pmoore][:pete]]

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.

Comment 8

[Pete Moore [:pmoore][:pete]]

(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.

Comment 9

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for taskcluster-login

Comment 10

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for taskcluster-treeherder

Comment 11

[Pete Moore [:pmoore][:pete]]

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

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 12

[[github robot]]

Commits pushed to master at https://github.com/taskcluster/taskcluster-login

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

https://github.com/taskcluster/taskcluster-login/commit/0787d5caf947964504da4d3cf7be40c75cc8e75b
Merge pull request #69 from taskcluster/bug1431750

Bug 1431750 - add additionalProperties to schemas where missing

Comment 13

[[github robot]]

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

[[github robot]]

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 15

[[github robot]]

Commits pushed to master at https://github.com/taskcluster/taskcluster-hooks

https://github.com/taskcluster/taskcluster-hooks/commit/5093ec6f0c63c843300d2447c39739e020ceb3de
Bug 1431750 - add additionalProperties to schemas where missing

https://github.com/taskcluster/taskcluster-hooks/commit/4d657cbea0871383b8389ac2f98c0051e04b1e40
Merge branch 'master' into bug1431750

https://github.com/taskcluster/taskcluster-hooks/commit/291548dc2e5c733e3ed28960bdf5aca4baadf127
Bug 1431750 - cleanup

https://github.com/taskcluster/taskcluster-hooks/commit/d2d5aafae7025c4bc19db2476026f77b92136278
Bug 1431750 - remove dead space

https://github.com/taskcluster/taskcluster-hooks/commit/94bb501a7bf23271e869e7931920de2f98ae7403
Merge pull request #59 from taskcluster/bug1431750

Bug 1431750 - add additionalProperties to schemas where missing

Comment 16

[[github robot]]

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

Comment 17

[Pete Moore [:pmoore][:pete]]

Attachment: 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.

Comment 18

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for jsonschema2go

Whoops wrong link before. :)

Comment 19

[Chris Cooper [:coop] (he/him)]

Pete: is this done? Everything looks merged.

Comment 20

[Pete Moore [:pmoore][:pete]]

We still need to update taskcluster-lib-api to prevent this happening again in future... Probably just one more PR to go for this bug.

Comment 21

[Chris Cooper [:coop] (he/him)]

(In reply to Pete Moore [:pmoore][:pete] from comment #20)
> We still need to update taskcluster-lib-api to prevent this happening again
> in future... Probably just one more PR to go for this bug.

Pete: is this work still pending?

Comment 22

[Pete Moore [:pmoore][:pete]]

Yes, but the remaining piece is essentially bug 1260285, so I'll add it as a dependency.

Comment 23

[Dustin J. Mitchell [:dustin] (he/him)]

I'll close this once bug 1260285 lands.

Comment 24

[Dustin J. Mitchell [:dustin] (he/him)]

As promised :)