[fix] some minor fixes #201

MangoIV · 2024-05-31T15:54:21Z

introduce Exception instance for ParseAdvisoryError
display if parsing out of band git attributes failed
also try to parse into a UTCTime instead of a ZonedTime

resolves #203
resolves #202
resolves #200

hsec-tools

Previous advisories are still valid

- introduce Exception instance for ParseAdvisoryError - display if parsing out of band git attributes failed

TristanCacqueray · 2024-05-31T16:46:02Z

code/hsec-tools/src/Security/Advisories/Filesystem.hs

-            Left _ -> emptyOutOfBandAttributes
+          liftIO (getAdvisoryGitInfo advisoryPath) >>= \case
+            Left gitErr -> 
+              liftIO (hPutStrLn stderr ("obtaining out of band attributes failed: \n" <> explainGitError gitErr))


May we return a validation error instead of a warning on the stderr?

chesterton's fence: why's the error ignoed in the first place, are there valid reasons to not abort here?

if not, then this was an actual bug but it seems too obvious to be one.

I think the idea is that we don't want to fail if there's no git information as there might still be one supplied by the advisory itself. I think this is an issue with the structure of the code.

I have decided to bubble the error up instead of using this oobValue with optional fields. I think it was just not very fortunately designed.

Thanks, that looks reasonable to me. Though I worry this is a complicated API for downstream users, perhaps we could do instead:

Make the oob attributes explicit in the advisory document, and/or

Publish a static version of the advisories with the oob attributes pre resolved.

What do you think @frasertweedale ?

Publishing a version with the oob pre resolved would be a good idea anyways. That would make testing without git possible and generally would allow distribution without git. I don’t think the interface for downstream users is more complicated now, tbh it was quite complicated with these fields that could independently be Nothing when in reality I don’t see a reason why they should be able to. Not attaching a reason for why the oob attributes being missing was problematic as well, imo, this caused the Git errors to never be shown.

MangoIV · 2024-05-31T16:51:21Z

something else I was wondering. Do we really want ZonedTime? It has some really awful properties like it missing an Ord instance. What do you think about changing everything to UTCTime, @TristanCacqueray?

MangoIV · 2024-05-31T17:30:41Z

… it seems like this is an endeavour for another day, toml-parser doesn’t support it (though it should)

MangoIV · 2024-05-31T19:54:29Z

I have refactored the out of band stuff such that we can bubble up information on why it failed. This is important if git doesn't work as expected.

…-advisories#201

- OutOfBandAttributes now have two mandatory fields - the git error is bubbled up until it can be handled in validation

MangoIV · 2024-06-01T07:16:35Z

I can backport 4b and fe to the minor version if you don’t want breaking changes but I don’t know if it’d be too helpful.

telser · 2024-06-07T14:24:54Z

I believe this will resolve an issue that currently causes https://github.com/MangoIV/cabal-audit to break for me. Though @MangoIV can correct me if that is incorrect.

MangoIV · 2024-06-07T14:39:40Z

Yes @frasertweedale already knows :)

blackheaven

Quick note aside: I'm not a fan of BlockArguments

@MangoIV can you check whether forcing git date format fixes the issue please?

code/hsec-tools/app/Main.hs

blackheaven · 2024-06-07T14:43:27Z

code/hsec-tools/src/Security/Advisories/Git.hs

+    parseTime :: String -> Either GitError ZonedTime
+    parseTime s = maybe (Left $ GitTimeParseError s) Right $
+       iso8601ParseM s
+         <|> utcToZonedTime utc <$> iso8601ParseM s


I'm not sure it does preserve the date

what do you mean?

I just validated that the osv files are not changed by this PR with:

mkdir osvs for i in $(find advisories/ -name "*.md" | grep -v reserved); do cabal run exe:hsec-tools -- osv $i > osvs/$(basename $i); done

It's not that obvious, we get a time and day without time zone, assuming UTC seems audacious.

IMO, we should rely only on UTCTime, enforcing it in the git command.

Oh I see, yes I'm on UTC. Then I remove my approval. Perhaps we could just parse an epoch timestamp if possible?

