search

wharfkit / antelope

Core types, client interfaces, and other tools for working with Antelope-based blockchains.
Other
42 stars 23 forks source link

Add content-type header #101

Closed apporc closed 5 months ago

apporc commented 5 months ago

required by https://github.com/wharfkit/atomicassets/pull/3

includenull commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

For reference: https://github.com/AntelopeIO/leap/issues/2188