Useless comment. It would be much more helpful to specify why it is important.

A docstring giving an overview of the API would be nice. For instance, it would be interesting to know what formats of report are supported, etc.

Could be shortened:

mappings = self.get_argument("mapping", None)
if mappings:
    mappings = self.get_component("mapping_db").GetMappings(mappings)

Could be shortened:

if mappings and test_group['code'] in mappings:
    test_group["mapped_code"] = mappings[test_group['code']][0]
    test_group["mapped_descrip"] = mappings[test_group['code']][1]

Also, it would be more readable to name [0] and [1], for instance:

code, description = mappings[test_group['code']]
test_group["mapped_code"] = code
test_group["mapped_descrip"] = description

Just a note: iteritems does not exist in python 3.x. If OWTF is planing on supporting python3.x, it would be time to stop adding more incompatible code. Would items be enough? Or will it add to much overhead in python2.x?

Is there a need for the temporary variable (for creating a copy and not a reference)? Or would the following work as well?

for key, value in grouped_plugin_outputs.items():
    test_groups[key]['data'] = value
    vulnerabilities.append(test_groups[key])

Also in your current version, are we sure that test_groups[key] exists?

Are you sure that such function is not already available? Either in OWTF or even in PTP?

Apparently no, there is no other reference where the ranking scale is defined (except hardcoded in https://github.com/owtf/owtf/blob/master/framework/interface/templates/plugin_report.html)

No there is not such a function exists.

As such, it would be better to separate the get_rank function from the ranking scale. By splitting the two, the ranking scale could be used somewhere else in the framework, in case another feature needs to translate the rankings.

Also, get_rank should check that the index is defined in the dict and default to a default value if it is not. For instance, something like:

return RANKS.get(rank, 'Unknown')

The string Unknown should be defined somewhere else, or Unranked could be used. You get the idea.

I would suggest specifying what is the return value in the docstring. Also, I am not sure to understand why the function should make the decision of which ranking to use. I believe that this should be done before calling it. Something like:

# From the API
rank_str(max(user_rank, owtf_rank))

Finally, the name of the function could be a bit more specific in what it does with something like rank_human_readable, rank_str. From what I read, the function does not get anything, it simply translates a ranking from numerical to string value.

Now I am realizing that we don't need such function. Instead, all we need is the dict translating the numerical ranking to a string.

The get_rank function can be removed and instead we could do the following:

# In some util file
from ptp.libptp.constants import INFO, LOW, MEDIUM, HIGH
OWTF_UNRANKED = -1
OWTF_PASSING = 0
OWTF_CRITICAL = 5
RANKS = {
    OWTF_UNRANKED: 'Unranked',
    OWTF_PASSING: 'Passing',
    INFO: 'Informational',
    LOW: 'Low',
    MEDIUM: 'Medium',
    HIGH: 'High',
    OWTF_CRITICAL: 'Critical'
}

# In the API
poutput["rank"] = RANKS.get(max(user_rank, owtf_rank), RANKS[OWTF_UNRANKED])

Of course this could be polished a bit more and the string translated value could also be defined as constants, etc. It is just a draft to show the alternative I am seeing.

@DePierre Can I get all INFO, LOW, MEDIUM, HIGH, UNRANKED, PASSING, CRITICAL variable from ptp.

@saganshul ptp does not have CRITICAL by design, since it deemed that only user could have the context to rank an issue as CRITICAL.

PASSING does not exist, since ptp does not evaluate if a report is passing or not. However, UNKNOWN is used when no ranking could be found. It should be noted though that UNKNOWN is 0, while OWTF uses 0 for PASSING.

See https://github.com/owtf/ptp/blob/develop/ptp/libptp/constants.py

I believe that this could be shortened with:

grouped_plugin_outputs = {poutput['plugin_code']: [] for poutput in plugin_outputs}

for poutput in plugin_outputs:
    if grouped_plugin_outputs.get(poutput['plugin_code']) is None:
        grouped_plugin_outputs[poutput['plugin_code']] = []

    poutput["rank"] = get_rank(poutput["user_rank"], poutput["owtf_rank"])
    grouped_plugin_outputs[poutput['plugin_code']].append(poutput)

This total thing can't be replaced with

grouped_plugin_outputs = {poutput['plugin_code']: [] for poutput in plugin_outputs}

This thing only does this part:

for poutput in plugin_outputs:
    if grouped_plugin_outputs.get(poutput['plugin_code']) is None:
        grouped_plugin_outputs[poutput['plugin_code']] = []

@saganshul I understand, I misread the indentation and was only considering the first three lines.

Would the following structure be better in your opinion?

from collections import defaultdict
 
grouped_plugin_outputs = defaultdict(list)
for output in plugin_outputs:
    output['rank'] = RANKS.get(max(output['user_rank'], output['owtf_rank']))
    grouped_plugin_outputs[output['plugin_code']].append(output)

Could we reduce the scope of the try/except statement? It is usually recommended to have the smallest scope for try/except.

I am not sure to understand the the point of this comment. What is overridden? Maybe I am not understanding the code snippet but I believe that the line below this comment does not override anything.

I don't believe that this belongs to the docstring of the API. As far as I can see, docxtemplater is only being used in a JS file that should be abstracted from this API. It does not and should not be a concern for the API whether docxtemplater or .docx format is used or not.

The docstring should focus on what the API does. As far as I can tell, it fetches data from the DB to prepare some JSON structure that contain information about a target, plugins run, rankings, etc. What information does it fetch? What does it do with it? What return value does it return?

I would prefer to have a docstring that explains what it does rather than a docstring that explains what another piece of code somewhere else can do with the data it created.

It might befar-fetched but wouldn't it be more modular to have the API set to /api/targets/{id}/export/{format} or /api/targets/{id}/export/{template} instead?

It would free the /api/targets/{id}/export/ API that could later default to a specific template/format, or be used to show a dialog box where the user can specify more options.

What do you think?

@DePierre Actually, we have plans to separate the React UI from Tornado API server. Means Render everything using React rather than Jinja2. We will make backend as simple API server.

Now, as you suggested API should be something like this -> /api/targets/{id}/export/{template} , but backend server will not have any information about templates, as the export functionality is completely client side and only React or UI will have information about available templates.

Use of /api/targets/{id}/export/ is to just return data from DB, docxtemplater will do the rest. I mean we don't need to have seperate api for template, because the same API will work for all template.

@saganshul Thanks for the additional info. I believe you already thought about it and it's fine by me. It was simply a remark.

Did you have time to consider the other review comments I made?

yeah! Just doing that :)

As such, it would be better to separate the get_rank function from the ranking scale. By splitting the two, the ranking scale could be used somewhere else in the framework, in case another feature needs to translate the rankings.

Also, get_rank should check that the index is defined in the dict and default to a default value if it is not. For instance, something like:

return RANKS.get(rank, 'Unknown')

The string Unknown should be defined somewhere else, or Unranked could be used. You get the idea.

I would suggest specifying what is the return value in the docstring. Also, I am not sure to understand why the function should make the decision of which ranking to use. I believe that this should be done before calling it. Something like:

# From the API
rank_str(max(user_rank, owtf_rank))

Finally, the name of the function could be a bit more specific in what it does with something like rank_human_readable, rank_str. From what I read, the function does not get anything, it simply translates a ranking from numerical to string value.

Maybe format this in pretty print format.

RANKS = {
    OWTF_UNRANKED: 'Unranked',
    OWTF_PASSING: 'Passing',
    OWTF_INFO: 'Informational',
    OWTF_LOW: 'Low',
    OWTF_MEDIUM: 'Medium',
    OWTF_HIGH: 'High',
    OWTF_CRITICAL: 'Critical',
}