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

Add logging for missing --rpcvhosts on server #16498

Closed
wants to merge 4 commits into from
Closed

Add logging for missing --rpcvhosts on server #16498

wants to merge 4 commits into from

Conversation

lazzarello
Copy link

#16457

I couldn't figure out where the error output in the referenced issue originated so I added some extra strings to the debug output when it bails.

This happens when any DNS name is used with geth attach ... and the server has not specified that same DNS name via --rpcvhosts

@GitCop
Copy link

GitCop commented Apr 13, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: fc240d6

  • Commits must be prefixed with the package(s) they modify

  • Commit: 3ff2ba9

  • Commits must be prefixed with the package(s) they modify

  • Commit: 31a6be4

  • Commits must be prefixed with the package(s) they modify

  • Commit: b4a5f49

  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@@ -96,6 +99,8 @@ func (c *Client) sendHTTP(ctx context.Context, op *requestOp, msg interface{}) e
defer respBody.Close()
var respmsg jsonrpcMessage
if err := json.NewDecoder(respBody).Decode(&respmsg); err != nil {
err := fmt.Errorf("Error creating HTTP connection to RPC socket. Did you set --rpcvhosts on the server? Actual error is \"%s\"", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

You are reassigning the err here. What should happen is that the server should respond with a json-rpc error, saying that the host header was wrong, and we should just bubble that up. So if that's not happening, it would be better to try to discover why that is.

Copy link
Author

Choose a reason for hiding this comment

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

indeed, I don't know why that is and don't know how to determine the source. I have debugging notes in the referenced issue.

@holiman
Copy link
Contributor

holiman commented Apr 13, 2018

Ah, I see. So the reponse is not jsonrpc, it's plain old http 403:

HTTP/1.1 403 Forbidden
Content-Type: text/plain; charset=utf-8
X-Content-Type-Options: nosniff
Date: Fri, 13 Apr 2018 07:53:00 GMT
Content-Length: 23

invalid host specified

Thus we get

Fatal: Failed to start the JavaScript console: api modules: invalid character 'i' looking for beginning of value

The client should not assume it's json unless either Content-Type: application/json or http 200 OK .

@holiman
Copy link
Contributor

holiman commented Apr 13, 2018

For some reason, doRequest throws away the headers, including the http status, and just returns the body.

@lazzarello
Copy link
Author

oic, this totally makes sense now. I wasn't looking at the headers because they were getting thrown out. I'll try and add a content-type to the client headers.

@holiman
Copy link
Contributor

holiman commented Apr 13, 2018

Did you see my alternate PR #16500 ?

@lazzarello
Copy link
Author

ha! I did now. thanks. this is my first PR in this project. I appreciate the help.

@lazzarello lazzarello closed this Apr 16, 2018
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.

3 participants