Skip to content

Commit

Permalink
Validate install_origins (#4018)
Browse files Browse the repository at this point in the history 
* Validate install_origins if present

* Edge cases

* prettify

* stray unused variable

* TMP: inject install_origins definition from schema updates + tweak to test case

* Convert tests to it.each()

* lint

* Check protocol

* Bring isOrigin implementation closer to Firefox's

* lint

* Allow empty array

* Apply suggestions from code review

Co-authored-by: William Durand <[email protected]>

* Update src/schema/formats.js

Co-authored-by: William Durand <[email protected]>

Co-authored-by: Luca Greco <[email protected]>
Co-authored-by: William Durand <[email protected]>
  • Loading branch information
@diox @rpl @willdurand
3 people authored Nov 23, 2021
1 parent e2ab09d commit 93b1d7e
Show file tree
Hide file tree
Showing 6 changed files with 253 additions and 1 deletion.
22 changes: 22 additions & 0 deletions src/schema/formats.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,28 @@ export function isSecureUrl(value) {
return ['https:', 'wss:'].includes(url.protocol);
}

export function isOrigin(value) {
let url;
try {
url = new URL(value);
} catch {
return false;
}
if (!/^https?:/.test(url.protocol)) {
return false;
}
if (value.includes('*')) {
return false;
}
// url.origin is punycode so a direct check against string won't work.
// url.href appends a slash even if not in the original string, so we
// additionally check that the value does not end with slash.
if (value.endsWith('/') || url.href !== new URL(url.origin).href) {
return false;
}
return true;
}

export function imageDataOrStrictRelativeUrl(value) {
// Do not accept a string which resolves as an absolute URL, or any
// protocol-relative URL, except PNG or JPG data URLs.
Expand Down
8 changes: 8 additions & 0 deletions src/schema/imported/manifest.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@
"pattern": "^__MSG_.*?__$"
}
]
},
"install_origins": {
"type": "array",
"maxItems": 5,
"items": {
"type": "string",
"format": "origin"
}
}
},
"required": [
Expand Down
8 changes: 8 additions & 0 deletions src/schema/updates/manifest.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@
},
"manifest_version": {
"maximum": 2
},
"install_origins": {
"type": "array",
"maxItems": 5,
"items": {
"type": "string",
"format": "origin"
}
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/schema/validator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
imageDataOrStrictRelativeUrl,
isAnyUrl,
isAbsoluteUrl,
isOrigin,
isStrictRelativeUrl,
isSecureUrl,
isUnresolvedRelativeUrl,
Expand Down Expand Up @@ -416,6 +417,7 @@ export class SchemaValidator {
validator.addFormat('strictRelativeUrl', isStrictRelativeUrl);
validator.addFormat('unresolvedRelativeUrl', isUnresolvedRelativeUrl);
validator.addFormat('secureUrl', isSecureUrl);
validator.addFormat('origin', isOrigin);

validator.addFormat(
'imageDataOrStrictRelativeUrl',
Expand Down
79 changes: 79 additions & 0 deletions tests/unit/parsers/test.manifestjson.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3664,4 +3664,83 @@ describe('ManifestJSONParser', () => {
);
});
});

