-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add validation against schema for JSON #117
base: master
Are you sure you want to change the base?
Conversation
This is definitely a useful feature, I'll look into the PR. Thank you
Do you happen to know what the RFCs say about this?
…On Thu, Aug 1, 2024 at 3:37 AM Marcin Charęza ***@***.***> wrote:
Hi there.
We were having some problems with freeconf/yang and freeconf/restconf
being a bit too lenient when it comes to accepting payloads, so I made a
small change adding validation against YANG schema for JSON payloads. The
validation only looks for expected and unexpected child nodes for
containers and list, nothing too crazy. I'll be the first to admit I don't
really grok all the abstractions you've come up with, so let me know if
this approach is OK.
You might notice that I've made Validate a public JSONRdr method, to be
called by code accepting the payload, and thus making it optional. I'm not
sure this is good, but wanted to get to you with what I have coded so far.
Obviously this is not used in freeconf/restconf. For testing purposes I
made a small change to PATCH chandler:
diff --git a/browser_handler.go b/browser_handler.go
index 57cd0dc..84e2122 100644--- a/browser_handler.go+++ b/browser_handler.go@@ -176,12 +176,12 @@ func (hndlr *browserHandler) ServeHTTP(compliance ComplianceOptions, ctx context
case "PATCH":
// CRUD - Upsert
var input node.Node- input, err = requestNode(r, contentType)+ editable, _ := target.Constrain("content=config")+ input, err = validatedRequestNode(r, editable)
if err != nil {
handleErr(compliance, err, r, w, acceptType)
return
}- editable, _ := target.Constrain("content=config")
err = editable.UpsertFrom(input)
case "PUT":
// CRUD - Remove and replace@@ -320,6 +320,14 @@ func requestNode(r *http.Request, contentType MimeType) (node.Node, error) {
return nodeRdr(contentType, r.Body)
}
+func validatedRequestNode(r *http.Request, selection *node.Selection) (node.Node, error) {+ reader := nodeutil.JSONRdr{In: r.Body}+ if err := reader.Validate(selection); err != nil {+ return nil, fmt.Errorf("invalid payload: %s", err)+ }+ return reader.Node()+}+
func (m MimeType) IsXml() bool {
return strings.HasSuffix(string(m), "xml")
}
One final note: I have ignored the form uploads.
Hope you find the change useful. Let me know what you think.
Best regards.
------------------------------
You can view, comment on, or merge this pull request online at:
#117
Commit Summary
- 2cd4d7a
<2cd4d7a>
Add validation against schema for JSON
File Changes
(2 files <https://github.com/freeconf/yang/pull/117/files>)
- *M* nodeutil/json_rdr.go
<https://github.com/freeconf/yang/pull/117/files#diff-3cfbaee2338588e683c654b6c873eb7b723139209497d41ff61bf98707877705>
(100)
- *M* nodeutil/json_rdr_test.go
<https://github.com/freeconf/yang/pull/117/files#diff-8232ce5a92fbb18ff25f4b5d61fd02319dc32b4cb93809b506258a97c2f90c12>
(138)
Patch Links:
- https://github.com/freeconf/yang/pull/117.patch
- https://github.com/freeconf/yang/pull/117.diff
—
Reply to this email directly, view it on GitHub
<#117>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACA7UVNMJJXAAMQJZNJKLZPHQUZAVCNFSM6AAAAABLZ656K2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ2DCNZRHA4TGOI>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Yeah, I guess I should have checked the RFCs first instead of just going straight for the code. Just glancing right now I see that RFC 6241 mentions validation capability. But I'm not sure how (and if) that influences restconf. RFC 8040 has a section about error handling. In particular, it mentions two error codes that seem to be of interest, that is unknown-attribute and missing-attribute. I think I can do some reading about this if you feel this is necessary. |
It is not nec. I feel like who ever is running the RESTCONF server should
be able to decide how strict they wish to be. I've wanted to have a strict
JSON reader before.
…On Tue, Aug 6, 2024 at 11:11 AM Marcin Charęza ***@***.***> wrote:
Yeah, I guess I should have checked the RFCs first instead of just going
straight for the code. Just glancing right now I see that RFC 6241 mentions validation
capability <https://datatracker.ietf.org/doc/html/rfc6241#section-8.6>.
But I'm not sure how (and if) that influences restconf. RFC 8040 has a section
about error handling
<https://datatracker.ietf.org/doc/html/rfc8040#section-7>. In particular,
it mentions two error codes that seem to be of interest, that is
unknown-attribute and missing-attribute.
I think I can do some reading about this if you feel this is necessary.
—
Reply to this email directly, view it on GitHub
<#117 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACA7XGERST3TAVXUOSFTLZQDRSPAVCNFSM6AAAAABLZ656K2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZRGUZTGNZUGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
That sounds sensible. Let me know when you check out the PR. No pressure, of course. We have the workaround and this is open source after all. |
yeah, august happens to be a horrible month for me work wise. thanks for
understanding
…On Wed, Aug 7, 2024 at 10:39 AM Marcin Charęza ***@***.***> wrote:
That sounds sensible. Let me know when you check out the PR. No pressure,
of course. We have the workaround and this is open source after all.
—
Reply to this email directly, view it on GitHub
<#117 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACA7VBMKEAZVEYOP2Y3F3ZQIWTPAVCNFSM6AAAAABLZ656K2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZTGYZTQOJWGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Hi there.
We were having some problems with freeconf/yang and freeconf/restconf being a bit too lenient when it comes to accepting payloads, so I made a small change adding validation against YANG schema for JSON payloads. The validation only looks for expected and unexpected child nodes for containers and list, nothing too crazy. I'll be the first to admit I don't really grok all the abstractions you've come up with, so let me know if this approach is OK.
You might notice that I've made
Validate
a publicJSONRdr
method, to be called by code accepting the payload, and thus making it optional. I'm not sure this is good, but wanted to get to you with what I have coded so far. Obviously this is not used in freeconf/restconf. For testing purposes I made a small change to PATCH chandler:One final note: I have ignored the form uploads.
Hope you find the change useful. Let me know what you think.
Best regards.