Skip to content

Commit 74e2935

Browse files
emizzleiurimatias
authored andcommitted
fix(@embark/profiler): Fix profile output and update messaging
The profiler was not formatted correctly in the console as `util.inspect` was being applied to the ASCII table before being output to the console REPL. In addition, functions containing solidity assertions (require, revert, assert) that cause the function to fail when estimating gas would print an error to embark’s console log, and would show nothing as their gas estimate in the table. Do not `util.inspect` command output if the result is a string. For API commands being run, allow the command to specify whether or not the output of the command should be HTML escaped. This could pose security risks! For functions that have errors during gas estimation, add a message in the embark console explaining that the error may be due to solidity assertions in the function that prevent the gas from being estimated correctly. For functions that error, show `-ERROR-` in the gas estimation column. Additionally, show a description in the table footer explaining that the error may be due to solidity assertions in the function. For events with no gas estimate, show `-EVENT-` in the gas estimate column of the profile table, and a description in the table footer explaining that there is no gas estimate for events. ### Warnings This PR allows the console command to specify whether or not it should allow for a string result of the command to be HTML-escaped before being sent in the API response. Combining this with Cockpit’s `dangerouslySetInnerHTML`, this could allow a plugin to register a console command that injects XSS in to Cockpit. ![Imgur](https://i.imgur.com/1Rqkjyx.png) ![Imgur](https://i.imgur.com/s6Y1Ecy.png) ![Imgur](https://i.imgur.com/BhsjkBs.png)
1 parent 1e9ed81 commit 74e2935

File tree

3 files changed

+35
-14
lines changed

3 files changed

+35
-14
lines changed

packages/core/console/src/index.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ export default class Console {
5050
const error = { name: "Console error", message: err, stack: err.stack };
5151
return cb(error);
5252
}
53-
cb(null, util.inspect(result));
53+
cb(null, typeof result !== "string" ? util.inspect(result) : result);
5454
});
5555
});
5656
this.ipc.on("console:executePartial", (cmd: string, cb: any) => {
@@ -93,18 +93,18 @@ export default class Console {
9393
private registerApi() {
9494
const plugin = this.plugins.createPlugin("consoleApi", {});
9595
plugin.registerAPICall("post", "/embark-api/command", (req: any, res: any) => {
96-
this.executeCmd(req.body.command, (err: any, result: any) => {
96+
this.executeCmd(req.body.command, (err: any, result: any, shouldEscapeHtml = true) => {
9797
if (err) {
9898
return res.send({ result: err.message || err });
9999
}
100100
let response = result;
101101
if (typeof result !== "string") {
102102
response = stringify(result, jsonFunctionReplacer, 2);
103-
} else {
103+
} else if (shouldEscapeHtml) {
104104
// Avoid HTML injection in the Cockpit
105105
response = escapeHtml(response);
106106
}
107-
const jsonResponse = {result: response};
107+
const jsonResponse = { result: response };
108108
if (res.headersSent) {
109109
return res.end(jsonResponse);
110110
}

packages/plugins/profiler/src/gasEstimator.js

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,25 @@ const async = require('async');
22
const ContractFuzzer = require('./fuzzer.js');
33
const Web3 = require('web3');
44

5-
class GasEstimator {
5+
export const GAS_ERROR = ' -ERROR- ';
6+
export const EVENT_NO_GAS = ' -EVENT- ';
7+
8+
export class GasEstimator {
69
constructor(embark) {
710
this.embark = embark;
811
this.logger = embark.logger;
912
this.events = embark.events;
1013
this.fuzzer = new ContractFuzzer(embark);
1114
}
1215

16+
printError(message, name, values = []) {
17+
this.logger.error(`Error getting gas estimate for "${name}(${Object.values(values).join(",")})"`, message);
18+
if (message.includes('always failing transaction')) {
19+
this.logger.error(`This may mean function assertions (revert, assert, require) are preventing the estimate from completing. Gas will be listed as "${GAS_ERROR}" in the profile.`);
20+
}
21+
this.logger.error(''); // new line to separate likely many lines
22+
}
23+
1324
estimateGas(contractName, cb) {
1425
const self = this;
1526
let gasMap = {};
@@ -19,13 +30,16 @@ class GasEstimator {
1930
if (err) return cb(err);
2031
let fuzzMap = self.fuzzer.generateFuzz(3, contract);
2132
let contractObj = new web3.eth.Contract(contract.abiDefinition, contract.deployedAddress);
22-
async.each(contract.abiDefinition.filter((x) => x.type !== "event"),
33+
async.each(contract.abiDefinition,
2334
(abiMethod, gasCb) => {
2435
let name = abiMethod.name;
2536
if (abiMethod.type === "constructor") {
2637
// already provided for us
2738
gasMap['constructor'] = parseFloat(contract.gasEstimates.creation.totalCost.toString());
2839
return gasCb(null, name, abiMethod.type);
40+
} else if (abiMethod.type === "event") {
41+
gasMap[name] = EVENT_NO_GAS;
42+
return gasCb(null, name, abiMethod.type);
2943
} else if (abiMethod.type === "fallback") {
3044
gasMap['fallback'] = parseFloat(contract.gasEstimates.external[""].toString());
3145
return gasCb(null, name, abiMethod.type);
@@ -35,9 +49,9 @@ class GasEstimator {
3549
// just run it and register it
3650
contractObj.methods[name]
3751
.apply(contractObj.methods[name], [])
38-
.estimateGas((err, gasAmount) => {
52+
.estimateGas({ from: web3.eth.defaultAccount }, (err, gasAmount) => {
3953
if (err) {
40-
self.logger.error(`Error getting gas estimate for "${name}"`, err.message || err);
54+
self.printError(err.message || err, name);
4155
return gasCb(null, name, abiMethod.type);
4256
}
4357
gasMap[name] = gasAmount;
@@ -49,13 +63,13 @@ class GasEstimator {
4963
contractObj.methods[name].apply(contractObj.methods[name], values)
5064
.estimateGas((err, gasAmount) => {
5165
if (err) {
52-
self.logger.error(`Error getting gas estimate for "${name}"`, err.message || err);
66+
self.printError(err.message || err, name, values);
5367
}
5468
getVarianceCb(null, gasAmount);
5569
});
5670
}, (err, variance) => {
5771
if (variance.every(v => v === variance[0])) {
58-
gasMap[name] = variance[0];
72+
gasMap[name] = variance[0] ?? GAS_ERROR;
5973
} else {
6074
// get average
6175
let sum = variance.reduce(function(memo, num) { return memo + num; });
@@ -77,5 +91,3 @@ class GasEstimator {
7791
});
7892
}
7993
}
80-
81-
module.exports = GasEstimator;

packages/plugins/profiler/src/index.js

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { warnIfPackageNotDefinedLocally } from 'embark-utils';
22

33
const asciiTable = require('ascii-table');
4-
const GasEstimator = require('./gasEstimator.js');
4+
import { GasEstimator, GAS_ERROR, EVENT_NO_GAS } from './gasEstimator';
55

66
class Profiler {
77
constructor(embark, _options) {
@@ -60,10 +60,19 @@ class Profiler {
6060

6161
let table = new asciiTable(contractName);
6262
table.setHeading('Function', 'Payable', 'Mutability', 'Inputs', 'Outputs', 'Gas Estimates');
63+
table.setAlign(5, asciiTable.RIGHT);
6364
profileObj.methods.forEach((method) => {
6465
table.addRow(method.name, method.payable, method.mutability, self.formatParams(method.inputs), self.formatParams(method.outputs), method.gasEstimates);
6566
});
66-
return returnCb(null, table.toString());
67+
const strTable = table.toString();
68+
let result = [strTable];
69+
if (strTable.includes(GAS_ERROR)) {
70+
result.push(`${GAS_ERROR} indicates there was an error during gas estimation (see console for details).`);
71+
}
72+
if (strTable.includes(EVENT_NO_GAS)) {
73+
result.push(`${EVENT_NO_GAS} indicates the method is an event, and therefore no gas was estimated.`);
74+
}
75+
return returnCb(null, result.join('\n'), false);
6776
});
6877
}
6978

0 commit comments

Comments
 (0)