Skip to content
This repository has been archived by the owner on Feb 26, 2020. It is now read-only.

Restrict in-frame navigation for dapps #146

Merged
merged 2 commits into from
Jul 3, 2018

Conversation

axelchalon
Copy link
Contributor

Closes #118

  • Prevent the dapp from navigating to resources outside the dapp in the same frame
  • Enforce ?appId when navigating to other files of the dapp (allows multi-page dapps to use the api; prevent appId spoofing)

@axelchalon axelchalon added the A0-pleasereview Pull request needs code review. label Jun 25, 2018
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

After the network dapps PR is merged, wouldn't it be enough to:

  • shell.openExternal if it starts with http
  • else append appId (if not existent)?

@@ -18,6 +18,7 @@ const electron = require('electron');
const fs = require('fs');
const path = require('path');
const url = require('url');
const { URL, URLSearchParams } = url;
Copy link
Contributor

Choose a reason for hiding this comment

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

const { format, URL, URLSearchParams} = require('url'), or put this line under all the requires.

The linter doesn't yell here, but I think it's one of the rules to first import then const ..., so let's apply it here too.

@axelchalon
Copy link
Contributor Author

After the network dapps PR is merged, wouldn't it be enough to:

* shell.openExternal if it starts with http
* else append appId (if not existent)?

The dapp could then display files from the filesystem in the same frame (incl. other dapps)

This could be an attack vector: if good dapp was compromised and redirects the user to the filesystem location of a malicious network dapp (already fetched), then electron would pass the appId of good dapp to the malicious dapp, and malicious dapp would have the permissions of good dapp

@amaury1093 amaury1093 added A8-looksgood Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Jul 3, 2018
@amaury1093 amaury1093 merged commit 68fda6c into master Jul 3, 2018
@amaury1093 amaury1093 deleted the ac-control-dapp-navigation branch July 3, 2018 16:55
axelchalon added a commit that referenced this pull request Jul 3, 2018
* Restrict in-frame navigation for dapps

* Lint
amaury1093 pushed a commit that referenced this pull request Jul 9, 2018
* Download network (registry) dapps and load them from filesystem

* Remove obsolete Rust files

* package-lock.json

* Linting

* Fix tests

* Lint

* Fix .includes() is not a function (:88)

* Remove rust files from CI build

* Update @parity/ui dependency

* Grumbles

* Restrict in-frame navigation for dapps (#146)

* Restrict in-frame navigation for dapps

* Lint

* Grumbles

* fs-extra pathExists doesn't throw if the path doesn't exist

* Fix bug uglifyjs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants