Implement IP address encryption for Dans-Plugins/AlternateAccountFinder/issues/45 by Copilot · Pull Request #58 · Dans-Plugins/AlternateAccountFinder

Copilot · 2025-09-21T16:08:03Z

Latest Updates (addressing code review):

Enhanced documentation explaining ECB mode tradeoffs and key management
Key file now stored in plugin data folder with restrictive permissions
Added prominent startup warning about key backup importance
Robust migration detection using decrypt-based approach (not heuristics)
All migrations wrapped in database transaction for atomicity
Fixed NullPointerException in getLoginCount
Better error handling for corrupted encrypted data
Input validation on all encrypt/decrypt operations
Improved error messages without exposing sensitive data

Implementation Details:

Uses AES/ECB/PKCS5Padding encryption with persistent key storage
Encryption key automatically generated and stored in plugin data folder
All IP storage methods updated with encryption/decryption
Deterministic encryption ensures same IPs encrypt to same values for comparison
Automatic migration of existing plaintext IPs on first startup
Comprehensive error handling and logging

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

dmccoystephenson · 2025-09-21T17:50:20Z

@copilot Try again

dmccoystephenson · 2025-09-24T03:51:35Z

@copilot Continue your work

Copilot · 2025-09-24T03:53:18Z

@copilot Continue your work

I'll continue the IP encryption implementation. I need to complete the LoginRepository integration since the encryption utility was created but never integrated into the repository layer.

Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot

Pull request overview

This PR implements IP address encryption to protect user privacy by encrypting IP addresses before storing them in the database. The implementation adds AES encryption with deterministic properties to enable alternate account detection while securing sensitive data.

Changes:

Added IpEncryption utility class with AES/ECB encryption for deterministic IP address encryption
Modified LoginRepository to encrypt IPs on write and decrypt on read operations
Added application-level migration logic to convert existing plaintext IPs to encrypted format
Updated database schema to increase address column size from VARCHAR(64) to VARCHAR(255)

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 23 comments.

File	Description
`IpEncryption.java`	New encryption utility implementing AES encryption with persistent key storage
`LoginRepository.java`	Updated to use IpEncryption for all IP address read/write operations
`AlternateAccountFinder.java`	Added migration logic and IpEncryption initialization during plugin startup
`V2__Encrypt_existing_ip_addresses.sql`	Schema migration to accommodate larger encrypted address values

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-02T00:57:30Z

src/main/java/com/dansplugins/detectionsystem/encryption/IpEncryption.java

+    public IpEncryption(Logger logger) {
+        this.logger = logger;
+        this.secretKey = getOrCreateKey();
+    }


If the encryption key is lost, rotated, or becomes corrupted, all existing encrypted IP addresses in the database will become permanently undecryptable. This will break the plugin's functionality. Consider implementing a key rotation mechanism or at minimum adding comprehensive documentation warning administrators about the critical importance of backing up the encryption key file. You might also want to log a prominent warning on startup about key backup.

Copilot · 2026-02-02T00:57:30Z

...resources/com/dansplugins/detectionsystem/db/migration/V2__Encrypt_existing_ip_addresses.sql

+-- Migration to handle IP address encryption
+-- This migration will be handled by the application code during startup
+-- to encrypt existing plaintext IP addresses in the database
+
+-- Increase the size of the address column to accommodate encrypted values
+-- Encrypted Base64 strings will be longer than IP addresses
+ALTER TABLE aaf_login_record ALTER COLUMN address VARCHAR(255);


The SQL migration file only modifies the schema but doesn't actually perform any data migration. The comment states "This migration will be handled by the application code during startup," but Flyway migrations should be complete and idempotent. The current approach creates a disconnect between the Flyway migration version and the actual data migration state. If the application fails to start or the Java migration fails, the Flyway schema will be marked as migrated to V2 even though the data wasn't migrated. Consider either:

Making V2 a placeholder with a clear marker that data migration happens in application code

Implementing the data migration directly in SQL (though this would require SQL-based encryption which isn't ideal)

Using a separate application-level migration tracking mechanism

Copilot · 2026-02-02T00:57:30Z

src/main/java/com/dansplugins/detectionsystem/encryption/IpEncryption.java

+            byte[] decrypted = cipher.doFinal(decoded);
+            return new String(decrypted, StandardCharsets.UTF_8);
+        } catch (Exception e) {
+            logger.severe("Failed to decrypt IP address: " + e.getMessage());


Similarly, the decrypt method's error logging could potentially expose sensitive information through exception messages. The same concerns about logging sensitive data apply here.

Suggested change

logger.severe("Failed to decrypt IP address: " + e.getMessage());

logger.severe("Failed to decrypt IP address.");

Copilot · 2026-02-02T00:57:31Z

src/main/java/com/dansplugins/detectionsystem/encryption/IpEncryption.java

+            Cipher cipher = Cipher.getInstance(TRANSFORMATION);
+            cipher.init(Cipher.ENCRYPT_MODE, secretKey);


Creating a new Cipher instance on every encrypt/decrypt operation is inefficient. Cipher initialization is computationally expensive. Consider using ThreadLocal to cache Cipher instances per thread, or use a cipher pool. This will significantly improve performance, especially under load when many players are joining simultaneously.

Copilot · 2026-02-02T00:57:31Z

src/main/java/com/dansplugins/detectionsystem/AlternateAccountFinder.java

+            // Check if migration is needed by looking for IP addresses that don't look like Base64
+            // Base64 strings will be much longer and contain only valid Base64 characters
+            int plaintextCount = dsl.selectCount()
+                    .from(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)
+                    .where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.notLike("%=%"))
+                    .and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.length().lessThan(50))
+                    .fetchOne(0, int.class);
+
+            if (plaintextCount > 0) {
+                getLogger().info("Found " + plaintextCount + " plaintext IP addresses to encrypt");
+
+                // Fetch all records with plaintext IPs
+                var records = dsl.selectFrom(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)
+                        .where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.notLike("%=%"))
+                        .and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.length().lessThan(50))
+                        .fetch();
+
+                int migrated = 0;
+                for (var record : records) {
+                    try {
+                        String plaintextIp = record.getAddress();
+                        String encryptedIp = ipEncryption.encrypt(plaintextIp);
+
+                        // Update the record with encrypted IP
+                        dsl.update(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)
+                                .set(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS, encryptedIp)
+                                .where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.MINECRAFT_UUID.eq(record.getMinecraftUuid()))
+                                .and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.eq(plaintextIp))
+                                .execute();
+
+                        migrated++;
+                    } catch (Exception e) {
+                        getLogger().warning("Failed to encrypt IP for record: " + e.getMessage());
+                    }
+                }
+


The migration query conditions could incorrectly identify already-encrypted data as plaintext in edge cases. For example:

Base64 strings without padding (when length is divisible by 3) won't contain '='

IPv6 addresses can be much longer than typical IPv4 addresses and might exceed 50 characters in some representations

If someone manually enters non-IP data, it could match these heuristics

A more reliable approach would be to add a 'migration_version' column to track which encryption version is used, or attempt to decrypt and catch exceptions to determine the current state.

Suggested change

// Check if migration is needed by looking for IP addresses that don't look like Base64

// Base64 strings will be much longer and contain only valid Base64 characters

int plaintextCount = dsl.selectCount()

.from(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)

.where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.notLike("%=%"))

.and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.length().lessThan(50))

.fetchOne(0, int.class);

if (plaintextCount > 0) {

getLogger().info("Found " + plaintextCount + " plaintext IP addresses to encrypt");

// Fetch all records with plaintext IPs

var records = dsl.selectFrom(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)

.where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.notLike("%=%"))

.and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.length().lessThan(50))

.fetch();

int migrated = 0;

for (var record : records) {

try {

String plaintextIp = record.getAddress();

String encryptedIp = ipEncryption.encrypt(plaintextIp);

// Update the record with encrypted IP

dsl.update(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)

.set(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS, encryptedIp)

.where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.MINECRAFT_UUID.eq(record.getMinecraftUuid()))

.and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.eq(plaintextIp))

.execute();

migrated++;

} catch (Exception e) {

getLogger().warning("Failed to encrypt IP for record: " + e.getMessage());

}

}

// Fetch all records and determine per-record whether migration is needed

var records = dsl.selectFrom(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)

.fetch();

int migrated = 0;

for (var record : records) {

String currentAddress = record.getAddress();

if (currentAddress == null || currentAddress.isEmpty()) {

continue;

}

boolean alreadyEncrypted = false;

try {

// If decryption succeeds without throwing, assume the value is already encrypted

ipEncryption.decrypt(currentAddress);

alreadyEncrypted = true;

} catch (Exception ignored) {

// Decryption failed – treat as plaintext that needs encryption

}

if (alreadyEncrypted) {

continue;

}

try {

String encryptedIp = ipEncryption.encrypt(currentAddress);

// Update the record with encrypted IP

dsl.update(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)

.set(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS, encryptedIp)

.where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.MINECRAFT_UUID.eq(record.getMinecraftUuid()))

.and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.eq(currentAddress))

