Allows passing sub protocols with ActionCable #41415

zedtux · 2021-02-11T21:42:00Z

Summary

The goal of this PR is to allow passing sub protocols to the backend via ActionCable.

Usage

const consumer = ActionCable.createConsumer()

consumer.addSubProtocol('custom-protocol')

consumer.connect()

which will sends the following protocol list : [ "actioncable-v1-json", "actioncable-unsupported", "custom-protocol" ]

zedtux · 2021-02-13T16:39:11Z

Apparently the failing builds aren't due to my changes.

zedtux · 2021-02-14T18:59:20Z

Code rebased on the main branch, tests are passing.

rails-bot · 2021-05-15T19:57:09Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

zedtux · 2021-05-15T21:18:12Z

Oh yeah, let's play the stupid bot game...

zzak · 2021-05-16T00:30:25Z

@zedtux Just a random question, but is passing the token as a header legit? What would the alternatives be? For example, could it be encrypted in the session?

Sorry for my random questions are not blocking this PR (it generally LGTM) but I want to understand the use-cases more.

zedtux · 2021-05-16T09:11:06Z

Thank you @zzak for your interest in my PR!

Well I made it based on what it exist in all the actioncable forks without really looking deep in specs, so I can't tell now if there are alternatives.

could it be encrypted in the session?

I'm using a JWT in this case (and all actioncable forks are doing the same) which are signed by the server and therefor can't be updated from the client side, but a JWT can be enrypted for sure.

If that is really required in order to get this PR merge, I can investigate, but in my use case I don't see the benefit.

Just let me know.

zzak · 2021-05-16T10:45:50Z

@zedtux Thanks for your reply. Can you point me to a couple of forks that implement the token passing part?

zedtux · 2021-05-17T06:13:02Z

@zzak please find bellow some of the existing "forks" (some are actually copies, not literally forks):

There are others but those can be a good start I think.

zedtux · 2021-07-27T11:26:35Z

Hello @zzak,

Could you please give me some feedbacks?

rails-bot · 2021-10-25T11:30:01Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

zedtux · 2021-10-25T12:04:58Z

Yeah well ... anyone from the Rails team to review this PR? Like @zzak, do you have some feedbacks?

zzak · 2021-11-01T02:56:50Z

@zedtux Hey sorry for the wait, I've been limiting my activity to the newsletter and not as focused on the day-to-day stuff.

This PR just needs someone with enough context to know if this is the best way to pass along the JWT for websockets, sorry I can't help more!

zedtux · 2021-11-01T21:14:23Z

Thanks for your reply. What can I do to make this happening?

zzak · 2021-11-03T22:00:19Z

@zedtux Ok, I had some time to look into this more and came up with a few thoughts:

Does HTTP_SEC_WEBSOCKET_PROTOCOL get filtered from logs automatically? Or can we make sure we don't log tokens somehow?
One of the libraries suggests encrypting the token, like decoded_token = JWT.decode token, Rails.application.secrets.secret_key_base, true, { :algorithm => 'HS256' }, can we do that too?
I'm not sure I love the name createConsumerFromMeta does it have to be an entirely new function?

zedtux · 2021-11-04T08:07:25Z

Thank you @zzak for your comment and time.

Does HTTP_SEC_WEBSOCKET_PROTOCOL get filtered from logs automatically? Or can we make sure we don't log tokens somehow?

In the case, by "logs", you mean "Browser logs" or "Browser console", it looks useless to me since:

The token is signed by the server (anyone trying to change it would break that signature and the server would reject it)
The token has an expiration time like cookie does (well, it should have, it depends on the backend configuration)

Also this token is provided to the client form a server request which is then visible in the server's response.

I hope I'm addressing correctly your point, but if I'm not, then please ask me more ☺️.

One of the libraries suggests encrypting the token, like decoded_token = JWT.decode token, Rails.application.secrets.secret_key_base, true, { :algorithm => 'HS256' }, can we do that too?

Oh sure, but that's to be done on the backend side. So far the scope of this PR is to implement a clean way of passing JWTs to the server in order to authenticate the connection. Now how the JWT has been generated by the server doesn't have any impact on this PR. Even better, the token passed to the server doesn't have to be a JWT, it could be anything the backend dev would like to have.

