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

[Fleet] Don't add extra quotes to YAML strings from manifest files #93585

Merged
merged 9 commits into from
Mar 5, 2021
2 changes: 1 addition & 1 deletion api_docs/fleet.json
Original file line number Diff line number Diff line change
Expand Up @@ -14114,7 +14114,7 @@
"link": "https://github.com/elastic/kibana/tree/masterx-pack/plugins/fleet/common/types/models/epm.ts#L234"
},
"signature": [
"\"text\" | \"password\" | \"integer\" | \"bool\" | \"yaml\""
"\"string\" | \"text\" | \"password\" | \"integer\" | \"bool\" | \"yaml\""
],
"initialIsOpen": false
},
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/fleet/common/types/models/epm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ export interface RegistryElasticsearch {
'index_template.mappings'?: object;
}

export type RegistryVarType = 'integer' | 'bool' | 'password' | 'text' | 'yaml';
export type RegistryVarType = 'integer' | 'bool' | 'password' | 'text' | 'yaml' | 'string';
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I am late to the official review, but I am surprised at this change. AFAIK there is no var type called string on package spec side, only text? https://github.com/elastic/package-spec/blob/master/versions/1/data_stream/manifest.spec.yml#L50-L55

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Right now packages use both, e.g. apm here has a lot of strings: https://github.com/elastic/package-storage/blob/snapshot/packages/apm/0.1.0-dev.7/manifest.yml#L44-L136

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that seems inconsistent with the package spec, I also see type: int which is also inconsistent. I'll file an issue elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

// EPR types this as `[]map[string]interface{}`
// which means the official/possible type is Record<string, any>
// but we effectively only see this shape
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,37 @@ export const validatePackagePolicyConfig = (
})
);
}
if (
(varDef.type === 'text' || varDef.type === 'string') &&
parsedValue &&
Array.isArray(parsedValue)
) {
const invalidStrings = parsedValue.filter((cand) => /^[*&]/.test(cand));
// only show one error if multiple strings in array are invalid
if (invalidStrings.length > 0) {
errors.push(
i18n.translate('xpack.fleet.packagePolicyValidation.quoteStringErrorMessage', {
defaultMessage:
'Strings starting with special YAML characters like * or & need to be enclosed in double quotes.',
})
);
}
}
}

if (
(varDef.type === 'text' || varDef.type === 'string') &&
parsedValue &&
!Array.isArray(parsedValue)
) {
if (/^[*&]/.test(parsedValue)) {
errors.push(
i18n.translate('xpack.fleet.packagePolicyValidation.quoteStringErrorMessage', {
defaultMessage:
'Strings starting with special YAML characters like * or & need to be enclosed in double quotes.',
})
);
}
}

return errors.length ? errors : null;
Expand Down
73 changes: 19 additions & 54 deletions x-pack/plugins/fleet/server/services/epm/agent/agent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,71 +183,36 @@ input: logs
it('should escape string values when necessary', () => {
const stringTemplate = `
my-package:
opencurly: {{opencurly}}
closecurly: {{closecurly}}
opensquare: {{opensquare}}
closesquare: {{closesquare}}
ampersand: {{ampersand}}
asterisk: {{asterisk}}
question: {{question}}
pipe: {{pipe}}
hyphen: {{hyphen}}
openangle: {{openangle}}
closeangle: {{closeangle}}
equals: {{equals}}
exclamation: {{exclamation}}
percent: {{percent}}
at: {{at}}
colon: {{colon}}
asteriskOnly: {{asteriskOnly}}
startsWithAsterisk: {{startsWithAsterisk}}
numeric: {{numeric}}
mixed: {{mixed}}`;
mixed: {{mixed}}
concatenatedEnd: {{a}}{{b}}
concatenatedMiddle: {{c}}{{d}}
mixedMultiline: |-
{{{ search }}} | streamstats`;

// List of special chars that may lead to YAML parsing errors when not quoted.
// See YAML specification section 5.3 Indicator characters
// https://yaml.org/spec/1.2/spec.html#id2772075
// {,},[,],&,*,?,|,-,<,>,=,!,%,@,:
const vars = {
opencurly: { value: '{', type: 'string' },
closecurly: { value: '}', type: 'string' },
opensquare: { value: '[', type: 'string' },
closesquare: { value: ']', type: 'string' },
comma: { value: ',', type: 'string' },
ampersand: { value: '&', type: 'string' },
asterisk: { value: '*', type: 'string' },
question: { value: '?', type: 'string' },
pipe: { value: '|', type: 'string' },
hyphen: { value: '-', type: 'string' },
openangle: { value: '<', type: 'string' },
closeangle: { value: '>', type: 'string' },
equals: { value: '=', type: 'string' },
exclamation: { value: '!', type: 'string' },
percent: { value: '%', type: 'string' },
at: { value: '@', type: 'string' },
colon: { value: ':', type: 'string' },
asteriskOnly: { value: '"*"', type: 'string' },
startsWithAsterisk: { value: '"*lala"', type: 'string' },
numeric: { value: '100', type: 'string' },
mixed: { value: '1s', type: 'string' },
a: { value: '/opt/package/*', type: 'string' },
b: { value: '/logs/my.log*', type: 'string' },
c: { value: '/opt/*/package/', type: 'string' },
d: { value: 'logs/*my.log', type: 'string' },
search: { value: 'search sourcetype="access*"', type: 'text' },
};

const targetOutput = {
'my-package': {
opencurly: '{',
closecurly: '}',
opensquare: '[',
closesquare: ']',
ampersand: '&',
asterisk: '*',
question: '?',
pipe: '|',
hyphen: '-',
openangle: '<',
closeangle: '>',
equals: '=',
exclamation: '!',
percent: '%',
at: '@',
colon: ':',
asteriskOnly: '*',
startsWithAsterisk: '*lala',
numeric: '100',
mixed: '1s',
concatenatedEnd: '/opt/package/*/logs/my.log*',
concatenatedMiddle: '/opt/*/package/logs/*my.log',
mixedMultiline: 'search sourcetype="access*" | streamstats',
},
};

Expand Down
9 changes: 2 additions & 7 deletions x-pack/plugins/fleet/server/services/epm/agent/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,8 @@ function replaceVariablesInYaml(yamlVariables: { [k: string]: any }, yaml: any)
}

const maybeEscapeString = (value: string) => {
// List of special chars that may lead to YAML parsing errors when not quoted.
// See YAML specification section 5.3 Indicator characters
// https://yaml.org/spec/1.2/spec.html#id2772075
const yamlSpecialCharsRegex = /[{}\[\],&*?|\-<>=!%@:]/;

// In addition, numeric strings need to be quoted to stay strings.
if ((value.length && !isNaN(+value)) || yamlSpecialCharsRegex.test(value)) {
// Numeric strings need to be quoted to stay strings.
if (value.length && !isNaN(+value)) {
return `"${value}"`;
}
return value;
Expand Down