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

Allow custom HTTP headers in BOSH and WebSockets #2907

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow custom HTTP headers in BOSH and WebSockets #2907

wants to merge 1 commit into from

Conversation

varnerac
Copy link
Contributor

@varnerac varnerac commented Oct 8, 2020

This PR adds custom HTTP headers to BOSH and WebSockets endpoints

Proposed changes include:

  • a description in mongooseim.cfg of how to add the custom HTTP headers
  • functionality in mod_bosh and mod_websockets to add the customer headers to responses
  • common test groups to exercise and verify the custom header functinonality

What's missing is docs at this point.

%% Uncomment to enable connection dropping, server-side pings
%% and/or custom HTTP headers. Header names must be lowercase.
% {timeout, 600000}, {ping_rate, 60000},
% {custom_headers, [{"access-control-Request-origin", "*"}]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gotta lowercase this R

@@ -122,14 +122,20 @@
{transport_options, [{max_connections, 1024}, {num_acceptors, 10}]},
{modules, [

{"_", "/http-bind", mod_bosh},
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be rewritten for TOML

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any thoughts on how you want it written for TOML? The precedent in mod_revproxy is tuple list of headers. I cannot tell if you're gonna move that mod to TOML or drop it.

Since Cowboy natively uses maps HTTP headers, I'd like to map the headers as a map in the TOML. Will that work, even if I stray from the extra headers convention in mod_revproxy?

Copy link
Member

@chrzaszcz chrzaszcz Nov 27, 2020

Choose a reason for hiding this comment

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

The whole mod_revproxy functionality is deprecated and will be dropped in the next release unless a new demand appears (which is unlikely). Custom headers would need to be added to TOML as a table (which is parsed as a map by tomerl). However, the whole TOML parser is being rewritten at the moment and a part of this code would need to be rewritten, unless you merge this PR to the toml-config branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll wait until #2929 is merged. I don't think it'd make sense to pollute that branch with this change, which is more functionality than config. I also plan on adding some default security headers like:

{ws_https_custom_headers, "[[listen.http.handlers.mod_websockets.custom_headers]]
  strict-transport-security: "max-age=31536000 ; includeSubDomains"
"}.

to the HTTPS endpoints as part of the standard as part of the standard vars-tool.config.in.

%% Upgrade to cowboy_loop behaviour to enable long polling
{cowboy_loop, Req, #rstate{}}.
{cowboy_loop, Req, #rstate{custom_headers = CustomHeaders}}.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use cowboy_req:set_resp_headers here instead of passing State everywhere.

-define(BOSH_BIND_PATH, "/http-bind").
-define(HANDSHAKE_TIMEOUT, 3000).
-define(eq(E, I), ?assertEqual(E, I)).
-define(FAST_PING_RATE, 500).
Copy link
Member

Choose a reason for hiding this comment

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

Some of these defines can be removed. I suspect it's a direct copy & paste from websockets suite? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed

@Neustradamus
Copy link
Contributor

@varnerac: Any news on it?

@varnerac
Copy link
Contributor Author

@varnerac: Any news on it?

I am about to return to this PR. I am just tied up in another project at the moment.

@Neustradamus
Copy link
Contributor

@varnerac: Do not forget ^^

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.

None yet

4 participants