Now that being said, having security recommendations in the Rails' documentation would be definitely a nice to have. I also would like to say that some gems can help on this like the ruby-jwe gem.

I'm not sure I love the name createConsumerFromMeta does it have to be an entirely new function?

As Phil Karlton said:

There are only two hard things in Computer Science: cache invalidation and naming things.

😄

So feel free to propose a better name and I'll update it.

does it have to be an entirely new function?

Well I do prefer this:

ActionCable.createConsumerFromMeta("my.token.123")

than this:

ActionCable.createConsumer(undefined, "my.token.123")

in order to make ActionCable using the HTML head meta tag to guess the server URL, it look handy to me. Also this function is optional, the old one remain so it doesn't break anything.

Now if no one agree on this, I can remove it, of course.

zzak · 2021-11-12T02:42:07Z

For HTTP_SEC_WEBSOCKET_PROTOCOL logs, I was asking more if that header is logged anywhere in Rails request logs that we should be aware of? Since we probably don't want to be logging tokens on the server.

Re: createConsumer why is it that the first argument would be undefined? Is there a reason not to specify the URL? Or are you assuming to use the default one?

For encrypting this value, I think it's good to have security by default -- is there any reason not to do it? This PR is not huge by any means so it may be worth adding here.

zedtux · 2021-11-12T07:45:01Z

For HTTP_SEC_WEBSOCKET_PROTOCOL logs, I was asking more if that header is logged anywhere in Rails request logs that we should be aware of? Since we probably don't want to be logging tokens on the server.

Okay, sorry, I better understand now. So no, that's not logged in any Rails logs (I actually had to add a logging of the token while I was implementing it).

Re: createConsumer why is it that the first argument would be undefined? Is there a reason not to specify the URL? Or are you assuming to use the default one?

I want to use the URL from the head of the page provided by the action_cable_meta_tag helper AND pass the token. Passing undefined makes the function using the returned value of getConfig("url").

For encrypting this value, I think it's good to have security by default -- is there any reason not to do it? This PR is not huge by any means so it may be worth adding here.

Encrypting the token is used full when you use it to pass data from the server to the client, but when you use an empty token, but signed, to authenticate the connexion (like I'm doing in my app), encrypting it is not an issue, but some resources usage for nothing in my opinion.

That means it has to be optional.

Now I have questions:

Should Rails have a gem like the ruby-jwe gem as a dependency in order to make that option working when it is enabled?
Otherwise, should the Rails doc ask to add to the Gemfile that gem before to enable the encrypting option?
Finally should the rails new command line to have a new flag to enable JWT encryption and therefore add the required gem to the Gemfile of the generated project?

zedtux · 2021-11-25T18:42:42Z

@zzak can you please have a look at my reply?

rails-bot · 2022-02-23T19:11:53Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

zedtux · 2022-02-23T19:13:44Z

Oh no ... 😕, not that stupid bots ... @zzak can we continue on this PR please or can you please mention the right person to review it ? 🙏

zedtux · 2022-03-21T07:36:09Z

Can we move on with this PR please ?

I'm ready to answer/address all the questions, like I did with @zzak. I really would like to see this PR merged 🙏.

jorgemanrubia

Hey @zedtux, thanks for working on this. I made some suggestions, but I think this makes a lot of sense 👍.

I see you have gone through some back and forth here with the PR getting auto-closed several times. I'll promise I'll get this merged after those changes :).

jorgemanrubia · 2022-05-11T13:47:24Z

actioncable/app/javascript/action_cable/consumer.js

