Skip to content

feat: add IDNA and password verification support - #926

Open
niteshpurohit wants to merge 9 commits into
feat/post-fork-dependency-initfrom
feat/adapters-idna-password
Open

niteshpurohit wants to merge 9 commits into
feat/post-fork-dependency-initfrom
feat/adapters-idna-password

Conversation

@niteshpurohit

Copy link
Copy Markdown
Member
  • Introduced IDNA lookup functionality in the idna.cpp file to handle internationalized domain names.
  • Implemented password verification using bcrypt and SHA-512 in the password_auth.cpp file.
  • Updated core error handling to include new dependency operations for IDNA and password verification.
  • Added tests for IDNA and password authentication to ensure correctness and reliability.
  • Enhanced the contract definitions to support new dependency statuses and operations.
  • Updated CMake configuration to include new dependencies for IDNA and password authentication.

closes: #57

- Introduced IDNA lookup functionality in the idna.cpp file to handle internationalized domain names.
- Implemented password verification using bcrypt and SHA-512 in the password_auth.cpp file.
- Updated core error handling to include new dependency operations for IDNA and password verification.
- Added tests for IDNA and password authentication to ensure correctness and reliability.
- Enhanced the contract definitions to support new dependency statuses and operations.
- Updated CMake configuration to include new dependencies for IDNA and password authentication.

closes: #57
@niteshpurohit niteshpurohit self-assigned this Sep 16, 2026
@niteshpurohit
niteshpurohit added this pull request to stack #915 September 16, 2026 05:21
Copilot AI lite review requested due to automatic review settings September 16, 2026 05:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Three moderate findings remain in cross-compilation configuration and password secret/error handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds optional IDNA and password-auth adapters backed by libidn2 and libxcrypt, with dependency integration, tests, and CI coverage.

Changes:

  • Adds bounded IDNA conversion and bcrypt/SHA-512 password verification.
  • Extends dependency contracts, acquisition, probing, and error normalization.
  • Adds adapter tests, build fixtures, and CI coverage.
File summaries
File Reviewed changes
tests/dependencies/probes/libxcrypt.cpp Adds libxcrypt capability probe.
tests/dependencies/probes/libidn2.cpp Adds libidn2 capability probe.
tests/dependencies/fixture/CMakeLists.txt Tests vendored dependency targets.
tests/build-variants/fixture/CMakeLists.txt Tests new feature variants.
tests/adapters/password_auth.cpp Adds password verification tests.
tests/adapters/idna.cpp Adds IDNA vector and boundary tests.
tests/adapters/dependency_error_logging.cpp Tests dependency error normalization.
src/core/contract/laghu/core/views.hpp Adds secure buffer cleansing support.
src/core/contract/laghu/core/contract.hpp Adds dependency statuses and operations.
src/adapters/password_auth.cpp Implements password verification; findings remain for cleansing copied secrets and classifying crypt_r errors.
src/adapters/idna.cpp Implements bounded IDNA conversion.
src/adapters/dependency.cpp Registers dependency names and statuses.
src/adapters/contract/laghu/adapters/password_auth.hpp Defines the password-auth contract.
src/adapters/contract/laghu/adapters/idna.hpp Defines the IDNA contract.
CMakeLists.txt Builds and tests the new adapters.
cmake/LaghuToolchain.cmake Adds dependency validation scenarios.
cmake/LaghuFeatures.cmake Registers new features.
cmake/LaghuDependencies.cmake Adds dependency acquisition; cross-compiling Autoconf configuration needs host/toolchain settings.
cmake/LaghuBuildIdentity.cmake Includes new adapter sources in build identity.
cmake/ExpectFeatureMetadata.cmake Updates feature metadata checks.
cmake/ExpectDependencyMetadata.cmake Updates dependency metadata checks.
cmake/ExpectDependencyConfigure.cmake Supports new dependency fixtures.
.github/workflows/toolchain.yml Adds adapter CI configurations.
.github/workflows/codeql.yml Builds adapters for CodeQL analysis.
Review details

Suppressed comments (1)

src/adapters/password_auth.cpp:247

  • All crypt_r failures other than ENOMEM are classified as dependency I/O. This misclassifies documented EINVAL (invalid setting), ENOSYS/ENOTSUP (unsupported algorithm), and ERANGE failures as retryable I/O, so malformed or unsupported bcrypt cases do not receive normalized dependency statuses. Map the native errno classes before calling normalize_dependency_error.
