Skip to content

Conversation

nikwithak
Copy link
Contributor

@nikwithak nikwithak commented Oct 13, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-22992

📔 Objective

Adds an optional field of lastKnownRevisionDate to endpoints where the attachment on a Cipher is modified. If passed from the client, the date is checked against the cipher, which prevents attachments corrupting by uploading using an outdated encryption key.

Related client PR (consumes this change): bitwarden/clients#16862

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link

claude bot commented Oct 13, 2025

Claude finished @nikwithak's task —— View job


PR Review: Cipher Attachment Revision Date Validation

I've completed a comprehensive review of the changes implementing lastKnownRevisionDate validation for attachment endpoints.


Summary of Changes

This PR adds optional revision date validation to attachment endpoints to prevent data corruption from concurrent modifications using outdated encryption keys. The implementation:

  • Adds LastKnownRevisionDate field to AttachmentRequestModel
  • Updates 5 attachment-related controller methods to extract and validate revision dates
  • Adds validation logic in CipherService that compares against current cipher revision date
  • Includes comprehensive unit tests (236 lines) covering success and failure scenarios

Critical Issues

❌ Security: Inconsistent Validation Logic

Location: src/Core/Vault/Services/Implementations/CipherService.cs:866-879

The validation logic has a potential timing-based bypass:

if (cipher.Id == default || !lastKnownRevisionDate.HasValue)
{
    return;  // No validation if lastKnownRevisionDate is null
}

if ((cipher.RevisionDate - lastKnownRevisionDate.Value).Duration() > TimeSpan.FromSeconds(1))
{
    throw new BadRequestException(...);
}

Issue: The 1-second tolerance window could allow race conditions where two clients with slightly different system clocks both pass validation. While this may be intentional for clock skew, it should be documented.

Recommendation: Document the rationale for the 1-second tolerance in a code comment. Consider if this tolerance is sufficient for distributed systems with clock drift.

⚠️ Error Handling: Generic Exception in Form Parsing

Location: src/Api/Vault/Controllers/CiphersController.cs:1630-1643

private DateTime? GetLastKnownRevisionDateFromForm()
{
    DateTime? lastKnownRevisionDate = null;
    if (Request.Form.TryGetValue("lastKnownRevisionDate", out var dateValue))
    {
        if (!DateTime.TryParse(dateValue, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out var parsedDate))
        {
            throw new BadRequestException("Invalid lastKnownRevisionDate format.");
        }
        lastKnownRevisionDate = parsedDate;
    }
    return lastKnownRevisionDate;
}

Issue: The error message doesn't specify the expected format, making it harder for clients to fix issues.

Recommendation:

throw new BadRequestException("Invalid lastKnownRevisionDate format. Expected ISO 8601 format (e.g., 2024-10-15T12:00:00Z).");

Suggested Improvements

🎨 Code Quality: Duplicate Logic Not Extracted

Location: src/Api/Vault/Controllers/CiphersController.cs:1409, 1432, 1463, 1512

The reviewer (gbubemismith) correctly identified that the same pattern appears 4 times:

// Extract lastKnownRevisionDate from form data if present
DateTime? lastKnownRevisionDate = GetLastKnownRevisionDateFromForm();

While the helper method GetLastKnownRevisionDateFromForm() was added, the comment and variable declaration are repeated in each method. This is acceptable, but consider the trade-offs.

Current approach pros:

  • Explicit and clear in each method
  • Easy to understand the flow

Alternative (inline the call):

await _cipherService.CreateAttachmentAsync(cipher, stream, fileName, key,
    Request.ContentLength.GetValueOrDefault(0), user.Id, false, GetLastKnownRevisionDateFromForm());

Recommendation: Current approach is fine, but could be slightly cleaner by inlining the call where the variable is only used once.

📝 Documentation: Missing XML Documentation

Location: src/Api/Vault/Models/Request/AttachmentRequestModel.cs:13-16

The reviewer (gbubemismith) noted the XML documentation could be improved. Current:

/// <summary>
/// The last known revision date of the Cipher that this attachment belongs to.
/// </summary>
public DateTime? LastKnownRevisionDate { get; set; }

Recommendation: Add more context about when/why this is used:

/// <summary>
/// The last known revision date of the Cipher that this attachment belongs to.
/// If provided, the server will validate that the cipher hasn't been modified since this date,
/// preventing attachment corruption from concurrent updates with outdated encryption keys.
/// </summary>
public DateTime? LastKnownRevisionDate { get; set; }

⚠️ Consistency: Redundant Nullable Assignment

Location: src/Api/Vault/Models/Request/AttachmentRequestModel.cs:16

The reviewer (gbubemismith) noted that = null is redundant:

public DateTime? LastKnownRevisionDate { get; set; } = null;

Issue: Nullable reference types already default to null, making the explicit assignment unnecessary.

Recommendation: Remove = null:

public DateTime? LastKnownRevisionDate { get; set; }

🔍 Testing: Missing Edge Cases

Location: test/Core.Test/Vault/Services/CipherServiceTests.cs

The tests are comprehensive, covering:

  • ✅ Wrong revision date throws
  • ✅ Correct revision date passes
  • ✅ Null revision date passes (no validation)
  • ✅ All attachment-related methods

