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

Added set_extra_headers() to WebSocketServer #40975

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

rohanrhu
Copy link
Contributor

@rohanrhu rohanrhu commented Aug 2, 2020

Hi, I'm resending PR to master branch with changes.

@rohanrhu rohanrhu requested a review from a team as a code owner August 2, 2020 16:08
@Calinou Calinou added this to the 4.0 milestone Aug 2, 2020
@Calinou Calinou added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 2, 2020
@Faless Faless self-assigned this Jul 16, 2021
@rohanrhu
Copy link
Contributor Author

Please merge this PR. I'm still using my revision on my server because I need sending CORS headers for my game to connect my server from itch.io.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

See comments (const usage and p_ parameters prefix).
Looks overall good but needs a rebase 👍 .

modules/websocket/doc_classes/WebSocketServer.xml Outdated Show resolved Hide resolved
modules/websocket/emws_server.cpp Outdated Show resolved Hide resolved
modules/websocket/emws_server.h Outdated Show resolved Hide resolved
modules/websocket/websocket_server.h Outdated Show resolved Hide resolved
modules/websocket/wsl_server.cpp Outdated Show resolved Hide resolved
modules/websocket/wsl_server.cpp Outdated Show resolved Hide resolved
modules/websocket/wsl_server.h Outdated Show resolved Hide resolved
modules/websocket/wsl_server.h Outdated Show resolved Hide resolved
@Faless
Copy link
Collaborator

Faless commented Jan 30, 2022

because I need sending CORS headers for my game to connect my server from itch.io.

While the PR scope is okay, I strongly advise (STRONGLY!) to put an HTTP proxy in front of it to manage both the extra headers and requests sanitization, e.g. with nginx

@rohanrhu
Copy link
Contributor Author

because I need sending CORS headers for my game to connect my server from itch.io.

While the PR scope is okay, I strongly advise (STRONGLY!) to put an HTTP proxy in front of it to manage both the extra headers and requests sanitization, e.g. with nginx

You mean using proxy for just sending additional headers? We can just send CORS headers when it is neccessary, why would we need to use a proxy?

@rohanrhu
Copy link
Contributor Author

Should I resolve conflicts or you do that?

@Faless
Copy link
Collaborator

Faless commented Jan 30, 2022

Should I resolve conflicts or you do that?

I can't resolve the conflicts on your branch, please rebase and squash the commits, see here for instructions:
https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase

@rohanrhu
Copy link
Contributor Author

rohanrhu commented Feb 3, 2022

Should I resolve conflicts or you do that?

I can't resolve the conflicts on your branch, please rebase and squash the commits, see here for instructions: https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase

Thank you @Faless I will resolve conflicts soon.

@Faless
Copy link
Collaborator

Faless commented Feb 17, 2022

We can just send CORS headers when it is neccessary, why would we need to use a proxy?

As a bare minimum (it can offer more than that):

  • Access/Error logging
  • Requests filtering

I.e. a basic form of protection against the dozens of malicious requests (port scanners/exploit checkers/etc) you'll receive every day and that will otherwise end up being consumed by godot (most likely at a higher cost, and resulting in more errors in the godot log unrelated to the actual game).

@rohanrhu
Copy link
Contributor Author

rohanrhu commented Apr 9, 2022

Hiiiii @Faless I resolved conflicts with latest revision.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Good job 👍
See review comment on adding the override keyword to set_extra_headers when overriding.
The commits also needs to be squashed together https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

modules/websocket/emws_server.h Outdated Show resolved Hide resolved
@rohanrhu
Copy link
Contributor Author

Hiiii @Faless I squashed it into one commit.

@akien-mga akien-mga requested a review from Faless April 11, 2022 20:56
@Faless
Copy link
Collaborator

Faless commented Apr 12, 2022

CI is failing: https://github.com/godotengine/godot/runs/5964150172?check_suite_focus=true .
I think you just need to re-update the class reference (godot --doctool ) and re-squash.

@rohanrhu
Copy link
Contributor Author

CI is failing: https://github.com/godotengine/godot/runs/5964150172?check_suite_focus=true . I think you just need to re-update the class reference (godot --doctool ) and re-squash.

Meow.. I did it.

@Faless
Copy link
Collaborator

Faless commented Apr 12, 2022

I think CI will fail again, the doctool seem to not have worked properly 😿 .
Can you please try applying this patch?

diff --git a/modules/websocket/doc_classes/WebSocketServer.xml b/modules/websocket/doc_classes/WebSocketServer.xml
index ef3279aac4..46b0274de3 100644
--- a/modules/websocket/doc_classes/WebSocketServer.xml
+++ b/modules/websocket/doc_classes/WebSocketServer.xml
@@ -60,6 +60,13 @@
 				If [code]false[/code] is passed instead (default), you must call [PacketPeer] functions ([code]put_packet[/code], [code]get_packet[/code], etc.), on the [WebSocketPeer] returned via [code]get_peer(id)[/code] to communicate with the peer with given [code]id[/code] (e.g. [code]get_peer(id).get_available_packet_count[/code]).
 			</description>
 		</method>
+		<method name="set_extra_headers">
+			<return type="void" />
+			<argument index="0" name="headers" type="PackedStringArray" default="PackedStringArray()" />
+			<description>
+				Sets additional headers to be sent to clients during the HTTP handshake.
+			</description>
+		</method>
 		<method name="stop">
 			<return type="void" />
 			<description>

@rohanrhu
Copy link
Contributor Author

I think CI will fail again, the doctool seem to not have worked properly 😿 . Can you please try applying this patch?

Done. I'm on Windows btw maybe about it?

@Faless Faless merged commit 7fab11d into godotengine:master Apr 12, 2022
@Faless
Copy link
Collaborator

Faless commented Apr 12, 2022

Thanks! 🐈 🏆

@akien-mga
Copy link
Member

Cherry-picked for 3.5.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants