Conversation
Contributor
|
New Issues (4)Checkmarx found the following issues in this Pull Request
|
Contributor
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #832 +/- ##
==========================================
+ Coverage 82.30% 82.77% +0.46%
==========================================
Files 346 351 +5
Lines 41401 41970 +569
==========================================
+ Hits 34075 34739 +664
+ Misses 7326 7231 -95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
addisonbeck
added a commit
that referenced
this pull request
Mar 10, 2026
Fixes two CI failures identified in SDK PR #832 after adding vault_url field to SsoCookieVendorConfig struct: 1. Swift demo app build failure: Added missing vault_url parameter to SsoCookieVendorConfig test instantiation in ContentView.swift. The demo app failed to compile because the struct now requires vault_url as a non-optional field. 2. rustfmt style check failure: Split long documentation comment line in platform_api.rs to satisfy rustfmt's line length requirements. The vault_url parameter documentation exceeded the maximum allowed line length. These corrections complete the vault_url implementation by ensuring all test code and demo applications are updated to match the new struct signature, and all code passes style checks. Refs: PM-33352
addisonbeck
changed the title
…nfig Adds explicit vault_url field to SsoCookieVendorConfig to fix two critical issues in SSO cookie acquisition redirect logic: 1. Port preservation: Prevents localhost:8000 being stripped to localhost, which breaks local development environments 2. Multi-subdomain routing: Fixes redirects going to api.bitwarden.com instead of vault.bitwarden.com when the cookie domain differs from the vault hostname The vault_url is extracted from the /api/config response.environment.vault field and provides the explicit full URL (scheme + host + port) for the browser redirect. This replaces implicit URL construction that was failing to preserve ports and handle subdomain variations correctly. All test cases updated to include vault_url field. Refs: PM-33352
…es trait signature Updates ServerCommunicationConfigPlatformApi trait's acquire_cookies() method to accept an explicit vault_url parameter alongside the existing hostname parameter. This enables platform implementations to receive the full vault URL (scheme + host + port) for SSO cookie acquisition redirects. This change prepares for fixing two critical issues: 1. Port preservation: Prevents localhost:8000 being stripped to localhost 2. Multi-subdomain routing: Ensures redirects use vault.bitwarden.com instead of api.bitwarden.com when domains differ The vault_url parameter takes priority over hostname-based URL construction to ensure correct redirect behavior in both local development and multi- subdomain production environments. Breaking change: This updates the trait signature. Implementations will be updated in the next commit to prevent compilation errors. Refs: PM-33352
…r vault_url Implements vault_url parameter support in both WASM and UniFFI foreign function interface bindings to enable client platforms to pass explicit vault URLs for SSO cookie acquisition. WASM TypeScript bindings (js_platform_api.rs): - Added vault_url parameter to TypeScript interface definition - Updated extern "C" signature to accept vault_url - Modified trait implementation to forward vault_url through ThreadBoundRunner UniFFI Swift/Kotlin bindings (server_communication_config.rs): - Added vault_url parameter to bridge implementation - Forwards vault_url to underlying platform trait Both bindings now enable clients to provide the full vault URL (scheme + host + port) for cookie acquisition redirects, which fixes port preservation issues in local development (localhost:8000 vs localhost) and multi-subdomain routing in production (vault.bitwarden.com vs api.bitwarden.com). Client-side usage in client.rs will be updated in the next commit. Refs: PM-33352
…uisition Completes the vault_url implementation in the SDK by updating client-side cookie acquisition logic and all test infrastructure to use the new field. Client business logic (client.rs): - Extracts vault_url from SsoCookieVendorConfig - Validates vault_url is not empty before cookie acquisition - Passes vault_url to platform_api.acquire_cookies() alongside hostname - Returns UnsupportedConfiguration error if vault_url is empty Test infrastructure updates: - Updated MockPlatformApi trait impl to accept vault_url parameter - Updated 14 test fixtures in client.rs to include vault_url field - Updated 3 test fixtures in repository.rs to include vault_url field - Updated 1 test fixture in config.rs to include vault_url field This completes the SDK-side implementation chain started in commits b8d7fec (struct field), 0245d0c (trait signature), and 392ee2c (FFI bindings). The SDK now provides full support for explicit vault URLs in SSO cookie acquisition, enabling clients to fix port preservation issues (localhost:8000 vs localhost) and multi-subdomain routing (vault vs api subdomains). All tests pass (30/30), no compilation warnings. Client implementations will be updated in subsequent commits. Refs: PM-33352
Fixes two CI failures identified in SDK PR #832 after adding vault_url field to SsoCookieVendorConfig struct: 1. Swift demo app build failure: Added missing vault_url parameter to SsoCookieVendorConfig test instantiation in ContentView.swift. The demo app failed to compile because the struct now requires vault_url as a non-optional field. 2. rustfmt style check failure: Split long documentation comment line in platform_api.rs to satisfy rustfmt's line length requirements. The vault_url parameter documentation exceeded the maximum allowed line length. These corrections complete the vault_url implementation by ensuring all test code and demo applications are updated to match the new struct signature, and all code passes style checks. Refs: PM-33352
CI lint job failed due to bare URLs in rustdoc comments violating the `-D rustdoc::bare-urls` lint rule. Fixed by wrapping example URLs in backticks to present them as code literals (non-clickable) rather than using angle brackets which create clickable hyperlinks. Example URLs in documentation should appear as inline code, not as live links, since they demonstrate URL format patterns rather than pointing to actual resources. Fixed example URLs in platform_api.rs documentation (lines 74-75).
The vault_url field in SsoCookieVendorConfig was initially added as a String, but should be Option<String> to match the pattern of other fields (idp_login_url, cookie_name, cookie_domain) in the same struct. This ensures consistent optionality semantics across the configuration structure. Changes: - Changed vault_url field type from String to Option<String> in SsoCookieVendorConfig struct (config.rs) - Updated client.rs extraction logic to use .as_ref().filter(|s| !s.is_empty()) pattern instead of separate empty string validation - Updated 37 test fixtures across client.rs, config.rs, and repository.rs to wrap vault_url values in Some(...) The trait signature in platform_api.rs correctly remains vault_url: String as the client validates and extracts the value before passing to platform implementations. Refs: PM-33352
coroiu
reviewed
Mar 13, 2026
Contributor
coroiu
left a comment
coroiu
left a comment
There was a problem hiding this comment.
Just one comment, rest of PR looks good
| async fn acquire_cookies(&self, hostname: String) -> Option<Vec<AcquiredCookie>> { | ||
| async fn acquire_cookies( | ||
| &self, | ||
| hostname: String, |
Contributor
There was a problem hiding this comment.
issue: I was initially positive to keeping this but as I'm reading this PR I think it will just be confusing since it's not clear what this value will actually be set to and how it can bee used. hostname is also a bad name and I have a follow-up PR where I'm renaming it everywhere to domain.
Currently this value is set to whatever domain the API request is trying to access. I could be ok with keeping it if we can make it clear that this is what the value represents.
Contributor
Author
There was a problem hiding this comment.
I went ahead and just removed hostname from the acquire_cookies argument list. I was imagining some scenario where implementations might need both, but thinking about it more now it's clear the platform API layer only ever needs vault_url. domain is really just the storage key now and that's all, as far as the SDK is concerned.
Removed on 232e2b4
addisonbeck
added a commit
that referenced
this pull request
Mar 13, 2026
… acquire_cookies Andreas Coroiu's code review feedback on PR #832 identified the hostname parameter as confusing and unused. Analysis confirmed platform implementations don't need it - test mocks prefixed it with underscore, and documentation explicitly stated to use vault_url for redirect construction instead. The SDK layer uses hostname internally as a repository key, but the platform API interface doesn't need to expose it. Platform implementations only need vault_url to construct browser redirects for cookie acquisition. If platforms need domain information for display purposes, they can derive it from vault_url. This simplification follows YAGNI principle by removing an unused, confusing parameter that added no value to the platform API interface. Changes: - Removed hostname parameter from ServerCommunicationConfigPlatformApi::acquire_cookies() trait method - Updated trait documentation to reflect single-parameter signature - Updated caller in ServerCommunicationConfigClient::acquire_cookie() to pass only vault_url - Updated test mock implementation signature - Updated WASM bindings (TypeScript interface and FFI) - Updated UniFFI bindings (mobile platforms) All cargo checks and 30 existing tests pass without modification. Refs: PM-33352
|
Claude finished @addisonbeck's task in 6m 29s —— View job I'll analyze this and get back to you. |
… acquire_cookies coroiu's code review feedback on PR #832 identified the hostname parameter as confusing and unused. Analysis confirmed platform implementations don't need it - test mocks prefixed it with underscore, and documentation explicitly stated to use vault_url for redirect construction instead. The SDK layer uses hostname internally as a repository key, but the platform API interface doesn't need to expose it. Platform implementations only need vault_url to construct browser redirects for cookie acquisition. If platforms need domain information for display purposes, they can derive it from vault_url. This simplification follows YAGNI principle by removing an unused, confusing parameter that added no value to the platform API interface. Changes: - Removed hostname parameter from ServerCommunicationConfigPlatformApi::acquire_cookies() trait method - Updated trait documentation to reflect single-parameter signature - Updated caller in ServerCommunicationConfigClient::acquire_cookie() to pass only vault_url - Updated test mock implementation signature - Updated WASM bindings (TypeScript interface and FFI) - Updated UniFFI bindings (mobile platforms) All cargo checks and 30 existing tests pass without modification. Refs: PM-33352
Review: PR #832 — Add vault_url to SsoCookieVendorConfig
Overall Assessment: APPROVE This is an incremental re-review. The previous review identified a single suggestion: missing test coverage for the vault_url validation branch. The new commit (208e5d5) directly addresses that gap by adding two test cases:
Both tests correctly exercise the validation logic at crates/bitwarden-server-communication-config/src/client.rs:137-142, follow existing test patterns in the module, and verify the platform API is never called when validation fails early. No new findings. Note: The existing unresolved thread from @coroiu regarding the hostname parameter in the WASM bindings (js_platform_api.rs) remains open — that discussion is about naming conventions and is being tracked for a follow-up PR. |
…rage AI code review identified missing test coverage for the vault_url validation branch (lines 137-142 in client.rs). The validation code checks that vault_url is present and non-empty before calling the platform API, returning UnsupportedConfiguration error if not. However, no tests covered these error cases. Good testing practice requires covering error paths, especially for new validation logic added in this PR. These tests ensure the validation works correctly when vault_url is misconfigured (None or empty string), and they prevent regressions if the validation logic changes in the future. Added test cases: - acquire_cookie_returns_unsupported_when_vault_url_none: Tests that acquire_cookie returns UnsupportedConfiguration when vault_url is None - acquire_cookie_returns_unsupported_when_vault_url_empty: Tests that acquire_cookie returns UnsupportedConfiguration when vault_url is empty string Both tests follow existing patterns: setup mock repository and platform API, configure the error condition, verify correct error variant is returned. Test results: - 32 tests passing (30 existing + 2 new) - All validation paths now covered - Formatting verified with rustfmt Refs: PM-33352
|
bw-ghapp bot
pushed a commit
to bitwarden/sdk-swift
that referenced
this pull request
Mar 18, 2026
…ommunication-config): add vault_url to SsoCookieVendorConfig (bitwarden/sdk-internal#832)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
📔 Objective
Add an explicit
vault_urlfield toSsoCookieVendorConfigto allow more flexibility in what servers are supported by this cookie flow.The
vault_urlis extracted from the/api/configresponse'senvironment.vaultfield by clients and provides the explicit full URL (scheme + host + port) for the browser redirect. This replaces implicit URL construction that was failing to preserve ports, handle subdomain variations, etc.🚨 Breaking Changes
SsoCookieVendorConfigstruct now requries a newvault_urlfieldThe parameter sent into
acquireCookiesis changed fromhostnametovaultUrl. This should be used to construct the browser link:${vaultUrl}/proxy-cookie-redirect-connector.htmlinstead of relying onEnvironmentServicewhich is prone to edge-case bugs