.execute();

migrated++;

} catch (Exception e) {

getLogger().warning("Failed to encrypt IP for record: " + e.getMessage());

}

}

if (migrated > 0) {

Copilot · 2026-02-02T00:57:34Z

src/main/java/com/dansplugins/detectionsystem/AlternateAccountFinder.java

+                    } catch (Exception e) {
+                        getLogger().warning("Failed to encrypt IP for record: " + e.getMessage());
+                    }
+                }


The exception handling in the migration swallows individual record failures and continues processing. While this prevents complete migration failure, it means some records might remain unencrypted without clear indication of which ones failed. Consider collecting failed records and reporting them in aggregate at the end, or maintaining a detailed log of which minecraft_uuid/address combinations failed to encrypt. This would allow administrators to identify and manually fix problematic records.

Copilot · 2026-02-02T00:57:35Z

src/main/java/com/dansplugins/detectionsystem/AlternateAccountFinder.java

+            int plaintextCount = dsl.selectCount()
+                    .from(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)
+                    .where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.notLike("%=%"))
+                    .and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.length().lessThan(50))
+                    .fetchOne(0, int.class);


The variable name 'plaintextCount' suggests it's counting plaintext records, but the query conditions (notLike '%=' and length < 50) are heuristics that may not accurately identify plaintext vs encrypted data. Consider renaming to 'potentialPlaintextCount' or 'unmigrated RecordsCount' to better reflect the uncertainty in the detection.

Copilot · 2026-02-02T00:57:35Z

src/main/java/com/dansplugins/detectionsystem/encryption/IpEncryption.java

