Skip to content

[PM-39812] fix: Apply SSO cookie request handlers to HTTPService download functions#2848

Open
fedemkr wants to merge 1 commit into
mainfrom
PM-39812/sso-cookies-fix-for-attachments
Open

[PM-39812] fix: Apply SSO cookie request handlers to HTTPService download functions#2848
fedemkr wants to merge 1 commit into
mainfrom
PM-39812/sso-cookies-fix-for-attachments

Conversation

@fedemkr

@fedemkr fedemkr commented Jul 1, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

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

📔 Objective

The download(from:) and download(filename:) methods in HTTPService bypassed
the request handler pipeline entirely, meaning SSOCookieVendorRequestHandler never
injected load-balancer cookies into download requests. On servers configured with SSO
cookie vendor bootstrap, this caused attachment downloads to throw errors.

Fix: Both download methods now call applyRequestHandlers(to:) before passing the
request to the underlying client, mirroring what send(_:) already did. A failable
HTTPRequest.init?(from: URLRequest) bridges the URLRequest-based download API into
the HTTPRequest-based handler pipeline.

Note

The response handler was not added here as that handles HTTPResponse instead of URL on the download functions. We could potentially add handling for that as well but requires more complex changes. So for now the request handler has been added which adds the cookie if it's available as we think it's an edge case that just at the moment of downloading an attachment the cookie expires.

…load functions

The download(from:) and download(filename:) methods bypassed the request
handler pipeline entirely, so SSOCookieVendorRequestHandler never injected
load-balancer cookies into download requests. This caused attachment
downloads to fail on servers with SSO cookie vendor bootstrap configured.
@fedemkr fedemkr added the ai-review Request a Claude code review label Jul 1, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context t:bug Change Type - Bug labels Jul 1, 2026
@fedemkr fedemkr marked this pull request as ready for review July 1, 2026 19:54
@fedemkr fedemkr requested review from a team and matt-livefront as code owners July 1, 2026 19:54
@fedemkr fedemkr added the enhancement New feature or request label Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the fix that routes HTTPService.download(from:) and download(filename:) through the request handler pipeline so SSOCookieVendorRequestHandler can inject load-balancer cookies into download requests. The new HTTPRequest.init?(from:) bridge and the private applyRequestHandlers(to:) helper correctly convert to/from URLRequest, and the change is scoped to a path (apiUnauthenticatedService) that has no token provider, so no auth headers leak into external download URLs. Test coverage is thorough — covering the new initializer's default, populated, and nil-URL cases plus header injection for both download methods. Extracting the shared response handlers into Support/ files is a clean, behavior-preserving refactor.

Code Review Details

No findings. The change is well-scoped, correct, and well-tested.

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.60%. Comparing base (993ce04) to head (714c92c).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
Networking/Sources/Networking/HTTPService.swift 0.00% 13 Missing ⚠️
Networking/Sources/Networking/HTTPRequest.swift 0.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2848       +/-   ##
===========================================
- Coverage   81.15%   40.60%   -40.55%     
===========================================
  Files        1027      356      -671     
  Lines       65896    16644    -49252     
===========================================
- Hits        53475     6758    -46717     
+ Misses      12421     9886     -2535     

☔ View full report in Codecov by Harness.
📢 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.

@matt-livefront matt-livefront left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These changes look good, looks like there's one test failure though.

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

Labels

ai-review Request a Claude code review app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context enhancement New feature or request t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants