Skip to content

Conversation

@sciros
Copy link
Collaborator

@sciros sciros commented Nov 24, 2025

This pull request introduces several enhancements and bug fixes across the codebase, most notably improving domain name handling in DomainEncoder, adding support for selecting a specific cohort for single-attempt MPIC orchestration, and expanding related test coverage. It also includes some refactoring, such as relocating exceptions and updating error messages.

Domain name handling improvements:

  • Improved DomainEncoder.prepare_target_for_lookup to correctly detect and handle already punycode-encoded domains (including inner labels), ensuring they are not double-encoded and raising clear errors for malformed domains. Added stricter validation for wildcards and malformed input.
  • Expanded unit tests for DomainEncoder to cover more edge cases, including already-encoded domains, wildcards, inner punycode labels, and malformed domains. [1] [2]

MPIC orchestration enhancements:

  • Added support for specifying a cohort_for_single_attempt parameter in MpicRequestOrchestrationParameters, allowing users to select a specific cohort for a single orchestration attempt. The coordinator now validates this parameter and raises a new CohortSelectionException if the requested cohort is invalid. [1] [2] [3] [4] [5] [6] [7]
  • Added and updated unit tests to verify correct handling of the new cohort selection feature, including validation and error scenarios.

Refactoring and error handling:

  • Moved CohortCreationException to mpic_request_errors.py and introduced a new CohortSelectionException for invalid cohort selection. Updated all imports and error messages accordingly. [1] [2] [3] [4] [5] [6] [7]
  • Updated version to 6.2.0 in __about__.py.

Test improvements:

  • Refactored DCV checker tests to combine and clarify test cases for malformed and well-formed IP addresses, ensuring only valid formats allow issuance. [1] [2]

@sciros sciros requested review from ahanafy and birgelee November 24, 2025 21:27
Copy link
Member

@birgelee birgelee left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for the work.

GENERAL_HTTP_ERROR = ('mpic_error:http', 'An HTTP error occurred: Response status {0}, Response reason: {1}')
INVALID_REDIRECT_ERROR = ('mpic_error:redirect:invalid', 'Invalid redirect. Redirect code: {0}, target: {1}')
COHORT_CREATION_ERROR = ('mpic_error:coordinator:cohort', 'The coordinator could not construct a cohort of size {0}')
COHORT_SELECTION_ERROR = ('mpic_error:coordinator:cohort_selection', 'The coordinator could not select cohort number {0} from available cohorts.')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not going to block on this, but somehow my C programing mindset gets a little scared when I see numbered format tokens in string literals particularly when the format string is declared separately from the format commend. I think this is all good but if one of these strings was rewritten to have fewer or more format tokens, would that cause an error (at least its not going to just buffer overflow like it would in C).

Copy link
Collaborator Author

@sciros sciros Dec 4, 2025

Choose a reason for hiding this comment

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

It won't cause issues like in C and thankfully there's unit tests that exercise the error code but it's not as elegant as maybe some other approach could be.
Anthropic Claude's take:

Key difference from C: Unlike C's format string vulnerabilities, Python's string formatting is type-safe and memory-safe. There's no buffer overflow risk, and mismatches produce clear runtime errors rather than undefined behavior or security issues.
Practical considerations:

  • The error would be caught immediately when that code path executes (unlike silent C corruption)
  • Unit tests covering these error cases would catch mismatches
  • f-strings (f"Response status {status}") would make this mismatch impossible since variables are directly embedded, though they'd require the values at definition time rather than later formatting

The pattern here (constants with deferred formatting) is reasonable for error definitions. The main risk is maintenance errors that tests should catch.

@sciros sciros merged commit 6fd0d0f into main Dec 4, 2025
1 check passed
@sciros sciros deleted the ds-punycode-and-cohort-number branch December 4, 2025 23:17
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.

2 participants