build icon indicating copy to clipboard operation
build copied to clipboard

Investigate Rack protection settings

Open florianm opened this issue 4 years ago • 1 comments

Problem

v0.3.7 works using the development server, but logs the user out unexpectedly when using the production server. This indicates a broken setting on the server side.

Fix

@smoyth found the following fix:

There is a part of Sinatra called Rack::Protection that protects against various sorts of attacks. One of those protections getting tripped which was causing the session to be cleared which was causing the logout.This Rack::Protection got added to the Gemfile due to Yaw's "upgrading dependencies" commit. It is generally a good thing to have so I tried disabling a few of the usual suspects (e.g. authenticity_token , json_csrf but that didn't fix it.Total disabling of all protections works: disable :protection [...] Considering that none of these protections were in place prior to this release, I think we can safely disable them.

Status quo

  • ODK Build 0.3.8 works and passes my test scenarios. Be aware these test the happy path and do not touch all possible edge cases. My tests do not prove the absence of any bugs.
  • Security is not decreased, as the patch only disables a newly added setting.
  • @issa-tseng has approved Tom's patch disabling protection in https://github.com/getodk/build/pull/255#issuecomment-988371285
  • Older releases didn't have Rack::Protection - do we need it now? Edit: It's a new feature that was introduced between Build releases, OK without for now.

Discussion

Help wanted: someone experienced in Ruby Rack / Sinatra could advise on which protections should be added.

florianm avatar Dec 08 '21 01:12 florianm

According to @smoyth...

I'm pretty sure the previous level was zero. This is a newer addition to Sinatra. The rack-protection gem did not exist in the project prior to this. I suppose the functionality could have gotten moved into that gem from the main Sinatra gem though. But I doubt it.

Given that, I'm happy with the disabled projections. It would be nice if a contributor wants to try to find the protections that would work, but I think it should be a low priority.

yanokwa avatar Dec 08 '21 05:12 yanokwa