Toml parser

[glguy]

David wrote:
Would you be interested in preparing a PR?

Sure!

• Previous advisories are still valid

I'm sure you'll take a look yourself to see that the behavior you expect as been preserved, but I'm able to run the tool on the 3 examples we have so far and see it working well.

I've also tested that TOML generation looks to match. The quoted TOML below was generated by the toml-parser library

[advisory]
aliases = ["CVE-2022-3433"]
cwe = [328, 400]
id = "HSEC-2023-0001"
keywords = ["json", "dos"]

[[affected]]
cvss = "CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:N/A:H"
package = "aeson"

[[affected.versions]]
fixed = "2.0.1.0"
introduced = "0.4.0.0"

[[references]]
type = "ARTICLE"
url = "https://cs-syd.eu/posts/2021-09-11-json-vulnerability"

[[references]]
type = "ARTICLE"
url = "https://frasertweedale.github.io/blog-fp/posts/2021-10-12-aeson-hash-flooding-protection.html"

[[references]]
type = "DISCUSSION"
url = "https://github.com/haskell/aeson/issues/864"

Note that the only difference is a little more whitespace and everything is alphabetized. If you decide you like certain fields to render before others, you can specify that with prettyTomlOrdered

Of course I'm open to requests for improvements to this PR!

[frasertweedale]

Thank you for the PR!

There are a handful of hlint suggestions detected in CI: https://github.com/haskell/security-advisories/actions/runs/5537522443/jobs/10106419242?pr=88

In general I'm OK with the change. I'm not a fan of the string-only error values in toml-parser, but we get some benefits with the change (such as printing the TOML, which will be handy if we need to do a bulk schema change).

BEFORE we merge this, I'd like to add parsing tests, including for a whole bunch of failure scenarios (unrecognised keys, invalid values, missing fields, etc). That will give me more confidence in this change. And we should have some tests anyway.

[glguy]

BEFORE we merge this, I'd like to add parsing tests, including for a whole bunch of failure scenarios (unrecognised keys, invalid values, missing fields, etc). That will give me more confidence in this change. And we should have some tests anyway.

toml-parser has a complete test suite included that has code coverage for all the parsing and decoding logic (checked via hpc) and passes all of the defacto-standard burnt-sushi toml-tests (which is a minor improvement over the library being replaced which fails three of them). In both dimensions this is an improvement over the previous condition.

I think you're right that you should have a test suite for your combined markdown format!

[glguy]

@frasertweedale I don't know enough about Nix to fix the Nix build action. Is that something you could help me with?

[frasertweedale]

@frasertweedale I don't know enough about Nix to fix the Nix build action. Is that something you could help me with?

ping @blackheaven

[glguy]

@frasertweedale The request review is because I think all the changes I'm responsible for are in and I think I'm ready to hand this off to the maintainers to decide what to do with (like implementing a test suite first). If there's other changes on my end, let me know.

[glguy]

@frasertweedale as a heads-up, I'm working toward what I think is a nice middle ground where the library will expose machine-readable error messages in low-level modules while the top-level Toml module will render out human readable error messages. So I should be able to support both sorts of users in the next release.

[frasertweedale]

@glguy thank you very much. I'll aim to establish a test suite in the coming week or so. Some other changes are coming that will require a rebase but we can definitely handle that on our side. So, it will probably be some weeks before we land this PR, but there seems to be a consensus to accept this change. So thank you very much for your contribution!

[TristanCacqueray]

Thanks, that looks good to me!

[glguy]

PR rebased and adapted for the new HsecId type

[TristanCacqueray]

Oops, I merged the conflicting change, I'll rebase shortly.

[TristanCacqueray]

I've rebased this PR on top of the new golden tests, it should be good to go now. Thanks @glguy!

[TristanCacqueray]

@frasertweedale may we merge this now?

[TristanCacqueray]

I just rebased to validate the tests.