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

chore: Allow text/plain content type descriptor for json formatted content body #2209

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

NagyZoltanPeter
Copy link
Contributor

Description

@weboko requested that from browser no-coors mode we need to supprt text/plain content-body requests.

Changes

  • For decoding content body we now allow text/plain content-type descriptor as well
  • Refactored rest type encode/decode functions to remove tons of code duplication.

How to test

  1. Setup a light client node.
  2. Send normal rest requests (POST/PUT) with curl as text/plain
  3. Should work as normal as with application/json content-type (notice no json format changed)

Issue

#2207

…ody. Refactored duplicated encode/decode functions for rest api
Copy link

github-actions bot commented Nov 9, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2209

Built from 5ff6902

Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

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

Didn't have time to actually run it, but from visual sanity check perspective it looks great (thanks for the refactoring!)

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much!

@NagyZoltanPeter
Copy link
Contributor Author

As @weboko pointed out, I missed converting /relay apis to accept text/plain requests, it is done now with some simplifications

@weboko
Copy link
Contributor

weboko commented Nov 11, 2023

Amazing, now it works. I noticed some of improvements that can be done:

  • browsers by default send Content-Type with encoding - text/plain;charset=UTF-8 and since it is made to accept only text/plain it would fail with 400, in we probably should check similarly to Express.js;
  • as additional precaution can we set Access-Control-Allow-Origi to * or make it configurable by config flag;

@fryorcraken
Copy link
Collaborator

as additional precaution can we set Access-Control-Allow-Origin to * or make it configurable by config flag;

Do not set it to * as it makes it a security issue as now any website can try to access a local running nwaku node using default port and would be authorized to then manipulate the nwaku node because the CORS would not stop it.

The CORS origin should only be localhost/127.0.0.1. @weboko, how are you hitting your webserver? I assume it's running in docker now? are you using the docker container ip to access the webserver? that might the issue, need to use the port exposed on localhost.

@NagyZoltanPeter
Copy link
Contributor Author

Added support for decoding content body when content-type header argument has more parameters above media type.

@weboko
Copy link
Contributor

weboko commented Nov 14, 2023

The CORS origin should only be localhost/127.0.0.1

Browser recognizes CORS based on Same origin policy which unfortunately takes port into account, so event if it is same domain but ports are different it violates same origin policy.

If REST is intended to be user only locally - it should be enough with MIME change in this PR.

@NagyZoltanPeter NagyZoltanPeter merged commit 6d81e38 into master Nov 14, 2023
10 checks passed
@NagyZoltanPeter NagyZoltanPeter deleted the chore-rest-decode-from-text branch November 14, 2023 15:59
@fryorcraken
Copy link
Collaborator

If REST is intended to be user only locally - it should be enough with MIME change in this PR.

Yes, REST should be user only locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants