bugfix / Solution for ../ being normalized #6262
Conversation
MrInterBugs
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughA boolean setting Changes
Sequence DiagramsequenceDiagram
actor User
participant Settings as Settings UI
participant Store as Request Store
participant Network as Network Layer
participant RawPath as Raw Path Utils
participant HTTP as HTTP Client
User->>Settings: Toggle "Preserve ../ Segments"
Settings->>Store: Update request.settings.preserveDotSegments
Note over Store,Network: Request execution includes preserveDotSegments
Store->>Network: Execute request (contains flag)
Network->>Network: Read preserveDotSegments
alt Adding OAuth/API query params
Network->>RawPath: addQueryParamPreservingPath(url,key,value,preserveDotSegments)
RawPath-->>Network: URL with preserved/raw path
end
alt preserveDotSegments enabled
Network->>RawPath: buildRawPathTransport(url)
RawPath-->>Network: transport wrapper (rewrites paths)
Network->>Network: attach transport to request
end
Network->>HTTP: Send request (processed URL/transport)
HTTP-->>Network: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/bruno-electron/src/ipc/network/raw-path.js (1)
51-78: Consider clarifying the while loop assignment.Line 62 uses assignment within the while condition (
while ((match = paramRegex.exec(segment)))), which is valid but can be less readable.For slightly improved clarity:
- let match; - let result = segment; - while ((match = paramRegex.exec(segment))) { + let result = segment; + let match = paramRegex.exec(segment); + while (match) { if (match[1]) { let name = match[1].replace(/[')"`]+$/, ''); name = name.replace(/^[('"`]+/, ''); if (name) { const existingPathParam = pathParams.find((param) => param.name === name); if (existingPathParam) { result = result.replace(':' + match[1], existingPathParam.value); } } } + match = paramRegex.exec(segment); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/bruno-app/src/components/RequestPane/Settings/index.js(5 hunks)packages/bruno-app/src/components/Sidebar/NewRequest/index.js(1 hunks)packages/bruno-electron/src/ipc/network/index.js(8 hunks)packages/bruno-electron/src/ipc/network/interpolate-vars.js(3 hunks)packages/bruno-electron/src/ipc/network/raw-path.js(1 hunks)packages/bruno-lang/v2/src/bruToJson.js(2 hunks)packages/bruno-schema/src/collections/index.js(1 hunks)packages/bruno-toml/src/jsonToToml.js(1 hunks)packages/bruno-toml/src/tomlToJson.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/bruno-electron/src/ipc/network/interpolate-vars.js (1)
packages/bruno-electron/src/ipc/network/raw-path.js (1)
buildUrlWithPathParams(80-133)
packages/bruno-app/src/components/Sidebar/NewRequest/index.js (2)
packages/bruno-app/src/components/RequestPane/Settings/index.js (2)
settings(29-29)settings(30-30)packages/bruno-electron/src/ipc/network/prepare-request.js (1)
settings(307-307)
packages/bruno-lang/v2/src/bruToJson.js (2)
packages/bruno-app/src/components/RequestPane/Settings/index.js (2)
settings(29-29)settings(30-30)packages/bruno-electron/src/ipc/network/prepare-request.js (1)
settings(307-307)
packages/bruno-toml/src/tomlToJson.js (3)
packages/bruno-app/src/components/RequestPane/Settings/index.js (2)
settings(29-29)settings(30-30)packages/bruno-lang/v2/src/bruToJson.js (1)
settings(462-462)packages/bruno-electron/src/ipc/network/prepare-request.js (1)
settings(307-307)
packages/bruno-toml/src/jsonToToml.js (3)
packages/bruno-app/src/components/RequestPane/Settings/index.js (2)
settings(29-29)settings(30-30)packages/bruno-lang/v2/src/bruToJson.js (1)
settings(462-462)packages/bruno-electron/src/ipc/network/prepare-request.js (1)
settings(307-307)
🔇 Additional comments (11)
packages/bruno-app/src/components/Sidebar/NewRequest/index.js (1)
210-210: LGTM!The new
preserveDotSegments: falsesetting is correctly initialized alongsideencodeUrl: falsefor curl-derived requests, maintaining consistency with the existing pattern.packages/bruno-lang/v2/src/bruToJson.js (1)
472-474: LGTM!The
preserveDotSegmentsparsing and assignment follows the established pattern for other settings likefollowRedirects, correctly handling both boolean and string 'true' values.Also applies to: 505-507
packages/bruno-schema/src/collections/index.js (1)
529-529: LGTM!The
preserveDotSegmentsfield is correctly added to the settings schema as a nullable boolean, consistent with other settings likeencodeUrlandfollowRedirects.packages/bruno-electron/src/ipc/network/interpolate-vars.js (1)
4-4: LGTM!The refactoring to use
buildUrlWithPathParamsfrom the new raw-path module cleanly integrates thepreserveDotSegmentsflag while simplifying the path parameter interpolation logic.Also applies to: 37-37, 151-151
packages/bruno-app/src/components/RequestPane/Settings/index.js (1)
17-18: LGTM!The
preserveDotSegmentssetting is properly integrated into the Settings component with:
- Sensible default value (false)
- Toggle handler following the established pattern
- Clear UI with descriptive label and explanation
Also applies to: 30-30, 49-50, 126-134
packages/bruno-electron/src/ipc/network/index.js (1)
22-22: LGTM!The
preserveDotSegmentsfeature is comprehensively implemented across all OAuth2 grant types and API key query parameter additions. The transport override logic correctly activates when the flag is enabled, providing the raw path handling needed for dot-segment preservation.Also applies to: 146-146, 176-176, 187-187, 198-198, 209-209, 265-265, 274-279
packages/bruno-electron/src/ipc/network/raw-path.js (5)
1-7: LGTM on imports and query extraction.The imports and
getRawQueryStringimplementation are clean and appropriate for the use case.
9-9: LGTM on URL scheme detection.The regex correctly identifies absolute URLs with standard URI schemes.
99-133: LGTM on dual-mode path construction.Both modes handle path parameter interpolation correctly:
- Non-preserve mode normalizes the path by filtering empty segments
- Preserve mode carefully maintains leading/trailing slashes and dot segments
The implementation aligns well with the PR objectives.
135-174: LGTM on transport wrapper implementation.The transport adapter correctly:
- Handles invalid inputs gracefully by returning undefined
- Merges query parameters from both the raw path and options
- Selects the appropriate protocol (http/https)
- Preserves the raw path including dot segments
176-180: LGTM on module exports.The exports clearly expose the three main functions as documented in the file summary.
|
@MrInterBugs, thank you for your PR contribution. I will review it and share it with the team |
MrInterBugs
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-electron/src/ipc/network/raw-path.js (1)
75-80: Consider simplifying quote removal and using template literals.The quote-stripping logic spans two separate regex replacements which makes it harder to follow. Additionally, line 80 uses string concatenation instead of template literals.
Consider this refactor for clarity:
- let name = match[1].replace(/[')"`]+$/, ''); - name = name.replace(/^[('"`]+/, ''); + let name = match[1].replace(/^[('"`]+|[')"`]+$/g, ''); if (name) { const existingPathParam = pathParams.find((param) => param.name === name); if (existingPathParam) { - result = result.replace(':' + match[1], existingPathParam.value); + result = result.replace(`:${match[1]}`, existingPathParam.value); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-electron/src/ipc/network/raw-path.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-electron/src/ipc/network/raw-path.js
🧠 Learnings (1)
📚 Learning: 2025-12-02T07:24:50.299Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.299Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-electron/src/ipc/network/raw-path.js
🧬 Code graph analysis (1)
packages/bruno-electron/src/ipc/network/raw-path.js (1)
packages/bruno-electron/src/ipc/network/interpolate-vars.js (4)
require(1-1)require(2-2)require(4-4)preserveDotSegments(37-37)
🔇 Additional comments (3)
packages/bruno-electron/src/ipc/network/raw-path.js (3)
27-60: LGTM! Query parameter handling is correct.The function properly handles both preserve and non-preserve modes, correctly overwriting existing parameters using
searchParams.set()in all code paths.
104-110: Error handling is properly implemented.The function correctly throws an Error instance with the original error details preserved, as addressed in previous review feedback.
189-193: Module exports are correct.The three exported functions match the main utilities implemented in this module and follow proper CommonJS patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/bruno-electron/src/ipc/network/raw-path.js (2)
62-89: Align param name matching in function-style segments with other usages.In
func(:param)segments,paramRegexuses\w+, so names likeuser-idorfoo.barwon’t be interpolated even though:user-idworks in plain segments. Consider widening the character class to stay consistent:- if (/^[A-Za-z0-9_.-]+\([^)]*\)$/.test(segment)) { - const paramRegex = /[:](\w+)/g; + if (/^[A-Za-z0-9_.-]+\([^)]*\)$/.test(segment)) { + const paramRegex = /:([A-Za-z0-9_.-]+)/g;This keeps function-style matching in line with the allowed characters you already use for the function name.
148-187: Confirm behavior whenoptions.pathis an absolute URL.The raw-path transport logic and query handling look good overall. One behavior to double‑check:
const useProxyPath = isAbsolutePath(options.path) && origin; const nextPath = useProxyPath ? `${origin}${rawPathWithQuery}` : rawPathWithQuery;If
options.pathis an absolute URL, its host is effectively ignored and the request path is forced to${origin}${rawPathWithQuery}. If that’s the intended “proxy through base origin” behavior, great; otherwise, you may want to treat absoluteoptions.pathdifferently (or normalize it before this check).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-electron/src/ipc/network/raw-path.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-electron/src/ipc/network/raw-path.js
🧠 Learnings (1)
📚 Learning: 2025-12-02T07:24:50.299Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.299Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-electron/src/ipc/network/raw-path.js
🔇 Additional comments (4)
packages/bruno-electron/src/ipc/network/raw-path.js (4)
4-9: Helpers for query string and absolute URL detection look good.Behavior for missing
?and non-string inputs is sensible, and the absolute-URL regex is appropriately strict for<scheme>://detection.
11-25: Raw path extraction is consistent with raw-transport expectations.The logic to strip scheme/host, normalize the path, and return
undefinedfor non-absolute or malformed URLs aligns with howbuildRawPathTransportshort-circuits.
27-60: Query parameter handling with and without preserve mode looks correct.Using
URLSearchParams.setin both the fast-path and fallback gives predictable overwrites, and the preserve branch correctly keeps the original path and hash intact.
189-193: Exports align with the intended public surface.The module exports are minimal and match the helpers actually used elsewhere in the PR.
| const buildUrlWithPathParams = (targetUrl, pathParams, preserveDotSegments) => { | ||
| if (!pathParams?.length) { | ||
| return targetUrl; | ||
| } | ||
|
|
||
| let url = targetUrl; | ||
| const urlSearchRaw = getRawQueryString(targetUrl); | ||
|
|
||
| if (!url.startsWith('http://') && !url.startsWith('https://')) { | ||
| url = `http://${url}`; | ||
| } | ||
|
|
||
| let parsed; | ||
| try { | ||
| parsed = new URL(url); | ||
| } catch (e) { | ||
| const error = new Error('Invalid URL format'); | ||
| error.originalError = e.message; | ||
| throw error; | ||
| } | ||
|
|
||
| if (!preserveDotSegments) { | ||
| const urlPathnameInterpolatedWithPathParams = parsed.pathname | ||
| .split('/') | ||
| .filter((path) => path !== '') | ||
| .map((path) => '/' + interpolatePathSegments(path, pathParams)) | ||
| .join(''); | ||
|
|
||
| const trailingSlash = parsed.pathname.endsWith('/') ? '/' : ''; | ||
| return parsed.origin + urlPathnameInterpolatedWithPathParams + trailingSlash + urlSearchRaw; | ||
| } | ||
|
|
||
| const origin = parsed.origin; | ||
| const pathAndAfter = url.slice(origin.length) || '/'; | ||
| const pathname = (pathAndAfter.split(/[?#]/)[0] || ''); | ||
| const hasLeadingSlash = pathname.startsWith('/'); | ||
| const endsWithSlash = pathname.endsWith('/'); | ||
| const interpolatedSegments = pathname | ||
| .split('/') | ||
| .filter((segment) => segment !== '') | ||
| .map((segment) => interpolatePathSegments(segment, pathParams)) | ||
| .join('/'); | ||
|
|
||
| let rebuiltPath = interpolatedSegments; | ||
| if (hasLeadingSlash) { | ||
| rebuiltPath = `/${rebuiltPath}`; | ||
| } | ||
| if (!rebuiltPath && hasLeadingSlash) { | ||
| rebuiltPath = '/'; | ||
| } | ||
| if (endsWithSlash && rebuiltPath && !rebuiltPath.endsWith('/')) { | ||
| rebuiltPath += '/'; | ||
| } | ||
|
|
||
| return origin + rebuiltPath + urlSearchRaw; | ||
| }; |
There was a problem hiding this comment.
Guard trailing slash re-append to avoid // in edge cases.
The overall structure (non‑preserve vs preserve) looks solid. One edge case in the non‑preserve branch:
If a trailing /:param/ segment gets interpolated to an empty string, urlPathnameInterpolatedWithPathParams can already be '/', while parsed.pathname.endsWith('/') is also true, so you end up returning origin + '/' + '/' + urlSearchRaw (https://example.com//).
You can guard the extra slash similarly to the preserve branch:
- if (!preserveDotSegments) {
- const urlPathnameInterpolatedWithPathParams = parsed.pathname
- .split('/')
- .filter((path) => path !== '')
- .map((path) => '/' + interpolatePathSegments(path, pathParams))
- .join('');
-
- const trailingSlash = parsed.pathname.endsWith('/') ? '/' : '';
- return parsed.origin + urlPathnameInterpolatedWithPathParams + trailingSlash + urlSearchRaw;
- }
+ if (!preserveDotSegments) {
+ const urlPathnameInterpolatedWithPathParams = parsed.pathname
+ .split('/')
+ .filter((path) => path !== '')
+ .map((path) => '/' + interpolatePathSegments(path, pathParams))
+ .join('');
+
+ const shouldAppendTrailingSlash =
+ parsed.pathname.endsWith('/') && !urlPathnameInterpolatedWithPathParams.endsWith('/');
+
+ return parsed.origin
+ + urlPathnameInterpolatedWithPathParams
+ + (shouldAppendTrailingSlash ? '/' : '')
+ + urlSearchRaw;
+ }This keeps existing behavior while avoiding accidental double slashes when a trailing segment disappears after interpolation.
🤖 Prompt for AI Agents
In packages/bruno-electron/src/ipc/network/raw-path.js around lines 91 to 146,
the non-preserve branch can produce a double slash when a trailing segment
interpolates to an empty string because you always append trailingSlash even if
urlPathnameInterpolatedWithPathParams already equals '/' — modify the return
logic to only append the trailing slash when it is needed: compute trailingSlash
as now, but before concatenating check if urlPathnameInterpolatedWithPathParams
is empty or already '/' and only add the trailingSlash when
urlPathnameInterpolatedWithPathParams does not already end with '/' (or is not
just an empty string that you've converted to '/'), mirroring the guard used in
the preserve branch so you never produce '//' while keeping existing behavior.
|
@MrInterBugs Can you create a new Github Issue outlining feature please ? We'd like to use the PR discussion to review the implementation itself, while have a broader feature discussion in the github issue |
|
No worries I think I linked to it further up but it is github.com/usebruno/bruno/issues/6189. |
3 participants
Description
This is a much cleaner attempt of a solution which allows ../../ to be included in requests sent through bruno without changing the behavior of any other requests.
It does this using a toggle to allow a curl like "path-as-is" otherwise it uses the normal bruno adapater for axios.
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.