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

Avoid public notifications about private resources #6

Closed

Conversation

michielbdejong
Copy link
Member

This is a first step of two for fixing #1. First, since currently WebSocket client connections are all unauthenticated, none of them should receive notifications about non-public resources.

In a follow-up PR, we should add code to check and store credentials of WebSocket clients, and then allow them to receive updates about non-public resources too, based on those credentials.

@kjetilk kjetilk added the triage Issues that need team review label Jan 21, 2019
@michielbdejong
Copy link
Member Author

@kjetilk bump

@@ -1,4 +1,4 @@
sudo: false
language: node_js
node_js:
- "0.12"
- "10"
Copy link

Choose a reason for hiding this comment

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

This should be done as a specific PR IMO, but apart from that this PR looks good ^_^

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I did it as three commits in one PR, but I can split it out as one PR per commit, no problem!

@kjetilk
Copy link
Member

kjetilk commented Feb 14, 2019

Sorry that I hadn't followed up on this. I have tried to read up on WebSockets but not grasped the security model. It seems like it supplies a token that could mitigate some attacks, and since I was unsure about what things this might break, I left it for now.

It seems very valid though.

Since there are many things that needs to be tested on NSS, I'm also a bit hesitant to release it right now, as we might not easily what breaks. OTOH, we shouldn't leave security problems open.

So, what should we do? Merge and release a beta?

@michielbdejong
Copy link
Member Author

I'm also unsure what this might break, so we should try to find that out. Solid-chat, for instance?

@michielbdejong
Copy link
Member Author

Just confirmed that the data browser's chat pane relies on the current behaviour to display new chat messages

Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

I'll just paste your comment here:

Just confirmed that the data browser's chat pane relies on the current behaviour to display new chat messages

just to make sure it isn't merged by accident :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Issues that need team review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants