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

[java] Add nullness for virtualauthenticator #15082

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Jan 14, 2025

User description

Description

In this PR I'm adding nullness annotations for classes:

  • org.openqa.selenium.virtualauthenticator.Credential
  • org.openqa.selenium.virtualauthenticator.HasVirtualAuthenticator
  • org.openqa.selenium.virtualauthenticator.VirtualAuthenticator
  • org.openqa.selenium.virtualauthenticator.VirtualAuthenticatorOptions

NullAway analysis: #14421

Motivation and Context

The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions.
Related issue: #14291

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement


Description

  • Added JSpecify @NullMarked annotations to virtual authenticator classes.

  • Introduced @Nullable annotations for nullable fields and methods.

  • Enhanced null-safety for Credential class with nullable user handle.

  • Improved IDE/static analyzer support for nullness checks.


Changes walkthrough 📝

Relevant files
Enhancement
Credential.java
Add nullness annotations to `Credential` class                     

java/src/org/openqa/selenium/virtualauthenticator/Credential.java

  • Added @NullMarked annotation to the class.
  • Marked userHandle field and related methods as @Nullable.
  • Enhanced null-safety in constructor and getter methods.
  • +6/-3     
    HasVirtualAuthenticator.java
    Add nullness annotations to `HasVirtualAuthenticator` interface

    java/src/org/openqa/selenium/virtualauthenticator/HasVirtualAuthenticator.java

    • Added @NullMarked annotation to the interface.
    +3/-0     
    VirtualAuthenticator.java
    Add nullness annotations to `VirtualAuthenticator` interface

    java/src/org/openqa/selenium/virtualauthenticator/VirtualAuthenticator.java

    • Added @NullMarked annotation to the interface.
    +2/-0     
    VirtualAuthenticatorOptions.java
    Add nullness annotations to `VirtualAuthenticatorOptions` class

    java/src/org/openqa/selenium/virtualauthenticator/VirtualAuthenticatorOptions.java

    • Added @NullMarked annotation to the class.
    +2/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14291 - PR Code Verified

    Compliant requirements:

    • Add JSpecify Nullness annotations to Selenium framework code
    • Annotations should specify which parameters and return values can be null
    • Improve IDE and static code analyzer support for null checks

    Requires further human verification:

    • Enhance interoperability with Kotlin (needs testing with Kotlin codebase)
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Safety

    Verify that the null handling in getUserHandle() method is correct and consistent with the class's usage patterns

    public byte @Nullable [] getUserHandle() {
      return userHandle == null ? null : Arrays.copyOf(userHandle, userHandle.length);
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add defensive copying for nullable array parameter in constructor to prevent potential null pointer exceptions

    Add a null check for the userHandle parameter in the constructor to prevent
    potential NPE when copying the array. While the field is marked @nullable, defensive
    copying should handle null values explicitly.

    java/src/org/openqa/selenium/virtualauthenticator/Credential.java [76-85]

     private Credential(
         byte[] id,
         boolean isResidentCredential,
         String rpId,
         PKCS8EncodedKeySpec privateKey,
         byte @Nullable [] userHandle,
         int signCount) {
       this.id = Require.nonNull("Id", id);
       this.isResidentCredential = isResidentCredential;
       this.rpId = rpId;
    +  this.userHandle = userHandle != null ? Arrays.copyOf(userHandle, userHandle.length) : null;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential null pointer exception and security concern by implementing defensive copying of the nullable array parameter, which is important for maintaining object immutability and preventing external modifications to internal state.

    8

    @mk868
    Copy link
    Contributor Author

    mk868 commented Jan 14, 2025

    This PR solves the following error:

    java/src/org/openqa/selenium/virtualauthenticator/Credential.java:46: error: [NullAway] passing @Nullable parameter 'null' where @NonNull is required
        return new Credential(id, false, Require.nonNull("rpId", rpId), privateKey, null, signCount);
                                                                                    ^
        (see http://t.uber.com/nullaway )
    

    There are 3 more problems in Credential#fromMap(Map<String, Object>) which will be solved in a dedicated PR

    @diemol diemol merged commit 83d5265 into SeleniumHQ:trunk Jan 15, 2025
    33 of 34 checks passed
    @mk868 mk868 deleted the jspecify-virtualauthenticator branch January 15, 2025 16:26
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants