Comments (9)
dhubler
commented on July 28, 2024
oh, I see, ok, I'll move that code up then and push a fix
…On Wed, Mar 22, 2023 at 2:35 PM sguay ***@***.***> wrote:
The server is returning a 200 OK even if set an error. The error handling
in https://github.dev/freeconf/restconf/util.go calls http.Error(w, msg,
code) and I clearly see while debugging that the code is set to 401:
func handleErr(compliance ComplianceOptions, err error, r *http.Request, w http.ResponseWriter) bool {
if err == nil {
return false
}
fc.Debug.Printf("web request error [%s] %s %s", r.Method, r.URL, err.Error())
msg := err.Error()
code := fc.HttpStatusCode(err)
if !compliance.SimpleErrorResponse {
errResp := errResponse{
Type: "protocol",
Tag: decodeErrorTag(code, err),
Path: decodeErrorPath(r.RequestURI),
Message: msg,
}
var buff bytes.Buffer
fmt.Fprintf(&buff, `{"ietf-restconf:errors":{"error":[`)
json.NewEncoder(&buff).Encode(&errResp)
fmt.Fprintf(&buff, `]}}`)
msg = buff.String()
}
http.Error(w, msg, code)
return true
}
http.Error(w, msg, code) generates the error message http: superfluous
response.WriteHeader call from github.com/freeconf/restconf.handleErr
(util.go:91)
This is the http.Error() function:
func Error(w ResponseWriter, error string, code int) {
w.Header().Set("Content-Type", "text/plain; charset=utf-8")
w.Header().Set("X-Content-Type-Options", "nosniff")
w.WriteHeader(code)
fmt.Fprintln(w, error)
}
The error occurs when calling w.WriteHeader(code). The variable wroteHeader
in the response is already set and cannot be changed.
func (w *response) WriteHeader(code int) {
if w.conn.hijacked() {
caller := relevantCaller()
w.conn.server.logf("http: response.WriteHeader on hijacked connection from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
return
}
if w.wroteHeader {
caller := relevantCaller()
w.conn.server.logf("http: superfluous response.WriteHeader call from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
return
}
.
.
.
This is caused by the sendOutput done in
freeconf/restconf/browser_handler.go at line 171 before the handleErr and
line 172.
sendOutput calls fmt.Fprintf at line 198. This is calling WriteHeader
eventually.
—
Reply to this email directly, view it on GitHub
<#27>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACA7VKNYBX3IRAF4FU5WLW5NA7PANCNFSM6AAAAAAWEGTD3U>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
from restconf.
dhubler
commented on July 28, 2024
I took a better look at this, and it is correct. it writes the JSON to a
buffer (not to client body) and sends that as the message.
To reproduce it working
1.) checkout the "401-test" branch
2.) cd example dir
3.) go main.go &
4.) curl -vv -H Accept:application/yang-data+json -X POST
http://localhost:8080/restconf/operations/car:reset
Are you sending your error after returning some nodes or fields without
errors? Because that would give an illegal buffer write error. Not sure
how to avoid that other than buffering the entire response.
…On Wed, Mar 22, 2023 at 4:08 PM Douglas Hubler ***@***.***> wrote:
oh, I see, ok, I'll move that code up then and push a fix
On Wed, Mar 22, 2023 at 2:35 PM sguay ***@***.***> wrote:
> The server is returning a 200 OK even if set an error. The error handling
> in https://github.dev/freeconf/restconf/util.go calls http.Error(w, msg,
> code) and I clearly see while debugging that the code is set to 401:
>
> func handleErr(compliance ComplianceOptions, err error, r *http.Request, w http.ResponseWriter) bool {
> if err == nil {
> return false
> }
> fc.Debug.Printf("web request error [%s] %s %s", r.Method, r.URL, err.Error())
> msg := err.Error()
> code := fc.HttpStatusCode(err)
> if !compliance.SimpleErrorResponse {
> errResp := errResponse{
> Type: "protocol",
> Tag: decodeErrorTag(code, err),
> Path: decodeErrorPath(r.RequestURI),
> Message: msg,
> }
> var buff bytes.Buffer
> fmt.Fprintf(&buff, `{"ietf-restconf:errors":{"error":[`)
> json.NewEncoder(&buff).Encode(&errResp)
> fmt.Fprintf(&buff, `]}}`)
> msg = buff.String()
> }
> http.Error(w, msg, code)
> return true
> }
>
> http.Error(w, msg, code) generates the error message http: superfluous
> response.WriteHeader call from github.com/freeconf/restconf.handleErr
> (util.go:91)
>
> This is the http.Error() function:
>
> func Error(w ResponseWriter, error string, code int) {
> w.Header().Set("Content-Type", "text/plain; charset=utf-8")
> w.Header().Set("X-Content-Type-Options", "nosniff")
> w.WriteHeader(code)
> fmt.Fprintln(w, error)
> }
>
> The error occurs when calling w.WriteHeader(code). The variable wroteHeader
> in the response is already set and cannot be changed.
>
> func (w *response) WriteHeader(code int) {
> if w.conn.hijacked() {
> caller := relevantCaller()
> w.conn.server.logf("http: response.WriteHeader on hijacked connection from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
> return
> }
> if w.wroteHeader {
> caller := relevantCaller()
> w.conn.server.logf("http: superfluous response.WriteHeader call from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
> return
> }
> .
> .
> .
>
> This is caused by the sendOutput done in
> freeconf/restconf/browser_handler.go at line 171 before the handleErr and
> line 172.
>
> sendOutput calls fmt.Fprintf at line 198. This is calling WriteHeader
> eventually.
>
> —
> Reply to this email directly, view it on GitHub
> <#27>, or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAACA7VKNYBX3IRAF4FU5WLW5NA7PANCNFSM6AAAAAAWEGTD3U>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
from restconf.
dhubler
commented on July 28, 2024
3.) go run main.go &
…On Wed, Mar 22, 2023 at 5:46 PM Douglas Hubler ***@***.***> wrote:
I took a better look at this, and it is correct. it writes the JSON to a
buffer (not to client body) and sends that as the message.
To reproduce it working
1.) checkout the "401-test" branch
2.) cd example dir
3.) go main.go &
4.) curl -vv -H Accept:application/yang-data+json -X POST
http://localhost:8080/restconf/operations/car:reset
Are you sending your error after returning some nodes or fields without
errors? Because that would give an illegal buffer write error. Not sure
how to avoid that other than buffering the entire response.
On Wed, Mar 22, 2023 at 4:08 PM Douglas Hubler ***@***.***> wrote:
> oh, I see, ok, I'll move that code up then and push a fix
>
> On Wed, Mar 22, 2023 at 2:35 PM sguay ***@***.***> wrote:
>
>> The server is returning a 200 OK even if set an error. The error
>> handling in https://github.dev/freeconf/restconf/util.go calls
>> http.Error(w, msg, code) and I clearly see while debugging that the code is
>> set to 401:
>>
>> func handleErr(compliance ComplianceOptions, err error, r *http.Request, w http.ResponseWriter) bool {
>> if err == nil {
>> return false
>> }
>> fc.Debug.Printf("web request error [%s] %s %s", r.Method, r.URL, err.Error())
>> msg := err.Error()
>> code := fc.HttpStatusCode(err)
>> if !compliance.SimpleErrorResponse {
>> errResp := errResponse{
>> Type: "protocol",
>> Tag: decodeErrorTag(code, err),
>> Path: decodeErrorPath(r.RequestURI),
>> Message: msg,
>> }
>> var buff bytes.Buffer
>> fmt.Fprintf(&buff, `{"ietf-restconf:errors":{"error":[`)
>> json.NewEncoder(&buff).Encode(&errResp)
>> fmt.Fprintf(&buff, `]}}`)
>> msg = buff.String()
>> }
>> http.Error(w, msg, code)
>> return true
>> }
>>
>> http.Error(w, msg, code) generates the error message http: superfluous
>> response.WriteHeader call from github.com/freeconf/restconf.handleErr
>> (util.go:91)
>>
>> This is the http.Error() function:
>>
>> func Error(w ResponseWriter, error string, code int) {
>> w.Header().Set("Content-Type", "text/plain; charset=utf-8")
>> w.Header().Set("X-Content-Type-Options", "nosniff")
>> w.WriteHeader(code)
>> fmt.Fprintln(w, error)
>> }
>>
>> The error occurs when calling w.WriteHeader(code). The variable wroteHeader
>> in the response is already set and cannot be changed.
>>
>> func (w *response) WriteHeader(code int) {
>> if w.conn.hijacked() {
>> caller := relevantCaller()
>> w.conn.server.logf("http: response.WriteHeader on hijacked connection from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
>> return
>> }
>> if w.wroteHeader {
>> caller := relevantCaller()
>> w.conn.server.logf("http: superfluous response.WriteHeader call from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
>> return
>> }
>> .
>> .
>> .
>>
>> This is caused by the sendOutput done in
>> freeconf/restconf/browser_handler.go at line 171 before the handleErr and
>> line 172.
>>
>> sendOutput calls fmt.Fprintf at line 198. This is calling WriteHeader
>> eventually.
>>
>> —
>> Reply to this email directly, view it on GitHub
>> <#27>, or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AAACA7VKNYBX3IRAF4FU5WLW5NA7PANCNFSM6AAAAAAWEGTD3U>
>> .
>> You are receiving this because you are subscribed to this thread.Message
>> ID: ***@***.***>
>>
>
from restconf.
sebastien-guay
commented on July 28, 2024
I tried your example and it works as expected. I debugged your example and compared the call stack with mine and here is what I see.
With your example we execute the line err = outputSel.LastErr in the else.
In my case we enter in if a.Input() != nil but not the next if input, err = readInput(r, a); err != nil .
Then we execute this last block where sendOutput calls fmt.Fprintf at line 198. This is calling WriteHeader eventually and set the 200 code. Then the handleErr is called but setting the error code is not allowed.
if outputSel := sel.Action(input); !outputSel.IsNil() && a.Output() != nil {
w.Header().Set("Content-Type", mime.TypeByExtension(".json"))
if err = sendOutput(w, outputSel, a); err != nil {
handleErr(err, r, w)
return
}
case "POST":
if meta.IsAction(sel.Meta()) {
// RPC
a := sel.Meta().(*meta.Rpc)
var input node.Node
if a.Input() != nil {
if input, err = readInput(r, a); err != nil {
handleErr(err, r, w)
return
}
}
if outputSel := sel.Action(input); !outputSel.IsNil() && a.Output() != nil {
w.Header().Set("Content-Type", mime.TypeByExtension(".json"))
if err = sendOutput(w, outputSel, a); err != nil {
handleErr(err, r, w)
return
}
} else {
err = outputSel.LastErr
}
} else {
// CRUD - Insert
payload = nodeutil.ReadJSONIO(r.Body)
err = sel.InsertFrom(payload).LastErr
}
Now I'm trying to understand why we don't have the same call stack.
from restconf.
sebastien-guay
commented on July 28, 2024
The difference with my request to the server is that my curl command has data specified with the "-d" option. That is why we have a different call stack and it seems we have an issue in that case.
I'm looking at the code and I think this block should be in the if a.Input() != nil:
if outputSel := sel.Action(input); !outputSel.IsNil() && a.Output() != nil {
w.Header().Set("Content-Type", mime.TypeByExtension(".json"))
if err = sendOutput(w, outputSel, a); err != nil {
handleErr(err, r, w)
return
}
sel.Action(input) uses input which is retrieved above by if input, err = readInput(r, a); err != ni
Currenytly if outputSel := sel.Action(input); !outputSel.IsNil() && a.Output() != nil and if a.Input() != nil are at the same level and they should not.
from restconf.
dhubler
commented on July 28, 2024
If i'm reading you correctly, we need to check outputSel.LastErr before going into that block where we potentially starting writing data in response body. I'll have to verify this later this evening but it's a good theory.
BTW: Even before this, I wanted get rid of LastErr and just return selection and err like normal go code. the poor decision to use LastErr thing dates back before I knew better.
from restconf.
sebastien-guay
commented on July 28, 2024
Just calling handleErr(outputSel.LastErr, r, w) is sending 401 and the response body correctly. sendoutput does not seem to be required.
from restconf.
dhubler
commented on July 28, 2024
just pushed the fix to master. I was able to verified origin error and
then verify it disappears w/the fix. thanks for reporting!
…On Thu, Mar 23, 2023 at 12:49 PM sguay ***@***.***> wrote:
Just calling handleErr(outputSel.LastErr, r, w) is sending 401 and the
response body correctly. sendoutput does not seem to be required.
—
Reply to this email directly, view it on GitHub
<#27 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACA7XDXI3OVHQOJ3OD7ODW5R5H7ANCNFSM6AAAAAAWEGTD3U>
.
You are receiving this because you commented.Message ID:
***@***.***>
from restconf.
sebastien-guay
commented on July 28, 2024
just pushed the fix to master. I was able to verified origin error and then verify it disappears w/the fix. thanks for reporting!
…
On Thu, Mar 23, 2023 at 12:49 PM sguay @.> wrote: Just calling handleErr(outputSel.LastErr, r, w) is sending 401 and the response body correctly. sendoutput does not seem to be required. — Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACA7XDXI3OVHQOJ3OD7ODW5R5H7ANCNFSM6AAAAAAWEGTD3U . You are receiving this because you commented.Message ID: @.>
Thanks a lot!
from restconf.
Related Issues (20)
- Feature request: Support XMLparsing HOT 1
- Feature request: Support XML output HOT 7
- ServeHTTP in server is not using Request context HOT 2
- Restconf server automatically opens a HTTP server HOT 1
- Feature request: Improve README HOT 1
- Feature request: Improve reflection support HOT 3
- Feature request: Improve error handling HOT 7
- Yang reference needs bump HOT 1
- Response content type not correctly set after XML pull request merge HOT 1
- local.go:ApplyStartupConfigFile function panics instead of returning an error HOT 2
- Restconf does not set the body in the right format (XML) for errors with a body when the accept in the request is XML HOT 2
- Restconf go.mod does not refer to latest freeconf/yang HOT 1
- Content-Type set in util::handle is overwritten by net/http/server.go HOT 1
- Response body for errors not valid in XML HOT 1
- Edit a leaf with `config false` HOT 2
- Question: how to post binary data with JSON HOT 3
- Bug in support for RFC 8525 HOT 3
- Question: how to view full config after change HOT 9
- Question: how to re-retrieve server configuration HOT 5
- For my clarification: Python way is completely "Go" free? Or not? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from restconf.