@@ -28,8 +28,9 @@ import Subscriptions from "./subscriptions"
 // automatically resubscribe.

 export default class Consumer {
-  constructor(url) {
+  constructor(url, token = null) {


I would leave the existing constructor as it is and, instead, add a new method to add subprotocols. It can be as simple as:

consumer = createConsumer("https://ws.example.com/cable") consumer.addSubProtocol("some subprotocol")

Then, the consumer can expose a list of consumer.subProtocols to use when initializing the WebSocket connection.

I'd use the name sub protocol instead of token to make it clear what this is doing. Using the sub protocol as a way to transport a JWT token is what motivates this, but we are adding a broader functionality here.

@zedtux also, we should document the change in actioncable/CHANGELOG.md (you can see the format in other frameworks such as activerecord/CHANGELOG.md, since right now it's empty for Action Cable).

Thank you very much for your comment 🙏 I will adapts as requested by tomorrow.

A quick update : I've already started to update my code, but I have to finish a milestone first. I'll come back on this on asap.

I'm done with the milestone, I will be able to work on this very soon !

I'm on it !

@jorgemanrubia I'm done. I have spent quite some time looking to assert the HTTP header content in order to check the subprotocols sent by WebSocket but it doesn't seem to be supported by mock-socket.

Let me know please if you see additional test to be added.

zedtux · 2022-11-18T18:56:48Z

@jorgemanrubia could you please have a look a this PR ?

zedtux · 2023-01-06T14:28:17Z

Hello @zzak and @jorgemanrubia,

First of all, happy new year ! :)

What about this PR ? Could you please review it / test it / merge it ? Is there anything else I should do ?

Thanks

zzak · 2023-01-07T22:21:57Z

@zedtux Hey sorry for the wait, I just re-read this thread and have some thoughts.

RE: createConsumer(undefined, token) vs. createConsumerFromMeta

I want to use the URL from the head of the page provided by the action_cable_meta_tag helper AND pass the token. Passing undefined makes the function using the returned value of getConfig("url").

Is it safe to assume that everyone will want that behavior? It seems like we are hiding the URL attribute in createConsumerFromMeta.

Also, now that I'm looking at the diff it seems createConsumerFromMeta is missing, I wonder if it got rebased out somehow?

RE: Encryption

As I understand it, this has to happen on the Rails backend and that this PR is simply adding the ability to pass data in this header to the client. Is that correct?

IMO, we can talk about the backend part after but I think if we're going to add this capability to actioncable, we should consider what the full feature looks like. Does that make sense?

Encrypting the token is used full when you use it to pass data from the server to the client, but when you use an empty token, but signed, to authenticate the connexion (like I'm doing in my app), encrypting it is not an issue, but some resources usage for nothing in my opinion.

I'm not 100% sure I follow, and I don't want to make any false assumptions, so I wonder if it's better if we had some examples of different use-cases.

Should Rails have a gem like the ruby-jwe gem as a dependency in order to make that option working when it is enabled?

I think it could be optional, but yes, this is what I mean by the full picture.

Otherwise, should the Rails doc ask to add to the Gemfile that gem before to enable the encrypting option?

Documentation is another option, we can simply explain how to use the feature properly, but I feel "The Rails Way" is to provide sane defaults and prevent the user from ...going off the rails. 😂

Finally should the rails new command line to have a new flag to enable JWT encryption and therefore add the required gem to the Gemfile of the generated project?

I think that goes along with the previous question before docs, if we're going to implement this properly we should consider the whole stack, not just actioncable IMO.

zedtux · 2023-01-10T08:56:02Z

@zzak thank you for your comment.

I think you missed the exchanges with @jorgemanrubia here where he asked me to revert the createConsumer function and remove the createConsumerFromMeta one.

He also told me :

I see you have gone through some back and forth here with the PR getting auto-closed several times. I'll promise I'll get this merged after those changes :).

As you said :

this PR is simply adding the ability to pass data in this header to the client.

To be a little bit more precise, it adds the ability to pass sub protocols from the client to the server.

You are raising good points, but I don't think it's the right place to discuss them, and I'm even not sure to be the right person to answer your questions.

I did the changes, the build is green, I don't see much I can do to get this PR merged now. Can anyone please just merge this PR ?

zzak · 2023-01-11T01:22:10Z

@zedtux Thanks for the explanation, and appreciate your patience with me.

Now that I understand what you mean, I think this PR is good to go.

Could you please update the title and description, I think we don't need to specify token but just any "data" can be passed via subprotocols. And also add a changelog?

Thanks!

zedtux · 2023-01-11T09:35:22Z

@zzak could you please review the PR title and description and let me know if it's fine like that ?

zedtux · 2023-01-24T20:25:40Z

Thank you very much all ! 🎉

Add changelog for #41415

TODO: Change to use main rails after rails/rails#41415 available

rails-bot bot added the actioncable label Feb 11, 2021

zedtux force-pushed the features/actioncable/token branch from cc7c5c4 to ba25ca4 Compare February 14, 2021 17:44

rails-bot bot added the stale label May 15, 2021

rails-bot bot removed the stale label May 15, 2021

rails-bot bot added the stale label Oct 25, 2021

rails-bot bot removed the stale label Oct 25, 2021

rails-bot bot added the stale label Feb 23, 2022

rails-bot bot removed the stale label Feb 23, 2022

jorgemanrubia reviewed May 11, 2022

View reviewed changes

zedtux force-pushed the features/actioncable/token branch 2 times, most recently from 1b31959 to 67d5b6a Compare October 19, 2022 06:58

Allows adding more protocols to the WebSocket sub protocols

6edb465

zedtux force-pushed the features/actioncable/token branch from 67d5b6a to 6edb465 Compare October 19, 2022 07:00

zzak added the ready PRs ready to merge label Jan 11, 2023

zedtux changed the title ~~Allows passing a token to be added to the WebSocket sub protocols~~ Allows passing sub protocols with ActionCable Jan 11, 2023

rafaelfranca merged commit 847cc9f into rails:main Jan 18, 2023

This was referenced Jan 24, 2023

How is this lib maintained? saravanabalagi/action-cable-react-jwt#5

Open

Rails builtin support to replace this library ruilisi/actioncable-jwt#18

Open

zzak added a commit to zzak/rails that referenced this pull request Jan 27, 2023

Add changelog for rails#41415

b1f6054

zzak added a commit that referenced this pull request Jan 28, 2023

Merge pull request #47165 from zzak/41415-changelog

6b30c3f

Add changelog for #41415

joaovitoras added a commit to zazos-team/actioncable-vue that referenced this pull request May 25, 2023

Change to use action cable jwt

79e4f9d

TODO: Change to use main rails after rails/rails#41415 available

Allows passing sub protocols with ActionCable #41415

Allows passing sub protocols with ActionCable #41415

Uh oh!

Conversation

zedtux commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Usage

Uh oh!

zedtux commented Feb 13, 2021

Uh oh!

zedtux commented Feb 14, 2021

Uh oh!

rails-bot bot commented May 15, 2021

Uh oh!

zedtux commented May 15, 2021

Uh oh!

zzak commented May 16, 2021

Uh oh!

zedtux commented May 16, 2021

Uh oh!

zzak commented May 16, 2021

Uh oh!

zedtux commented May 17, 2021

Uh oh!

zedtux commented Jul 27, 2021

Uh oh!

rails-bot bot commented Oct 25, 2021

Uh oh!

zedtux commented Oct 25, 2021

Uh oh!

zzak commented Nov 1, 2021

Uh oh!

zedtux commented Nov 1, 2021

Uh oh!

zzak commented Nov 3, 2021

Uh oh!

zedtux commented Nov 4, 2021

Does HTTP_SEC_WEBSOCKET_PROTOCOL get filtered from logs automatically? Or can we make sure we don't log tokens somehow?

One of the libraries suggests encrypting the token, like decoded_token = JWT.decode token, Rails.application.secrets.secret_key_base, true, { :algorithm => 'HS256' }, can we do that too?

I'm not sure I love the name createConsumerFromMeta does it have to be an entirely new function?

Uh oh!

zzak commented Nov 12, 2021

Uh oh!

zedtux commented Nov 12, 2021

Uh oh!

zedtux commented Nov 25, 2021

Uh oh!

rails-bot bot commented Feb 23, 2022

Uh oh!

zedtux commented Feb 23, 2022

Uh oh!

zedtux commented Mar 21, 2022

Uh oh!

jorgemanrubia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgemanrubia May 11, 2022

Choose a reason for hiding this comment

Uh oh!

jorgemanrubia May 11, 2022

Choose a reason for hiding this comment

Uh oh!

zedtux May 11, 2022

Choose a reason for hiding this comment

Uh oh!

zedtux May 23, 2022

Choose a reason for hiding this comment

Uh oh!

zedtux Sep 24, 2022

Choose a reason for hiding this comment

Uh oh!

zedtux Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

zedtux Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

zedtux commented Nov 18, 2022

zedtux commented Feb 11, 2021 •

edited

Loading

jorgemanrubia left a comment •

edited

Loading

zedtux commented Jan 10, 2023 •

edited

Loading