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

Add a possibility to manage own nodes on Nodes screen #541

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

bludnic
Copy link
Member

@bludnic bludnic commented Oct 12, 2023

I will try to make it work at least for Electron. Managing custom nodes in the web version is problematic due to the CSP (Content Security Policy).

The CSP rules are returned in the server response. It will block any request from the client that doesn't match *.adamant.im. So the user custom nodes will not work by default.

image

Checklist

  • Add UI components for Add/Edit a custom node
  • Integrate those components into NodesTable
  • Make the custom nodes persistent (page reload, tab close) using LocalStorage
  • Make it work in the Electron app

@vercel
Copy link

vercel bot commented Oct 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
adamant-im ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 27, 2023 5:32pm

@bludnic bludnic temporarily deployed to testing October 12, 2023 03:02 — with GitHub Actions Inactive
@bludnic bludnic changed the title Add a possibility to manage own nodes on Nodes screen (Electron) Add a possibility to manage own nodes on Nodes screen Oct 12, 2023
@github-actions
Copy link

github-actions bot commented Oct 12, 2023

Deployed to https://msg-adamant-pr-541.surge.sh 🚀

@bludnic
Copy link
Member Author

bludnic commented Oct 12, 2023

@bludnic bludnic self-assigned this Oct 12, 2023
@bludnic
Copy link
Member Author

bludnic commented Oct 12, 2023

One possible solution could be by generating CSP dynamically based on a user cookie. The cookie will contain an array of custom nodes.

Scenario:

  1. User adds a custom node on the Nodes screen
  2. A cookie will be set CUSTOM_NODES=http://custom-node1.com,http://custom-node2.com
  3. Refresh the page. It is required to send that cookie to the server (we can do it programmatically, or let the user do it by itself)
  4. The msg.adamant.im will read that cookie, and add custom domains to CSP in the response

@adamant-al
Copy link
Member

One possible solution could be by generating CSP dynamically based on a user cookie. The cookie will contain an array of custom nodes.

Scenario:

  1. User adds a custom node on the Nodes screen
  2. A cookie will be set CUSTOM_NODES=http://custom-node1.com,http://custom-node2.com
  3. Refresh the page. It is required to send that cookie to the server (we can do it programmatically, or let the user do it by itself)
  4. The msg.adamant.im will read that cookie, and add custom domains to CSP in the response

It seems complicated for me and looks not secure. Also, currently the app doesn't use cookies.

What do you think, @martiliones?

@martiliones
Copy link
Member

If the list of nodes is controlled by a user and affects only their own client, should we block fetch requests in the first place? There is no safe way to manage CSP because the client (user) defines which nodes are considered safe, not the server.

The only way to make request to URL that's not in the list is by executing 3rd party script, in which case the script could also edit CSP options.

@adamant-al
Copy link
Member

If the list of nodes is controlled by a user and affects only their own client,

Yes.

should we block fetch requests in the first place?

What do you mean?

There is no safe way to manage CSP because the client (user) defines which nodes are considered safe, not the server.
The only way to make request to URL that's not in the list is by executing 3rd party script, in which case the script could also edit CSP options.

That's why I don't like it.

@martiliones
Copy link
Member

should we block fetch requests in the first place?

What do you mean?

I think we can disable CSP for fetch requests but keep it enabled for content like images/scripts/iframe etc. As an alternative, we can make "Allow unknown nodes" option.

@adamant-al
Copy link
Member

I think we can disable CSP for fetch requests but keep it enabled for content like images/scripts/iframe etc. As an alternative, we can make "Allow unknown nodes" option.

Is it possible to "disable CSP for fetch requests"? Provide some details.

What is "Allow unknown nodes" option?

As I understand, CSP is generally static and set on the web server. We set it for msg.adamant.im.

@martiliones
Copy link
Member

Is it possible to "disable CSP for fetch requests"? Provide some details.

We can set connect-src * to allow requests to any url made by script:
image

loading image has been blocked by CSP while fetch request to node passed

What is "Allow unknown nodes" option?

A checkbox in the settings that user should mark to use 3rd party nodes, we can warn the user that they are responsible for trusting the node.

@adamant-al
Copy link
Member

We can set connect-src * to allow requests to any url made by script
loading image has been blocked by CSP while fetch request to node passed

Is it safe?

A checkbox in the settings that user should mark to use 3rd party nodes, we can warn the user that they are responsible for trusting the node.

Is it possible to allow CSP in runtime?

@bludnic @RealGoodProgrammer
What do you think?

@martiliones
Copy link
Member

martiliones commented Oct 13, 2023

Is it safe?

Unless we make requests to untrusted URLs ourselves, it should be safe

Is it possible to allow CSP in runtime?

I meant we can check it in JavaScript

What I think we really need to care about is this line where we are allowing to inject scripts and run code using eval():

script-src 'self' 'unsafe-inline' 'unsafe-eval' *.adamant.im;

@bludnic can we remove 'unsafe-inline' and 'unsafe-eval'?

@bludnic
Copy link
Member Author

bludnic commented Oct 14, 2023

@martiliones I guess not, we cant remove that rules.

'unsafe-eval' is required by @liskhq/lisk-validator
'unsafe-inline' make possible using onload when loading fonts

@bludnic bludnic temporarily deployed to testing October 27, 2023 17:31 — with GitHub Actions Inactive
@bludnic bludnic marked this pull request as draft November 2, 2023 00:10
@bludnic bludnic removed their assignment Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants