Skip to content

Commit

Permalink
Fix recordId issue in app builder and SOQL security hack
Browse files Browse the repository at this point in the history
recordId is undefined in the app builder. Replace it with fake id
soql query was escaped completely in apex and so made the recordId failed
replace quote in the js before replace :recordId to avoid exploit in the rendering :
$0.recordId = "0013N00000Gxf5aQAB' OR Id != null"
  • Loading branch information
scolladon committed Apr 30, 2020
1 parent 42c65fd commit 8322e9d
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 6 deletions.
3 changes: 2 additions & 1 deletion .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@
.sfdx
coverage/
.vscode
_site/
_site/
tests/
12 changes: 9 additions & 3 deletions force-app/main/default/classes/SOQLDataProvider.cls
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ public inherited sharing virtual class SOQLDataProvider extends ChartDataProvide
private static final String SOQL_LIMIT_STATEMENT = 'LIMIT';
private static final String LABEL_ALIAS = 'label';
private static final String VALUE_ALIAS = 'value';
private static final String UNDEFINED_RECORDID = '\'xxxxxxxxxxxxxxx\'';

public static final String QUERY_NULL_EXCEPTION = 'Query is null';
public static final String QUERY_WITHOUT_LABEL_EXCEPTION = 'Query must contains "label" alias';
Expand Down Expand Up @@ -64,9 +65,14 @@ public inherited sharing virtual class SOQLDataProvider extends ChartDataProvide
}

final List<ChartDataProvider.ChartData> chartDatas = new List<ChartDataProvider.ChartData>();
for (
AggregateResult aResult : Database.query(String.escapeSingleQuotes(query))
) {
System.debug(this.query);
// When building the chart in the app builder and using :recordId in the query
// The context is not set and :recordId is undefined
// In this case we can't get not data but it is still possible to build the chart in the App Builder
if (this.query.contains(UNDEFINED_RECORDID)) {
return chartDatas;
}
for (AggregateResult aResult : Database.query(this.query)) {
ChartDataProvider.ChartData aChartData = new ChartDataProvider.ChartData();
aChartData.label = (String) aResult.get(LABEL_ALIAS);
aChartData.detail = new List<Object>{ aResult.get(VALUE_ALIAS) };
Expand Down
14 changes: 14 additions & 0 deletions force-app/main/default/classes/SOQLDataProviderTest.cls
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,20 @@ private class SOQLDataProviderTest {
}
}

@isTest
static void testAppBuilderContext() {
Test.startTest();
final SOQLDataProvider aSOQLDataProvider = new SOQLDataProvider();
aSOQLDataProvider.init(
'SELECT StageName label, SUM(Amount) value FROM Opportunity WHERE IsClosed = false AND AccountId = ' +
SOQLDataProvider.UNDEFINED_RECORDID +
' WITH SECURITY_ENFORCED GROUP BY StageName LIMIT 10'
);
final List<ChartDataProvider.ChartData> chartDatas = aSOQLDataProvider.getData();
Test.stopTest();
System.assertEquals(0, chartDatas.size(), 'chartDatas must be empty');
}

@isTest
static void testGetData() {
insert new Opportunity(
Expand Down
17 changes: 15 additions & 2 deletions force-app/main/default/lwc/chartBuilder/chartBuilder.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ export default class ChartBuilder extends LightningElement {
}
}

// This is where the data are build and given to the chart.
// It is set either directly via the app builder
// or by the soql setter which call the imperative apex method
// or by the handler setter which call the imperative apex method
_details = [];
@api
get details() {
Expand All @@ -69,6 +73,7 @@ export default class ChartBuilder extends LightningElement {
this.error = false;
} catch (error) {
this.errorCallback(error);
this._details = null;
data = null;
}
return data;
Expand All @@ -86,7 +91,14 @@ export default class ChartBuilder extends LightningElement {
set soql(v) {
this._soql = v;
if (this._soql) {
this._soql = this._soql.replace(/:recordId/g, `'${this.recordId}'`);
this._soql = this._soql.replace(
/:recordId/g,
`'${
this.recordId
? this.recordId.replace("'", "\\'")
: ChartBuilder.FAKE_ID
}'`
);
this._getChartDataHandler(
ChartBuilder.SOQL_DATA_PROVIDER_APEX_TYPE,
this._soql
Expand Down Expand Up @@ -130,7 +142,7 @@ export default class ChartBuilder extends LightningElement {
this.details = result;
})
.catch(error => {
this.errorCallback(error);
this.errorCallback(error.body.message);
});
}

Expand Down Expand Up @@ -204,6 +216,7 @@ export default class ChartBuilder extends LightningElement {
]
};

static FAKE_ID = 'xxxxxxxxxxxxxxx';
static SOQL_DATA_PROVIDER_APEX_TYPE = 'SOQLDataProvider';
static DEFAULT_CSS_CLASS = 'slds-card slds-p-around_small';
}

0 comments on commit 8322e9d

Please sign in to comment.