fix: update origin handling to support wildcard origins#472
fix: update origin handling to support wildcard origins#472randikabanura wants to merge 2 commits intocedarcode:masterfrom randikabanura:master
Conversation
| return false unless expected_origin | ||
|
|
||
| expected_origin.include?(client_data.origin) | ||
| Array(expected_origin).any? { |allowed_origin| allowed_origin === client_data.origin } |
There was a problem hiding this comment.
This follows how the wildcard origin matching handled in ActionCable: https://github.com/rails/rails/blob/cfc9b90f70d64a83b0aadec72bd858b197e2d733/actioncable/lib/action_cable/connection/base.rb#L234
santiagorodriguez96
left a comment
santiagorodriguez96
left a comment
There was a problem hiding this comment.
Great start! I still have some concerns about this that I shared in the respective issue (#471) but I think it's a great idea to add this
| return false unless expected_origin | ||
|
|
||
| expected_origin.include?(client_data.origin) | ||
| Array(expected_origin).any? { |allowed_origin| allowed_origin === client_data.origin } |
There was a problem hiding this comment.
chore: although it's not implied by the name of the variable, expected_origin is, in fact, already an array so we can avoid the conversion:
| Array(expected_origin).any? { |allowed_origin| allowed_origin === client_data.origin } | |
| expected_origin.any? { |allowed_origin| allowed_origin === client_data.origin } |
| return unless valid_origin?(expected_origin) | ||
|
|
||
| URI.parse(client_data.origin).host if expected_origin.size == 1 |
There was a problem hiding this comment.
issue: I see why we have to do this, but I don't think we should be changing the rpId depending on the origin of the request. After all, the credentials are scoped to the rpId so by doing this we will be generating credentials scoped to the different origins.
If that's desired, I think users of this gem should instead use our instance based configuration for handling this.
However, I think we have other options similar to what we did when we added the support for declaring multiple allowed origins: we could assume we won't be able to infer the rpId correctly if a wildcard origin is used so we just simply don't do it – which will force the users of the gem to explicitly set it in their configuration.
What do you think?
| def fake_wildcard_origin | ||
| /http:\/\/localhost.*/ | ||
| end |
There was a problem hiding this comment.
chore: this is not used, right? Should we remove it or was it added for a particular reason?
|
|
||
| let(:origin) { fake_origin } | ||
| let(:actual_origin) { origin } | ||
| let(:origin) { fake_wildcard_origin } |
There was a problem hiding this comment.
suggestion: I would keep the "normal" origin declaration here and just add an additional context under the "origin validation" section. What do you think?
| end | ||
|
|
||
| def fake_wildcard_origin | ||
| /http:\/\/localhost.*/ |
There was a problem hiding this comment.
thought: perhaps we should document in some way that, in order for wildcard domains to work, a regexp should be provided.
From the Rails' guides I found that they do it for ActionCable's allowed_request_origins:
Action Cable will only accept requests from specified origins, which are
passed to the server config as an array. The origins can be instances of
strings or regular expressions, against which a check for the match will be performed.config.action_cable.allowed_request_origins = ["https://rubyonrails.com", %r{http://ruby.*}]
What do you think?
2 participants
resolves #471