fix: rework FeatureExt generation #1718

arthurschreiber · 2025-12-20T07:36:29Z

Fixes Microsoft Fabric connection issues by correcting the LOGIN7 packet structure to comply with the MS-TDS specification (section 2.2.6.4).

The fabric login test was passing `isFabric: true` to Login7Payload constructor, but this parameter doesn't exist in the Options interface. The test was using `as any` to bypass TypeScript type checking. This parameter was originally planned as part of PR #1668 for Microsoft Fabric support, but was removed per David Engel's commit which showed that the Fabric-related changes work for all endpoints without needing a special flag. Cleanup from PR #1668 (feat: support Azure SQL Database in Microsoft Fabric).

Per the MS-TDS specification section 2.2.6.4, the LOGIN7 message structure must be: 1. Fixed header (36 bytes) 2. OffsetLength section (58 bytes) 3. Data section (variable-length content) 4. FeatureExt section (at the END, after all other variable data) The previous implementation incorrectly placed the FeatureExt data IN THE MIDDLE of the variable data section (after ServerName, before LibraryName). This caused Microsoft Fabric to reject the LOGIN7 packet since Fabric has stricter packet validation. Changes: - cbExtension now correctly set to 4 (just the ibFeatureExtLong pointer size), not 4 + extensions.length - ibFeatureExtLong now points to the actual end of the packet where FeatureExt data is placed - FeatureExt data is appended at the very end, after all other variable-length data (LibraryName, Language, Database, SSPI, AttachDbFile, ChangePassword) This should resolve connection issues with Microsoft Fabric (issue #1563).

claude · 2025-12-20T07:38:38Z

Code Review - PR #1718

Summary

This PR contains two commits that clean up and fix the LOGIN7 packet generation for Microsoft Fabric compatibility. The changes address issue #1563 where connections to Microsoft Fabric were failing with ECONNRESET errors.

✅ Strengths

Correct MS-TDS Specification Compliance (src/login7-payload.ts:254-387)
- The fix properly implements the LOGIN7 packet structure per MS-TDS spec section 2.2.6.4
- FeatureExt data is now correctly placed at the END of the packet, after all variable-length data
- Previously, FeatureExt was incorrectly inserted in the middle (after ServerName, before LibraryName), which caused Fabric to reject packets
Clear Documentation
- Excellent inline comments explaining the packet structure and why the changes are necessary
- Commit messages are detailed and reference the MS-TDS specification
Clean Test Cleanup (test/unit/login7-payload-test.ts:241-249)
- Removed the orphaned isFabric parameter that doesn't exist in the Options interface
- Removed unnecessary as any type assertion that was bypassing TypeScript checks
- This cleanup properly reflects the decisions made in PR feat: support Azure SQL Database in Microsoft Fabric #1668
Backward Compatibility
- The fix only affects TDS 7.4+ connections (line 263)
- Older TDS versions continue to use the existing code path

🔍 Potential Issues & Questions

Critical - Variable Data Offset Tracking

Location: src/login7-payload.ts:263-270, 338, 352-378

There's a potential bug with dataOffset tracking when sspi data is present:

// Line 338: SSPI buffer is added
buffers.push(this.sspi);
// BUT dataOffset is NOT incremented here!

// Later, at line 382-386, we calculate ibFeatureExtLong:
extensionOffsetBuffer.writeUInt32LE(dataOffset, 0);

Problem: If sspi data exists, it's added to the buffers array but dataOffset isn't incremented to account for its length. This means ibFeatureExtLong will point to the wrong location (it will overlap with SSPI data instead of pointing after it).

Impact: This could cause connection failures when using SSPI/Windows authentication with TDS 7.4+.

Suggested Fix:

if (this.sspi) {
  if (this.sspi.length > 65535) {
    offset = fixedData.writeUInt16LE(65535, offset);
  } else {
    offset = fixedData.writeUInt16LE(this.sspi.length, offset);
  }
  
  buffers.push(this.sspi);
  dataOffset += this.sspi.length;  // ADD THIS LINE
} else {
  offset = fixedData.writeUInt16LE(0, offset);
}

Question - Test Coverage

The existing test at line 267-287 calculates expectedLength which includes:

4 + // Extension offset
1 + (1 + 4 + 1) + (1 + 4 + 1) + 1; // Feature ext

Does this test actually verify that:

The ibFeatureExtLong offset pointer is correct?
The FeatureExt data appears at the expected position (end of packet)?

Suggestion: Consider adding an assertion that actually reads the offset value from the packet and verifies it points to the correct location.

Minor - cbExtension Comment Accuracy

Location: src/login7-payload.ts:259-260

The comment states:

"For TDS 7.4+, this is the size of the extension block (just the 4-byte offset pointer)."

However, according to MS-TDS spec, cbExtension should represent the size of the data at ibExtension, which is the 4-byte ibFeatureExtLong offset pointer. The comment is technically correct but could be clearer:

Suggested rewording:

// (cchUnused / cbExtension): 2-byte
// For TDS 7.4+, cbExtension is the size of the ibFeatureExtLong offset pointer (4 bytes).
// The actual FeatureExt data is appended at the end and pointed to by ibFeatureExtLong.

🧪 Test Coverage Assessment

Good:

Test correctly removes the invalid isFabric parameter
Test validates the packet length calculation

Missing:

No test verifying the ibFeatureExtLong offset value is correct
No test for SSPI + FeatureExt combination (which may expose the offset bug)
No integration test actually connecting to Microsoft Fabric

Recommendation: Add a test that:

Builds a LOGIN7 payload with FeatureExt
Parses the resulting buffer
Verifies ibFeatureExtLong points to the correct position
Verifies FeatureExt data is at that position

🔒 Security Considerations

No security issues identified. The changes:

Don't introduce injection vulnerabilities
Properly handle buffer sizes and offsets (except for the noted SSPI offset issue)
Don't expose sensitive data

⚡ Performance Considerations

Positive:

The deferred write pattern (allocating extensionOffsetBuffer and updating it later) is efficient
No unnecessary buffer copies

Neutral:

Minimal performance impact overall

📋 Recommendations

CRITICAL: Fix the dataOffset tracking for SSPI data (see issue above)
HIGH: Add test coverage for the offset pointer verification
MEDIUM: Clarify the cbExtension comment
LOW: Consider adding an integration test for Microsoft Fabric connections

✨ Overall Assessment

This is a well-researched fix that addresses a real compatibility issue with Microsoft Fabric. The approach is correct per the MS-TDS specification, and the code is well-documented. However, there appears to be a critical bug with dataOffset tracking when SSPI data is present that needs to be addressed before merging.

The test cleanup is appropriate and removes technical debt from PR #1668.

Recommendation: Request changes to fix the SSPI offset tracking issue before merging.

When SSPI data was added to the buffers array, dataOffset was not being incremented to account for its length. This caused incorrect offset calculations for subsequent fields (attachDbFile, changePassword) and the FeatureExt pointer. This bug was identified in the code review of PR #1718.

claude · 2025-12-20T09:10:03Z

Code Review - PR #1718: Fix FeatureExt Generation

Thank you for this comprehensive fix! This PR addresses a critical issue with Microsoft Fabric connections by correctly implementing the MS-TDS specification for LOGIN7 packet structure. I've conducted a thorough review of the changes.

Summary

This PR makes three important fixes:

Test cleanup: Removes the orphaned isFabric parameter that was bypassing TypeScript checks
Protocol compliance: Correctly places FeatureExt data at the END of LOGIN7 packets per MS-TDS spec 2.2.6.4
Bug fix: Increments dataOffset when SSPI data is added to prevent offset calculation errors

Detailed Analysis

✅ Strengths

Well-documented changes: The commit messages are excellent - they clearly explain the "why" behind each change with references to the MS-TDS specification.
Correct implementation: The LOGIN7 packet structure now properly follows the MS-TDS specification:
- Fixed header (36 bytes)
- OffsetLength section (58 bytes)
- Data section (variable-length content)
- FeatureExt section (at the END) ✓
Clean TypeScript fix: Removing the as any type assertion is a good practice - it surfaces actual type mismatches rather than hiding them.
Critical bug caught: The SSPI dataOffset increment fix (commit 3) is important - this would have caused incorrect offset calculations for:
- attachDbFile
- changePassword
- ibFeatureExtLong pointer

🔍 Issues & Concerns

1. Missing Test Coverage for SSPI + FeatureExt Combination

Issue: There's no test case that validates the combination of SSPI data with TDS 7.4+ (which includes FeatureExt).

Impact: The critical bug fix in commit 3 (SSPI dataOffset increment) would not be caught by the existing tests.

The existing tests:

test/unit/login7-payload-test.ts:86 - Tests SSPI with TDS 7.2 (no FeatureExt)
test/unit/login7-payload-test.ts:133 - Tests TDS 7.4 with FedAuth (no SSPI)
test/unit/login7-payload-test.ts:240 - Fabric test with FedAuth (no SSPI)

Recommendation: Add a test case for TDS 7.4+ with SSPI data to ensure the dataOffset increment fix works correctly.

2. Potential Edge Case: Empty FeatureExt

Code location: src/login7-payload.ts:264-270

if (this.tdsVersion >= versions['7_4']) {
  featureExtData = this.buildFeatureExt();
  // cbExtension = 4 (just the ibFeatureExtLong pointer size)
  offset = fixedData.writeUInt16LE(4, offset);
  // Reserve space for the 4-byte offset pointer
  extensionOffsetBuffer = Buffer.alloc(4);
  buffers.push(extensionOffsetBuffer);
  dataOffset += 4;
}

Question: What happens if buildFeatureExt() returns an empty buffer? Looking at src/login7-payload.ts:405-449, it seems buildFeatureExt() always returns at least the UTF8_SUPPORT feature (6 bytes) + terminator (1 byte) = minimum 7 bytes. This is fine, but it might be worth adding a comment to clarify this assumption.

3. Code Clarity: Variable Naming

Code location: src/login7-payload.ts:261-262

The variables featureExtData and extensionOffsetBuffer are only used when tdsVersion >= versions['7_4']. While the current implementation is correct, consider extracting the TDS 7.4+ logic into a helper function to reduce complexity in the toBuffer() method.

🔒 Security Assessment

✅ No security concerns identified

Buffer operations are safe (using Buffer.alloc())
No user-controlled data affects buffer sizes directly
Password scrambling remains intact
No SQL injection or XSS vectors introduced

⚡ Performance Considerations

✅ Performance impact is minimal

One additional Buffer.alloc(4) call for TDS 7.4+
The deferred writing of extensionOffsetBuffer is efficient
No unnecessary buffer copies

🧪 Test Coverage Recommendations

Add test case: SSPI + TDS 7.4 + FeatureExt

describe('for a login payload with SSPI data and TDS 7.4', function() {
  it('generates the expected data with correct offsets', function() {
    const payload = new Login7Payload({
      tdsVersion: 0x74000004,
      // ... other options
    });
    payload.sspi = Buffer.from([0xa0, 0xa1, 0xa2]);
    payload.fedAuth = { type: 'ADAL', echo: true, workflow: 'default' };
    // ... validate packet structure
  });
});

Add integration test: If possible, test against a real Microsoft Fabric instance to validate the fix for issue not able to connect to Microsoft Fabric #1563.

📋 Checklist

✅ Code follows TDS specification
✅ Removes bad TypeScript workarounds
✅ Fixes critical offset calculation bug
✅ Well-documented commits
⚠️ Test coverage could be improved (SSPI + TDS 7.4 case)
⚠️ Consider refactoring for better code organization

💭 Final Recommendation

APPROVE with minor suggestions

This is a solid fix that addresses a real protocol compliance issue. The implementation correctly follows the MS-TDS specification, and the bug fixes are important for correctness.

Suggested improvements before merge:

Add test case for SSPI + TDS 7.4 combination
Consider adding a comment in buildFeatureExt() documenting minimum buffer size
Optional: Extract TDS 7.4 extension logic into a helper method for clarity

The PR is ready to merge if the team is comfortable with current test coverage, though adding the SSPI + TDS 7.4 test would significantly increase confidence in the offset calculation fix.

Great work on identifying and fixing these issues! 🎉

Add two new tests to verify the LOGIN7 packet structure: 1. Verifies ibFeatureExtLong offset pointer points to valid FeatureExt data 2. Verifies FeatureExt is correctly positioned after SSPI data These tests ensure the FeatureExt data is placed at the END of the packet as required by the MS-TDS specification section 2.2.6.4.

claude · 2025-12-20T09:18:37Z

Pull Request Review - PR #1718: fix: rework FeatureExt generation

I've completed a comprehensive review of this PR. Overall, this is excellent work that properly fixes a critical issue with Microsoft Fabric connections. The implementation is well-thought-out and addresses a fundamental protocol-level problem.

✅ Strengths

1. Correct Implementation of MS-TDS Specification

The fix properly places FeatureExt data at the END of the LOGIN7 packet per MS-TDS spec section 2.2.6.4
The previous implementation incorrectly interleaved FeatureExt in the middle of variable data, which caused Microsoft Fabric to reject connections
Comments in the code clearly explain the spec requirements and structure

2. Critical Bug Fix: SSPI Offset Tracking

Great catch on line 339 (dataOffset += this.sspi.length;) - this was a genuine bug that would cause offset corruption when SSPI data is present
This bug would have affected attachDbFile, changePassword, and ibFeatureExtLong offsets
The fix properly tracks the data offset after SSPI is added

3. Excellent Test Coverage

Two comprehensive tests validate the fix:
- Test 1 (line 292): Verifies ibFeatureExtLong points to valid FeatureExt data and positioning is correct
- Test 2 (line 385): Validates proper positioning when SSPI data is present
Tests verify both the structure and the semantic correctness (FeatureExt terminator at end)
Tests check that FeatureExt comes after ALL variable data fields

4. Clean Code Quality

Removed the as any type bypass in the fabric test (line 246-249) - this is proper TypeScript hygiene
Clear variable naming (featureExtData, extensionOffsetBuffer)
Well-commented explaining the deferred write pattern

🔍 Code Quality Observations

Minor: Test Robustness

The test at line 357-362 calculates variableDataEnd using Math.max() but only checks a subset of fields:

const variableDataEnd = Math.max(
  ibHostName + cchHostName * 2,
  ibDatabase + cchDatabase * 2,
  ibAttachDBFile + cchAttachDBFile * 2,
  ibChangePassword + cchChangePassword * 2
);

Observation: This doesn't include all variable-length fields that appear before FeatureExt in the data section:

Missing: ibUserName, ibPassword, ibAppName, ibServerName, ibCltIntName, ibLanguage
Missing: ibSSPI (though it's 0 in this test case)

Impact: Low - the test still passes because the fields that ARE checked happen to come after the omitted ones in this specific test payload. However, for complete correctness, all variable fields should be included.

Suggestion: Consider adding all variable fields to ensure the test validates the true end of variable data:

const variableDataEnd = Math.max(
  ibHostName + cchHostName * 2,
  data.readUInt16LE(40) + data.readUInt16LE(42) * 2, // ibUserName + cchUserName * 2
  data.readUInt16LE(44) + data.readUInt16LE(46) * 2, // ibPassword + cchPassword * 2
  data.readUInt16LE(48) + data.readUInt16LE(50) * 2, // ibAppName + cchAppName * 2
  data.readUInt16LE(52) + data.readUInt16LE(54) * 2, // ibServerName + cchServerName * 2
  data.readUInt16LE(60) + data.readUInt16LE(62) * 2, // ibCltIntName + cchCltIntName * 2
  data.readUInt16LE(64) + data.readUInt16LE(66) * 2, // ibLanguage + cchLanguage * 2
  ibDatabase + cchDatabase * 2,
  data.readUInt16LE(78) + data.readUInt16LE(80), // ibSSPI + cbSSPI
  ibAttachDBFile + cchAttachDBFile * 2,
  ibChangePassword + cchChangePassword * 2
);

🔒 Security

✅ No security concerns identified

No injection vulnerabilities introduced
Buffer operations are safe (fixed-size allocations, proper offset tracking)
No sensitive data exposure

⚡ Performance

✅ Minimal performance impact

The deferred-write pattern (storing featureExtData and extensionOffsetBuffer, then appending at the end) is efficient
One additional buffer allocation (extensionOffsetBuffer = Buffer.alloc(4)) - negligible overhead
No unnecessary loops or redundant operations

🧪 Test Coverage

✅ Excellent test coverage

Tests verify packet structure per MS-TDS specification
Tests cover both with and without SSPI data
Tests validate semantic correctness (terminator position, feature IDs)
Edge cases covered (SSPI offset tracking)

Minor suggestion: Consider adding a test for the cbExtension value to ensure it's always 4 (not 4 + extensions.length as in the old code). The current test at line 324 does check this, which is good.

🐛 Potential Issues

None identified

The implementation correctly:

✅ Sets cbExtension to 4 (just the pointer size)
✅ Reserves space for ibFeatureExtLong in the buffer array
✅ Increments dataOffset properly for SSPI data
✅ Updates extensionOffsetBuffer with the final offset before concatenation
✅ Appends FeatureExt data at the very end

📋 Best Practices

✅ Follows repository conventions

Commit messages follow conventional format
Test structure matches existing test patterns
Code style is consistent with the codebase

✅ Documentation

Comments explain the MS-TDS spec requirements
Comments clarify the deferred-write pattern
Commit messages provide context and reference the spec

📝 Summary

This PR represents high-quality work that:

✅ Fixes a critical bug preventing Microsoft Fabric connections (issue not able to connect to Microsoft Fabric #1563)
✅ Fixes a hidden offset bug with SSPI data handling
✅ Implements the MS-TDS specification correctly
✅ Adds comprehensive test coverage
✅ Maintains backward compatibility for TDS < 7.4

Recommendation: ✅ APPROVE WITH MINOR SUGGESTION

The code is ready to merge. The only suggestion is to enhance the test robustness by including all variable fields in the variableDataEnd calculation, but this is a nice-to-have rather than a blocker.

Great work on this fix! This will enable Node.js applications to connect to Microsoft Fabric, which has been a pain point for the community for over a year.

Review conducted by Claude Code - analyzing PR #1718 in tediousjs/tedious

codecov · 2025-12-20T09:30:00Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.93%. Comparing base (d49317d) to head (8939da7).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1718      +/-   ##
==========================================
+ Coverage   79.92%   79.93%   +0.01%     
==========================================
  Files          90       90              
  Lines        4887     4890       +3     
  Branches      920      921       +1     
==========================================
+ Hits         3906     3909       +3     
  Misses        676      676              
  Partials      305      305

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Clarify cbExtension comment to explicitly state it's the size of the ibFeatureExtLong pointer (4 bytes), not the FeatureExt data - Extend test to validate all variable-length fields when checking FeatureExt positioning, ensuring complete coverage per MS-TDS spec

claude · 2025-12-20T09:35:18Z

Code Review - PR #1718: Fix FeatureExt Generation

Summary

This PR correctly addresses a technical debt issue from PR #1668 by removing the non-existent isFabric parameter and properly implementing the MS-TDS specification for FeatureExt data placement in Login7 packets.

Strengths

1. Correct Implementation of MS-TDS Specification

The change properly places FeatureExt data at the END of the packet per MS-TDS spec (src/login7-payload.ts:382-389)
The ibFeatureExtLong offset pointer is correctly written as a 4-byte value with cbExtension set to 4 bytes, not the FeatureExt data length (src/login7-payload.ts:265-271)
The offset is correctly updated after all variable-length data has been written (src/login7-payload.ts:386)

2. Bug Fix - SSPI dataOffset Tracking

Fixed a critical bug where dataOffset was not incremented after SSPI data (src/login7-payload.ts:340)
This ensures the FeatureExt offset calculation is correct when SSPI authentication is used
The added test case specifically validates this scenario (test/unit/login7-payload-test.ts:404-446)

3. Excellent Test Coverage

Two comprehensive test cases validate the MS-TDS spec compliance (test/unit/login7-payload-test.ts:291-456)
Tests verify: cbExtension is exactly 4 bytes, ibFeatureExtLong points to a valid position, FeatureExt data appears AFTER all variable-length fields, FeatureExt terminator (0xFF) is the last byte, and correct behavior with SSPI data present
Tests include detailed offset calculations checking all variable-length fields

4. Code Quality

Removed unsafe 'as any' type casting from tests (test/unit/login7-payload-test.ts:249)
Added clear, helpful comments explaining the MS-TDS spec requirements
Variable names are descriptive (featureExtData, extensionOffsetBuffer)

Observations and Suggestions

1. Comment Clarity (Minor)
Line src/login7-payload.ts:258 has a formatting issue - missing a forward slash at the start of the comment line.

2. Edge Case Documentation (Enhancement)
Consider documenting what happens if buildFeatureExt() returns an empty buffer or fails. Currently the code handles undefined checks well, but the behavior could be more explicitly documented.

3. Test Assertion Messages (Minor)
Some test assertions could benefit from more descriptive failure messages. The dynamic messages at test/unit/login7-payload-test.ts:376-377 are excellent - consider applying this pattern to other assertions.

Security Analysis

No security concerns identified:

Password scrambling remains unchanged and secure
No new attack surface introduced
Buffer operations are safe (using writeUInt32LE, Buffer.alloc, etc.)
No credential leakage risk

Performance Analysis

Minimal performance impact:

The change defers buffer concatenation slightly but doesn't add computational complexity
Buffer.alloc(4) is a trivial allocation
Overall O(n) complexity unchanged
The fix actually improves correctness without performance degradation

Test Coverage

Excellent coverage:

166 lines of new test code for 24 lines of production code
Tests validate both the fix and edge cases
Removed the problematic 'as any' workaround, demonstrating proper TypeScript usage

Recommendation

APPROVE - This is a well-implemented bug fix with excellent test coverage. The code correctly implements the MS-TDS specification and fixes a legitimate bug in the SSPI offset tracking.

Optional Follow-up

After merging, consider:

Fixing the comment formatting issue on line 258
Verifying this works with actual Microsoft Fabric connections (integration test)

Great work on cleaning up the technical debt and improving spec compliance!

claude · 2025-12-20T12:29:31Z

Pull Request Review: Fix FeatureExt Generation for Microsoft Fabric

Summary

This PR fixes Microsoft Fabric connection issues by correcting the LOGIN7 packet structure to comply with the MS-TDS specification (section 2.2.6.4). The key change is ensuring FeatureExt data is placed at the end of the packet, with ibFeatureExtLong pointing to it.

Code Quality ✅

Strengths:

Clear, well-commented code explaining the MS-TDS spec requirements
The deferred offset calculation pattern is correct and well-implemented
Comments at src/login7-payload.ts:254-261 clearly explain the two-phase approach
Variable naming (featureExtData, extensionOffsetBuffer) is descriptive

Minor Suggestion:

Consider extracting magic numbers into named constants:

const FEATURE_EXT_POINTER_SIZE = 4; // Size of ibFeatureExtLong
offset = fixedData.writeUInt16LE(FEATURE_EXT_POINTER_SIZE, offset);

Bug Analysis 🔍

Fixed Issue:
The original code incorrectly placed FeatureExt data immediately after the offset pointer, violating MS-TDS spec 2.2.6.4 which requires FeatureExt at the packet end.

Critical Bug Found - SSPI dataOffset tracking:
✅ FIXED in this PR - Line 340 now correctly updates dataOffset += this.sspi.length;. The previous code pushed SSPI to buffers but didn't increment dataOffset, causing ibFeatureExtLong to point incorrectly when SSPI was present. This is a critical fix verified by the new test at line 118-164.

Code Correctness:

✅ The logic correctly handles both TDS 7.4+ (with FeatureExt) and TDS < 7.4 (without)
✅ The offset calculation defers the actual offset write until all variable data is processed
✅ All variable-length fields are correctly accounted for before FeatureExt placement

Test Coverage ✅

Excellent test coverage:

Test 1 (lines 295-402): Comprehensive validation of FeatureExt positioning
- Verifies cbExtension is 4 (pointer size, not data size) ✅
- Validates ibFeatureExtLong points within packet bounds ✅
- Checks first feature ID is valid (FEDAUTH or UTF8_SUPPORT) ✅
- Critical: Verifies FeatureExt comes after all variable data by checking all field offsets ✅
- Validates terminator (0xFF) is the last byte ✅
Test 2 (lines 404-455): SSPI-specific test
- Tests the critical SSPI offset bug fix ✅
- Verifies FeatureExt placement when SSPI data is present ✅

Suggestions:

Add an edge case test for empty/minimal payload (no optional fields)
Consider testing with very long SSPI data (> 65535 bytes) to verify cbSSPILong interaction

Performance ⚡

No concerns:

Buffer allocation is efficient (single Buffer.concat() at the end)
The deferred offset write adds negligible overhead
No unnecessary copies or temporary allocations

Security 🔒

No security issues identified:

No buffer overflows (all writes use proper bounds)
No injection vulnerabilities
SSPI length is properly capped at 65535 (line 333-336)
Password scrambling remains unchanged

Specification Compliance 📋

MS-TDS Section 2.2.6.4 Compliance:

✅ ibExtension points to ibFeatureExtLong (4-byte offset)
✅ cbExtension is 4 (size of pointer, not data)
✅ FeatureExt data is at the end of the packet
✅ ibFeatureExtLong correctly points to FeatureExt data

Recommendations

Before Merge:

✅ Remove the as any cast from line 249 of the test (already done in this PR - good!)
Consider adding a brief comment at line 384 explaining why we use extensionOffsetBuffer.writeUInt32LE() instead of writing directly

Future Enhancements:

Add integration test with actual Microsoft Fabric endpoint (if feasible in CI)
Document the MS-TDS section 2.2.6.4 reference in code comments

Verdict: ✅ Approve with Minor Suggestions

This is a high-quality fix that:

Correctly implements the MS-TDS specification
Fixes the critical SSPI offset tracking bug
Includes comprehensive test coverage
Has clear, well-documented code

The minor suggestions are optional improvements. This PR is ready to merge.

Review generated with attention to MS-TDS specification compliance and TDS 7.4 protocol requirements.

claude added 2 commits December 19, 2025 23:35

arthurschreiber changed the title ~~test: remove orphaned isFabric parameter from fabric test~~ fix: rework FeatureExt generation Dec 20, 2025

arthurschreiber mentioned this pull request Dec 20, 2025

not able to connect to Microsoft Fabric #1563

Open

style: fix indentation in login7-payload tests

8939da7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: rework FeatureExt generation #1718

fix: rework FeatureExt generation #1718

arthurschreiber commented Dec 20, 2025 •

edited

Loading

Uh oh!

claude bot commented Dec 20, 2025

Uh oh!

claude bot commented Dec 20, 2025

Uh oh!

claude bot commented Dec 20, 2025

Uh oh!

codecov bot commented Dec 20, 2025 •

edited

Loading

Uh oh!

claude bot commented Dec 20, 2025

Uh oh!

claude bot commented Dec 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: rework FeatureExt generation #1718

Are you sure you want to change the base?

fix: rework FeatureExt generation #1718

Conversation

arthurschreiber commented Dec 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Dec 20, 2025

Code Review - PR #1718

Summary

✅ Strengths

🔍 Potential Issues & Questions

Critical - Variable Data Offset Tracking

Question - Test Coverage

Minor - cbExtension Comment Accuracy

🧪 Test Coverage Assessment

🔒 Security Considerations

⚡ Performance Considerations

📋 Recommendations

✨ Overall Assessment

Uh oh!

claude bot commented Dec 20, 2025

Code Review - PR #1718: Fix FeatureExt Generation

Summary

Detailed Analysis

✅ Strengths

🔍 Issues & Concerns

1. Missing Test Coverage for SSPI + FeatureExt Combination

2. Potential Edge Case: Empty FeatureExt

3. Code Clarity: Variable Naming

🔒 Security Assessment

⚡ Performance Considerations

🧪 Test Coverage Recommendations

📋 Checklist

💭 Final Recommendation

Uh oh!

claude bot commented Dec 20, 2025

Pull Request Review - PR #1718: fix: rework FeatureExt generation

✅ Strengths

1. Correct Implementation of MS-TDS Specification

2. Critical Bug Fix: SSPI Offset Tracking

3. Excellent Test Coverage

4. Clean Code Quality

🔍 Code Quality Observations

Minor: Test Robustness

🔒 Security

⚡ Performance

🧪 Test Coverage

🐛 Potential Issues

None identified

📋 Best Practices

📝 Summary

Recommendation: ✅ APPROVE WITH MINOR SUGGESTION

Uh oh!

codecov bot commented Dec 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude bot commented Dec 20, 2025

Code Review - PR #1718: Fix FeatureExt Generation

Summary

Strengths

Observations and Suggestions

Security Analysis

Performance Analysis

Test Coverage

Recommendation

Optional Follow-up

Uh oh!

claude bot commented Dec 20, 2025

Pull Request Review: Fix FeatureExt Generation for Microsoft Fabric

Summary

Code Quality ✅

Bug Analysis 🔍

Test Coverage ✅

Performance ⚡

Security 🔒

Specification Compliance 📋

Recommendations

Verdict: ✅ Approve with Minor Suggestions

arthurschreiber commented Dec 20, 2025 •

edited

Loading

codecov bot commented Dec 20, 2025 •

edited

Loading