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

Adjust uri to uri-reference for security schema fixed fields #4328

Merged
merged 2 commits into from
Feb 13, 2025

Conversation

markrzen
Copy link

@markrzen markrzen commented Feb 4, 2025

Resolves #4327

Tick one of the following options:

  • schema changes are included in this pull request
  • schema changes are needed for this pull request but not done yet
  • no schema changes are needed for this pull request

@markrzen markrzen requested review from a team as code owners February 4, 2025 00:09
@ralfhandl ralfhandl linked an issue Feb 6, 2025 that may be closed by this pull request
@mikekistler
Copy link
Contributor

Why not change all the format: uri to format: uri-reference. I could not find any fields defined as URIs and forbid relative references (but I am surprised that the OAuth ones allow them).

@markrzen
Copy link
Author

markrzen commented Feb 6, 2025

I didn't make that change initially only because I didn't have time to consume the whole specification to confirm that all the uri references didn't have specification that explicitly forbid relative.

I am happy to update the PR if someone more knowledgable than I can confirm that is accurate.

My hesitation came from the following, which planted the "seed of doubt" that there may be some exclusion hidden away on one of the uri usages.:

Unless specified otherwise, all fields that are URLs MAY be relative references as defined by [RFC3986] Section 4.2.

@handrews
Copy link
Member

handrews commented Feb 6, 2025

The XML Object namespace MUST be non-relative. That's the only one I can find.

@handrews
Copy link
Member

handrews commented Feb 6, 2025

@markrzen Update from today's TDC call: If you're up for changing the other format: uri to format: uri-reference except the XML Object's namespace field, please go ahead and do so. If not, let us know and we can take this as-is and one of us will do a follow-up PR.

@markrzen
Copy link
Author

markrzen commented Feb 6, 2025

@handrews On it right now.

@markrzen
Copy link
Author

markrzen commented Feb 6, 2025

@handrews I could only find the xml object in the meta.yaml, and that document already looked correct.

Copy link
Member

@karenetheridge karenetheridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one change that should be reverted; the others look ok to me (not sure about the security ones though).

@@ -12,7 +12,7 @@ properties:
$ref: '#/$defs/info'
jsonSchemaDialect:
type: string
format: uri
format: uri-reference
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is the twin to JSON Schema's $schema keyword, and therefore cannot be a uri-reference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karenetheridge philosophically I agree with you, but the way the OAS field is written unfortunately allows a relative URI-reference. Of course you'd have to resolve that before you could use it as a dialect URI, and the only way that might work is with a custom dialect, but there's no reason it couldn't work that way.

I do agree that it should have been written as requiring a non-relative URI.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to make the change if needed. Waiting for maintainer to come to an agreement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@handrews what do you mean "the way the OAS field is written unfortunately allows a relative URI-reference"? What are you referencing here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karenetheridge It's the combination of two things:

§4.6.7 states:

Unless specified otherwise, all fields that are URIs MAY be relative references as defined by [RFC3986] Section 4.2.

while the fixed fields description for jsonSchemaDialect says:

The default value for the $schema keyword within Schema Objects contained within this OAS document. This MUST be in the form of a URI.

Strictly speaking, this text allows a URI reference. The value used with $schema of course must be a full URI, but that can be produced by resolving the relative URI-reference normally, just as with any other field that the OAS says "MUST be in the form of a URI". What is used with $schema is the resolved value, not the literal text.

At least that is my interpretation. We can always kick it up to @OAI/tsc for a ruling if you disagree. Again, I prefer your reading (and really dislike that we don't just explicitly say "URI-reference" and instead say "URI means URI-reference unless otherwise stated") but that's what we have to work with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least we need to tighten up the language that leaves this vague. It is not reasonable to say in one place that a value is a URI, and then at the top say "actually that means URI-reference" because now there's no way to say that no, really, this actually has to be a fully-resolved URI.

Also, in any place where we do allow a URI-reference, we should be explicit as to how that value will be resolved -- whether it's against the URI used to identify the entry document, the current document, or the Host header on the current HTTP request under consideration (when relevant) -- I can see places where each of these might be the appropriate choice for URI resolution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karenetheridge take a look at the XML Object's namespace field for how we say a field is a fully-resolve URI (again, I agree we should use the correct terminology per RFC3986 and not what we're doing now, but we do have ways to say we don't want a non-relative reference). We are very explicit about how OAD URI-referencess and API URL-references are resolved. Each of those topics gets a full section in the 3.1 specification. In any event, this is drifting outside of the scope of this PR and should continue in a discussion or issue.

Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 👍

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karenetheridge it sounded like you accepted my reading of the spec for jsonSchemaDialect in the last comment, so I'm going to go ahead and merge this. If I'm wrong, lmk and we'll sort it out.

@handrews
Copy link
Member

@karenetheridge or not... can you remove your "changes requested"?

@handrews
Copy link
Member

Oh this is because there's only one approval- it's too early in the morning for me to be doing code reviews apparently. @karenetheridge or @mikekistler if either of you is willing to give an approval that will do it.

Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@seriousme
Copy link
Contributor

Apparently it needs explicit approval from @karenetheridge as the two approvals from @mikekistler and @handrews are in but the flow is still blocked.

@markrzen
Copy link
Author

Someone with write access to the repo can likely dismiss the request for change.

@handrews
Copy link
Member

I'll check with @karenetheridge on this directly to resolve, she might just not have seen this in the last few days.

@karenetheridge karenetheridge dismissed their stale review February 13, 2025 03:41

Apparently we don't explicitly disallow uri-reference for jsonSchemaDialect, so we have to allow it for now.

We should still amend the spec to make this more clear, and specify which uri this uri-reference should resolve against (I suppose it would have to be the uri of the containing document itself).

@ralfhandl ralfhandl merged commit d22d504 into OAI:v3.1-dev Feb 13, 2025
2 checks passed
@ralfhandl
Copy link
Contributor

@markrzen Thanks for your contribution, and extra thanks for your patience 😊

@seriousme
Copy link
Contributor

Thanks to all for making this work!
Sorry if the question is inappropriate here, but can you indicate when the v3.1-dev will be "officially released" as update to v3.1 in the gh-pages branch?

Reason for asking is that I maintain tooling that builds on the "officially released" schema's and I'd like to offer @markrzen the update merged in this PR in my tooling. (see Mark's initial post in #4327)

Kind regards,
Hans

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

Successfully merging this pull request may close these issues.

tokenUrl in 3.1 schema does not support relative urls as specified
6 participants