Treat a null bundle definition as absent instead of panicking#412
Open
arpitjain099 wants to merge 1 commit into
Open
Treat a null bundle definition as absent instead of panicking#412arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
Bundle.Definitions is a map[string]*Schema, so a bundle.json with a JSON
null definition ("definitions": {"foo": null}) stores a nil pointer under
that key. The usual value, ok := m[key] lookup then returns ok == true with a
nil *Schema, and dereferencing it panicked in Parameter.Validate,
Output.Validate, ValuesOrDefaults, and IsOutputSensitive.
Guard the nil pointer at each of those sites so a null definition is handled
the same as a missing one, reusing the existing not-found error path. Adds a
regression test that builds a bundle with a null definition referenced by a
parameter and an output.
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi, thanks for maintaining cnab-go.
A bundle.json is allowed to carry a JSON null in its definitions map, for example:
Since Bundle.Definitions is a map[string]*Schema, that null gets stored as a nil pointer under the key. The common
value, ok := m[key]idiom then returns ok == true with a nil *Schema, and the following dereference panics. The four places that look a definition up and then read a field off it are affected:Parameter.Validate(bundle/parameters.go) readingschema.DefaultOutput.Validate(bundle/outputs.go) readingschema.DefaultValuesOrDefaults(bundle/bundle.go) usingsfor coercionIsOutputSensitive(bundle/outputs.go) readingdef.WriteOnlySo any caller that runs
Bundle.Validate,ValuesOrDefaults, orIsOutputSensitiveon a bundle that a user or another tool produced with a null definition crashes with a nil pointer dereference rather than getting an error back.The fix guards the nil pointer at each site (
if !ok || schema == nil, andok && def != nilfor the sensitivity check) so a null definition is handled the same way as a missing one. That reuses the existing not-found error path, so behavior for well-formed bundles is unchanged.I added a regression test (TestBundle_NilDefinition) that builds a bundle with a null definition referenced by both a parameter and an output, and asserts Validate, ValuesOrDefaults, and IsOutputSensitive all return an error instead of panicking. It panics before the change and passes after.
go test ./bundle/...is green.Happy to adjust the approach if you would rather reject a null definition earlier during unmarshalling instead.