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

fix: don't allow . or .. in feature url #8479

Merged
Merged
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
6 changes: 5 additions & 1 deletion src/lib/routes/util.ts
Copy link
Member Author

Choose a reason for hiding this comment

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

this change applies to features, tags, projects, context fields and addons

Original file line number Diff line number Diff line change
@@ -13,7 +13,11 @@ export const customJoi = joi.extend((j) => ({
},
validate(value, helpers) {
// Base validation regardless of the rules applied
if (encodeURIComponent(value) !== value) {
if (
encodeURIComponent(value) !== value ||
value === '..' ||
value === '.'
Comment on lines +17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeh, this is good. Are there other cases we might have missed?

I checked the docs for this function, and there's also - _ ! ~ * ' ( ):

image

Do we want to allow them? I guess there's no harm in it because they don't break urls the same way. Sure, let's go.

) {
// Generate an error, state and options need to be passed
return { value, errors: helpers.error('isUrlFriendly.base') };
}
36 changes: 29 additions & 7 deletions src/lib/schema/feature-schema.test.ts
Original file line number Diff line number Diff line change
@@ -9,7 +9,29 @@ test('should require URL firendly name', () => {
};

const { error } = featureSchema.validate(toggle);
expect(error.details[0].message).toEqual('"name" must be URL friendly');
expect(error?.details[0].message).toEqual('"name" must be URL friendly');
});

test("shouldn't allow . nor .. as name", () => {
const toggle1 = {
name: '.',
enabled: false,
impressionData: false,
strategies: [{ name: 'default' }],
};
const toggle2 = {
name: '..',
enabled: false,
impressionData: false,
strategies: [{ name: 'default' }],
};

expect(featureSchema.validate(toggle1).error?.details[0].message).toEqual(
'"name" must be URL friendly',
);
expect(featureSchema.validate(toggle2).error?.details[0].message).toEqual(
'"name" must be URL friendly',
);
});

test('should be valid toggle name', () => {
@@ -91,7 +113,7 @@ test('should not allow weightType=fix with floats', () => {
};

const { error } = featureSchema.validate(toggle);
expect(error.details[0].message).toEqual('Weight only supports 1 decimal');
expect(error?.details[0].message).toEqual('Weight only supports 1 decimal');
});

test('should not allow weightType=fix with more than 1000', () => {
@@ -115,7 +137,7 @@ test('should not allow weightType=fix with more than 1000', () => {
};

const { error } = featureSchema.validate(toggle);
expect(error.details[0].message).toEqual(
expect(error?.details[0].message).toEqual(
'"variants[0].weight" must be less than or equal to 1000',
);
});
@@ -139,7 +161,7 @@ test('should disallow weightType=unknown', () => {
};

const { error } = featureSchema.validate(toggle);
expect(error.details[0].message).toEqual(
expect(error?.details[0].message).toEqual(
'"variants[0].weightType" must be one of [variable, fix]',
);
});
@@ -255,7 +277,7 @@ test('should not accept empty constraint values', () => {
};

const { error } = featureSchema.validate(toggle);
expect(error.details[0].message).toEqual(
expect(error?.details[0].message).toEqual(
'"strategies[0].constraints[0].values[0]" is not allowed to be empty',
);
});
@@ -300,7 +322,7 @@ test('Filter queries should reject tag values with missing type prefix', () => {
tag: ['simple', 'simple'],
};
const { error } = querySchema.validate(query);
expect(error.details[0].message).toEqual(
expect(error?.details[0].message).toEqual(
'"tag[0]" with value "simple" fails to match the tag pattern',
);
});
@@ -318,7 +340,7 @@ test('Filter queries should reject project names that are not alphanum', () => {
project: ['project name with space'],
};
const { error } = querySchema.validate(query);
expect(error.details[0].message).toEqual(
expect(error?.details[0].message).toEqual(
'"project[0]" must be URL friendly',
);
});