describe('install_origins', () => {
it.each([
{
origins: ['https://example.com/testing'],
expectedError: 'JSON_INVALID',
}, // Extra path
{ origins: ['file:/foo/bar'], expectedError: 'JSON_INVALID' }, // Disallowed scheme
{ origins: [' '], expectedError: 'JSON_INVALID' },
{ origins: ['https://foo.bar.栃木.jp/'], expectedError: 'JSON_INVALID' }, // Trailing slash
{ origins: [''], expectedError: 'JSON_INVALID' },
{ origins: [[]], expectedError: 'MANIFEST_FIELD_INVALID' },
{ origins: [{}], expectedError: 'MANIFEST_FIELD_INVALID' },
{ origins: {}, expectedError: 'MANIFEST_FIELD_INVALID' },
{ origins: null, expectedError: 'MANIFEST_FIELD_INVALID' },
{ origins: 42, expectedError: 'MANIFEST_FIELD_INVALID' },
{ origins: '', expectedError: 'MANIFEST_FIELD_INVALID' },
{
origins: new Array(6).fill('https://example.com'),
expectedError: 'JSON_INVALID',
}, // Too large
{ origins: ['https://*.example.com'], expectedError: 'JSON_INVALID' }, // Wildcard
{ origins: ['example.com'], expectedError: 'JSON_INVALID' }, // No scheme
{ origins: ['https://'], expectedError: 'JSON_INVALID' }, // No hostname
{
origins: ['https://foo.com', 'https://bar.com/path'],
expectedError: 'JSON_INVALID',
}, // One valid, one invalid
{
origins: ['https://foo.com', null],
expectedError: 'MANIFEST_FIELD_INVALID',
}, // One valid, one invalid
])(
'should disallow invalid install origins "$origins"',
({ origins, expectedError }) => {
const addonLinter = new Linter({ _: ['bar'] });
const json = validManifestJSON({
install_origins: origins,
browser_specific_settings: {
gecko: {
id: 'foo@bar',
},
},
});
const manifestJSONParser = new ManifestJSONParser(
json,
addonLinter.collector
);
expect(manifestJSONParser.isValid).toEqual(false);
expect(addonLinter.collector.errors.length).toEqual(1);
expect(addonLinter.collector.errors[0].code).toEqual(expectedError);
}
);

it.each([
{ origins: ['https://example.com'] },
{ origins: ['https://foo.example.com'] },
{ origins: ['https://xn--fo-9ja.com'] }, // IDNs are accepted in punycode (ascii)...
{ origins: ['https://foo.bar.栃木.jp'] }, // ... or unicode.
{ origins: ['https://example.com:8888'] },
{ origins: ['https://foo.example.com', 'https://foo.bar.栃木.jp:9999'] },
{ origins: [] }, // Empty array is valid
])('should allow valid install origins "$origins"', ({ origins }) => {
const addonLinter = new Linter({ _: ['bar'] });
const json = validManifestJSON({
install_origins: origins,
browser_specific_settings: {
gecko: {
id: 'foo@bar',
},
},
});
const manifestJSONParser = new ManifestJSONParser(
json,
addonLinter.collector
);
expect(manifestJSONParser.isValid).toEqual(true);
});
});
});
135 changes: 134 additions & 1 deletion tests/unit/schema/test.formats.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
imageDataOrStrictRelativeUrl,
isAbsoluteUrl,
isAnyUrl,
isOrigin,
isSecureUrl,
isStrictRelativeUrl,
isValidVersionString,
Expand Down Expand Up @@ -123,18 +124,68 @@ describe('formats', () => {
expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);
expect(isOrigin(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
});

it('trailing slash', () => {
const value = 'https://example.com/';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('full URL', () => {
const value = 'https://example.com/something?like=true&this=that';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('domain with no path but with query string', () => {
const value = 'https://example.com?like=true&this=that';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('domain with no path but with hash', () => {
const value = 'https://example.com#fool';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('domain only but in mixed case', () => {
const value = 'https://ExAmPle.coM';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);
expect(isOrigin(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
});
Expand All @@ -147,6 +198,44 @@ describe('formats', () => {
expect(isSecureUrl(value)).toEqual(false);
expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('wildcard', () => {
const value = 'https://*.bar.com';

// FIXME: should these be true ?
expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('punycode', () => {
const value = 'https://xn--fo-9ja.com';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);
expect(isOrigin(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
});

it('IDN', () => {
const value = 'https://foo.bar.栃木.jp';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isSecureUrl(value)).toEqual(true);
expect(isOrigin(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
});

it('path only', () => {
Expand All @@ -158,6 +247,7 @@ describe('formats', () => {

expect(isAbsoluteUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('protocol relative', () => {
Expand All @@ -169,9 +259,10 @@ describe('formats', () => {
expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('ftp: scheme', () => {
it('ftp: scheme with a path', () => {
const value = 'ftp://example.org/favicon.ico';

expect(isAnyUrl(value)).toEqual(true);
Expand All @@ -180,6 +271,31 @@ describe('formats', () => {
expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('ftp: scheme', () => {
const value = 'ftp://example.org';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('file: scheme without path', () => {
const value = 'file://';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('http: scheme', () => {
Expand All @@ -188,6 +304,19 @@ describe('formats', () => {
expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false); // Because it contains path
});

it('http: scheme without path', () => {
const value = 'http://example.net';

expect(isAnyUrl(value)).toEqual(true);
expect(isAbsoluteUrl(value)).toEqual(true);
expect(isOrigin(value)).toEqual(true);

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
Expand All @@ -204,6 +333,7 @@ describe('formats', () => {
expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('image data PNG', () => {
Expand All @@ -215,6 +345,7 @@ describe('formats', () => {

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('image data JPG', () => {
Expand All @@ -226,6 +357,7 @@ describe('formats', () => {

expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});

it('image data SVG', () => {
Expand All @@ -238,6 +370,7 @@ describe('formats', () => {
expect(isStrictRelativeUrl(value)).toEqual(false);
expect(isSecureUrl(value)).toEqual(false);
expect(imageDataOrStrictRelativeUrl(value)).toEqual(false);
expect(isOrigin(value)).toEqual(false);
});
});
});

0 comments on commit 93b1d7e

Please sign in to comment.