fix(scripting): wire bru.sendRequest() into the shared cookie jar#7518
Open
mareczek wants to merge 3 commits intousebruno:mainfrom
Open
fix(scripting): wire bru.sendRequest() into the shared cookie jar#7518mareczek wants to merge 3 commits intousebruno:mainfrom
mareczek wants to merge 3 commits intousebruno:mainfrom
Conversation
bru.sendRequest() created a bare axios instance with no cookie hooks, so Set-Cookie headers from scripting requests were silently discarded and stored cookies were never forwarded. Add attachCookieInterceptors() with two axios interceptors: - request: injects stored cookies from the global jar - response: persists Set-Cookie headers back into the jar (including on errors) Fixes usebruno#1394 usebruno#1493 usebruno#5419
mareczek
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughAdds Axios interceptors to integrate a cookie jar into the send-request flow: request interceptor injects jar cookies into outgoing requests; response and error interceptors persist Set-Cookie headers back to the jar. Tests cover injection, merging, persistence, and error resilience. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Application
participant Axios as Axios Instance
participant CookieJar as Cookie Jar
participant Server as Server/Response
Client->>Axios: sendRequest(url, config)
Axios->>CookieJar: getCookieStringForUrl(fullUrl)
CookieJar-->>Axios: cookieString
Axios->>Axios: merge cookieString into Cookie header
Axios->>Server: HTTP Request (with Cookie header)
alt Success Response
Server-->>Axios: Response (may include Set-Cookie)
Axios->>CookieJar: saveCookies(fullUrl, setCookieHeaders)
CookieJar-->>Axios: saved
else Error Response
Server-->>Axios: Error response (may include Set-Cookie)
Axios->>CookieJar: saveCookies(fullUrl, setCookieHeaders)
CookieJar-->>Axios: saved
end
Axios-->>Client: Response or propagated Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-requests/src/scripting/send-request.ts`:
- Around line 56-63: The error handler in send-request.ts currently reads the
raw error?.config?.url before saving cookies; update it to resolve the final
request URL the same way you handled the success path (i.e., account for
config.baseURL + config.url) before calling saveCookies. Locate the error
callback (the arrow function using error, referencing error?.config?.url and
saveCookies) and replace the direct access with the same URL resolution logic
used elsewhere in this module so you pass the fully resolved URL and headers to
saveCookies.
- Around line 51-52: The response interceptor and error handler use
response.config?.url without resolving against response.config?.baseURL, causing
saveCookies(url, ...) to receive relative URLs; update both places to resolve
the final URL the same way the request interceptor does (combine
response.config.url with response.config.baseURL, falling back to
response.request?.responseURL if available) before calling saveCookies,
referencing the response interceptor, error handler, response.config,
config.baseURL, and saveCookies so the saved cookie domain always uses the
fully-resolved URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fb18281-1a1b-43b3-810b-89266c55f45d
📒 Files selected for processing (2)
packages/bruno-requests/src/scripting/send-request.spec.tspackages/bruno-requests/src/scripting/send-request.ts
…docstrings Address CodeRabbit review comments: - attachCookieInterceptors: resolve baseURL+url in response success handler - attachCookieInterceptors: resolve baseURL+url in response error handler - Add JSDoc to attachCookieInterceptors and reposition createSendRequest JSDoc
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-requests/src/scripting/send-request.ts (1)
46-49: Prefer name-aware merge forCookieheader values.At Line 46, direct string concatenation can emit duplicate cookie names when both caller headers and jar contain the same key, which can make precedence backend-dependent. A key-aware merge (caller value wins) would make behavior deterministic.
Possible merge approach
- const merged = existing ? `${existing}; ${cookieString}` : cookieString; + const splitCookies = (header: string) => header + .split(';') + .map((part) => part.trim()) + .filter(Boolean); + const toName = (cookiePart: string) => cookiePart.split('=')[0]?.trim().toLowerCase(); + const existingParts = existing ? splitCookies(existing) : []; + const existingNames = new Set(existingParts.map((part) => toName(part)).filter(Boolean)); + const jarParts = splitCookies(cookieString).filter((part) => !existingNames.has(toName(part))); + const merged = [...existingParts, ...jarParts].join('; ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-requests/src/scripting/send-request.ts` around lines 46 - 49, The current concat logic that builds merged from existing and cookieString can produce duplicate cookie names; update the merge to be name-aware: parse existing and cookieString into individual cookie name=value pairs, build a map where values from the existing header (caller) take precedence over the jar cookies, then reserialize the map back into a single cookie header string and set it via config.headers.set('Cookie', ...). Locate the variables merged, existing, cookieString and the config.headers.set call in send-request.ts and replace the concatenation with this deterministic name-aware merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-requests/src/scripting/send-request.ts`:
- Around line 46-49: The current concat logic that builds merged from existing
and cookieString can produce duplicate cookie names; update the merge to be
name-aware: parse existing and cookieString into individual cookie name=value
pairs, build a map where values from the existing header (caller) take
precedence over the jar cookies, then reserialize the map back into a single
cookie header string and set it via config.headers.set('Cookie', ...). Locate
the variables merged, existing, cookieString and the config.headers.set call in
send-request.ts and replace the concatenation with this deterministic name-aware
merge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: edb7d472-38ab-4d43-a3b9-10374f99b51a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
packages/bruno-requests/src/scripting/send-request.ts
…ose of lowering the automatically assigned tag: size/L because this fix is TINY
Author
|
@helloanoop , @lohit-bruno , @naman-bruno , @bijin-bruno let's get a move on it! :) |
|
Ran into this issue today as well, that I am trying to refresh an access token in a cookie with a pre request script, but the refresh cookie is not sent to the server, and the response probably won't set it to the cookie store either. |
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.
Problem
bru.sendRequest()was completely disconnected from Bruno's global cookie store. This caused two silent failures:Cookies set by earlier requests were never forwarded — session cookies, authentication tokens, and any cookie stored by a previous response were invisible to
bru.sendRequest().Set-Cookieheaders in the scripting response were discarded — the most common case being a CSRF token returned as a cookie from a preflightGETrequest in a pre-request script. The token was silently dropped, so the subsequent main request never received it.This is a direct regression from Postman's
pm.sendRequest(), which automatically participates in Postman's cookie jar. Users migrating from Postman expect the same behaviour frombru.sendRequest().Related issues: #1394, #1493, #5419
Root cause
The main Bruno network layer (
bruno-electron/src/ipc/network/index.js) explicitly manages cookies around every request:bru.sendRequest()inbruno-requests/src/scripting/send-request.tscalledmakeAxiosInstance()with no such hooks, so the sharedcookieJar(backed bytough-cookie) was never read from or written to during scripting requests.Fix
Added
attachCookieInterceptors()— a small helper that registers two Axios interceptors on the instance used bybru.sendRequest():getCookieStringForUrl(url)and injects any matching cookies into the outgoingCookieheader, merging with cookies the caller set explicitly.saveCookies(url, headers)to persist anySet-Cookieheaders back into the shared jar — including on 4xx/5xx responses, since servers often refresh CSRF tokens even on error.Both interceptors swallow their own exceptions so a cookie-jar failure can never abort a user's request.
Before
After
Changes
packages/bruno-requests/src/scripting/send-request.tsattachCookieInterceptors()and call it insidecreateSendRequest()packages/bruno-requests/src/scripting/send-request.spec.tscookie jar integrationtest suite (7 new tests)Testing
All 19 tests pass (
7 new + 12 existing):New tests cover:
CookieheaderSet-Cookieheaders on successSet-Cookieheaders on HTTP error responsesbru.sendRequest→saveCookies→getCookieStringForUrl)Migration / backwards compatibility
No breaking changes. The cookie injection only adds a
Cookieheader when the jar actually contains matching cookies. Requests to domains with no stored cookies are completely unaffected.Contribution Checklist
Summary by CodeRabbit
New Features
Tests