search

freeconf / yang

Standards-based management for Golang microservices
Apache License 2.0
38 stars 14 forks source link

Add validation against schema for JSON #117

Open urxvtcd opened 3 months ago

urxvtcd commented 3 months ago

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.

dhubler commented 3 months ago

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/browserhandler.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:

https://github.com/freeconf/yang/pull/117 Commit Summary

File Changes

(2 files https://github.com/freeconf/yang/pull/117/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/freeconf/yang/pull/117, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACA7UVNMJJXAAMQJZNJKLZPHQUZAVCNFSM6AAAAABLZ656K2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ2DCNZRHA4TGOI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

urxvtcd commented 2 months ago

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.

dhubler commented 2 months ago

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 https://github.com/freeconf/yang/pull/117#issuecomment-2271533740, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACA7XGERST3TAVXUOSFTLZQDRSPAVCNFSM6AAAAABLZ656K2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZRGUZTGNZUGA . You are receiving this because you commented.Message ID: @.***>

urxvtcd commented 2 months ago

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.

dhubler commented 2 months ago

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 https://github.com/freeconf/yang/pull/117#issuecomment-2273638962, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACA7VBMKEAZVEYOP2Y3F3ZQIWTPAVCNFSM6AAAAABLZ656K2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZTGYZTQOJWGI . You are receiving this because you commented.Message ID: @.***>