Skip to content

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
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    2 participants