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 Json string literal in resource property #80

Merged
merged 2 commits into from
Jun 4, 2019

Conversation

koiker
Copy link
Contributor

@koiker koiker commented May 30, 2019

Fix: This fix solves the issue with JSON payload in resource AWS::StepFunctions::StateMachine and property DefinitionString that has a JSON string that must remain a json string when converted to YAML.

Details:

When the script load a json file all key/values are converted to objects and those objects and later converted to yaml. This behavior made some payloads like DefinitionString to become Yaml but the property is a json and in a Yaml document this property must be converted to a string literal.
The way to solve this is to run a parser that will check for Key == AWS::StepFunctions::StateMachine and when found this resource we try to find the property DefinitionString. If found the property we convert the object into a string using json.dumps
and create a python object to be specifically parsed in the yaml representer to use the style |.
Here is where the things get interesting.
Just adding the style | to generate a literal didn't work as expected as some simple strings got converted correctly and others don't.

You can reproduce the behavior with this script:

import yaml


class Literal(str):
    pass


def representer_literal(dumper, value):
    return dumper.represent_scalar(u"tag:yaml.org,2002:str", value, style='|')


if __name__ == '__main__':
    var = Literal(u"{\n  line1\n  line2\n  line3\n}")
    yaml.add_representer(Literal, representer_literal)
    print(yaml.dump({'a': var}))
    # Output:
    # a: |-
    #   {
    #     line1
    #     line2
    #     line3
    #   }
    var = Literal(u"{\n \"texto\"   \n line1\n  line2\n  line3\n}")
    print(yaml.dump({'a': var}))
    # Output:
    # a: "{\n \"texto\"   \n line1\n  line2\n  line3\n}"

After analysing the problem I found that pyYaml in the Emitter step there is a string validation to check against leading and trailing whitespaces.
The code is here: https://github.com/yaml/pyyaml/blob/master/lib/yaml/emitter.py

The way to circumvent this characteristic is to override the method analyze_scalarfrom class Emitter and when we find a scalar that is a instance of our object type LiteralString we return the ScalarAnalisys with the parameters that we want (In this case to allow_block_plain=True

Other scalar types just get his normal flow using the super()

This solution allow an easy way to add more resource properties as the code has an array of tuples of (<Resource_Type>, <Property_Name>)


Fix: Updated the PyYaml requirement in setup.py to >= 4.1 to avoid remote execution vulnerability in CVE-2017-18342
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…pFunctions::StateMachine and attribute DefinitionString that has a JSON string that must remain a json string when converted to YAML.

Fix: Updated the PyYaml requirement in setup.py to >= 4.1 to avoid remote execution vulnerability in CVE-2017-18342
@stilvoid
Copy link
Member

stilvoid commented Jun 4, 2019

Hi @koiker, thanks for this! Happy with the changes but following merging your other PRs, there are now merge conflicts. If you fix those, I'll merge this :)

@codecov-io
Copy link

codecov-io commented Jun 4, 2019

Codecov Report

Merging #80 into master will decrease coverage by 0.23%.
The diff coverage is 97.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
- Coverage   99.31%   99.08%   -0.24%     
==========================================
  Files          10       11       +1     
  Lines         293      327      +34     
==========================================
+ Hits          291      324      +33     
- Misses          2        3       +1
Impacted Files Coverage Δ
cfn_clean/__init__.py 100% <100%> (ø) ⬆️
cfn_tools/literal.py 100% <100%> (ø)
cfn_flip/yaml_dumper.py 100% <100%> (ø) ⬆️
cfn_flip/__init__.py 100% <100%> (ø) ⬆️
cfn_tools/yaml_dumper.py 97.22% <92.3%> (-2.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3107ee2...23e7a8b. Read the comment docs.

@stilvoid
Copy link
Member

stilvoid commented Jun 4, 2019

Awesome :)

@stilvoid stilvoid merged commit ba70c1f into master Jun 4, 2019
@stilvoid stilvoid deleted the fix_json_string_literal branch June 4, 2019 15:54
chizou added a commit to chizou/troposphere that referenced this pull request Jul 2, 2019
This fixes an issue with cfn-flip that incorrectly translates
JSON for a step function as documented below:

awslabs/aws-cfn-template-flip#80
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.

3 participants