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

Issue 242: add an @interfaces expandable endpoint #414

Closed
wants to merge 5 commits into from

Conversation

erral
Copy link
Sponsor Member

@erral erral commented Oct 22, 2017

See #242

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 96.583% when pulling 8b7f54c on issue-242-erral-interfaces into 88a6779 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 96.583% when pulling 8b7f54c on issue-242-erral-interfaces into 88a6779 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 96.583% when pulling 8b7f54c on issue-242-erral-interfaces into 88a6779 on master.

@erral erral changed the title [WIP] Issue 242: add an @interfaces expandable endpoint Issue 242: add an @interfaces expandable endpoint Oct 22, 2017
@buchi
Copy link
Member

buchi commented Oct 23, 2017

👍 LGTM

I wonder if exposing all those interface names publicly may lead to unwanted information disclosure.

@erral
Copy link
Sponsor Member Author

erral commented Oct 23, 2017

Yeah... during the training we wondered if we could restrict somehow the list of exposed interfaces, or at least set a list of known-good-set of interfaces, and expose just them...

Interfaces
==========

Getting the interfaces provided by the current context:
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erral we usually try to add a short introduction to non-Plone devs what this documentation section is about. E.g. a short introduction what interfaces are, maybe with a link to the Plone docs.

@tisto tisto self-requested a review October 25, 2017 18:08
@tisto
Copy link
Sponsor Member

tisto commented Oct 25, 2017

@erral LGTM as well. Though, as @buchi said, the interfaces are currently not visible by anonymous users. Therefore we might have to think about this...

@erral
Copy link
Sponsor Member Author

erral commented Oct 26, 2017

I share your concerns about the public visibility of the interfaces. Nevertheless they are needed, at least to know the navigation contexts when the front-end wants to build the global navigation for instance.

@ebrehault any comment on this?

@coveralls
Copy link

coveralls commented Oct 29, 2017

Coverage Status

Coverage increased (+0.02%) to 96.657% when pulling 5671b33 on issue-242-erral-interfaces into 8a84333 on master.

@coveralls
Copy link

coveralls commented Nov 4, 2017

Coverage Status

Coverage increased (+0.3%) to 96.389% when pulling d855284 on issue-242-erral-interfaces into 41b9bf6 on master.

@tkimnguyen
Copy link
Sponsor Member

What would you recommend as a way of restricting who has access to this?

@jensens
Copy link
Sponsor Member

jensens commented Nov 17, 2017

So, what is the problem with exposing?

Does exposing information about interfaces - and so about installed add-ons and other internals - give attackers hints about possible vulnerabilities outside of the core? But also FTI info and other endpoints expose that information.

I doubt it a problem, but I would love to hear opposing opinions.

@mauritsvanrees
Copy link
Sponsor Member

In PloneHotfix20161129 we removed the docstrings of lots of zope.interface methods and attributes to avoid 'publishing' them, that is making them available for browsers to call. See zopefoundation/Zope#81. In some cases they could really be used for potential harm (setting values), in others it was: better safe than sorry.

Just giving the name of interfaces should be fine. But it does give you info about an object that is useful for a UI but also for potential hackers.
So a way to whitelist some interfaces, or to completely disable this (return an empty list) for the security conscious would be good.

@jensens
Copy link
Sponsor Member

jensens commented Feb 28, 2020

What is the state here? Do we want it or not? If not and hence there was no activity for a long time, I propose to close this PR within next two weeks. If you do not feel OK with this, please speak up and provide us a rough schedule.

@tisto
Copy link
Sponsor Member

tisto commented Feb 28, 2020

@jensens it is something we definitely want. Though, there are lots of things to consider and to discuss. I'd prefer to keep it open. Maybe something that can be discussed at one of the next sprints this year...

@jensens
Copy link
Sponsor Member

jensens commented May 12, 2022

This one got forgotten. Is it still valid or meanwhile superseded?

@erral
Copy link
Sponsor Member Author

erral commented Jun 21, 2023

I am closing this because the main reason for this, the navigation root thing, has been fixed using another endpoint. If we need to expose the interfaces in the future we can use this code as an inspiration, but we do not need this right now.

@erral erral closed this Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants