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

Replace origin_whitelist with permitted_origins #1625

Merged
merged 4 commits into from Jul 12, 2020
Merged

Replace origin_whitelist with permitted_origins #1625

merged 4 commits into from Jul 12, 2020

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Jul 11, 2020

  • Added permitted_origin in lieu of origin_whitelist
  • Deprecated origin_whitelist with a warning

Closes #1620

@jkowens
Copy link
Member

jkowens commented Jul 11, 2020

@rhymes ah I don't think the failure is your change. I believe it's the issue described here #1624. Your PR looks good. What do you think about changing the option to permitted_origins?

@rhymes rhymes changed the title Replace origin_whitelist with origin_permitted Replace origin_whitelist with permitted_origins Jul 11, 2020
@rhymes
Copy link
Contributor Author

rhymes commented Jul 11, 2020

@jkowens done :)

@jkowens
Copy link
Member

jkowens commented Jul 11, 2020

Awesome, thanks! Hopefully we can fix the build in the near term, just to confirm this stays green. I don't think it should be too hard to remove Thin, but of course those are famous last words 😅

@rhymes rhymes marked this pull request as ready for review July 12, 2020 11:53
@rhymes
Copy link
Contributor Author

rhymes commented Jul 12, 2020

@jkowens rebased on top of master, tests pass! :D


if options.key? :origin_whitelist
warn "Rack::Protection origin_whitelist option is deprecated and will be removed, " \
"use origin_whitelist instead.\n"
Copy link
Member

Choose a reason for hiding this comment

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

@rhymes just realized this warning message needs tweaked 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahahaha great catch!

@jkowens jkowens requested a review from namusyaka July 12, 2020 13:29
@jkowens
Copy link
Member

jkowens commented Jul 12, 2020

@namusyaka can we go ahead and merge this in for the 2.1 release?

Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

expect(send(method.downcase, '/', {}, 'HTTP_ORIGIN' => 'http://malicious.com')).not_to be_ok
end

it "accepts #{method} requests with whitelisted Origin" do
mock_app do
use Rack::Protection::HttpOrigin, :origin_whitelist => ['http://www.friend.com']
use Rack::Protection::HttpOrigin, :permitted_origins => ['http://www.friend.com']
Copy link
Member

Choose a reason for hiding this comment

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

perimitted_origins: ['http://www.friend.com'] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

@namusyaka namusyaka merged commit 49fc62e into sinatra:master Jul 12, 2020
@rhymes rhymes deleted the rhymes/replace-origin-whitelist-with-permitted branch July 13, 2020 12:29
@rhymes
Copy link
Contributor Author

rhymes commented Jul 13, 2020

Thank you @namusyaka!

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.

[rack-protection] Consider renaming origin_whitelist to something else
3 participants