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

GNIP 87: Reduce information returned to users to only what is strictly required and accessible #7282

Closed
2 of 5 tasks
marthamareal opened this issue Apr 12, 2021 · 7 comments
Closed
2 of 5 tasks
Assignees
Labels
gnip A GeoNodeImprovementProcess Issue security Pull requests that address a security vulnerability
Milestone

Comments

@marthamareal
Copy link
Contributor

marthamareal commented Apr 12, 2021

GNIP - Users information leak

Overview

Currently

  1. GeoNode's /owners API endpoint returns the full list of users, including their personal information (!!!!). Also it returns users with no resources.
  2. The evaluation of visible resources depends on the following conditions.

Proposed By

@marthamareal
@giohappy

Assigned to Release

This proposal is for GeoNode .

State

  • Under Discussion
  • In Progress
  • Completed
  • Rejected
  • Deferred

Motivation

Details about the motivations. Why people should accept this proposal. What are the benefits compared to the current situation.

Proposal

  1. Return only owners with resources a user can access in owners api and fields(id and username ) for non-admin users
  2. Remove the dependence of these conditions on the evaluation of allowed_ressources.

Backwards Compatibility

Declare its Backwards Compatibility.

Future evolution

Explain which could be future evolutions.

Feedback

Update this section with relevant feedbacks, if any.

Voting

Project Steering Committee:

  • Alessio Fabiani +1
  • Francesco Bartoli:
  • Giovanni Allegri: +1
  • Simone Dalmasso:
  • Toni Schoenbuchner: +1
  • Florian Hoedt: +1

Links

Remove unused links below.

@gannebamm
Copy link
Contributor

Is this is GNIP, or a bugfix? I do not think we have to make formal decisions here, do we?

@afabiani
Copy link
Member

This is a bugfix IMHO or at least an enhancement. It smells like a security issue to me.

@giohappy giohappy changed the title Users information leak Reduce information returned to users to only what is strictly required and accessible Apr 12, 2021
@giohappy
Copy link
Contributor

giohappy commented Apr 12, 2021

@gannebamm @afabiani I asked @marthamareal to change it into a GNIP becasue the proposal is to change the responses returned by the API (and the contents displayed in the user pages).
It's not a bugfix by itself. There could be good reasons to have the full info returned, but we want to change it since it can be a security issue.

@giohappy
Copy link
Contributor

giohappy commented Apr 12, 2021

the following two points are particularly relevant:

  1. Here we filter out any resource the user doesn't have access to, independently from the advanced workflow. The filtering was applied only in case either ADMIN_MODERATE_UPLOADS, RESOURCE_PUBLISHING or GROUP_PRIVATE_RESOURCES were true before. @afabiani I see you added a commit for this condition within this commit. You titled it as an optimization, but I think that the filter set by @marthamareal should also be fine from the point of view of performances.
  2. Here we stop returning items with count==0. This affects any endpoint, nor only the owners endpoints. I think it makes sense to avoid returning information that the user souldn't reach in any case.

@t-book
Copy link
Contributor

t-book commented Apr 12, 2021

my +1 a very welcome fix!

@gannebamm gannebamm added the security Pull requests that address a security vulnerability label Apr 12, 2021
@afabiani afabiani added this to the 3.2 milestone Apr 12, 2021
@afabiani afabiani added the gnip A GeoNodeImprovementProcess Issue label Apr 12, 2021
@afabiani
Copy link
Member

@giohappy @marthamareal for the next time, the GNIPs have some special rules:

  1. You need to assign a number to the GNIP accordingly to this list, and report the number on the title
  2. You need to add the GNIP to the wiki and update the status accordingly

@afabiani
Copy link
Member

This is actually a regression, my fault. Thanks for spotting this out.

@afabiani afabiani changed the title Reduce information returned to users to only what is strictly required and accessible GNIP 87: Reduce information returned to users to only what is strictly required and accessible Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gnip A GeoNodeImprovementProcess Issue security Pull requests that address a security vulnerability
Projects
None yet
Development

No branches or pull requests

5 participants