+     * @param ipAddress The IP address to encrypt
+     * @return Base64 encoded encrypted IP address
+     */
+    public String encrypt(String ipAddress) {


The encrypt method doesn't validate the input. While IP addresses from InetAddress.getHostAddress() should be safe, the method is public and could theoretically be called with null or invalid input. Consider adding null checks and input validation to make the API more robust. At minimum, add a null check to fail fast with a clear error message rather than deep in the encryption logic.

Suggested change

public String encrypt(String ipAddress) {

public String encrypt(String ipAddress) {

if (ipAddress == null || ipAddress.trim().isEmpty()) {

throw new IllegalArgumentException("IP address to encrypt must not be null or empty");

}

Copilot · 2026-02-02T00:57:35Z

src/main/java/com/dansplugins/detectionsystem/AlternateAccountFinder.java

+    private void migrateExistingIpAddresses(DSLContext dsl, IpEncryption ipEncryption) {
+        try {
+            // Check if migration is needed by looking for IP addresses that don't look like Base64
+            // Base64 strings will be much longer and contain only valid Base64 characters
+            int plaintextCount = dsl.selectCount()
+                    .from(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)
+                    .where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.notLike("%=%"))
+                    .and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.length().lessThan(50))
+                    .fetchOne(0, int.class);
+
+            if (plaintextCount > 0) {
+                getLogger().info("Found " + plaintextCount + " plaintext IP addresses to encrypt");
+
+                // Fetch all records with plaintext IPs
+                var records = dsl.selectFrom(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)
+                        .where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.notLike("%=%"))
+                        .and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.length().lessThan(50))
+                        .fetch();
+
+                int migrated = 0;
+                for (var record : records) {
+                    try {
+                        String plaintextIp = record.getAddress();
+                        String encryptedIp = ipEncryption.encrypt(plaintextIp);
+
+                        // Update the record with encrypted IP
+                        dsl.update(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD)
+                                .set(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS, encryptedIp)
+                                .where(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.MINECRAFT_UUID.eq(record.getMinecraftUuid()))
+                                .and(com.dansplugins.detectionsystem.jooq.Tables.AAF_LOGIN_RECORD.ADDRESS.eq(plaintextIp))
+                                .execute();
+
+                        migrated++;
+                    } catch (Exception e) {
+                        getLogger().warning("Failed to encrypt IP for record: " + e.getMessage());
+                    }
+                }
+
+                getLogger().info("Successfully migrated " + migrated + " IP addresses to encrypted format");
+            } else {
+                getLogger().info("No plaintext IP addresses found - migration not needed");
+            }


If the migration runs multiple times (e.g., after a failed migration or server restart), the heuristics used to detect plaintext IPs could potentially try to re-encrypt already encrypted data. While the check for '=' and length < 50 provides some protection, it's not foolproof. Base64 strings without padding (when plaintext length is a multiple of 3) won't contain '=' and could be re-encrypted. Consider adding a one-time migration flag to the database (e.g., a migration_status table) to ensure the migration only runs once, even if the heuristics would incorrectly identify some records.

Copilot · 2026-02-02T00:57:35Z

src/main/java/com/dansplugins/detectionsystem/encryption/IpEncryption.java

+public final class IpEncryption {
+
+    private static final String ALGORITHM = "AES";
+    private static final String TRANSFORMATION = "AES/ECB/PKCS5Padding";


Using AES in ECB mode is a significant security weakness. ECB mode does not provide semantic security and reveals patterns in encrypted data. While deterministic encryption is needed for database lookups, a better approach would be to use AES-SIV (Synthetic IV) mode or to implement a keyed hash function (HMAC) for indexing purposes while storing the actual encrypted data with a secure mode like AES-GCM. If AES-SIV is not available, consider using AES in CBC mode with a deterministic IV derived from the plaintext (though this still has limitations).

dmccoystephenson · 2026-02-03T01:08:48Z

@copilot address comments

…d error handling - Add comprehensive documentation explaining ECB mode security tradeoffs and key management - Use plugin data folder for key file instead of hardcoded path - Add prominent warning on startup about key backup importance - Set restrictive file permissions (600) on encryption key file - Add input validation for encrypt/decrypt methods - Add isEncrypted() method for reliable plaintext vs encrypted detection - Improve migration logic to use decrypt-based detection instead of heuristics - Wrap migration in database transaction for atomicity - Fix NullPointerException in getLoginCount when no record found - Improve error handling in getAccountInfo for corrupted data - Better error logging without exposing sensitive IP addresses - Fail startup with clear error if key file is corrupted - Collect and report all failed migrations instead of silent failures Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot · 2026-02-03T01:12:03Z

@copilot address comments

I've addressed the critical code review feedback in commit 3f24094:

Key improvements:

✅ Enhanced documentation explaining ECB mode security tradeoffs and key management importance
✅ Key file now stored in plugin data folder with restrictive permissions (600)
✅ Added prominent startup warning about key backup criticality
✅ Robust migration using decrypt-based detection instead of fragile heuristics
✅ All migrations wrapped in database transaction for atomicity
✅ Fixed NullPointerException in getLoginCount (returns 0 when no record found)
✅ Better error handling for corrupted encrypted data
✅ Input validation on all encrypt/decrypt operations
✅ Improved error logging without exposing sensitive IP addresses
✅ Fail startup with clear error if key file is corrupted
✅ Collect and report all failed migrations for manual intervention

The implementation now has stronger security, better error handling, and clearer warnings about the critical nature of the encryption key backup.

Initial plan

05b5587

Copilot AI assigned Copilot and dmccoystephenson Sep 21, 2025

Copilot started work on behalf of dmccoystephenson September 21, 2025 16:08 View session

Changes before error encountered

2b84f00

Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot AI requested a review from dmccoystephenson September 21, 2025 16:13

Copilot started work on behalf of dmccoystephenson September 21, 2025 17:50 View session

Copilot started work on behalf of dmccoystephenson September 24, 2025 03:51 View session

Copilot AI and others added 2 commits September 24, 2025 03:56

Complete IP address encryption implementation

da75f17

Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Remove accidentally committed class file

1e74f99

Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] @Dans-Plugins/AlternateAccountFinder/issues/45~~ Implement IP address encryption for Dans-Plugins/AlternateAccountFinder/issues/45 Sep 24, 2025

Copilot finished work on behalf of dmccoystephenson September 24, 2025 03:57

dmccoystephenson requested a review from Copilot February 2, 2026 00:48

Copilot started reviewing on behalf of dmccoystephenson February 2, 2026 00:48 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

Copilot started work on behalf of dmccoystephenson February 3, 2026 01:09 View session

Copilot finished work on behalf of dmccoystephenson February 3, 2026 01:12

	logger.severe("Failed to decrypt IP address: " + e.getMessage());
	logger.severe("Failed to decrypt IP address.");

		Cipher cipher = Cipher.getInstance(TRANSFORMATION);
		cipher.init(Cipher.ENCRYPT_MODE, secretKey);

Conversation

Copilot AI commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmccoystephenson commented Sep 21, 2025

Uh oh!

dmccoystephenson commented Sep 24, 2025

Uh oh!

Copilot AI commented Sep 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

dmccoystephenson commented Feb 3, 2026

Uh oh!

Copilot AI commented Feb 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Sep 21, 2025 •

edited

Loading