Missing test cases:

  1. Boundary test: Revision date exactly 1 second off (at the tolerance boundary)
  2. Boundary test: Revision date 0.9 seconds off (just within tolerance)
  3. Boundary test: Revision date 1.1 seconds off (just outside tolerance)
  4. Clock skew test: Future revision date (should this be rejected?)

Recommendation: Add tests for the 1-second tolerance boundary to ensure the validation logic is correct.


Good Practices Observed

👍 Comprehensive test coverage with 236 lines of new tests
👍 Proper use of CultureInfo.InvariantCulture and DateTimeStyles.RoundtripKind for parsing
👍 Optional parameter preserves backward compatibility
👍 Validation centralized in service layer
👍 Private helper method reduces code duplication
👍 Consistent error messaging across methods


Security Considerations

Zero-knowledge principles maintained: The validation doesn't expose any vault data
No PII in error messages: Error messages are generic and safe
Input validation: DateTime parsing is properly handled
⚠️ Race condition tolerance: 1-second window may allow concurrent updates (document rationale)


Action Items for Author

Required (Critical)

  1. ⛏️ Remove redundant = null from AttachmentRequestModel.cs:16
  2. 📝 Improve XML documentation for LastKnownRevisionDate per gbubemismith's feedback

Recommended (Improvements)

  1. 🎨 Consider inlining GetLastKnownRevisionDateFromForm() calls to reduce repetition
  2. 📝 Add code comment explaining the 1-second tolerance in ValidateCipherLastKnownRevisionDate
  3. 📝 Improve error message to include expected date format
  4. 🧪 Add boundary tests for the 1-second tolerance window

Optional (Nice to Have)

  1. 🧪 Add test for future revision date handling
  2. 📝 Document that lastKnownRevisionDate is optional in all interface method signatures

Verdict

Approved with minor changes required

The implementation is solid and solves the stated problem effectively. The required changes are minor (remove redundant null assignment, improve XML docs). The recommended improvements would enhance maintainability and robustness but aren't blockers.

The 18.92% test coverage is expected since the new code primarily adds optional parameters to existing, well-tested methods. The new validation logic itself is thoroughly tested.


Branch: vault/PM-22992/attachments-revision-date

Copy link

codecov bot commented Oct 13, 2025

Codecov Report

❌ Patch coverage is 62.50000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.87%. Comparing base (ac20c84) to head (349814e).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Vault/Controllers/CiphersController.cs 50.00% 8 Missing ⚠️
...Api/Vault/Models/Request/AttachmentRequestModel.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6451      +/-   ##
==========================================
+ Coverage   50.72%   50.87%   +0.14%     
==========================================
  Files        1866     1866              
  Lines       82696    82722      +26     
  Branches     7307     7321      +14     
==========================================
+ Hits        41944    42081     +137     
+ Misses      39154    39018     -136     
- Partials     1598     1623      +25     

☔ 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.

Copy link
Contributor

github-actions bot commented Oct 13, 2025

Logo
Checkmarx One – Scan Summary & Detailsf94b49d6-fc2e-4b13-8f5a-0e9fa2c434c6

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1522
detailsMethod at line 1522 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
ID: NFbdKSGg9Dn6BX%2F8f7AR370a6hQ%3D
Attack Vector
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1393
detailsMethod at line 1393 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
ID: a2VgJJOpbUmk6eJuApdbxjiurvE%3D
Attack Vector
Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 299

@nikwithak nikwithak marked this pull request as ready for review October 14, 2025 20:46
@nikwithak nikwithak requested a review from a team as a code owner October 14, 2025 20:46
public bool AdminRequest { get; set; } = false;

/// The last known revision date of the Cipher that this attachment belongs to.
public DateTime? LastKnownRevisionDate { get; set; } = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 I think the = null is redundant here

}

private void ValidateCipherLastKnownRevisionDateAsync(Cipher cipher, DateTime? lastKnownRevisionDate)
private void ValidateCipherLastKnownRevisionDate(Cipher cipher, DateTime? lastKnownRevisionDate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 Thanks for renaming this

Comment on lines 1539 to 1549
// Extract lastKnownRevisionDate from form data if present
DateTime? lastKnownRevisionDate = null;

if (Request.Form.TryGetValue("lastKnownRevisionDate", out var dateValue))
{
if (!DateTime.TryParse(dateValue, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out var parsedDate))
{
throw new BadRequestException("Invalid lastKnownRevisionDate format.");
}
lastKnownRevisionDate = parsedDate;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we use the exact same logic in different spots, I think it's a good sign to extract this into a private method

public long FileSize { get; set; }
public bool AdminRequest { get; set; } = false;

/// The last known revision date of the Cipher that this attachment belongs to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ (non-blocking): I think we can use the proper xml documentation here

Suggested change
/// The last known revision date of the Cipher that this attachment belongs to.
/// <summary>
/// The last known revision date of the Cipher that this attachment belongs to.
/// </summary>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏🏼 Thanks for the tests

{
if (!DateTime.TryParse(dateValue, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out var parsedDate))
{
throw new BadRequestException("Invalid lastKnownRevisionDate format.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Non-blocking. This kind of begs the question, what format are we expecting, and is this specific error being handled by the client? Or is this just a helpful hint for debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's the ISO8601 standard - It's more a helpful hint for debugging. If the client is getting this error message, it indicates a bug with the date format in the first place.

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

Successfully merging this pull request may close these issues.

3 participants