Skip to content

Validate performance metrics and attributes before storing them #1917

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

Merged
merged 9 commits into from
Aug 20, 2019
23 changes: 22 additions & 1 deletion packages/performance/src/resources/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,14 @@ describe('Firebase Performance > trace', () => {

expect(trace.getMetric('cacheHits')).to.eql(600);
});

it('throws error if metric doesnt exist and has invalid name', () => {
expect(() => trace.incrementMetric('_invalidMetric', 1)).to.throw();
});
});

describe('#putMetric', () => {
it('creates new metric if one doesnt exist.', () => {
it('creates new metric if one doesnt exist and has valid name.', () => {
trace.putMetric('cacheHits', 200);

expect(trace.getMetric('cacheHits')).to.eql(200);
Expand All @@ -131,6 +135,10 @@ describe('Firebase Performance > trace', () => {

expect(trace.getMetric('cacheHits')).to.eql(400);
});

it('throws error if metric doesnt exist and has invalid name', () => {
expect(() => trace.putMetric('_invalidMetric', 1)).to.throw();
});
});

describe('#getMetric', () => {
Expand Down Expand Up @@ -172,6 +180,19 @@ describe('Firebase Performance > trace', () => {

expect(trace.getAttributes()).to.eql({ level: '7' });
});

it('throws error if attribute name is invalid', () => {
expect(() => trace.putAttribute('_invalidAttribute', '1')).to.throw();
});

it('throws error if attribute value is invalid', () => {
const longAttributeValue =
'too-long-attribute-value-over-one-hundred-characters-too-long-attribute-value-over-one-' +
'hundred-charac';
expect(() =>
trace.putAttribute('validName', longAttributeValue)
).to.throw();
});
});

describe('#getAttribute', () => {
Expand Down
33 changes: 30 additions & 3 deletions packages/performance/src/resources/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ import {
import { Api } from '../services/api_service';
import { logTrace } from '../services/perf_logger';
import { ERROR_FACTORY, ErrorCode } from '../utils/errors';
import {
isValidCustomAttributeName,
isValidCustomAttributeValue
} from '../utils/attributes_utils';
import { isValidCustomMetricName } from '../utils/metric_utils';
import { PerformanceTrace } from '@firebase/performance-types';

const enum TraceState {
Expand Down Expand Up @@ -146,7 +151,7 @@ export class Trace implements PerformanceTrace {
*/
incrementMetric(counter: string, num = 1): void {
if (this.counters[counter] === undefined) {
this.counters[counter] = 0;
this.putMetric(counter, 0);
}
this.counters[counter] += num;
}
Expand All @@ -158,7 +163,13 @@ export class Trace implements PerformanceTrace {
* @param num Set custom metric to this value
*/
putMetric(counter: string, num: number): void {
this.counters[counter] = num;
if (isValidCustomMetricName(counter)) {
this.counters[counter] = num;
} else {
throw ERROR_FACTORY.create(ErrorCode.INVALID_CUSTOM_METRIC_NAME, {
customMetricName: counter
});
}
}

/**
Expand All @@ -176,7 +187,23 @@ export class Trace implements PerformanceTrace {
* @param value
*/
putAttribute(attr: string, value: string): void {
this.customAttributes[attr] = value;
const isValidName = isValidCustomAttributeName(attr);
const isValidValue = isValidCustomAttributeValue(value);
if (isValidName && isValidValue) {
this.customAttributes[attr] = value;
return;
}
// Throw appropriate error when the attribute name or value is invalid.
if (!isValidName) {
throw ERROR_FACTORY.create(ErrorCode.INVALID_ATTRIBUTE_NAME, {
attributeName: attr
});
}
if (!isValidValue) {
throw ERROR_FACTORY.create(ErrorCode.INVALID_ATTRIBUTE_VALUE, {
attributeValue: value
});
}
}

/**
Expand Down
51 changes: 50 additions & 1 deletion packages/performance/src/utils/attribute_utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ import {
getVisibilityState,
VisibilityState,
getServiceWorkerStatus,
getEffectiveConnectionType
getEffectiveConnectionType,
isValidCustomAttributeName,
isValidCustomAttributeValue
} from './attributes_utils';

import '../../test/setup';
Expand Down Expand Up @@ -172,4 +174,51 @@ describe('Firebase Performance > attribute_utils', () => {
expect(getEffectiveConnectionType()).to.be.eql(0);
});
});

describe('#isValidCustomAttributeName', () => {
it('returns true when name is valid', () => {
expect(isValidCustomAttributeName('validCustom_Attribute_Name')).to.be
.true;
});

it('returns false when name is blank', () => {
expect(isValidCustomAttributeName('')).to.be.false;
});

it('returns false when name is too long', () => {
expect(
isValidCustomAttributeName('invalid_custom_name_over_forty_characters')
).to.be.false;
});

it('returns false when name starts with a reserved prefix', () => {
expect(isValidCustomAttributeName('firebase_invalidCustomName')).to.be
.false;
});

it('returns false when name does not begin with a letter', () => {
expect(isValidCustomAttributeName('_invalidCustomName')).to.be.false;
});

it('returns false when name contains prohibited characters', () => {
expect(isValidCustomAttributeName('invalidCustomName&')).to.be.false;
});
});

describe('#isValidCustomAttributeValue', () => {
it('returns true when value is valid', () => {
expect(isValidCustomAttributeValue('valid_attribute_value')).to.be.true;
});

it('returns false when value is blank', () => {
expect(isValidCustomAttributeValue('')).to.be.false;
});

it('returns false when value is too long', () => {
const longAttributeValue =
'too_long_attribute_value_over_one_hundred_characters_too_long_attribute_value_over_one_' +
'hundred_charac';
expect(isValidCustomAttributeValue(longAttributeValue)).to.be.false;
});
});
});
19 changes: 19 additions & 0 deletions packages/performance/src/utils/attributes_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ const enum EffectiveConnectionType {
CONNECTION_4G = 4
}

const RESERVED_ATTRIBUTE_PREFIXES = ['firebase_', 'google_', 'ga_'];
const ATTRIBUTE_FORMAT_REGEX = new RegExp('^[a-zA-Z]\\w*$');
const MAX_ATTRIBUTE_NAME_LENGTH = 40;
const MAX_ATTRIBUTE_VALUE_LENGTH = 100;

export function getServiceWorkerStatus(): ServiceWorkerStatus {
const navigator = Api.getInstance().navigator;
if ('serviceWorker' in navigator) {
Expand Down Expand Up @@ -88,3 +93,17 @@ export function getEffectiveConnectionType(): EffectiveConnectionType {
return EffectiveConnectionType.UNKNOWN;
}
}

export function isValidCustomAttributeName(name: string): boolean {
if (name.length === 0 || name.length > MAX_ATTRIBUTE_NAME_LENGTH) {
return false;
}
const matchesReservedPrefix = RESERVED_ATTRIBUTE_PREFIXES.some(prefix =>
name.startsWith(prefix)
);
return !matchesReservedPrefix && !!name.match(ATTRIBUTE_FORMAT_REGEX);
}

export function isValidCustomAttributeValue(value: string): boolean {
return value.length !== 0 && value.length <= MAX_ATTRIBUTE_VALUE_LENGTH;
}
16 changes: 14 additions & 2 deletions packages/performance/src/utils/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ export const enum ErrorCode {
NO_API_KEY = 'no api key',
INVALID_CC_LOG = 'invalid cc log',
FB_NOT_DEFAULT = 'FB not default',
RC_NOT_OK = 'RC response not ok'
RC_NOT_OK = 'RC response not ok',
INVALID_ATTRIBUTE_NAME = 'invalid attribute name',
INVALID_ATTRIBUTE_VALUE = 'invalid attribute value',
INVALID_CUSTOM_METRIC_NAME = 'invalide custom metric name'
}

const ERROR_DESCRIPTION_MAP: { readonly [key in ErrorCode]: string } = {
Expand All @@ -40,12 +43,21 @@ const ERROR_DESCRIPTION_MAP: { readonly [key in ErrorCode]: string } = {
[ErrorCode.INVALID_CC_LOG]: 'Attempted to queue invalid cc event',
[ErrorCode.FB_NOT_DEFAULT]:
'Performance can only start when Firebase app instance is the default one.',
[ErrorCode.RC_NOT_OK]: 'RC response is not ok'
[ErrorCode.RC_NOT_OK]: 'RC response is not ok',
[ErrorCode.INVALID_ATTRIBUTE_NAME]:
'Attribute name {$attributeName} is invalid.',
[ErrorCode.INVALID_ATTRIBUTE_VALUE]:
'Attribute value {$attributeValue} is invalid.',
[ErrorCode.INVALID_CUSTOM_METRIC_NAME]:
'Custom metric name {$customMetricName} is invalid'
};

interface ErrorParams {
[ErrorCode.TRACE_STARTED_BEFORE]: { traceName: string };
[ErrorCode.TRACE_STOPPED_BEFORE]: { traceName: string };
[ErrorCode.INVALID_ATTRIBUTE_NAME]: { attributeName: string };
[ErrorCode.INVALID_ATTRIBUTE_VALUE]: { attributeValue: string };
[ErrorCode.INVALID_CUSTOM_METRIC_NAME]: { customMetricName: string };
}

export const ERROR_FACTORY = new ErrorFactory<ErrorCode, ErrorParams>(
Expand Down
45 changes: 45 additions & 0 deletions packages/performance/src/utils/metric_utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/**
* @license
* Copyright 2019 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF unknown KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { expect } from 'chai';

import { isValidCustomMetricName } from './metric_utils';

import '../../test/setup';

describe('Firebase Performance > metric_utils', () => {
describe('#isValidCustomMetricName', () => {
it('returns true when name is valid', () => {
expect(isValidCustomMetricName('validCustom_Metric_Name')).to.be.true;
});

it('returns false when name is blank', () => {
expect(isValidCustomMetricName('')).to.be.false;
});

it('returns false when name is too long', () => {
const longMetricName =
'too_long_metric_name_over_one_hundred_characters_too_long_metric_name_over_one_' +
'hundred_characters_too';
expect(isValidCustomMetricName(longMetricName)).to.be.false;
});

it('returns false when name starts with a reserved prefix', () => {
expect(isValidCustomMetricName('_invalidMetricName')).to.be.false;
Copy link

Choose a reason for hiding this comment

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

@kim-f Hello, I have noticed that i'm getting an uncaught exception for _fp as defined in

export const FIRST_PAINT_COUNTER_NAME = '_fp';

can this be updated to ignore the already defined constants?

});
});
});
26 changes: 26 additions & 0 deletions packages/performance/src/utils/metric_utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* @license
* Copyright 2019 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

const MAX_METRIC_NAME_LENGTH = 100;
const RESERVED_AUTO_PREFIX = '_';

export function isValidCustomMetricName(name: string): boolean {
if (name.length === 0 || name.length > MAX_METRIC_NAME_LENGTH) {
return false;
}
return !name.startsWith(RESERVED_AUTO_PREFIX);
}
1 change: 1 addition & 0 deletions packages/polyfill/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import 'promise-polyfill/lib/polyfill';
import 'core-js/features/array/find';
import 'core-js/features/array/find-index';
import 'core-js/features/array/from';
import 'core-js/features/array/some';
import 'core-js/features/object/assign';
import 'core-js/features/object/entries';
import 'core-js/features/object/values';
Expand Down