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.

Sign up for GitHub

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 2 commits into from
Oct 21, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd7fbd1
fix: don't allow . or .. as flag/tag/context/project names
Tymek Oct 18, 2024
24789d2
test: no . nor .. in feature name
Tymek Oct 18, 2024
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
View file
Open in desktop
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
Expand Up @@ -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') };
}
Expand Down
36 changes: 29 additions & 7 deletions src/lib/schema/feature-schema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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',
);
});
Expand All @@ -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]',
);
});
Expand Down Expand Up @@ -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',
);
});
Expand Down Expand Up @@ -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',
);
});
Expand All @@ -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',
);
});
Expand Down