[[nodiscard]] core::Error crypt_failure(int native_code,
                                        const DependencyLogSink& log_sink) noexcept {
  const core::DependencyStatus status = native_code == ENOMEM
                                            ? core::DependencyStatus::exhaustion
                                            : core::DependencyStatus::io;
  const core::Error error = normalize_dependency_error(
      core::DependencyId::libxcrypt, core::DependencyOperation::password_verify,
      status, static_cast<std::int32_t>(native_code));
  • Files reviewed: 24/24 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cmake/LaghuDependencies.cmake Outdated
Comment thread src/adapters/password_auth.cpp Outdated
- Added cross-compilation support for password authentication dependencies.
- Updated maximum password byte limit to align with libxcrypt's specifications.
- Introduced new error statuses for better input validation.
- Refactored password verification logic to improve error handling.
- Added tests for native error normalization and password limits.
Copilot AI review requested due to automatic review settings September 16, 2026 06:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical static-linking and moderate correctness and secret-cleansing issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/adapters/password_auth.cpp:325

  • crypt_data is libxcrypt's caller-owned work area for the password computation, but this stack object is never cleansed on any return path. The existing guard only wipes the copied password buffer, leaving plaintext-derived intermediate state and/or the computed hash in native scratch after verification. Add an RAII best-effort wipe for the native workspace before returning.

src/adapters/idna.cpp:125

  • This converts the caller's UTF-8 input through a 254-byte StaticCString, so any IDN whose UTF-8 representation exceeds 253 bytes is rejected before libidn2. The 253-byte hostname bound applies to the resulting ASCII/A-label form; valid internationalized hostnames can have a larger UTF-8 representation while producing an ASCII result within the bound. Use a separately bounded input buffer and retain the existing post-conversion AsciiHostname::from_ascii check.
  const auto input = hostname.to_c_string<AsciiHostname::storage_capacity>();

src/adapters/idna.cpp:62

  • libidn2 returns IDN2_TOO_BIG_DOMAIN and IDN2_TOO_BIG_LABEL for inputs that exceed its domain/label bounds, but this fallback classifies every non-allocation/non-codeset error as corrupt_data. A Unicode hostname that exceeds the bounded IDNA contract will therefore report corrupt data instead of invalid_range; map the two size return codes to DependencyStatus::invalid_range so callers can distinguish a range violation from malformed dependency output.
[[nodiscard]] core::DependencyStatus idn2_status(int status) noexcept {
  if (status == IDN2_MALLOC) {
    return core::DependencyStatus::exhaustion;
  }
  if (status == IDN2_NO_CODESET) {
    return core::DependencyStatus::unavailable;
  }
  return core::DependencyStatus::corrupt_data;
  • Files reviewed: 25/25 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread cmake/LaghuDependencies.cmake
Comment thread cmake/LaghuBuildIdentity.cmake Outdated
- Added support for password authentication features in the build identity process.
- Updated CMake scripts to include new source files related to password authentication.
- Enhanced error handling for IDNA processing to manage range errors effectively.
- Introduced new tests to validate UTF-8 input and native length boundaries for IDNA.
Copilot AI review requested due to automatic review settings September 16, 2026 22:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical and moderate findings affect cross-compilation, dependency linkage, IDNA normalization, credential scrubbing, and contract compatibility.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

cmake/LaghuDependencies.cmake:779

  • The vendored libxcrypt build and install commands run from the archive root, but libcrypt.la and the generated install-* targets are defined by libxcrypt's lib/Makefile; unlike the libidn2 branch above, these commands never change into <SOURCE_DIR>/lib. The vendored IDNA/password fixture will therefore fail when ExternalProject runs make libcrypt.la (and again during install). Invoke these targets from the lib subdirectory or use the corresponding top-level recursive targets.
    set(build_command "${laghu_make_program}" libcrypt.la)
    set(install_command
      "${laghu_make_program}" install-libLTLIBRARIES install-nodist_includeHEADERS)

cmake/LaghuDependencies.cmake:809

  • The vendored libidn2 build is configured with --with-included-libunistring, so the produced libidn2.a still has a bundled libunistring archive dependency for static consumers. Exposing only this raw archive drops that dependency; the generated symbol probe (and later adapter link) will fail with unresolved libunistring symbols in the default STATIC/full configuration. Export the bundled archive as a transitive target or create a genuinely self-contained artifact before assigning only IMPORTED_LOCATION here.
  add_library("${private_target}_artifact" "${library_type}" IMPORTED GLOBAL)
  set_target_properties("${private_target}_artifact" PROPERTIES
    IMPORTED_LOCATION "${library_path}")
  add_dependencies("${private_target}_artifact" "laghu_vendor_${id}")
  add_library("${private_target}" INTERFACE)
  target_include_directories("${private_target}" SYSTEM INTERFACE "${install_directory}/include")
  target_link_libraries("${private_target}" INTERFACE "${private_target}_artifact")
  add_dependencies("${private_target}" "laghu_vendor_${id}")

src/adapters/idna.cpp:153

  • IDN2_NFC_INPUT tells libidn2 that the caller has already normalized the input; it does not perform NFC normalization. This contract accepts arbitrary UTF-8 and passes it straight through, so canonically equivalent non-NFC hostnames can be rejected or mapped inconsistently instead of receiving the required TR46 processing. Remove this flag (or normalize before calling) and add a non-NFC vector to the adapter test.
  constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;

src/adapters/password_auth.cpp:325

  • crypt_data is libxcrypt's caller-provided scratch/output buffer and can retain password-derived state after crypt_r returns. The existing guard only cleanses bounded_password, so both successful and failed verifications leave native credential material on the stack until that memory is reused. Add an RAII cleanse for the complete crypt_data object before every return from this scope.
  crypt_data native_data{};
  errno = 0;
  const char* native_result = crypt_r(bounded_password->c_str(), bounded_hash->c_str(), &native_data);
  • Files reviewed: 26/26 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread cmake/LaghuDependencies.cmake Outdated
Comment thread src/core/contract/laghu/core/contract.hpp
- Added checks for missing cross-compilation tools (C compiler, ar, ranlib) to ensure proper error handling.
- Enhanced the test suite to validate scenarios where cross-compilation tools are missing, preventing silent failures.
- Updated the acquisition function to set appropriate variables for cross-compilation, improving robustness.
Copilot AI review requested due to automatic review settings September 16, 2026 23:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

One critical build issue and four moderate correctness and contract issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/adapters/idna.cpp:153

  • IDN2_NFC_INPUT tells libidn2 to assume the input is already NFC-normalized; this API accepts arbitrary UTF-8 and does not normalize it first. A valid decomposed hostname such as u\u0308ber.example can therefore be rejected or encoded differently instead of receiving the required TR46/NFC processing. Remove this flag (or normalize the input before setting it) and add a decomposed Unicode golden vector.
  constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;

src/adapters/idna.cpp:76

  • This fallback maps caller-supplied hostname validation failures such as libidn2's IDN2_LDH/IDN2_DISALLOWED (for example, -leading.example, which passes the local character filter) to corrupt_data. That makes ordinary invalid input security-relevant corruption and produces the wrong normalized contract error; classify documented input-validation statuses as invalid_input and reserve corrupt_data for malformed native data/encoding.
[[nodiscard]] core::DependencyStatus idn2_status(int status) noexcept {
  if (status == IDN2_MALLOC) {
    return core::DependencyStatus::exhaustion;
  }
  if (status == IDN2_NO_CODESET) {
    return core::DependencyStatus::unavailable;
  }
  if (status == IDN2_TOO_BIG_DOMAIN || status == IDN2_TOO_BIG_LABEL) {
    return core::DependencyStatus::invalid_range;
  }
  return core::DependencyStatus::corrupt_data;

src/adapters/password_auth.cpp:337

  • crypt_r is allowed to return a failure token beginning with * instead of nullptr (the vendored libxcrypt build leaves failure tokens enabled). bounded_crypt_output accepts that token, so this return path reports unsupported algorithms and other native failures as a normal password mismatch and bypasses the promised dependency-error normalization. Detect a leading * and route it through password_auth_native_error before comparing hashes.
  return constant_time_hash_equal(bounded_hash->view(), *output);

src/core/contract/laghu/core/contract.hpp:52

  • These new enum members are inserted before all existing DependencyStatus values, changing the underlying numeric value of every pre-existing status (for example, unavailable and io). Because this is a public contract and statuses are carried in dependency log records, that silently breaks consumers that persist or exchange the enum values; append the new statuses or assign explicit stable values instead.
  invalid_input,
  invalid_range,
  unavailable,
  exhaustion,
  • Files reviewed: 26/26 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread cmake/LaghuDependencies.cmake
- Added a new test for password authentication failure token normalization to ensure proper error handling.
- Enhanced IDNA error classification by adding checks for various encoding errors.
- Updated existing tests to include new error handling scenarios for IDNA and password authentication.
Copilot AI review requested due to automatic review settings September 16, 2026 23:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved IDNA compilation and normalization defects, plus libidn2 vendored build and linkage issues, remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

cmake/LaghuDependencies.cmake:787

  • ExternalProject_Add(BUILD_COMMAND ...) accepts one command vector; the COMMAND tokens embedded in build_command are passed literally as arguments to the first make, not treated as separators for three commands. The vendored libidn2 build therefore invokes something equivalent to make -C gl all COMMAND make ... and fails before producing libidn2.la. Use separate external-project steps or a wrapper script/one command that sequences these sub-builds.
    set(build_command
      "${laghu_make_program}" -C "<SOURCE_DIR>/gl" all
      COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/unistring" all
      COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/lib" libidn2.la)

cmake/LaghuDependencies.cmake:827

  • In STATIC mode this imported target exposes only libidn2.a, while --with-included-libunistring builds libunistring as a separate archive that libidn2.a references. No bundled unistring archive or platform dependency is added to the target's link interface, so any real idna consumer will fail with unresolved u8_*/unistring symbols.
  add_library("${private_target}" INTERFACE)
  target_include_directories("${private_target}" SYSTEM INTERFACE "${install_directory}/include")
  target_link_libraries("${private_target}" INTERFACE "${private_target}_artifact")

src/adapters/idna.cpp:165

  • IDN2_NFC_INPUT tells libidn2 that the caller has already provided NFC input; it does not request NFC normalization. This contract accepts UTF-8 and claims TR46 processing, so decomposed-but-valid sequences (for example u plus combining diaeresis) will be rejected with IDN2_NOT_NFC instead of being normalized to the same ASCII hostname as the precomposed form. Let libidn2 perform normalization (or normalize before setting this flag) and add a decomposed Unicode vector.
  constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;
  • Files reviewed: 27/27 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/adapters/idna.cpp
Comment thread src/adapters/idna.cpp Outdated
- Added private include directory for IDNA adapter.
- Introduced internal IDNA status normalization function.
- Updated IDNA test cases to include new status checks.
- Improved error handling for Punycode outputs in tests.
Copilot AI review requested due to automatic review settings September 17, 2026 00:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical and moderate findings affect dependency builds, secret cleanup, IDNA behavior, and CI coverage.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (10)

Previously missed (1) — in code that hasn't changed since the last review.

src/adapters/idna.cpp:172

  • IDN2_NFC_INPUT tells libidn2 to assume the caller already supplied NFC and to reject non-NFC input. That makes valid UTF-8 hostnames such as a decomposed Unicode label fail with IDN2_NOT_NFC, whereas the public contract and the issue's IDNA2008/TR46 requirement call for normalization during lookup. Remove this flag (and cover a decomposed golden vector).

.github/workflows/toolchain.yml:49

  • The new laghu_password_auth_failure_token_test is registered, but this job builds only laghu_password_auth_test and its regex only matches laghu.adapters.password_auth. The native failure-token normalization regression therefore never runs in the full-adapter CI job; include that target and laghu.adapters.password_auth.failure_token in the verification.
          cmake --build "$RUNNER_TEMP/laghu-full" --target \
            laghu_crypto_provider_test laghu_idna_test laghu_password_auth_test
          ctest --test-dir "$RUNNER_TEMP/laghu-full" --output-on-failure \
            -R '^laghu\.(crypto\.provider|adapters\.(idna|password_auth))$'

.github/workflows/toolchain.yml:62

  • The new laghu_password_auth_failure_token_test is registered, but this system-adapter job builds only laghu_password_auth_test and its regex only matches laghu.adapters.password_auth. The native failure-token normalization regression is not exercised here; include that target and laghu.adapters.password_auth.failure_token in the verification.
          cmake --build "$RUNNER_TEMP/laghu-system-adapters" --target \
            laghu_idna_test laghu_password_auth_test
          ctest --test-dir "$RUNNER_TEMP/laghu-system-adapters" --output-on-failure \
            -R '^laghu\.adapters\.(idna|password_auth)$'

.github/workflows/toolchain.yml:86

  • The new laghu_password_auth_failure_token_test is registered, but this Clang adapter job builds only laghu_password_auth_test and its regex only matches laghu.adapters.password_auth. The native failure-token normalization regression is not exercised here; include that target and laghu.adapters.password_auth.failure_token in the verification.
          cmake --build "$RUNNER_TEMP/laghu-clang-adapters" --target \
            laghu_idna_test laghu_password_auth_test
          ctest --test-dir "$RUNNER_TEMP/laghu-clang-adapters" --output-on-failure \
            -R '^laghu\.adapters\.(idna|password_auth)$'

.github/workflows/toolchain.yml:179

  • The new laghu_password_auth_failure_token_test is registered, but this FreeBSD adapter job builds only laghu_password_auth_test and its regex only matches laghu.adapters.password_auth. The native failure-token normalization regression is not exercised here; include that target and laghu.adapters.password_auth.failure_token in the verification.
            cmake --build /tmp/laghu-adapters --target \
              laghu_idna_test laghu_password_auth_test
            ctest --test-dir /tmp/laghu-adapters --output-on-failure \
              -R '^laghu\.adapters\.(idna|password_auth)$'

.github/workflows/toolchain.yml:242

  • The new laghu_password_auth_failure_token_test is registered, but this sanitizer adapter job builds only laghu_password_auth_test and its regex only matches laghu.adapters.password_auth. The native failure-token normalization regression is not exercised under ASan/UBSan; include that target and laghu.adapters.password_auth.failure_token in the verification.
          cmake --build "$RUNNER_TEMP/laghu-asan-adapters" --target \
            laghu_idna_test laghu_password_auth_test
          ctest --test-dir "$RUNNER_TEMP/laghu-asan-adapters" --output-on-failure \
            -R '^laghu\.adapters\.(idna|password_auth)$'

.github/workflows/toolchain.yml:145

  • The new laghu_password_auth_failure_token_test is registered, but this Apple Clang job builds only laghu_password_auth_test and its regex only matches laghu.adapters.password_auth. The native failure-token normalization regression is not exercised here; include that target and laghu.adapters.password_auth.failure_token in the verification.
          cmake --build "$RUNNER_TEMP/laghu-adapters" --target \
            laghu_idna_test laghu_password_auth_test
          ctest --test-dir "$RUNNER_TEMP/laghu-adapters" --output-on-failure \
            -R '^laghu\.adapters\.(idna|password_auth)$'

cmake/LaghuDependencies.cmake:802

  • The Autoconf environment forwards only CC, AR, and RANLIB; it drops CMake toolchain flags such as CMAKE_C_FLAGS, CMAKE_SYSROOT, and linker flags. A cross toolchain that relies on a non-default sysroot or target ABI flags can therefore build these libraries against host headers/libraries or fail despite the --host setting. Propagate the relevant compile/link environment (or a generated wrapper) into configure and make.
  set(configure_environment "${CMAKE_COMMAND}" -E env
    "CC=${laghu_autoconf_c_compiler}" "AR=${laghu_autoconf_ar}" "RANLIB=${laghu_autoconf_ranlib}"
    "MAKE=${laghu_make_program}")

cmake/LaghuToolchain.cmake:656

  • These three new tests omit LAGHU_EXPECT_CROSSCOMPILING, but ExpectDependencyConfigure.cmake unconditionally forwards -DLAGHU_EXPECT_CROSSCOMPILING=${LAGHU_EXPECT_CROSSCOMPILING}. An undefined value is still passed as a defined-but-empty cache variable; the fixture rejects that as expected=native actual=cross after it sets CMAKE_CROSSCOMPILING for these scenarios, so the tests never reach the missing-tool checks. Make the wrapper omit this argument when it is undefined (or otherwise adjust the fixture's cross-mode expectation).
        -DSCENARIO=autoconf_cross_missing_${missing_tool}
        -DEXPECT_FAIL=ON
        "-DEXPECT_TEXT=rule=cross_tool_missing tool=${cmake_tool}"

src/adapters/idna.cpp:160

  • AsciiHostname::from_ascii establishes that an empty hostname is invalid_input, but this path never rejects an empty TextView before the native call. An empty input can therefore be surfaced as a libidn2 corrupt_data failure instead of the caller-input error. Reject an empty input at this boundary before invoking libidn2.
  const auto input = hostname.to_c_string<utf8_input_storage_capacity>();
  if (!input.has_value()) {
    return std::unexpected{input.error()};
  • Files reviewed: 28/28 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread cmake/LaghuDependencies.cmake
Comment thread src/adapters/password_auth.cpp
- Added validation for empty hostname input in IDNA conversion to prevent errors.
- Updated test cases to include checks for decomposed German characters in IDNA conversion.
- Enhanced CMake configuration for cross-compilation, ensuring proper environment setup for Autoconf.
- Added new tests to verify the cross-toolchain environment and its dependencies.
Copilot AI review requested due to automatic review settings September 17, 2026 00:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical dependency-build and IDNA issues, plus unresolved password-buffer zeroization findings, block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

cmake/LaghuDependencies.cmake:866

  • ExternalProject_Add treats BUILD_COMMAND as one command line; the COMMAND tokens in this list are not separate build steps. The libidn2 build will therefore pass COMMAND to the first make invocation (or otherwise fail to execute the later commands), so the vendored IDNA dependency cannot be built. Use a wrapper command/script or separate external-project steps to run these three make invocations sequentially.
    set(build_command
      "${laghu_make_program}" -C "<SOURCE_DIR>/gl" all
      COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/unistring" all
      COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/lib" libidn2.la)

cmake/LaghuDependencies.cmake:877

  • The libxcrypt archive's libcrypt.la and install-* Automake targets are generated in its lib/ subdirectory, so invoking them from <SOURCE_DIR> makes every vendored password build fail with a missing-target error. Run both commands with -C <SOURCE_DIR>/lib (as is already done for libidn2) so the default vendored configuration can produce the imported artifact.
    set(build_command "${laghu_make_program}" libcrypt.la)
    set(install_command
      "${laghu_make_program}" install-libLTLIBRARIES install-nodist_includeHEADERS)

src/adapters/idna.cpp:90

  • IDN2_ZERO_LENGTH_LABEL is a normal input-validation result for values such as the added two..labels.example vector, but it is not included here. It therefore falls through to corrupt_data, misclassifying malformed caller input and violating the dependency-error normalization contract. Map this status to invalid_input along with the other IDNA validation statuses.
  if (status == IDN2_ENCODING_ERROR || status == IDN2_PUNYCODE_BAD_INPUT ||
      status == IDN2_PUNYCODE_OVERFLOW ||
      status == IDN2_INVALID_ALABEL || status == IDN2_UALABEL_MISMATCH ||
      status == IDN2_NOT_NFC || status == IDN2_2HYPHEN ||
      status == IDN2_HYPHEN_STARTEND || status == IDN2_LEADING_COMBINING ||
      status == IDN2_DISALLOWED || status == IDN2_CONTEXTJ ||
      status == IDN2_CONTEXTJ_NO_RULE || status == IDN2_CONTEXTO ||
      status == IDN2_CONTEXTO_NO_RULE || status == IDN2_UNASSIGNED ||
      status == IDN2_BIDI || status == IDN2_DOT_IN_LABEL ||
      status == IDN2_INVALID_TRANSITIONAL || status == IDN2_INVALID_NONTRANSITIONAL ||
      status == IDN2_ALABEL_ROUNDTRIP_FAILED) {

src/adapters/password_auth.cpp:300

  • The guard only cleanses the StaticCString held inside bounded_password, but TextView::to_c_string constructs a separate local StaticCString and moves/copies its array into the returned Result; the default move leaves the source bytes intact when that local is destroyed. This leaves a plaintext password copy on the stack after verification, so use a secret-specific non-copying buffer or a move/destructor contract that scrubs every instance.
  auto bounded_password = password.to_c_string<password_c_string_capacity>();
  if (!bounded_password.has_value()) {
    return std::unexpected{bounded_password.error()};
  }
  PasswordCStringGuard password_guard{*bounded_password};

src/adapters/password_auth.cpp:326

  • crypt_r receives caller-owned crypt_data storage, which libxcrypt can use for password-derived state and input buffering, but this object is never wiped on either the success or error paths. The existing password-buffer guard therefore does not remove all sensitive material from the stack; add an RAII zeroization guard for native_data before returning.
  crypt_data native_data{};
  errno = 0;
  const char* native_result =
      crypt_r(bounded_password->c_str(), bounded_hash->c_str(), &native_data);
  • Files reviewed: 28/28 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread cmake/LaghuDependencies.cmake
Comment thread src/adapters/idna.cpp
- Added `validate_input_label_structure` to check for empty labels in hostnames.
- Integrated input validation into `idna_to_ascii` to ensure proper hostname structure.
- Introduced `SecretPassword` class to manage password storage securely, preventing copy operations.
- Added tests for empty label handling in IDNA and for the cleansing behavior of `SecretPassword`.
Copilot AI review requested due to automatic review settings September 17, 2026 01:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Unresolved findings affect libidn2 build/linking, IDNA compilation and normalization, password-buffer cleansing, and an IDNA boundary test.

Review details

Suppressed comments (7)

cmake/LaghuDependencies.cmake:866

  • ExternalProject_Add treats BUILD_COMMAND as one command vector; COMMAND is not a separator for multiple build commands here and will be passed to the first make invocation as a literal argument. Consequently the vendored libidn2 build cannot run the three required make steps. Use an ExternalProject_Add_Step chain or a wrapper script/CMake script that executes the commands separately.
    set(build_command
      "${laghu_make_program}" -C "<SOURCE_DIR>/gl" all
      COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/unistring" all
      COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/lib" libidn2.la)

cmake/LaghuDependencies.cmake:869

  • The bundled libidn2 build creates a separate unistring library (--with-included-libunistring), but this install step installs only libidn2 and the imported target later links only libidn2. In STATIC mode the resulting archive has unresolved libunistring symbols; in DYNAMIC mode its bundled runtime dependency is not installed, so the new vendored IDNA fixture and real adapter link/runtime can fail. Install and expose the bundled libunistring artifact as a transitive dependency, or configure/build libidn2 as a truly self-contained artifact.
    set(install_command
      "${laghu_make_program}" -C "<SOURCE_DIR>/lib"
      install-libLTLIBRARIES install-includeHEADERS)

src/adapters/idna.cpp:203

  • IDN2_NFC_INPUT means that the caller asserts the input is already NFC; it does not ask libidn2 to normalize arbitrary UTF-8. This adapter accepts decomposed input (the added golden vector passes u\u0308) and therefore this flag causes that vector to be rejected or processed without TR46 NFC mapping instead of producing the expected A-label. Omit this flag or normalize before the native call while retaining the non-transitional and STD3 flags.
  constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;

src/adapters/idna.cpp:106

  • IDN2_NOT_NFC is not a libidn2 status exported by idn2.h; the public constant is IDN2_NFC. As written, this translation unit cannot compile against the declared system floor or the vendored header. Replace it with the exported constant.
      status == IDN2_NOT_NFC || status == IDN2_2HYPHEN ||

src/adapters/password_auth.cpp:307

  • Only SecretPassword is cleansed here. crypt_data is libxcrypt's caller-owned scratch/output buffer and can retain password-derived material after crypt_r; every return below leaves that stack object untouched. Wrap it in an RAII secure zeroizer over the complete crypt_data object before returning.
  crypt_data native_data{};
  errno = 0;
  const char* native_result =
      crypt_r(bounded_password->c_str(), bounded_hash->c_str(), &native_data);

src/adapters/password_auth.cpp:287

  • to_c_string creates a second stack copy of the caller's encoded credential, but no path cleanses this StaticCString; the newly added StaticCString::cleanse() is never used. Cleanse this copy via RAII so early parse/native-error returns do not leave the verifier hash in stack memory.
  const auto bounded_hash = encoded_hash.to_c_string<encoded_password_capacity>();
  if (!bounded_hash.has_value()) {
    return std::unexpected{bounded_hash.error()};
  }
  const auto parsed_hash = parse_password_hash(bounded_hash->view());

tests/adapters/idna.cpp:125

  • This 255-byte hostname is still at the inclusive 255-octet libidn2 bound (four 63-byte labels plus separators), so libidn2 can accept it and the adapter will then reject it at its separate 253-byte output contract. The assertion requiring a libidn2 range status is therefore not reliable. Use an input definitely over the native bound, such as 257 bytes with a separator after the fourth 63-byte label, or assert the adapter-level invalid_range result.
  std::array<char, 255> too_big_domain{};
  for (std::size_t index = 0; index < too_big_domain.size(); ++index) {
    too_big_domain[index] = (index == 63 || index == 127 || index == 191) ? '.' : 'a';
  }
  • Files reviewed: 28/28 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

- Changed the return statement in the from() method of SecretPassword to return the output directly instead of using std::move.
- This improves code clarity and avoids unnecessary move semantics for the SecretPassword object.
Copilot AI review requested due to automatic review settings September 17, 2026 02:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Unresolved moderate issues remain in dependency builds/linking, IDNA normalization and bounds, secret wiping, hash bounds, and enum compatibility.

Review details

Suppressed comments (7)

cmake/LaghuDependencies.cmake:907

  • In STATIC mode, the bundled libidn2 build uses the included libunistring archive, but this interface exposes only libidn2.a. The symbol probe and any consumer therefore link the IDNA archive without its u8_* dependency and fail with unresolved symbols. Export the bundled libunistring archive as a transitive link dependency (and make its build/install artifact available) when linking the static target.
  target_link_libraries("${private_target}" INTERFACE "${private_target}_artifact")

cmake/LaghuDependencies.cmake:866

  • ExternalProject_Add treats BUILD_COMMAND as one command; COMMAND is not a separator here and will be passed to the first make invocation as a literal target/argument. Vendored libidn2 builds will therefore fail before the library is produced. Use a single wrapper/script (or otherwise model the three dependent build steps) rather than embedding multiple COMMAND tokens in this value.
    set(build_command
      "${laghu_make_program}" -C "<SOURCE_DIR>/gl" all
      COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/unistring" all
      COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/lib" libidn2.la)

src/adapters/idna.cpp:203

  • IDN2_NFC_INPUT tells libidn2 that the caller has already supplied NFC input; it does not request normalization. As a result, the decomposed bu\xCC\x88cher.example vector in this PR will be rejected or processed without the required NFC/TR46 mapping, contrary to the stated IDNA2008/TR46 behavior. Omit this flag so libidn2 performs normalization.
  constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;

src/adapters/idna.cpp:188

  • The same unbounded-scan problem exists for hostnames: to_c_string() checks for embedded NUL before checking its capacity, so an oversized borrowed TextView is scanned in full instead of being rejected at the adapter's 1020-byte input bound. Check hostname.size() against utf8_input_storage_capacity first.
  const auto input = hostname.to_c_string<utf8_input_storage_capacity>();
  if (!input.has_value()) {
    return std::unexpected{input.error()};

src/adapters/password_auth.cpp:307

  • crypt_data contains libxcrypt's native working state and output, including password-derived material. Every return after crypt_r leaves native_data uncleansed on the stack, so the new password adapter can retain sensitive state for later stack reuse or crash/core-dump exposure. Wrap this object in an RAII cleanup guard that performs a volatile wipe before all exits.
  crypt_data native_data{};
  errno = 0;
  const char* native_result =
      crypt_r(bounded_password->c_str(), bounded_hash->c_str(), &native_data);

src/adapters/password_auth.cpp:285

  • The hash length is not checked before TextView::to_c_string() scans for embedded NULs. A caller can pass an arbitrarily large borrowed view and force an unbounded linear scan before the adapter rejects it, defeating the advertised bounded authentication path. Reject encoded_hash.size() >= encoded_password_capacity before conversion.
  const auto bounded_hash = encoded_hash.to_c_string<encoded_password_capacity>();
  if (!bounded_hash.has_value()) {
    return std::unexpected{bounded_hash.error()};

src/core/contract/laghu/core/contract.hpp:52

  • These new statuses are inserted before every existing DependencyStatus value. Because the enum is a fixed-width public contract and is stored in DependencyLogRecord, this renumbers existing unavailable, unsupported_version, and later statuses for consumers decoding the record. Append new values after the existing list or assign explicit numeric values to preserve the established encoding.
  invalid_input,
  invalid_range,
  unavailable,
  exhaustion,
  • Files reviewed: 28/28 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement IDNA and password adapters

3 participants