hsec-tools already does this several times (assuming UTC when there's not enough information) That's why I opened the issue about removing ZonedTime from hsec-tools. It makes stuff harder as well. It is however the correct time to assume, it's not wrong, it's just not semantically as nice. See #203 cc @frasertweedale who commented on #203

blackheaven · 2024-06-07T14:47:10Z

code/hsec-tools/src/Security/Advisories/Git.hs

-    parseTime s = maybe (Left $ GitTimeParseError s) Right $ iso8601ParseM s
+    parseTime :: String -> Either GitError ZonedTime
+    parseTime s = maybe (Left $ GitTimeParseError s) Right $
+       iso8601ParseM s


We should force the format at git level: https://stackoverflow.com/a/7651782

I tried to pass --date=local and it didn't work. This is in general not really ideal, we shouldn't depend on the git installation if git itself doesn't guarantee a stable interface to these things and/ or this is not documented.

There's also haskell/time#257 which I don't know whether it's the case.

https://github.com/MangoIV/cabal-audit/actions/runs/9321254777/job/25659857545

here's a run. the build step is Test git which uses the date format local and it still outputs a non-zoned iso8601. I wouldn't have done the separate parse try without trying that first.

I assume (read "guess") that it is valid to output a non-zoned iso8601 even with local if the computer is itself in UTC which is why I'm not sure whether parsing an unzoned time into a UTCTime should just work.

Let's do this, it seems to be the lesser bad solution we have.

I'm confused, setting it in git is a non-solution because it doesn't work. We have to do it like this, no?

I'm confused, I thought that, requiring a ZonedTime from git wasn't working, requiring a UTCTime doesn't work either?

oh but then we always lose zone information, what does that improve? should we do #203 then?

@frasertweedale was talking of a flag few minutes ago (at ZuriHac), maybe we can have a try

MangoIV · 2024-06-09T16:22:08Z

I have replaced ZonedTime with UTCTime; there's now not a single place where ZonedTime occurs anymore

MangoIV · 2024-06-09T17:15:20Z

how do I check if the advisories are still valid btw? :3

MangoIV · 2024-06-09T17:15:27Z

don't wanna break your stuff :3

MangoIV · 2024-06-09T17:15:42Z

thank you for your review @blackheaven @TristanCacqueray <3

blackheaven · 2024-06-09T17:17:04Z

how do I check if the advisories are still valid btw? :3

the CI handles it :)

frasertweedale · 2024-06-10T13:50:08Z

Thank you @MangoIV! Merging now.

MangoIV added 2 commits May 31, 2024 17:52

[fix] some minor fixes

4b3059e

- introduce Exception instance for ParseAdvisoryError - display if parsing out of band git attributes failed

[fix] also try parsing UTC with defaulted time zone from git

fe0ca8d

TristanCacqueray reviewed May 31, 2024

View reviewed changes

MangoIV requested a review from TristanCacqueray May 31, 2024 16:49

MangoIV force-pushed the mangoiv/fixes branch 2 times, most recently from dbf5de8 to 8e8b11e Compare May 31, 2024 19:53

MangoIV added a commit to MangoIV/cabal-audit that referenced this pull request May 31, 2024

[chore] some fixes and simplification after applying haskell/security…

2eaae7e

…-advisories#201

[feat] bubble up git errors

c1703e8

- OutOfBandAttributes now have two mandatory fields - the git error is bubbled up until it can be handled in validation

MangoIV force-pushed the mangoiv/fixes branch from 8e8b11e to c1703e8 Compare May 31, 2024 20:02

MangoIV mentioned this pull request Jun 1, 2024

[chore] documentation update for #21, repair nix dev env MangoIV/cabal-audit#29

Merged

blackheaven requested changes Jun 7, 2024

View reviewed changes

[chore] add back dollars

d8d8e44

MangoIV requested a review from blackheaven June 7, 2024 15:26

frasertweedale self-requested a review June 7, 2024 15:32

TristanCacqueray approved these changes Jun 7, 2024

View reviewed changes

[chore] replace all uses of ZonedTime with UTCTime

75850bc

blackheaven approved these changes Jun 9, 2024

View reviewed changes

[chore] major version bump for hsec-core

949c3bd

MangoIV force-pushed the mangoiv/fixes branch from 90c2f19 to 949c3bd Compare June 10, 2024 08:06

blackheaven approved these changes Jun 10, 2024

View reviewed changes

frasertweedale merged commit c2960f7 into haskell:main Jun 10, 2024
8 checks passed

[fix] some minor fixes #201

[fix] some minor fixes #201

Conversation

MangoIV commented May 31, 2024 • edited Loading

hsec-tools

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MangoIV commented May 31, 2024

MangoIV commented May 31, 2024

MangoIV commented May 31, 2024

MangoIV commented Jun 1, 2024

telser commented Jun 7, 2024

MangoIV commented Jun 7, 2024

blackheaven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MangoIV Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MangoIV commented Jun 9, 2024

MangoIV commented Jun 9, 2024

MangoIV commented Jun 9, 2024

MangoIV commented Jun 9, 2024

blackheaven commented Jun 9, 2024

frasertweedale commented Jun 10, 2024

MangoIV commented May 31, 2024 •

edited

Loading

MangoIV Jun 7, 2024 •

edited

Loading