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

add json template #74

Closed
wants to merge 2 commits into from
Closed

add json template #74

wants to merge 2 commits into from

Conversation

cmorell
Copy link

@cmorell cmorell commented Feb 21, 2020

Adding json template
now, PR straight to master

@cmorell cmorell requested a review from izar as a code owner February 21, 2020 18:52
@ghost
Copy link

ghost commented Feb 21, 2020

Congratulations 🍻. DeepCode analyzed your code in 0.221 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

@izar
Copy link
Collaborator

izar commented Feb 21, 2020

It would be really preferable not to introduce format-dependent code into the template engine. We are aiming to have it as simple as possible. Can't JSON be correctly expressed by using just the template?

@cmorell
Copy link
Author

cmorell commented Feb 21, 2020

Hi,

I understand your point, but I found problems with right/left curly brace with the current template_engine.
Also with the last comma to separate all the potential threats inside the list of findings

@cmorell
Copy link
Author

cmorell commented Feb 21, 2020

I mean, I've tested to add html entities ({ or {) and is didin't work. If I use "{" I think template engine interpret it like a start of a function, like "{findings:repeat:"

@izar
Copy link
Collaborator

izar commented Feb 22, 2020 via email

@izar
Copy link
Collaborator

izar commented Feb 24, 2020 via email

@cmorell
Copy link
Author

cmorell commented Feb 24, 2020

Hi,
I've found it and more or less solve the problem, but, the problem now is the comma to separate elements inside the array of findings.

Solution found in: https://stackoverflow.com/questions/51562600/escaping-curly-braces-in-string-to-be-formatted-an-undefined-number-of-times

{{
"name":"{tm.name}",
"description":"{tm.description}",
"potential threats":
[
{findings:repeat:
⁍
	"name":   "{{item.id}}",
	"description":"{{item.description}}",
	"targeted element": "{{item.target}}",
	"severity" : "{{item.severity}}",
	"example instances":"{{item.example}}",
	"mitigations":"{{item.mitigations}}",
	"references":"{{item.references}}"
⁌, 
}
]
}}

After it, we need to change last occurrence of }, to } to make a valid json

@nineinchnick
Copy link
Collaborator

If you need a JSON output you should generate it in Python after building the model, not using a template:

tm = TM("some model")
# add assets and dataflows
tm.resolve()

data = {
  "name":tm.name,
  "description":tm.description,
  "threats": [{"name": f.id} for f in TM._BagOfFindings],
}

with open('report.json', 'w') as f:
    json.dump(data, f)

@cmorell
Copy link
Author

cmorell commented Feb 25, 2020

Hi @nineinchnick
It was the first approach, but @izar suggest me to use template option.
For me go back and use your approach will be easy

Just let me know

@nineinchnick
Copy link
Collaborator

Generating structured data using an unstructured template creates a risk of the output file to be invalid, because it's easy to make a typo in the template or not properly escape template data. If you save a JSON file from Python it's always going to be a correct JSON file and whatever you feed it to will be able to parse it. If you generate it using a template and make a typo, like a missing brace or extra comma, it's hard to pin point exactly where is it. That's the issue you're currently having - an extra comma.

@nineinchnick
Copy link
Collaborator

#102 should also address this but in a more robust way

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