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

v2 - security - correct the proxy design implementation #1998

Open
baywet opened this issue Dec 19, 2024 · 4 comments
Open

v2 - security - correct the proxy design implementation #1998

baywet opened this issue Dec 19, 2024 · 4 comments
Labels
type:bug A broken experience

Comments

@baywet
Copy link
Member

baywet commented Dec 19, 2024

The current proxy for OpenAPISchema was implemented using inheritance, probably to save time, which forces all properties to be virtual.

Not only this is not the correct proxy design pattern implementation which requires using an interface instead, but this poses a potential security and reliability issue

@baywet baywet added the type:bug A broken experience label Dec 19, 2024
@baywet
Copy link
Member Author

baywet commented Dec 27, 2024

This is also causing bugs as we copy the values from the target, but throw away the copy all the time here.

OpenApiSchema resolved = new OpenApiSchema(_target);

schema = new OpenApiSchemaReference("foo", null);
// Assume foo is not nullable.
schema.Nullable = true;
// The reference to foo is still not nullable.

The implementation should leverage lazy loading, and KEEP the new OpenAPI schema once created.

@baywet
Copy link
Member Author

baywet commented Dec 27, 2024

Another issue is that the compiler doesn't help people know which properties are writeable for a reference since it currently inherits from OpenAPISchema.

I believe that we should introduce a new interface IOpenAPISchema which ONLY has the getters for the properties.
Then all places that currently return an OpenAPISchema should be updated to return that interface. Forcing consumers to do a type assertion and handle the different cases (any property being or not writeable, or updating the source component).

The target should probably also be a public property, in case the consumers want to walk up the reference.

Lastly, the reference resolving mechanism is cumbersome for write scenarios: you need to manually register the components before they can be resolved.

@baywet
Copy link
Member Author

baywet commented Dec 30, 2024

Additional details from additional internal discussions:

  • we don't want to use an interface if we can avoid it, as adding properties would be a breaking change. Maybe introducing an abstract base class is a better middle ground.
  • We'll need to work on additional registration methods to make creation of documents from other object models (yoko, asp.net, etc...) easier. I'll start addressing that in an upcoming PR.

@darrelmiller
Copy link
Member

The OpenApiSchema should not clone the object.

I'm not convinced about the derived base class thing. Can you add just setters in a derived class?
I'm fine with making target public.

We need to make all the references implement IOpenApiReferenceable because the walker relies on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug A broken experience
Projects
None yet
Development

No branches or pull requests

2 participants