security-advisories icon indicating copy to clipboard operation
security-advisories copied to clipboard

better documentation of Git-sourced fields when parsing

Open MangoIV opened this issue 1 year ago • 12 comments

The fact we use the Git history to deduce the published and modified date fields is non-obvious and can confuse users, because parsing our advisory content will fail if they are divorced from the Git repo. We should improve the documentation about this.

original summary retained below


Summary

  • remove an advisory
  • toml parsing fails

Expected behavior

  • toml parsing works on some bag of advisories, there shouldn't have to be any constraints on them

Cause of the bug

MangoIV avatar Mar 29 '24 15:03 MangoIV

yeah I have no idea of where this is coming from; the toml parser also fails if I just 1;1 copy the advisories to some empty directories.

MangoIV avatar Mar 29 '24 15:03 MangoIV

ListAdvisoryValidationError
    "/nix/store/99blf5rfm6rp0sr18168xclkw1gnr47y-empty-dir-with-advisories"
    [ AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    , AdvisoryError
        [ MatchMessage
            { matchAnn = Nothing
            , matchPath = []
            , matchMessage =
                "missing mandatory key: advisory.date"
            }
        ]
        "missing mandatory key: advisory.date in <top-level>\n"
    ]

MangoIV avatar Mar 29 '24 15:03 MangoIV

is it possible that this has to do something with .git? this is the only way I could explain how just copying it to the store would break it.

MangoIV avatar Mar 29 '24 15:03 MangoIV

We use the git history to deduce the date and modified data. Advisory files that are not in a git repo need to include those fields explicitly.

frasertweedale avatar Mar 29 '24 21:03 frasertweedale

I can't reproduce the issue as described in the summary (remove one advisory). I deduce that this all occurred outside a git repo, which is the actual cause (and expected behaviour).

FWIW, we will soon work on an index/archive format of some kind (#170) for efficient distribution of advisories, and which will allow them to be divorced from the git repo without losing metadata. In it's simplest form, this could be a re-rendering of the advisories with the derived date fields present in the TOML.

frasertweedale avatar Mar 29 '24 22:03 frasertweedale

Ahhh makes sense yes. Thank you!

MangoIV avatar Mar 29 '24 22:03 MangoIV

FWIW, we will soon work on an index/archive format of some kind (https://github.com/haskell/security-advisories/issues/170) for efficient distribution of advisories, and which will allow them to be divorced from the git repo without losing metadata. In it's simplest form, this could be a re-rendering of the advisories with the derived date fields present in the TOML.

I think that's a good idea; fwiw if you provided not-only a toml parser but also pretty-printer, that should be really easy to do ;)

MangoIV avatar Mar 30 '24 16:03 MangoIV

Cheers, I'm going to leave this issue open as a reminder to improve the documentation about this implementation detail.

frasertweedale avatar Mar 30 '24 22:03 frasertweedale

thank you <3

MangoIV avatar Mar 30 '24 22:03 MangoIV

osv seems to be appropriate

MangoIV avatar Mar 31 '24 10:03 MangoIV

I guess as a jsonl

MangoIV avatar Mar 31 '24 10:03 MangoIV

without any code changes, this started breaking downstream @frasertweedale https://github.com/MangoIV/cabal-audit/actions/runs/9102576510/job/25656820603 I don't know what is wrong, I can use cabal-audit as usual locally, it only fails in the github runner now. I don't know what the issues is there.

MangoIV avatar May 31 '24 15:05 MangoIV