From e92c8000babb42412e2fbe4cb4ab7ff6d65a81eb Mon Sep 17 00:00:00 2001 From: Dennis Schubert Date: Thu, 2 Jul 2015 03:11:22 +0200 Subject: [PATCH 1/2] Do not leak private profile fields in JSON format Signed-off-by: Dennis Schubert --- app/presenters/person_presenter.rb | 43 ++++++++++++++---------- app/presenters/profile_presenter.rb | 26 ++++++++------ spec/presenters/person_presenter_spec.rb | 4 +-- 3 files changed, 44 insertions(+), 29 deletions(-) diff --git a/app/presenters/person_presenter.rb b/app/presenters/person_presenter.rb index 37f759353..f53087eee 100644 --- a/app/presenters/person_presenter.rb +++ b/app/presenters/person_presenter.rb @@ -1,38 +1,47 @@ class PersonPresenter < BasePresenter def base_hash - { id: id, - guid: guid, - name: name, + { + id: id, + guid: guid, + name: name, diaspora_id: diaspora_handle } end def full_hash - base_hash.merge({ - relationship: relationship, - block: is_blocked? ? BlockPresenter.new(current_user_person_block).base_hash : false, - contact: (!own_profile? && has_contact?) ? { id: current_user_person_contact.id } : false, + base_hash.merge( + relationship: relationship, + block: is_blocked? ? BlockPresenter.new(current_user_person_block).base_hash : false, + contact: (!own_profile? && has_contact?) ? {id: current_user_person_contact.id} : false, is_own_profile: own_profile? - }) + ) end def full_hash_with_avatar - full_hash.merge({avatar: AvatarPresenter.new(profile).base_hash}) + full_hash.merge(avatar: AvatarPresenter.new(profile).base_hash) end def full_hash_with_profile - full_hash.merge({profile: ProfilePresenter.new(profile).full_hash}) + attrs = full_hash + + if own_profile? || person_is_following_current_user + attrs.merge!(profile: ProfilePresenter.new(profile).private_hash) + else + attrs.merge!(profile: ProfilePresenter.new(profile).public_hash) + end + + attrs end - def as_json(options={}) + def as_json(_options={}) attrs = full_hash_with_avatar if own_profile? || person_is_following_current_user - attrs.merge!({ - :location => @presentable.location, - :birthday => @presentable.formatted_birthday, - :bio => @presentable.bio - }) + attrs.merge!( + location: @presentable.location, + birthday: @presentable.formatted_birthday, + bio: @presentable.bio + ) end attrs @@ -51,7 +60,7 @@ class PersonPresenter < BasePresenter contact = current_user_person_contact return :not_sharing unless contact - [:mutual, :sharing, :receiving].find do |status| + %i(mutual sharing receiving).find do |status| contact.public_send("#{status}?") end || :not_sharing end diff --git a/app/presenters/profile_presenter.rb b/app/presenters/profile_presenter.rb index 3581ef9f9..71e624fcb 100644 --- a/app/presenters/profile_presenter.rb +++ b/app/presenters/profile_presenter.rb @@ -2,20 +2,26 @@ class ProfilePresenter < BasePresenter include PeopleHelper def base_hash - { id: id, - tags: tags.pluck(:name), - bio: bio_message.plain_text_for_json, - location: location_message.plain_text_for_json, - gender: gender, - birthday: formatted_birthday, - searchable: searchable + { + id: id, + searchable: searchable } end - def full_hash - base_hash.merge({ + def public_hash + base_hash.merge( avatar: AvatarPresenter.new(@presentable).base_hash, - }) + tags: tags.pluck(:name) + ) + end + + def private_hash + public_hash.merge( + bio: bio_message.plain_text_for_json, + birthday: formatted_birthday, + gender: gender, + location: location_message.plain_text_for_json + ) end def formatted_birthday diff --git a/spec/presenters/person_presenter_spec.rb b/spec/presenters/person_presenter_spec.rb index 97f01887c..54424e2ee 100644 --- a/spec/presenters/person_presenter_spec.rb +++ b/spec/presenters/person_presenter_spec.rb @@ -16,12 +16,12 @@ describe PersonPresenter do let(:presenter){ PersonPresenter.new(person, current_user) } it "doesn't share private information when the users aren't connected" do - expect(presenter.as_json).not_to have_key(:location) + expect(presenter.full_hash_with_profile[:profile]).not_to have_key(:location) end it "has private information when the person is sharing with the current user" do expect(person).to receive(:shares_with).with(current_user).and_return(true) - expect(presenter.as_json).to have_key(:location) + expect(presenter.full_hash_with_profile[:profile]).to have_key(:location) end it "returns the user's private information if a user is logged in as herself" do From 8624ebb92164f878eeb9811727ba6fba1e7720c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Thu, 2 Jul 2015 11:09:05 +0200 Subject: [PATCH 2/2] bump to 0.5.1.2 --- Changelog.md | 6 ++++++ config/defaults.yml | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index 3dedf127c..954d1dba2 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,3 +1,9 @@ +# 0.5.1.2 + +diaspora\* versions prior 0.5.1.2 leaked potentially private profile data (namely the bio, birthday, gender and location fields) to +unauthorized users. While the frontend properly hid them, the backend missed a check to not include them in responses. +Thanks to @cmrd-senya for finding and reporting the issue. + # 0.5.1.1 Update rails to 4.2.2, rack to 1.6.2 and jquery-rails to 4.0.4. This fixes diff --git a/config/defaults.yml b/config/defaults.yml index 442cd798a..aa8622b89 100644 --- a/config/defaults.yml +++ b/config/defaults.yml @@ -4,7 +4,7 @@ defaults: version: - number: "0.5.1.1" # Do not touch unless doing a release, do not backport the version number that's in master + number: "0.5.1.2" # Do not touch unless doing a release, do not backport the version number that's in master heroku: false environment: url: "http://localhost:3000/"