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

add an alternate version of EI_EXPOSE_REP #2898

Open
xenoterracide opened this issue Mar 11, 2024 · 4 comments
Open

add an alternate version of EI_EXPOSE_REP #2898

xenoterracide opened this issue Mar 11, 2024 · 4 comments

Comments

@xenoterracide
Copy link
Contributor

xenoterracide commented Mar 11, 2024

So this particularly error seems kind of ludicrous to me

Essentially this is only a security vulnerability if someone has added malicious code to your jvm. In most cases if that happens you're beyond screwed. For my circumstances I'm going to ignore this.

That said, I generally agree that it's not a great idea to expose mutable variables. However, I think that these aren't a problem because they aren't publicly mutable. You could only change them reflectively (which I'm allowing for hibernate). Essentially they're effectively final.

I think the best solution/compromise would be a rule like EI_EXPOSE_PUBLIC_REP, that is not flagged as security. Public mutability is just not a great idea. I would not ignore this since it's something I'd want flagged as bad code.

EI	com.xenoterracide.jpa.AbstractUuidEntityBase.getId() may expose internal representation by returning AbstractUuidEntityBase.id
[Bug type EI_EXPOSE_REP (click for details)](https://github.com/spotbugs/spotbugs/issues/new#EI_EXPOSE_REP)
In class com.xenoterracide.jpa.AbstractUuidEntityBase
In method com.xenoterracide.jpa.AbstractUuidEntityBase.getId()
Field com.xenoterracide.jpa.AbstractUuidEntityBase.id
At AbstractUuidEntityBase.java:[line 42]

I think I would argue protected methods too, but definitely package protected ones, especially if you can detect a module-info.class which would prevent any weird shenanigans on that.

Here's that class

// © Copyright 2024 Caleb Cushing
// SPDX-License-Identifier: AGPL-3.0-or-later

package com.xenoterracide.jpa;

import jakarta.persistence.EmbeddedId;
import jakarta.persistence.MappedSuperclass;
import java.util.Objects;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;

/**
 * The type Abstract uuid entity base.
 *
 * @param <T> the type parameter
 */
@MappedSuperclass
public abstract class AbstractUuidEntityBase<T extends AbstractUuidDomainId> implements Identifiable<@NonNull T> {

  /**
   * Surrogate Identifier.
   */
  private @NonNull T id;

  /**
   * NO-OP Parent Constuctor.
   */
  protected AbstractUuidEntityBase() {}

  /**
   * Instantiates a new Abstract uuid entity base.
   *
   * @param id the id
   */
  protected AbstractUuidEntityBase(@NonNull T id) {
    this.id = id;
  }

  @EmbeddedId
  @Override
  public @NonNull T getId() {
    return id;
  }

  /**
   * Sets id.
   *
   * @param id the id
   */
  @Initializer
  void setId(@NonNull T id) {
    this.id = id;
  }

  @Override
  public final int hashCode() {
    return Objects.hashCode(this.getId());
  }

  /**
   * Can equal boolean.
   *
   * @param that the other object to compare to
   * @return the boolean
   */
  protected abstract boolean canEqual(@NonNull AbstractUuidEntityBase<?> that);

  @Override
  public final boolean equals(@Nullable Object other) {
    if (other instanceof AbstractUuidEntityBase<?> that) {
      return that.canEqual(this) && Objects.equals(this.getId(), that.getId());
    }
    return false;
  }
}
// © Copyright 2024 Caleb Cushing
// SPDX-License-Identifier: AGPL-3.0-or-later

package com.xenoterracide.jpa;

import com.github.f4b6a3.uuid.UuidCreator;
import jakarta.persistence.Column;
import jakarta.persistence.Transient;
import java.io.Serial;
import java.io.Serializable;
import java.util.Objects;
import java.util.UUID;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;

/**
 * The type Abstract uuid domain id.
 */
public abstract class AbstractUuidDomainId implements Serializable {

  @Serial
  @Transient
  private static final long serialVersionUID = 1L;

  /**
   * The actual database UUID for id.
   */
  private UUID id = UuidCreator.getTimeOrderedEpoch();

  /**
   * NO-OP Parent Constuctor.
   */
  protected AbstractUuidDomainId() {}

  /**
   * Gets id.
   *
   * @return the id
   */
  @Column(name = "id", updatable = false, nullable = false)
  UUID getId() {
    return id;
  }

  /**
   * Sets id.
   *
   * @param id the id
   */
  @Initializer
  void setId(@NonNull UUID id) {
    this.id = id;
  }

  @Override
  public final int hashCode() {
    return Objects.hashCode(this.getId());
  }

  /**
   * Can equal boolean.
   *
   * @param that the other object
   * @return the boolean
   */
  protected abstract boolean canEqual(@NonNull AbstractUuidDomainId that);

  @Override
  public final boolean equals(@Nullable Object other) {
    if (other instanceof AbstractUuidDomainId that) {
      return that.canEqual(this) && Objects.equals(this.getId(), that.getId());
    }
    return false;
  }

  @Override
  public final String toString() {
    return this.id.toString();
  }
}
@ThrawnCA
Copy link
Contributor

Essentially this is only a security vulnerability if someone has added malicious code to your jvm.

That type of security concern tends to originate from the heyday of Java Applets.

@xenoterracide
Copy link
Contributor Author

Yeah, but there's also that one where You can execute arbitrary toad if you throw an exception in the constructor... Either way my point is that mutable objects are actually also a bad coding practice (arguably), and generally would be better to replace with builder, record, and or effectively final objects (kind of what I'm talking here). It occurs to me I didn't think to mention builder since the builder itself is usually mutable so that would have to be recognized in some way although you could just suppress the class I suppose...

@ThrawnCA
Copy link
Contributor

ThrawnCA commented Mar 13, 2024

You can execute arbitrary toad

https://content.api.news/v3/images/bin/8b236653aec2926939f9bf4c695efbda?width=1024 happens ;)

Either way my point is that mutable objects are actually also a bad coding practice (arguably)

That's rather beyond the scope of what SpotBugs will ever enforce.

@xenoterracide
Copy link
Contributor Author

Is it though? I mean it's something that spot bugs can do analysis on and already is. Really all I'm asking for is the ability to check but only check for public APIs. It's the same as this security vulnerability I just want to limit it to public APIs.

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

2 participants