Configuration parsing: validate section headers with quotes#5073
Merged
Configuration parsing: validate section headers with quotes#5073
Conversation
pks-t
approved these changes
May 22, 2019
Member
pks-t
left a comment
There was a problem hiding this comment.
Makes sense to me! One remark, but happy to merge either way as it's not caused by your changes.
| if (git__isspace(c)){ | ||
| name[name_length] = '\0'; | ||
| result = parse_section_header_ext(reader, line, name, section_out); | ||
| result = parse_section_header_ext(reader, line, pos, name, section_out); |
Member
There was a problem hiding this comment.
I really don't like this method name. It's not an extended version of the normal parse_section_header, but instead it parses the subsection-part of section headers (e.g. the quoted part). So I think it should just be called parse_subsection_header or something like that.
Update the configuration parsing error messages to be lower-cased for consistency with the rest of the library.
When we don't specify a particular column, don't write it in the error message. (column "0" is unhelpful.)
When we reach a whitespace after a section name, we assume that what will follow will be a quoted subsection name. Pass the current position of the line being parsed to the subsection parser, so that it can validate that subsequent characters are additional whitespace or a single quote. Previously we would begin parsing after the section name, looking for the first quotation mark. This allows invalid characters to embed themselves between the end of the section name and the first quotation mark, eg `[section foo "subsection"]`, which is illegal.
The `parse_section_header_ext` name suggests that it as an extended function for parsing the section header. It is not. Rename it to `parse_subsection_header` to better reflect its true mission.
3003ac8 to
355b02a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When parsing configuration section headers, we previously stopped at the first space, and skipped to the first quotation mark. This allows us to support valid section headers like
[foo "bar"]or[foo "bar"].However, by skipping from the end of the section name to the first quotation mark, we ignore any invalid characters there. For example,
[foo skipped "bar"]which is not legal.Validate that there is only whitespace between the end of the section name and the first quotation mark. This also adds additional tests that were useful for my understanding and should help prevent regression.
Fixes #5072