Skip to content

Commit

Permalink
Update OmniAuth to use POST in order to address CVE-2015-9284
Browse files Browse the repository at this point in the history
OmniAuth was warning about this issue in the logs:

WARN -- omniauth: (twitter)
  You are using GET as an allowed request method for OmniAuth. This may leave
  you open to CSRF attacks. As of v2.0.0, OmniAuth by default allows only POST
  to its own routes. You should review the following resources to guide your
  mitigation:
  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
  omniauth/omniauth#960
  https://nvd.nist.gov/vuln/detail/CVE-2015-9284
  omniauth/omniauth#809

The trouble is that the fix requires using POST instead of GET in the
"Sign in with Twitter" link, but doing so with a link requires using
JavaScript. This can be done using Rails' UJS (Unobtrusive JavaScript)
library, but that would require us to actually adopt it (since it's
currently not used anywhere), it also makes the code harder and more
expensive to test, since the testing environment starts needing a
JavaScript engine.

The alternative to JavaScript is switching to using a form with a submit
button to perform the action with a POST method. I ended up taking this
latter approach of using a form with a submit button, and then using CSS
to make it render the same as a hyperlink.

The CSS I used to render a button as a link was based on the following
answer from StackOverflow:
https://stackoverflow.com/a/12642009/9447571

With some extra modifications to adapt to the link colors and the
underline on hover of our current links. The effect looked good enough
to me on my local testing with a couple of browsers.

There are some links to "signing in" from the box with instructions of
how to play one of the challenges, or how to unlock more answers.
I ended up handling those using JavaScript, to locate the "signin"
form by HTML element id and then submit that form.

I also had to update the RSpec tests to use `click_button` instead of
`click_link`, since the "Sign in with Twitter" is now a button and
not a link anymore.

Additionally to the changes described above, I also updated the
"Sign Out" link to use a form and a button and the /signout URL
to only accept requests via POST.

Tested locally, also confirmed that all RSpec tests passed as expected.
  • Loading branch information
filbranden committed Sep 2, 2022
1 parent 3141b7e commit a804786
Show file tree
Hide file tree
Showing 10 changed files with 65 additions and 17 deletions.
42 changes: 42 additions & 0 deletions app/assets/stylesheets/site.scss.erb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,48 @@ a:visited {
color: #000066;
}

/* Style for button as a link */

button.btn-link {
align-items: normal;
background-color: rgba(0, 0, 0, 0);
border-color: #06A;
border-style: none;
box-sizing: content-box;
color: #06A;
cursor: pointer;
display: inline;
font: inherit;
height: auto;
padding: 0;
perspective-origin: 0 0;
text-align: start;
text-decoration: none;
transform-origin: 0 0;
width: auto;
-moz-appearance: none;
-webkit-logical-height: 1em; /* Chrome ignores auto, so we have to use this hack to set the correct height */
-webkit-logical-width: auto; /* Chrome ignores auto, but here for completeness */
}

button.btn-link:hover {
text-decoration: underline;
}

/* Mozilla uses a pseudo-element to show focus on buttons, */
/* but anchors are highlighted via the focus pseudo-class. */

@supports (-moz-appearance:none) { /* Mozilla-only */
button.btn-link::-moz-focus-inner { /* reset any predefined properties */
border: none;
padding: 0;
}
button.btn-link:focus { /* add outline to focus pseudo-class */
outline-style: dotted;
outline-width: 1px;
}
}

.comment {
padding:0.5em 0 0 0; margin:0;
}
Expand Down
2 changes: 1 addition & 1 deletion app/views/challenges/show.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<p>Check out <a href="/about#resources"> these helpful resources</a> to improve your Vim skills... <em>Game on.</em></p>

<% if !current_user %>
<div class="note clearfix">Unlock <b><%= @submissions.count_remaining %></b> remaining solutions by <b><%= link_to "signing in", "/auth/twitter?x_auth_access_type=read&use_authorize=true" %></b> and submitting your own entry</div>
<div class="note clearfix">Unlock <b><%= @submissions.count_remaining %></b> remaining solutions by <b><%= link_to "signing in", "#", onclick: "document.getElementById('signin').submit();" %></b> and submitting your own entry</div>
<% elsif @submissions.count_remaining > 0 %>
<div class="note clearfix">Unlock <b><%= @submissions.count_remaining %></b> remaining solutions by submitting a higher ranked entry</div>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/challenges/user.erb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<%= render :partial => "shared/submissions", :locals => { submissions: @submissions } %>
<% if !current_user %>
<div class="note clearfix">Unlock <b><%= @submissions.count_remaining %></b> remaining solutions by <b><%= link_to "signing in", "/auth/twitter?x_auth_access_type=read&use_authorize=true" %></b> and submitting your own entry</div>
<div class="note clearfix">Unlock <b><%= @submissions.count_remaining %></b> remaining solutions by <b><%= link_to "signing in", "#", onclick: "document.getElementById('signin').submit();" %></b> and submitting your own entry</div>
<% elsif @submissions.count_remaining > 0 %>
<div class="note clearfix">Unlock <b><%= @submissions.count_remaining %></b> remaining solutions by submitting a higher ranked entry</div>
<% end %>
Expand Down
12 changes: 9 additions & 3 deletions app/views/layouts/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,15 @@
<% if current_user %>
<li>Welcome <b><%= profile_link(current_user.nickname) %></b></li>
<li><%= link_to "Submit challenge", new_challenge_path %></li>
<li><%= link_to "Sign out", signout_path %></li>
<li><%= form_for :signout, url: signout_path do |f| %>
<%= f.button "Sign out", class: "btn-link" %>
<% end %></li>
<% else %>
<li><%= link_to "Sign in with Twitter", "/auth/twitter?x_auth_access_type=read&use_authorize=true" %></li>
<li><%= form_for :signin, url: "/auth/twitter", html: { id: 'signin' } do |f| %>
<%= f.hidden_field "x_auth_access_type", value: "read" %>
<%= f.hidden_field "use_authorize", value: "true" %>
<%= f.button "Sign in with Twitter", class: "btn-link" %>
<% end %></li>
<% end %>
</ul>
</div>
Expand All @@ -46,7 +52,7 @@
</div>
<div id="instructions" class="grid_5">
<pre class="terminal">
<span class="shell">Your VimGolf key: <%= current_user.nil? ? link_to("please sign in", "/auth/twitter?x_auth_access_type=read&use_authorize=true") : current_user['key'] %></span>
<span class="shell">Your VimGolf key: <%= current_user.nil? ? link_to("please sign in", "#", onclick: "document.getElementById('signin').submit();") : current_user['key'] %></span>

<span class="shell">$</span> gem install vimgolf
<span class="shell">$</span> vimgolf setup
Expand Down
2 changes: 1 addition & 1 deletion config/initializers/omniauth.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
OmniAuth.config.allowed_request_methods = [:get, :post]
OmniAuth.config.allowed_request_methods = [:post]

Rails.application.config.middleware.use OmniAuth::Builder do
provider(
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html

match "/auth/twitter/callback",to: "sessions#create", via: [:get, :post]
get "/signout", to: "sessions#destroy", as: :signout
post "/signout", to: "sessions#destroy", as: :signout

post "/entry", to: "entry#create"

Expand Down
2 changes: 1 addition & 1 deletion spec/features/deleting_a_challenge_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

visit root_path

click_link "Sign in with Twitter"
click_button "Sign in with Twitter"
click_link "challenge1"
click_button "Delete Challenge"

Expand Down
10 changes: 5 additions & 5 deletions spec/features/entry_feature_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
context '#comment' do
scenario 'can comment on an entry', js: true do
visit root_path
click_link "Sign in with Twitter"
click_button "Sign in with Twitter"
click_link 'test'
click_link 'Comment'
fill_in 'comment_text', with: 'test comment'
Expand All @@ -59,7 +59,7 @@
context '#destroy' do
scenario 'can delete an entry', js: true do
visit root_path
click_link "Sign in with Twitter"
click_button "Sign in with Twitter"
click_link 'test'
click_link 'Comment / Edit'
expect do
Expand All @@ -83,7 +83,7 @@

scenario 'owner can delete every entries', js: true do
visit root_path
click_link "Sign in with Twitter"
click_button "Sign in with Twitter"
click_link 'test'
click_link 'Comment / Edit'
expect do
Expand Down Expand Up @@ -122,7 +122,7 @@
context '#comment' do
scenario 'can comment on an entry', js: true do
visit root_path
click_link "Sign in with Twitter"
click_button "Sign in with Twitter"
click_link 'test'
click_link 'Comment'
fill_in 'comment_text', with: 'test comment participator'
Expand All @@ -137,7 +137,7 @@
context '#destroy' do
scenario 'can delete an entry', js: true do
visit root_path
click_link "Sign in with Twitter"
click_button "Sign in with Twitter"
click_link 'test'
click_link 'Comment / Edit'
expect do
Expand Down
4 changes: 2 additions & 2 deletions spec/features/sign_in_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
scenario "as a first-time user" do
visit root_path

click_link "Sign in with Twitter"
click_button "Sign in with Twitter"

expect(current_path).to eq(root_path)
expect(page).to have_text "Welcome @the science guy"
Expand All @@ -27,7 +27,7 @@

visit root_path

click_link "Sign in with Twitter"
click_button "Sign in with Twitter"

expect(page).to have_text "Welcome @the science guy"
expect(user.reload.name).to eq("bill nye")
Expand Down
4 changes: 2 additions & 2 deletions spec/features/submitting_a_challenge_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
scenario "with missing fields" do
visit root_path

click_link "Sign in with Twitter"
click_button "Sign in with Twitter"

click_link "Submit challenge"

Expand All @@ -26,7 +26,7 @@
scenario "with properly filled out fields" do
visit root_path

click_link "Sign in with Twitter"
click_button "Sign in with Twitter"

click_link "Submit challenge"

Expand Down

0 comments on commit a804786

Please sign in to comment.