Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trying to understand why last_affected and fixed are exclusionary? #146

Closed
kurtseifried opened this issue Apr 4, 2023 · 11 comments
Closed

Comments

@kurtseifried
Copy link
Contributor

In the schema:

            {
              "if": {
                "properties": {
                  "events": {
                    "contains": {
                      "required": ["last_affected"]
                    }
                  }
                }
              },
              "then": {
                "not": {
                  "properties": {
                    "events": {
                      "contains": {
                        "required": ["fixed"]
                      }
                    }
                  }
                }
              }
            }

Why are they exclusionary? I couldn't find anything in the docs. Was there a discussion about this that I can view that explains it?

@kurtseifried
Copy link
Contributor Author

Or was it meant to have all 4 in there as per:

Only a single type (either "introduced", "fixed", "last_affected",
"limit") is allowed in each event object. For instance,
{"introduced": "1.0.0", "fixed": "1.0.2"} is invalid.

@oliverchang
Copy link
Contributor

This is this sentence in the schema:

"Entries in the events array can contain either "last_affected" or "fixed" events, but not both."

The intention is to prevent confusing combinations of last_affected and fixed within the same events array, because they make each other redundant.

@kurtseifried
Copy link
Contributor Author

Ok, can we update the docs to mention that this is actually enforced in the schema?

@bagder
Copy link

bagder commented May 3, 2023

Why can't we have both? As a mere human, I think they provide information and it certainly doesn't hurt to have both even if one of them would suffice.

@andrewpollock
Copy link
Collaborator

@kurtseifried I'm trying to determine if there's anything actionable here.

Are you asking to more explicitly state that things documented as being required by the schema are enforced by the schema definition?

@kurtseifried
Copy link
Contributor Author

@kurtseifried I'm trying to determine if there's anything actionable here.

Are you asking to more explicitly state that things documented as being required by the schema are enforced by the schema definition?

At a minimum, it needs to be documented, and enforced in the schema, so those both need an update.

Also, I think, in line with @bagder it is not necessary to make it exclusionary especially as there are many cases where "The intention is to prevent confusing combinations of last_affected and fixed within the same events array, because they make each other redundant." is not correct (Cisco software train version, Linux Kernel, etc.).

@andrewpollock
Copy link
Collaborator

At a minimum, it needs to be documented, and enforced in the schema, so those both need an update.

I think the point @oliverchang was trying to make was that these are both the case today, if I am reading correctly?

Perhaps you could send us a PR to illustrate what you feel needs to be updated?

@kurtseifried
Copy link
Contributor Author

The problem is, if it is up to me I would remove the requirement entirely and allow both. As such I can't really say why it is required, so I can't really submit a PR for this as I don't understand what the intent is other than "it might be confusing"(but for many of my use cases it is not).

@rhalar
Copy link

rhalar commented Jun 13, 2023

I have to admit I do agree that having both is confusing and potentially very vague as to what is or isn't vulnerable, if @kurtseifried would be willing to expand on his reasoning?

I've mentioned part my confusion in another issue (#150 (comment)), but I'm also interested to understand when having both isn't redundant, namely you say:

"The intention is to prevent confusing combinations of last_affected and fixed within the same events array, because they make each other redundant." is not correct

oliverchang pushed a commit that referenced this issue Jun 23, 2023
This is a first pass at further clarifying the `"last_affected"` field
and addressing #146 and #150.

Preview is available
[here](https://hayleycd.github.io/osv-schema/#requirements)

---------

Signed-off-by: Hayley Denbraver <[email protected]>
Co-authored-by: Chris Bloom <[email protected]>
@hayleycd
Copy link

For any future readers on this issue, please refer to this comment in #150 for clarity.

oliverchang pushed a commit that referenced this issue Jul 14, 2023
Hopefully wraps up #150 and #146 

View rendered example
[here](https://hayleycd.github.io/osv-schema/#last_affected-vs-fixed-example).

Changes were also made to the [affected.ranges.events
fields](https://hayleycd.github.io/osv-schema/#affectedrangesevents-fields)
to bring the formatting into line with the rest of the document. Fields
were being rendered like this: `"last_affected"` where `last_affected`
is preferred.

---------

Signed-off-by: Hayley Denbraver <[email protected]>
@hayleycd
Copy link

I think we can close this given #174 and #159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants