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

Don't interpolate provider description fields #1836

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Apr 3, 2024

Fixes #1776

@iwahbe iwahbe self-assigned this Apr 3, 2024
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 61.99%. Comparing base (844da26) to head (d94a326).

Files Patch % Lines
pkg/tfgen/docs.go 68.42% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1836      +/-   ##
==========================================
+ Coverage   60.75%   61.99%   +1.23%     
==========================================
  Files         303      309       +6     
  Lines       42375    35571    -6804     
==========================================
- Hits        25747    22051    -3696     
+ Misses      15154    12046    -3108     
  Partials     1474     1474              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iwahbe iwahbe force-pushed the iwahbe/1776/missing-in-docs-descriptions branch from 028b489 to 473ad93 Compare April 3, 2024 16:38
@iwahbe iwahbe force-pushed the iwahbe/1776/missing-in-docs-descriptions branch from 473ad93 to d7e1645 Compare April 3, 2024 16:41
This is a correctness correction, since untrusted (from the user) inputs may
contain interpolation keys (`%s`, `%#v`, etc) and thus cause `MISSING` to be
inserted into the output.

Fixes #1776
@iwahbe iwahbe requested review from guineveresaenger and a team April 3, 2024 16:53
@iwahbe iwahbe enabled auto-merge (rebase) April 3, 2024 16:53
Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

Approving to unblock.

It'd be clearer if you separated out the refactoring changes from your meaningful changes.
I don't understand which part is the actual fix since most of the changes look like you are propagating errors?

Have you verified that the existing tests catch the error and we won't regress here?

@iwahbe iwahbe merged commit 9257734 into master Apr 3, 2024
9 of 10 checks passed
@iwahbe iwahbe deleted the iwahbe/1776/missing-in-docs-descriptions branch April 3, 2024 17:12
@guineveresaenger
Copy link
Contributor

This LGTM but I would've like to see a test showing the (MISSING) behavior.

to answer the reviewer question: wrapping the input into %s every time prevents other raw upstream format strings from being written to the buffer.

Comment on lines -1480 to +1482
fprintf(output, docs[tfBlock.headerStart:tfBlock.start])
fprintf("%s", docs[tfBlock.headerStart:tfBlock.start])
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 is probably the actual fix.

@iwahbe
Copy link
Member Author

iwahbe commented Apr 3, 2024

It'd be clearer if you separated out the refactoring changes from your meaningful changes. I don't understand which part is the actual fix since most of the changes look like you are propagating errors?

I did a fmt.*f cleanliness check over the whole file. Anytime you call a function like fmt.Sprintf(someString, arg1, arg2) you are interpolating arg1 and arg2 into someString. If you know what someString is, then thats fine. If someString is partially user defined, then it's a problem. My expectation is the actual fix is here, but it could have been several places.

Have you verified that the existing tests catch the error and we won't regress here?

Yes. That d7e1645 fails CI (check) shows the area is under test.

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.

TFGen starts to generate (MISSING) into Description
3 participants