Skip to content

Conversation

@tudddorrr
Copy link
Contributor

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Sep 17, 2025
@claude
Copy link

claude bot commented Sep 17, 2025

🔍 Code Review


🐛 Potential Bugs or Issues

  • Missing URL encoding: The startDate and endDate parameters in LeaderboardsAPI.cs:26-27 are directly concatenated into the query string without URL encoding. If these dates contain special characters (e.g., '+' in timezone offsets like '2024-01-01T00:00:00+00:00'), they won't be properly encoded, leading to malformed requests.

  • No date format validation: The date string parameters accept any string value without validation. Invalid date formats could cause API errors that aren't handled gracefully.


🔒 Security Concerns

  • Query parameter injection: Without proper encoding, malformed date strings could potentially inject additional query parameters if they contain '&' or '=' characters.

Performance Considerations

None identified - the changes are minimal and follow existing patterns.


🔄 Backwards Compatibility

None identified - the new optional parameters maintain backwards compatibility.


💡 Suggested Improvements

  1. Add URL encoding (following the pattern in StatsAPI.cs:84):

    if (!string.IsNullOrEmpty(startDate)) query["startDate"] = Uri.EscapeDataString(startDate);
    if (!string.IsNullOrEmpty(endDate)) query["endDate"] = Uri.EscapeDataString(endDate);
  2. Consider adding date validation or documenting the expected format (e.g., ISO 8601) to prevent runtime errors.

  3. Add XML documentation to clarify the expected date format for API consumers:

    /// <summary>
    /// Filter entries created after this date (ISO 8601 format)
    /// </summary>
    public string startDate = "";

@tudddorrr tudddorrr merged commit 2edc7f7 into develop Sep 17, 2025
2 checks passed
@tudddorrr tudddorrr deleted the leaderboard-entry-date-filtering branch September 17, 2025 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants