search

SAP / odfuzz

Apache License 2.0
19 stars 12 forks source link

PR-Put and Post requests for FioriDAST #79

Closed prashdsouza closed 3 years ago

prashdsouza commented 3 years ago

To generate uris and body based on the method type that is passed to DirectBuilder. For consumption by FioriDAST only =>FioriDAST/pull/301

phanak-sap commented 3 years ago

Hi @lubosmj, thank you for unexpected review. The rationale behind this PR (and changes before them, leading to 0.14a1 and 0.14a2 alpha tags) is that CLI usage SHOULD NOT be affected by these changes in any way. It will be only tested manually that when executed odfuzz by CLI, no errors are thrown and it produces expected output.

Adding support for DELETE, PUT and POST is at the moment in the code path of DirectBuilder usage - which is for integration with other SAP tools, using the odfuzz as a python library; something which was not designed up front.

CLI usage and standard DispatchedBuilder code path should be covered intentionally later. Reasons is to reduce risk, 1. because it is actively used this way; 2. because it is not covered well by automation in this way; 3. because cyclic dependencies between modules, especially in the mutation part makes it harder. It was problematic just when we tried it for the DELETE urls, which at least have the simplicity of just URL and not body.

At the moment not sure how exactly will be the PUT/POST usage feature introduced to CLI code path, but one option which I am considering, is to move towards to more loosely coupled and more independend/testable modules, ditching DispatchedBuilder and use DirectBuilder internally as well. Sadly, in the DispatchedBuilder are not only part relevant to networking, but seems to be covering critical data for the mutation loop.

AlapanGhosh commented 3 years ago

The conflict with built-in format function must be addressed.

done

lubosmj commented 3 years ago

Hi @lubosmj, thank you for unexpected review. The rationale behind this PR (and changes before them, leading to 0.14a1 and 0.14a2 alpha tags) is that CLI usage SHOULD NOT be affected by these changes in any way. It will be only tested manually that when executed odfuzz by CLI, no errors are thrown and it produces expected output.

Since the return value of the method generate() has changed globally, CLI will be affected definitely. Please, take a look again at the code snipped I attached in my comment https://github.com/SAP/odfuzz/pull/79#pullrequestreview-581832441. We cannot leave the main fuzzing loop untouched.

The actual usage in fuzzer.py is: https://github.com/SAP/odfuzz/blob/bf08492ce8914a56f27a7f7bd3d0e5352064e722/odfuzz/fuzzer.py#L169

But, it should look more like this:

queries, _ = q.generate()

(I am referring to this change: https://github.com/SAP/odfuzz/pull/79/files#diff-c1bcb89af3503c0e2ab10ddba16324168b0478ae444d36fa7e5e4ce492d24569R606-R609. The change targets just SingleQueryable. However, in the main fuzzing loop, we consider both SingleQueryable and MultipleQueryable. In the master branch, we have: [query] vs [query1, query2, query3, ...]. With this PR, we have [query, body] vs [query1, query2, query3, ...].)