Make `additonalProperties` jsonschema property required when properties are specified

RESOLVED FIXED

Status

RESOLVED FIXED
a year ago
15 days ago

People

(Reporter: pmoore, Assigned: dustin)

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
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.
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
Created attachment 8943978 [details] [review]
Github Pull Request for taskcluster-auth
Created attachment 8943980 [details] [review]
Github Pull Request for taskcluster-github
Created attachment 8943989 [details] [review]
Github Pull Request for aws-provisioner
Created attachment 8943996 [details] [review]
Github Pull Request for taskcluster-hooks
Created attachment 8943997 [details] [review]
Github Pull Request for taskcluster-index
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.
Summary: Make `additonalProperties` jsonschema property required when subschema is of type object → Make `additonalProperties` jsonschema property required when properties are specified
(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.
Attachment #8943980 - Attachment is obsolete: true
Attachment #8943997 - Attachment is obsolete: true
Created attachment 8944032 [details] [review]
Github Pull Request for taskcluster-login
Created attachment 8944037 [details] [review]
Github Pull Request for taskcluster-treeherder
****************************************************

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 16

a year 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
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.
Created attachment 8947012 [details] [review]
Github Pull Request for jsonschema2go

Whoops wrong link before. :)
Attachment #8946698 - Attachment is obsolete: true
Pete: is this done? Everything looks merged.
Flags: needinfo?(pmoore)
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.
Flags: needinfo?(pmoore)
(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?
Flags: needinfo?(pmoore)
Yes, but the remaining piece is essentially bug 1260285, so I'll add it as a dependency.
Depends on: 1260285
Flags: needinfo?(pmoore)

I'll close this once bug 1260285 lands.

Assignee: pmoore → dustin

As promised :)

Status: ASSIGNED → RESOLVED
Last Resolved: 19 days ago
Resolution: --- → FIXED
Component: Platform and Services → Services
Product: Taskcluster → Taskcluster
You need to log in before you can comment on or make changes to this bug.