diff --git a/danger/cookie_setting/Dangerfile b/danger/cookie_setting/Dangerfile new file mode 100644 index 0000000000000000000000000000000000000000..c109f0558860e5f0e740cbe3d33f7dc9f03e4db3 --- /dev/null +++ b/danger/cookie_setting/Dangerfile @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +helper.all_changed_files.each do |filename| + next unless filename.end_with?('.rb') + next if filename.start_with?('spec/', 'ee/spec/', 'jh/spec/', 'qa/') + + cookie_setting.add_suggestions_for(filename) +end diff --git a/danger/plugins/cookie_setting.rb b/danger/plugins/cookie_setting.rb new file mode 100644 index 0000000000000000000000000000000000000000..f197ff2b37549aba792dce14188abd4ce198ea9b --- /dev/null +++ b/danger/plugins/cookie_setting.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require_relative '../../tooling/danger/cookie_setting' + +module Danger + class CookieSetting < ::Danger::Plugin + def add_suggestions_for(filename) + Tooling::Danger::CookieSetting.new(filename, context: self).suggest + end + end +end diff --git a/doc/development/cookies.md b/doc/development/cookies.md new file mode 100644 index 0000000000000000000000000000000000000000..a8bb36fa2b3c7d57b10d19376589afaca231c6a9 --- /dev/null +++ b/doc/development/cookies.md @@ -0,0 +1,26 @@ +--- +stage: none +group: unassigned +info: Any user with at least the Maintainer role can merge updates to this content. For details, see https://docs.gitlab.com/ee/development/development_processes.html#development-guidelines-review. +--- + +# Cookies + +In general, there is usually a better place to store data for users than in cookies. For backend development PostgreSQL, Redis, and [session storage](session.md) are available. For frontend development, cookies may be more secure than `localStorage`, `IndexedDB` or other options. + +In general do not put sensitive information such as user IDs, potentially user-identifying information, tokens, or other secrets into cookies. See our [Secure Coding Guidelines](secure_coding_guidelines.md) for more information. + +## Cookies on Rails + +Ruby on Rails has cookie setting and retrieval [built-in to ActionController](https://guides.rubyonrails.org/action_controller_overview.html#cookies). Rails uses a cookie to track the user's session ID, which allows access to session storage. [Devise also sets a cookie](https://github.com/heartcombo/devise/blob/main/lib/devise/strategies/rememberable.rb) when users select the **Remember Me** checkbox when signing in, which allows a user to re-authenticate after closing and re-opening a browser. + +You can [set cookies with options](https://api.rubyonrails.org/v7.1.3.4/classes/ActionDispatch/Cookies.html) such as `:path` , `:expires`, `:domain` , and `:httponly` . Do not change from the defaults for these options unless it is required for the functionality you are implementing. + +WARNING: +[Cookies set by GitLab are unset by default when users log out](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/controllers/sessions_controller.rb#L104). If you set a cookie with the `:domain` option, that cookie must be unset using the same `:domain` parameter. Otherwise the browser will not actually clear the cookie, and we risk persisting potentially-sensitive data which should have been cleared. + +## Cookies in Frontend Code + +Some of our frontend code sets cookies for persisting data during a session, such as dismissing alerts or sidebar position preferences. We use the [`setCookie` and `getCookie` helpers from `common_utils`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/lib/utils/common_utils.js#L697) to apply reasonable defaults to these cookies. + +Be aware that, after 2021, browsers have started [aggressively purging cookies](https://clearcode.cc/blog/browsers-first-third-party-cookies/) and `localStorage` data set by JavaScript scripts in an effort to fight tracking scripts. If cookies seem to be unset every day or every few days, it is possible the data is getting purged, and you might want to preserve the data server-side rather than in browser-local storage. diff --git a/spec/tooling/danger/cookie_setting_spec.rb b/spec/tooling/danger/cookie_setting_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..cf1d576c7a554e58988d8904b3aae72eaf394937 --- /dev/null +++ b/spec/tooling/danger/cookie_setting_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rspec-parameterized' +require 'gitlab/dangerfiles/spec_helper' + +require_relative '../../../tooling/danger/cookie_setting' +require_relative '../../../tooling/danger/project_helper' + +RSpec.describe Tooling::Danger::CookieSetting, feature_category: :tooling do + include_context 'with dangerfile' + + let(:fake_danger) { DangerSpecHelper.fake_danger } + let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) } + let(:comment_text) { "\n#{described_class::SUGGESTION}" } + let(:file_lines) { file_diff.map { |line| line.delete_prefix('+') } } + + before do + allow(cookie_setting).to receive(:project_helper).and_return(fake_project_helper) + allow(cookie_setting.project_helper).to receive(:file_lines).and_return(file_lines) + allow(cookie_setting.helper).to receive(:added_files).and_return([filename]) + allow(cookie_setting.helper).to receive(:changed_lines).with(filename).and_return(file_diff) + + cookie_setting.define_singleton_method(:add_suggestions_for) do |filename| + Tooling::Danger::CookieSetting.new(filename, context: self).suggest + end + end + + subject(:cookie_setting) { fake_danger.new(helper: fake_helper) } + + context 'for single line method call' do + let(:file_diff) do + <<~DIFF.split("\n") + + def index + + #{code} + + + + render text: 'OK' + + end + DIFF + end + + context 'when file is a non-spec Ruby file' do + let(:filename) { 'app/controllers/user_settings/active_sessions_controller.rb' } + + using RSpec::Parameterized::TableSyntax + + context 'when comment is expected' do + where(:code) do + [ + 'cookies[:my_key] = true', + 'cookies["my_key"] = true', + 'cookies[\'my_key\'] = true', + 'cookies[:my_key] = { value: "nbd", expires: 1.year, domain: "example.com" }', + 'cookies.encrypted[:my_key] = true', + 'cookies.permanent[:my_key] = true', + 'cookies.signed[:my_key] = true', + 'cookies.signed.encrypted.permanent[:my_key] = true', + 'cookies[Example::Class::With::CONSTANT] = true', + 'cookies[Example::Class::With::CONSTANT] = { value: "nbd", expires: 1.year, domain: "example.com" }', + 'cookies.encrypted[Example::Class::With::CONSTANT] = { value: "nbd", domain: "example.com" }', + 'cookies.permanent[Example::Class::With::CONSTANT] = { value: "nbd", domain: "example.com" }', + 'cookies.signed[Example::Class::With::CONSTANT] = { value: "nbd", domain: "example.com" }' + ] + end + + with_them do + specify do + expect(cookie_setting).to receive(:markdown).with(comment_text.chomp, file: filename, line: 2) + + cookie_setting.add_suggestions_for(filename) + end + end + end + + context 'when no comment is expected' do + where(:code) do + [ + 'cookies[:my_cookie].blank?', + 'cookies.signed[Some::Class::With::CONSTANTS].blank?', + 'cookies[:my_cookie] == "true"', + 'cookies.encrypted[:my_cookie] += "true"' + ] + end + + with_them do + specify do + expect(cookie_setting).not_to receive(:markdown) + + cookie_setting.add_suggestions_for(filename) + end + end + end + end + end +end diff --git a/tooling/danger/cookie_setting.rb b/tooling/danger/cookie_setting.rb new file mode 100644 index 0000000000000000000000000000000000000000..1a8c992f2ea641670ef8ea5d28e84077dfc1e764 --- /dev/null +++ b/tooling/danger/cookie_setting.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require_relative 'suggestion' + +module Tooling + module Danger + class CookieSetting < Suggestion + MATCH = %r{cookies(?:\.encrypted|\.signed|\.permanent)*\[([^\]]+)\]\s*=[^=]} + REPLACEMENT = nil + DOCUMENTATION_LINK = 'https://docs.gitlab.com/ee/development/cookies.html#cookies-on-rails' + + SUGGESTION = <<~MESSAGE_MARKDOWN.freeze + It looks like you are setting a server-side cookie. Please note that if you set + the `:domain` attribute for this cookie, you must ensure the cookie is unset when + the user logs out. Most cookies do not require this attribute. + + ---- + + For more information, see [cookies documentation](#{DOCUMENTATION_LINK}). + MESSAGE_MARKDOWN + end + end +end