microsoft / restler-fuzzer

RESTler is the first stateful REST API fuzzing tool for automatically testing cloud services through their REST APIs and finding security and reliability bugs in these services.
MIT License
2.52k stars 283 forks source link

Change JsonSerializer to require all non-optional fields #852

Closed eddyashton closed 4 months ago

eddyashton commented 5 months ago

I was surprised by how permissive the JSON deserialization is, and it resulted in error messages hiding useful information.

My motivating use case was an annotations file like this:

{
  "x-restler-global-annotations": [
    { }
  ]
}

Trying to compile a grammar with that produces an extremely generic null-reference message:

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object. at Restler.Annotations.getAnnotationsFromJson(JToken annotationJson) in /data/src/restler-fuzzer/src/compiler/Restler.Compiler/Annotations.fs:line 168 at Restler.Annotations.getGlobalAnnotationsFromFile(String filePath) in /data/src/restler-fuzzer/src/compiler/Restler.Compiler/Annotations.fs:line 220 at Restler.Workflow.generateGrammarFromSwagger(String grammarOutputDirectoryPath, Config config) in /data/src/restler-fuzzer/src/compiler/Restler.Compiler/Workflow.fs:line 104 at Restler.Workflow.generateRestlerGrammar(Config config) in /data/src/restler-fuzzer/src/compiler/Restler.Compiler/Workflow.fs:line 250 at Program.main(String[] argv) in /data/src/restler-fuzzer/src/compiler/Restler.CompilerExe/Program.fs:line 43

It turns out that even though ProducerConsumerUserAnnotation says producer_endpoint : string (implying a non-optional field), it will parse {} and produce an object with {producer_endpoint: null}. Despite sensible checks on the result of tryDeserialize, this is missed and blows up later. I think this should be a parsing error, caught by all callers of Utilities.JsonSerialization.tryDeserialize and reported correctly.

This PR makes that change, by specifying a ContractResolver from FSharpLu.Json. The original example now produces a descriptive error:

Unhandled exception. Newtonsoft.Json.JsonSerializationException: Required property 'producer_endpoint' not found in JSON. Path '', line 1, position 2. ...

(Disclaimer - I'm not familiar with F# or .NET, and I've only just started experimenting with restler, so this is a purely speculative change from a bit of docs-probing. Please let me know if there's a better way to achieve this, or some reason that the original behaviour is required)

eddyashton commented 5 months ago

CI failed, but it looks like this was due to a race condition, concurrently accessing files:

  Failed Restler.Test.Dictionary+DictionaryTests.quoting is correct in the grammar [1 s]
  Error Message:
   System.IO.IOException : The process cannot access the file 'C:\Users\VssAdministrator\AppData\Local\Temp\restlerTest\202142\grammar.json' because it is being used by another process.
  Stack Trace:
    ...
  Failed Restler.Test.Dictionary+DictionaryTests.path and body parameter set to the same uuid suffix payload [30 ms]
  Error Message:
   System.IO.IOException : The process cannot access the file 'C:\Users\VssAdministrator\AppData\Local\Temp\restlerTest\202142\grammar.json' because it is being used by another process.

Is this a known flaky test? Should the CI simply be re-run?

marina-p commented 5 months ago

CI failed, but it looks like this was due to a race condition, concurrently accessing files:

  Failed Restler.Test.Dictionary+DictionaryTests.quoting is correct in the grammar [1 s]
  Error Message:
   System.IO.IOException : The process cannot access the file 'C:\Users\VssAdministrator\AppData\Local\Temp\restlerTest\202142\grammar.json' because it is being used by another process.
  Stack Trace:
    ...
  Failed Restler.Test.Dictionary+DictionaryTests.path and body parameter set to the same uuid suffix payload [30 ms]
  Error Message:
   System.IO.IOException : The process cannot access the file 'C:\Users\VssAdministrator\AppData\Local\Temp\restlerTest\202142\grammar.json' because it is being used by another process.

Is this a known flaky test? Should the CI simply be re-run?

Correct, this failure is a known race condition and it's fine to merge if the test passes on re-run.