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

Improve error for Keyguard iframe with unexpected src #397

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
stages:
- build
- test
- docker
- deploy

# include private configs for the remaining stages
include:
- project: "it/ci-config"
file: "/testnet/deploy_webapp.yml"

variables:
# nginx configuration for this webapp
NGINX_CONFIG: |
server {
listen 80;
listen [::]:80;
server_name localhost;

location / {
root /usr/share/nginx/html;
index index.html index.htm;
try_files $$uri $$uri/ =404;
}
}

build:
stage: build
image: node:lts
artifacts:
paths:
- dist/
cache:
key: node-cache
paths:
- node_modules/
script:
- yarn install
- yarn build
# Make sure the dist directory exists, so that _this_ job fails and not the next one
- test -d dist || (echo "No dist directory\!" && exit 1)
- test -f dist/index.html || (echo "No files in dist\!" && exit 1)
allow_failure: false

test:
stage: test
image: node:lts
cache:
key: node-cache
paths:
- node_modules/
script:
- yarn lint
allow_failure: true

57 changes: 32 additions & 25 deletions client/src/RequestBehavior.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,46 +64,53 @@ export class RedirectRequestBehavior extends RequestBehavior {
export class IFrameRequestBehavior extends RequestBehavior {
private static IFRAME_PATH_SUFFIX = '/request/iframe/';

private _iframe: HTMLIFrameElement | null;
private _client: PostMessageRpcClient | null;
private _iframeEndpoint: string | null = null;
private _iframePromise: Promise<HTMLIFrameElement> | null = null;
private _clientPromise: Promise<PostMessageRpcClient> | null = null;

constructor() {
super(BehaviorType.IFRAME);
this._iframe = null;
this._client = null;
}

public async request(endpoint: string, command: KeyguardCommand, args: any[]): Promise<any> {
if (this._iframe && this._iframe.src !== `${endpoint}${IFrameRequestBehavior.IFRAME_PATH_SUFFIX}`) {
throw new Error('Keyguard iframe is already opened with another endpoint');
}

const origin = RequestBehavior.getAllowedOrigin(endpoint);

if (!this._iframe) {
this._iframe = await this.createIFrame(endpoint);
}
if (!this._iframe.contentWindow) {
throw new Error(`IFrame contentWindow is ${typeof this._iframe.contentWindow}`);
}

if (!this._client) {
this._client = new PostMessageRpcClient(this._iframe.contentWindow, origin);
await this._client.init();
}

return await this._client.call(command, ...args);
const client = await this._getClient(endpoint);
return client.call(command, ...args);
}

public async createIFrame(endpoint: string): Promise<HTMLIFrameElement> {
return new Promise((resolve, reject) => {
if (this._iframeEndpoint && this._iframeEndpoint !== endpoint) {
throw new Error('Keyguard iframe is already opened with another endpoint' +
`(opened: ${this._iframeEndpoint}, expected: ${endpoint})`);
}
this._iframeEndpoint = endpoint;

this._iframePromise = this._iframePromise || new Promise((resolve, reject) => {
const $iframe = document.createElement('iframe');
$iframe.name = 'NimiqKeyguardIFrame';
$iframe.style.display = 'none';
document.body.appendChild($iframe);
$iframe.src = `${endpoint}${IFrameRequestBehavior.IFRAME_PATH_SUFFIX}`;
$iframe.onload = () => resolve($iframe);
$iframe.onerror = reject;
Copy link
Member

Choose a reason for hiding this comment

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

In reject case clear the _iframePromise to avoid that all subsequent createIframe calls always fail.

Comment on lines 93 to 94
Copy link
Member

Choose a reason for hiding this comment

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

[Optional]
Might also want to clean up onload and onerror when not needed anymore but makes code more verbose and shouldn't have a real impact here.

}) as Promise<HTMLIFrameElement>;
});

return this._iframePromise;
}

private _getClient(endpoint: string): Promise<PostMessageRpcClient> {
this._clientPromise = this._clientPromise || new Promise(async (resolve) => {
const iframe = await this.createIFrame(endpoint);
if (!iframe.contentWindow) {
throw new Error(`IFrame contentWindow is ${typeof iframe.contentWindow}`);
}

const origin = RequestBehavior.getAllowedOrigin(endpoint);
const client = new PostMessageRpcClient(iframe.contentWindow, origin);
await client.init();

resolve(client);
Comment on lines +102 to +111
Copy link
Member

Choose a reason for hiding this comment

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

Wrap in try ... catch and reject in error case.
Or easier: replace the Promise construction by a self invoking async function (that automatically returns a promise).

On error also clear the _clientPromise.

});

return this._clientPromise;
}
}