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(src,include,test): fixed multiple cases where a bad yaml was accepted #1318

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Sep 12, 2024

Also, i added test cases.

The yaml parser did not catch some strings related yaml issues, like:

"foo","bar"

This is linked to the discussion here: falcosecurity/falco#3281

Note: i am not an expert therefore please let me know if i can do anything better (or if the fix makes no sense at all :D )

…pted.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Copy link
Owner

@jbeder jbeder left a comment

Choose a reason for hiding this comment

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

Looks good overall. One minor comment on the tests.

Thanks for the PR!

EXPECT_THROW(Load(R"(,"foo")"), ParserException);
EXPECT_THROW(Load(R"(,foo)"), ParserException);

const char *doc_ok = R"(
Copy link
Owner

Choose a reason for hiding this comment

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

Do you want to EXPECT_EQ what these actually parse to? (And maybe put them in the above "valid" test or their own one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

Plus, check that the content is what we actually expect.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@jbeder jbeder merged commit da82fd9 into jbeder:master Sep 13, 2024
33 checks passed
@FedeDP FedeDP deleted the fix/string_parsing_issues branch September 13, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants