Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fixed JSON validation schema for trust data #75

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

phbelitz
Copy link
Member

The JSON validation schema for the trust data doesn't allow special characters like '-' in delegation names, thus failing the validation even though it should be valid. This has been fixed.

fixes #74

Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

we might want to push this to develop as it fails for bandit due to known issues fixed on develop. Could do a standard release tomorrow afternoon

@@ -20,7 +20,7 @@
"meta": {
"type": "object",
"patternProperties": {
"^(root|targets(\/\\w*)?)$": {
"^(root|targets(\/[\\w_:\\.-]*)?)$": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the spec?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the spec, it can be any string. https://github.com/theupdateframework/tuf/blob/f7695dace85444041489b83d5a66cd39c761bbd6/tuf/formats.py#L113

So i guess i'll change it to "^(root|targets(\/.*)?)$" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely should exempt some characters, like / (i.e. [^/]) or null bytes. I clearly vote for listing allowed characters explicitly and not use . though, otherwise you must be very certain to have thought of all characters to exempt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so back to \w_:\.- ? these are all that came to my mind

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say so. We could add +, ;, ~ and the like, but I wouldn't assume anyone does that. What do others think? @annekebr @Starkteetje

Also, \w includes _, and . does not need to be escaped when inside []. (Inside a character class, each character stands for itself, except \w and the like, and - which denotes ranges unless it appears at the beginning or end of the character class.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AnyString allows whitespace, so we certainly dont want that

Doing docker trust key generate something I get

key name "hallo du" must start with lowercase alphanumeric characters and can include "-" or "_" after the first character
key name "hallo++du" must start with lowercase alphanumeric characters and can include "-" or "_" after the first character
key name "halloDu" must start with lowercase alphanumeric characters and can include "-" or "_" after the first character

so I guess, dash and underscore are sufficient in addition to lowercase alphanum

@@ -75,7 +75,7 @@
},
"name": {
"type": "string",
"pattern": "^targets\/\\w*$"
"pattern": "^targets\/[\\w_:\\.-]*$"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

@phbelitz phbelitz force-pushed the hotfix/json-schema-validation branch 2 times, most recently from d1a16c3 to 65b5c0d Compare December 17, 2020 10:39
@phbelitz phbelitz changed the base branch from master to develop December 17, 2020 10:40
Copy link
Member

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my code comment

@phbelitz phbelitz force-pushed the hotfix/json-schema-validation branch 3 times, most recently from bb117cb to ffe58f8 Compare December 17, 2020 10:56
peterthomassen
peterthomassen previously approved these changes Dec 17, 2020
@peterthomassen peterthomassen dismissed their stale review December 17, 2020 11:30

Teetje had some extra info

The JSON validation schema for the trust data doesn't allow special characters like '-' in delegation names, thus failing the validation even though it should be valid. This has been fixed.

fixes #74
@phbelitz phbelitz force-pushed the hotfix/json-schema-validation branch from ffe58f8 to 544e820 Compare December 17, 2020 11:31
@phbelitz phbelitz changed the title hotfix: Fixed JSON validation schema for trust data fix: Fixed JSON validation schema for trust data Dec 17, 2020
Copy link
Member

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@phbelitz phbelitz merged commit 544e820 into develop Dec 17, 2020
@phbelitz phbelitz mentioned this pull request Dec 18, 2020
@phbelitz phbelitz deleted the hotfix/json-schema-validation branch February 15, 2021 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regex in schema json prevent connaisseur to validate signers with dash
4 participants