spring-data-couchbase: SimpleCouchbaseRepository.findById() and CouchbaseTemplate.findById() do not validate the document type

It is possible to save() a document of one type, and retrieve it using another type.

The below example will succeed, where one might expect it to fail.

public class Group {
    @Id
    @GeneratedValue(strategy = GenerationStrategy.UNIQUE)
    String id;
    @Field
    String name;
}

@Repository
public interface GroupRepository extends CouchbaseRepository<Group, String> {
}

public class User {
    @Id
    @GeneratedValue(strategy = GenerationStrategy.UNIQUE)
    String id;
    @Field
    String name;
}

@Repository
public interface UserRepository extends CouchbaseRepository<User, String> {
}

@SpringBootApplication
@EnableCouchbaseRepositories
public class ExampleApplication implements ApplicationRunner {

    @Autowired
    private UserRepository userRepository;

    @Autowired
    private GroupRepository groupRepository;

    public static void main(final String[] args) {
        SpringApplication.run(ExampleApplication.class, args);
    }

    @Override
    public void run(final ApplicationArguments args) throws Exception {
        User user = userRepository.save(new User());
        System.out.println(user.id);
        System.out.println(groupRepository.findById(user.id)
                .get().id);
    }
}

The underlying code in SimpleCouchbaseRepository#findById

calls

return Optional.ofNullable(couchbaseOperations.findById(entityInformation.getJavaType()).one(id.toString()));

CouchbaseTemplate should at least try and validate that the supplied type matches the retrieved document type, and either return null (my personal preference), or throw an exception that the underlying type does not match (still acceptable).

We had implemented a workaround for the previous Spring Data Couchbase 3.x series, that prevented this behaviour as it was flagged by our qa/security team as a security issue.

It’s probably just a good idea to solve it at the source and for everyone?

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 32 (17 by maintainers)

Most upvoted comments

_class is not a security compartment

You’re right it’s not directly related to security, but it’s used to enforce type safety. And bugs with type safety can lead to exposing sensitive information. So indirectly I assert that it IS a security concern.

Java is generally considered a type safe language (there are limits and arguments to be made) but it’s not javascript or ruby.

At the Java level when defining and using a Repository there is an explicit contract on the types that the repository will accept and produce.

@Repository
public interface GroupRepository extends CouchbaseRepository<Group, String> {
}

GroupRepository#save() will only compile if Group types are passed in.

And Repository#findById() will only return instances of Group.

However it seems that although type information is persisted and maintained by save() it is ignored by findById(). Strangely enough if findAll() where to return documents of a different type, this would be considered a bug… after all the explicit type contract was neglected, was it not?

In the context of a Repository findById() is the outlier that does not enforce type safety. Why is it special?

To be thorough I think Repository#deleteById(), #deleteAllById, #existsById also suffers from the same problem.

Security is on buckets.

Yes it is.

I also remember Couchbase Professional Services recommending that for micro services we have them share a bucket. Due to scaling limitations of Couchbase, If I remember correctly it performs really badly with more than 10 buckets. And we have 60 odd microservices at last count.

We do have a seperate security bucket for really sensitive information. So there is that.

But even within a single micro service it can and does in our case, contain multiple document types. Represented as distinct resources via a REST API.

And I agree with you that the micro service probably needs to enforce this, not Couchbase itself. I’m not arguing that it be handled by Couchbase for me. I’m just pointing out that Spring Data Couchbase is intended to be a high level interface to Couchbase which provides specific and different (from the raw SDK) paradigms for accessing it. I think the responsibility lies here.

This would prevent an application from having Employee documents read as either Manager entities or IndividualContributor entities.

I’m not discounting this use case, but I am saying I believe it’s definitely not valid in the context of a Repository.

And I think we probably crossover into talking about polymorphism and how to represent that for Couchbase documents, in terms of ORMs JPA and Hibernate are probably a good base line here.

And to reenforce the argument, if one does not want type safety and wants to duck type documents, then they probably shouldn’t be Spring Data Couchbase? As it’s design makes the document types explicit unlike the underlying Couchbase SDK.

The current behavior is the equivalent to the “as(OtherEntity.class)” projection on the query api.

I assume you mean GetResult#contentAs(OtherEntity.class)?

So CouchbaseTemplate has no specific generic type, and always requires the user to pass in the type they want returned. This makes sense, it’s at a lower level than the SimpleCouchbaseRepository implementation.

Given it does accept any class type in the insertById() method, and persist that class type into a field in the raw document. And there is a natural assumed symmetry between insertById() and findById().

I believe it should also allow one to enforce the type of the underlying documents. Maybe that’s by a configuration flag, or by a new explicit type safe method, typeSafeFindById() or perhaps findById(Some.class).verifyType().one("123").

If a microservice is going to rely _class for security, then it will need to enforce it.

But how?

By the time CouchbaseTemplate#findById() has returned, the type information is gone… lost.

And if your answer is going to be, well you can just add another query method to your repository… as a work around. Right sure, so for all the ById methods in the CouchbaseRepository interface, I’ll re-implement them because the out of the box methods have broken the explicit type contract as defined in the repository class definition…

I’m preemptively saying I think this argument is weak at best.

I believe that CouchbaseTemplate needs to provide this functionality out of the box.

aaronjwhiteside on Feb 3, 2021