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 content-type header #101

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Add content-type header #101

merged 1 commit into from
Jan 30, 2024

Conversation

apporc
Copy link
Contributor

@apporc apporc commented Jan 29, 2024

No description provided.

@apporc
Copy link
Contributor Author

apporc commented Jan 29, 2024

required by wharfkit/atomicassets#3

@aaroncox aaroncox merged commit d5c2cae into master Jan 30, 2024
6 checks passed
@apporc apporc deleted the apporc branch January 31, 2024 02:30
@includenull
Copy link

This change is causing CORS errors when using some nodes.

This change means that the header is always set to application/json when params are present, which breaks requests that expect normal POST such as chain RPC.

The headers should be passed through the call method, not determined automatically.

@apporc
Copy link
Contributor Author

apporc commented Feb 1, 2024

This api only supports json body, this is implied by JSON.stringify params.

CORS errors is not related to this commit, you should set Access-Control-Allow-Origin on the api server side.

@includenull
Copy link

This was tested and proven, I don't know how you can deny this commit broke the library.

It is not acceptable to require almost all RPC API providers to change their configs to support a change in this library.

@apporc
Copy link
Contributor Author

apporc commented Feb 2, 2024

No, it's not proven at all, it's just our nodes(wax.greymass.com) used the wrong config. Stop blaming me for that.
See here for the RFC: :https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.5

A sender that generates a message containing a payload body SHOULD
generate a Content-Type header field in that message unless the
intended media type of the enclosed representation is unknown to the
sender. If a Content-Type header field is not present, the recipient
MAY either assume a media type of "application/octet-stream"
([RFC2046], Section 4.5.1) or examine the data to determine its type.

Also here atomicassets' api middleware: https://expressjs.com/en/resources/middleware/body-parser.html#bodyparserjsonoptions

The bodyParser object exposes various factories to create middlewares. All middlewares will populate the req.body property with the parsed body when the Content-Type request header matches the type option, or an empty object ({}) if there was no body to parse, the Content-Type was not matched, or an error occurred.

We already use 'Access-Control-Allow-Origin: *' in our nodeos configuration, it's just there are some misconfiguration for the prelight request, that it failed the 'OPTIONS' call. And i am not automatically setting the 'Content-Type' header, we have been treating the request body as json from the library's beggining.

@aaroncox
Copy link
Member

aaroncox commented Feb 2, 2024

What should happen is nodeos fixes its defaults in a future release, and after that release hits a saturation point and nearly everyone is running it - then we update this library to do things the "right" way.

Maybe Leap 6.0 is a good target to make this fix.

If we do this right now according to the standards, we'll cause connectivity problems with nearly every app who upgrade to the newest version of this library. Many of them depend on 3rd party APIs, who's operators are completely unaware this is even a problem right now.

This would cause a flurry of apps complaining to API providers to "update their APIs" in an uncoordinated manner.

It sucks that we're stuck doing things against the standards... but in a decentralized ecosystem like this we can't control or even communicate with everyone running the software.

@apporc
Copy link
Contributor Author

apporc commented Feb 2, 2024

Yeah, i agree this is not acceptable as a sudden change, cause it may break some services. I just saw the code already treat request body as json, didn't realize it could break things if we specify 'Content-Type' for it. express thinks if you don't specify 'Content-Type' you are wrong, nodeos (by default) thinks you specify it you are wrong. You need to start nodeos this way to remove cors issues: nodeos -access-control-allow-origin "*" --access-control-allow-headers '*' --http-validate-host false

@aaroncox
Copy link
Member

aaroncox commented Feb 2, 2024

For reference: AntelopeIO/leap#2188

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