feat(auth): akamai auth integration#8199
Open
shubh-bruno wants to merge 6 commits into
Open
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughAdds Akamai EdgeGrid authentication end-to-end: UI (request/collection/folder), signing helper and axios interceptor, language/filestore support, Postman converters, runtime wiring (CLI/Electron), and extensive unit and end-to-end tests with fixtures. ChangesEdgeGrid Authentication Support
Sequence Diagram (high-level request flow): sequenceDiagram
participant UI as User (UI)
participant App as Bruno App
participant Redux as Redux Store
participant Prepare as prepareRequest
participant Axios as Axios
participant Sign as signEdgeGridRequest
participant Server as EdgeGrid verifier
UI->>App: Configure EdgeGrid auth
App->>Redux: dispatch updateAuth/updateCollectionAuth/updateFolderAuth
UI->>App: Send request
App->>Prepare: prepareRequest builds edgeGridConfig
Prepare->>Axios: axiosRequest.edgeGridConfig set
Axios->>Sign: addEdgeGridInterceptor invoked
Sign->>Sign: compute Authorization header
Axios->>Server: send signed request
Server->>Server: verify signature
Server->>Axios: response
Axios->>App: return response
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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: 6
🧹 Nitpick comments (9)
packages/bruno-app/src/components/CollectionSettings/Auth/EdgeGridAuth/index.tsx (2)
142-144: ⚡ Quick winRemove unnecessary fragment wrapper.
The fragment on lines 142-144 wrapping
ADVANCED_FIELDS.map(renderField)is redundant. Per coding guidelines, avoid unnecessary wrapper elements to minimize diffs and keep code clean.♻️ Simplify JSX
- <> - {ADVANCED_FIELDS.map(renderField)} - </> + {ADVANCED_FIELDS.map(renderField)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/CollectionSettings/Auth/EdgeGridAuth/index.tsx` around lines 142 - 144, Remove the unnecessary fragment wrapping ADVANCED_FIELDS.map(renderField) in the EdgeGridAuth component; replace the fragment-wrapped JSX with the expression {ADVANCED_FIELDS.map(renderField)} directly (locate the fragment around ADVANCED_FIELDS and renderField in index.tsx) and ensure the surrounding JSX remains valid after the fragment is removed.Source: Coding guidelines
100-146: ⚡ Quick winAdd data-testid attributes for Playwright testing.
Per coding guidelines, testable elements should include
data-testidattributes. Consider adding test IDs to field containers, the advanced settings toggle, and input wrappers to enable stable E2E test selection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/CollectionSettings/Auth/EdgeGridAuth/index.tsx` around lines 100 - 146, Add data-testid attributes to make elements selectable for tests: in renderField (EdgeGridFieldConfig) add data-testid on the outer div (e.g., data-testid={`edgegrid-field-${key}`}) and on the SingleLineEditor wrapper div (e.g., data-testid={`edgegrid-input-${key}`}); add a data-testid to the advanced settings header element rendered for ADVANCED_FIELDS (e.g., data-testid="edgegrid-advanced-header") and to the advanced fields container (e.g., data-testid="edgegrid-advanced-fields"); also add a data-testid to the SensitiveFieldWarning instance (e.g., data-testid={`edgegrid-warning-${key}`}) so Playwright can target the warning. Ensure you update usages around renderField, BASIC_FIELDS, ADVANCED_FIELDS, SingleLineEditor, SensitiveFieldWarning and StyledWrapper to include these attributes.Source: Coding guidelines
packages/bruno-app/src/components/RequestPane/Auth/EdgeGridAuth/index.tsx (2)
106-154: ⚡ Quick winAdd data-testid attributes for Playwright testing.
Per coding guidelines, testable elements should include
data-testidattributes for stable E2E test selection. Consider adding test IDs to field containers, the advanced settings section, and input wrappers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/RequestPane/Auth/EdgeGridAuth/index.tsx` around lines 106 - 154, Add stable data-testid attributes to the rendered DOM elements for Playwright: update the renderField function to add data-testid on the outer field container (use the EdgeGridFieldConfig key e.g., data-testid={`edgegrid-field-${key}`}) and on the input wrapper div (single-line-editor-wrapper) and add a specific id for secret warnings (e.g., data-testid={`edgegrid-secret-warning-${key}`}) when rendering SensitiveFieldWarning; also add a test id to the advanced settings header (the div with className "advanced-settings-header", e.g., data-testid="edgegrid-advanced-settings") so both BASIC_FIELDS and ADVANCED_FIELDS rendered via renderField are testable.Source: Coding guidelines
150-152: ⚡ Quick winRemove unnecessary fragment wrapper.
The fragment on lines 150-152 wrapping
ADVANCED_FIELDS.map(renderField)is redundant. Per coding guidelines, avoid unnecessary wrapper elements.♻️ Simplify JSX
- <> - {ADVANCED_FIELDS.map(renderField)} - </> + {ADVANCED_FIELDS.map(renderField)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/RequestPane/Auth/EdgeGridAuth/index.tsx` around lines 150 - 152, The JSX fragment wrapping ADVANCED_FIELDS.map(renderField) is redundant; remove the <>...</> wrapper and render ADVANCED_FIELDS.map(renderField) directly in the component returned by the function where ADVANCED_FIELDS and renderField are used (in index.tsx) so the output is identical but without the unnecessary fragment element.Source: Coding guidelines
packages/bruno-app/src/components/FolderSettings/Auth/index.js (1)
216-228: ⚡ Quick winRemove unnecessary fragment wrapper.
The fragment wrapping the single
<EdgeGridAuth>component is redundant. Other auth mode cases return the component directly without a fragment (see lines 104-113 for basic auth, etc.). Only theoauth2case uses a fragment because it renders multiple children.♻️ Simplify case statement
case 'edgegrid': { - return ( - <> - <EdgeGridAuth - collection={collection} - item={folder} - updateAuth={updateFolderAuth} - request={request} - save={() => handleSave()} - /> - </> - ); + return ( + <EdgeGridAuth + collection={collection} + item={folder} + updateAuth={updateFolderAuth} + request={request} + save={() => handleSave()} + /> + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/FolderSettings/Auth/index.js` around lines 216 - 228, In the case 'edgegrid' branch remove the unnecessary React fragment and return the EdgeGridAuth component directly; update the switch case that renders EdgeGridAuth (currently wrapped in <></>) to simply return <EdgeGridAuth collection={collection} item={folder} updateAuth={updateFolderAuth} request={request} save={() => handleSave()} /> preserving all props and handlers (EdgeGridAuth, updateFolderAuth, handleSave, collection, folder, request).Source: Coding guidelines
tests/auth/edgegrid/fixtures/collections/yml/EdgeGrid GET 401.yml (1)
15-20: ⚡ Quick winStrengthen the negative-path assertion.
Add an assertion for
res.body.authenticated == "false"so this test verifies behavior, not only status code.As per coding guidelines, tests should “cover… problematic paths” and “use multiple assertions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/auth/edgegrid/fixtures/collections/yml/EdgeGrid` GET 401.yml around lines 15 - 20, Add a second assertion under runtime.assertions to check the response body for authentication state: add an assertion with expression "res.body.authenticated" operator "eq" and value "false" so the test verifies the negative-path response content in addition to the status check; update the assertions array that currently contains the "res.status eq 401" entry (the runtime.assertions block) to include this new assertion.Source: Coding guidelines
tests/auth/edgegrid/edgegrid.spec.ts (1)
60-62: ⚡ Quick winUse
afterEachinstead ofafterAllfor per-test cleanup.Closing collections after all tests means the cleanup only runs once at the end of the suite. For proper test isolation, use
afterEachto clean up after each individual test.♻️ Proposed fix
- test.afterAll(async ({ page }) => { + test.afterEach(async ({ page }) => { await closeAllCollections(page); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/auth/edgegrid/edgegrid.spec.ts` around lines 60 - 62, The test cleanup currently runs in test.afterAll which only executes once after the whole suite; change it to test.afterEach so closeAllCollections(page) runs after each test for proper isolation. Locate the teardown hook using closeAllCollections in the edgegrid.spec.ts test suite and replace the test.afterAll(async ({ page }) => { await closeAllCollections(page); }); hook with test.afterEach(async ({ page }) => { await closeAllCollections(page); }); ensuring the same async signature and page fixture are preserved.tests/auth/edgegrid/edgegrid-runner.spec.ts (2)
43-43: ⚡ Quick winAdd type annotation for the
pageparameter.Same as the previous helper - add the
Pagetype annotation.📝 Proposed fix
-const runAndValidate = async (page, collectionName: string) => { +const runAndValidate = async (page: Page, collectionName: string) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/auth/edgegrid/edgegrid-runner.spec.ts` at line 43, The helper function runAndValidate currently has an untyped first parameter; add the Playwright Page type by changing the signature to accept page: Page and ensure Page is imported (e.g., from '`@playwright/test`' or your existing Playwright import) so the helper matches the other helpers' typings; update any related usages if needed to satisfy the new type.
31-31: ⚡ Quick winAdd type annotation for the
pageparameter.The
pageparameter is missing a type annotation. For consistency and type safety, add the PlaywrightPagetype.📝 Proposed fix
-const sendAllRequests = async (page, collectionName: string) => { +const sendAllRequests = async (page: Page, collectionName: string) => {And add the import:
import { test, Page } from '../../../playwright';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/auth/edgegrid/edgegrid-runner.spec.ts` at line 31, The sendAllRequests function is missing a type on its first parameter; update the signature of sendAllRequests to annotate the page parameter with Playwright's Page type (e.g., page: Page) and add the corresponding import for Page from your playwright test harness (as suggested, import Page from '../../../playwright' alongside test) so the function signature and imports use the Playwright Page type for proper type safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/bruno-app/src/components/CollectionSettings/Auth/EdgeGridAuth/StyledWrapper.js`:
- Around line 54-83: The styled-component uses hardcoded colors in selectors
.advanced-settings-hint, .field-info svg, and .field-tooltip (background-color
and color); replace those literal color values with theme tokens accessed via
the styled-component props (e.g. props.theme.colors.* or
props.theme.font.color.*) so the styles read from the app theme instead of fixed
hex/rgb values—update the color for .advanced-settings-hint, the svg color
inside .field-info, and both background-color and color inside .field-tooltip to
use the appropriate theme keys.
In
`@packages/bruno-app/src/components/RequestPane/Auth/EdgeGridAuth/StyledWrapper.js`:
- Around line 54-83: The CSS in StyledWrapper.js hardcodes several colors
instead of using the styled-components theme: replace the literal color values
in .advanced-settings-hint (rgb(107 114 128)), .field-info svg (rgb(107 114
128)), and .field-tooltip (background-color: `#374151` and color: `#fff`) with the
appropriate props.theme tokens used across the app (e.g., theme color tokens for
gray/neutral text and tooltip bg/foreground) so the styles read from props.theme
(like the existing font size usage) and thus support theming/dark mode; update
the selectors .advanced-settings-hint, .field-info svg, and .field-tooltip
accordingly.
In `@packages/bruno-requests/src/auth/edgegrid-helper.js`:
- Around line 1-2: The file uses CommonJS and plain JavaScript causing loss of
types for addEdgeGridInterceptor and signEdgeGridRequest; convert it to
TypeScript by renaming to edgegrid-helper.ts, replace require(...) with ES
module imports (e.g., import crypto from 'crypto' and import { URL } from
'node:url'), add exported TypeScript interfaces/types for the EdgeGrid config
and request parameter shapes used by addEdgeGridInterceptor and
signEdgeGridRequest, and update any tests to .ts/.spec.ts accordingly so callers
get strong types instead of any.
In `@packages/bruno-tests/src/auth/edgegrid.js`:
- Around line 97-99: The parsed x-eg-max-body-size header is used directly via
parseInt and can yield NaN or non-positive values that break truncation/hashing;
validate the header value after parseInt in the code that sets maxBodySize (and
before any hashing/truncation) by checking Number.isInteger(parsed) && parsed >
0 (optionally clamp to a sensible MAX_LIMIT) and fallback to
MAX_BODY_SIZE_DEFAULT when invalid, ensuring all uses of maxBodySize
(truncation/hashing routines) rely on this validated value.
In `@tests/auth/edgegrid/edgegrid-runner.spec.ts`:
- Around line 53-79: Tests in edgegrid-runner.spec.ts are using the shared
fixture pageWithUserData (violates parallel-safety); change each test to use the
standard page fixture and create an isolated temp user-data dir per test (use
createTmpDir or equivalent) before launching/visiting the app, then pass that
page into the existing helpers (sendAllRequests and runAndValidate) unchanged;
update the two test.describe blocks so the test signatures use async ({ page })
instead of ({ pageWithUserData: page }) and ensure any setup/teardown that
referenced the shared pageWithUserData (e.g., closeAllCollections) uses the
per-test temp dir or per-test cleanup tied to createTmpDir.
In `@tests/import/postman/import-edgegrid-collection.spec.ts`:
- Around line 88-103: Capture the original dialog.showOpenDialog value by having
test.beforeAll call await electronApp.evaluate(() => dialog.showOpenDialog) and
assign that return value to the outer-scope originalShowOpenDialog variable,
then in test.afterAll restore it by calling await
electronApp.evaluate((original) => { dialog.showOpenDialog = original },
originalShowOpenDialog) (with a null/undefined guard before restoring); keep the
existing closeAllCollections(page) call so cleanup runs, ensuring the stub is
restored in the renderer/main context explicitly and reliably.
---
Nitpick comments:
In
`@packages/bruno-app/src/components/CollectionSettings/Auth/EdgeGridAuth/index.tsx`:
- Around line 142-144: Remove the unnecessary fragment wrapping
ADVANCED_FIELDS.map(renderField) in the EdgeGridAuth component; replace the
fragment-wrapped JSX with the expression {ADVANCED_FIELDS.map(renderField)}
directly (locate the fragment around ADVANCED_FIELDS and renderField in
index.tsx) and ensure the surrounding JSX remains valid after the fragment is
removed.
- Around line 100-146: Add data-testid attributes to make elements selectable
for tests: in renderField (EdgeGridFieldConfig) add data-testid on the outer div
(e.g., data-testid={`edgegrid-field-${key}`}) and on the SingleLineEditor
wrapper div (e.g., data-testid={`edgegrid-input-${key}`}); add a data-testid to
the advanced settings header element rendered for ADVANCED_FIELDS (e.g.,
data-testid="edgegrid-advanced-header") and to the advanced fields container
(e.g., data-testid="edgegrid-advanced-fields"); also add a data-testid to the
SensitiveFieldWarning instance (e.g., data-testid={`edgegrid-warning-${key}`})
so Playwright can target the warning. Ensure you update usages around
renderField, BASIC_FIELDS, ADVANCED_FIELDS, SingleLineEditor,
SensitiveFieldWarning and StyledWrapper to include these attributes.
In `@packages/bruno-app/src/components/FolderSettings/Auth/index.js`:
- Around line 216-228: In the case 'edgegrid' branch remove the unnecessary
React fragment and return the EdgeGridAuth component directly; update the switch
case that renders EdgeGridAuth (currently wrapped in <></>) to simply return
<EdgeGridAuth collection={collection} item={folder}
updateAuth={updateFolderAuth} request={request} save={() => handleSave()} />
preserving all props and handlers (EdgeGridAuth, updateFolderAuth, handleSave,
collection, folder, request).
In `@packages/bruno-app/src/components/RequestPane/Auth/EdgeGridAuth/index.tsx`:
- Around line 106-154: Add stable data-testid attributes to the rendered DOM
elements for Playwright: update the renderField function to add data-testid on
the outer field container (use the EdgeGridFieldConfig key e.g.,
data-testid={`edgegrid-field-${key}`}) and on the input wrapper div
(single-line-editor-wrapper) and add a specific id for secret warnings (e.g.,
data-testid={`edgegrid-secret-warning-${key}`}) when rendering
SensitiveFieldWarning; also add a test id to the advanced settings header (the
div with className "advanced-settings-header", e.g.,
data-testid="edgegrid-advanced-settings") so both BASIC_FIELDS and
ADVANCED_FIELDS rendered via renderField are testable.
- Around line 150-152: The JSX fragment wrapping
ADVANCED_FIELDS.map(renderField) is redundant; remove the <>...</> wrapper and
render ADVANCED_FIELDS.map(renderField) directly in the component returned by
the function where ADVANCED_FIELDS and renderField are used (in index.tsx) so
the output is identical but without the unnecessary fragment element.
In `@tests/auth/edgegrid/edgegrid-runner.spec.ts`:
- Line 43: The helper function runAndValidate currently has an untyped first
parameter; add the Playwright Page type by changing the signature to accept
page: Page and ensure Page is imported (e.g., from '`@playwright/test`' or your
existing Playwright import) so the helper matches the other helpers' typings;
update any related usages if needed to satisfy the new type.
- Line 31: The sendAllRequests function is missing a type on its first
parameter; update the signature of sendAllRequests to annotate the page
parameter with Playwright's Page type (e.g., page: Page) and add the
corresponding import for Page from your playwright test harness (as suggested,
import Page from '../../../playwright' alongside test) so the function signature
and imports use the Playwright Page type for proper type safety.
In `@tests/auth/edgegrid/edgegrid.spec.ts`:
- Around line 60-62: The test cleanup currently runs in test.afterAll which only
executes once after the whole suite; change it to test.afterEach so
closeAllCollections(page) runs after each test for proper isolation. Locate the
teardown hook using closeAllCollections in the edgegrid.spec.ts test suite and
replace the test.afterAll(async ({ page }) => { await closeAllCollections(page);
}); hook with test.afterEach(async ({ page }) => { await
closeAllCollections(page); }); ensuring the same async signature and page
fixture are preserved.
In `@tests/auth/edgegrid/fixtures/collections/yml/EdgeGrid` GET 401.yml:
- Around line 15-20: Add a second assertion under runtime.assertions to check
the response body for authentication state: add an assertion with expression
"res.body.authenticated" operator "eq" and value "false" so the test verifies
the negative-path response content in addition to the status check; update the
assertions array that currently contains the "res.status eq 401" entry (the
runtime.assertions block) to include this new assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c20896bd-8017-40fc-b1b9-59023ce11c2c
📒 Files selected for processing (58)
packages/bruno-app/src/components/CollectionSettings/Auth/AuthMode/index.jspackages/bruno-app/src/components/CollectionSettings/Auth/EdgeGridAuth/StyledWrapper.jspackages/bruno-app/src/components/CollectionSettings/Auth/EdgeGridAuth/index.tsxpackages/bruno-app/src/components/CollectionSettings/Auth/index.jspackages/bruno-app/src/components/FolderSettings/Auth/index.jspackages/bruno-app/src/components/FolderSettings/AuthMode/index.jspackages/bruno-app/src/components/RequestPane/Auth/AuthMode/index.jspackages/bruno-app/src/components/RequestPane/Auth/EdgeGridAuth/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/Auth/EdgeGridAuth/index.tsxpackages/bruno-app/src/components/RequestPane/Auth/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/collections/index.jspackages/bruno-cli/src/runner/prepare-request.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-converters/src/postman/bruno-to-postman.jspackages/bruno-converters/src/postman/postman-to-bruno.jspackages/bruno-converters/tests/postman/edgegrid-auth.spec.jspackages/bruno-converters/tests/postman/postman-to-bruno/process-auth.spec.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/network/prepare-request.jspackages/bruno-filestore/src/formats/yml/common/auth.tspackages/bruno-lang/v2/src/bruToJson.jspackages/bruno-lang/v2/src/collectionBruToJson.jspackages/bruno-lang/v2/src/jsonToBru.jspackages/bruno-lang/v2/src/jsonToCollectionBru.jspackages/bruno-requests/src/auth/edgegrid-helper.jspackages/bruno-requests/src/auth/edgegrid-helper.spec.jspackages/bruno-requests/src/auth/index.tspackages/bruno-requests/src/index.tspackages/bruno-schema-types/src/common/auth.tspackages/bruno-schema/src/collections/index.jspackages/bruno-tests/src/auth/edgegrid.jspackages/bruno-tests/src/auth/index.jstests/auth/edgegrid/edgegrid-levels.spec.tstests/auth/edgegrid/edgegrid-runner.spec.tstests/auth/edgegrid/edgegrid.spec.tstests/auth/edgegrid/fixtures/collections/bru/EdgeGrid Base URL Override 200.brutests/auth/edgegrid/fixtures/collections/bru/EdgeGrid GET 200.brutests/auth/edgegrid/fixtures/collections/bru/EdgeGrid GET 401.brutests/auth/edgegrid/fixtures/collections/bru/EdgeGrid Inherit 200.brutests/auth/edgegrid/fixtures/collections/bru/EdgeGrid POST JSON 200.brutests/auth/edgegrid/fixtures/collections/bru/EdgeGrid POST Pretty JSON 200.brutests/auth/edgegrid/fixtures/collections/bru/EdgeGrid Signed Headers 200.brutests/auth/edgegrid/fixtures/collections/bru/bruno.jsontests/auth/edgegrid/fixtures/collections/bru/collection.brutests/auth/edgegrid/fixtures/collections/bru/environments/Local.brutests/auth/edgegrid/fixtures/collections/yml/EdgeGrid Base URL Override 200.ymltests/auth/edgegrid/fixtures/collections/yml/EdgeGrid GET 200.ymltests/auth/edgegrid/fixtures/collections/yml/EdgeGrid GET 401.ymltests/auth/edgegrid/fixtures/collections/yml/EdgeGrid Inherit 200.ymltests/auth/edgegrid/fixtures/collections/yml/EdgeGrid POST JSON 200.ymltests/auth/edgegrid/fixtures/collections/yml/EdgeGrid POST Pretty JSON 200.ymltests/auth/edgegrid/fixtures/collections/yml/EdgeGrid Signed Headers 200.ymltests/auth/edgegrid/fixtures/collections/yml/environments/Local.ymltests/auth/edgegrid/fixtures/collections/yml/opencollection.ymltests/auth/edgegrid/init-user-data/preferences.jsontests/import/postman/fixtures/postman-edgegrid-collection.jsontests/import/postman/import-edgegrid-collection.spec.ts
Comment on lines
+54
to
+83
| .advanced-settings-hint { | ||
| margin: -0.25rem 0 0.75rem; | ||
| font-size: ${(props) => props.theme.font.size.sm}; | ||
| color: rgb(107 114 128); | ||
| } | ||
|
|
||
| .field-info { | ||
| position: relative; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| margin-left: 6px; | ||
| cursor: pointer; | ||
|
|
||
| svg { | ||
| color: rgb(107 114 128); | ||
| } | ||
|
|
||
| .field-tooltip { | ||
| position: absolute; | ||
| left: 0; | ||
| bottom: 100%; | ||
| z-index: 10; | ||
| width: max-content; | ||
| max-width: 15rem; | ||
| margin-bottom: 0.25rem; | ||
| padding: 0.5rem; | ||
| border-radius: 0.375rem; | ||
| background-color: #374151; | ||
| color: #fff; | ||
| font-size: 0.75rem; |
Contributor
There was a problem hiding this comment.
Replace hardcoded colors with theme tokens
Line 57, Line 68, Line 81, and Line 82 use hardcoded color values inside a styled-component. This breaks the theme-driven color rule and can drift across themes.
Suggested patch
.advanced-settings-hint {
margin: -0.25rem 0 0.75rem;
font-size: ${(props) => props.theme.font.size.sm};
- color: rgb(107 114 128);
+ color: ${(props) => props.theme.colors.text.subtext2};
}
.field-info {
@@
svg {
- color: rgb(107 114 128);
+ color: ${(props) => props.theme.colors.text.subtext2};
}
.field-tooltip {
@@
- background-color: `#374151`;
- color: `#fff`;
+ background-color: ${(props) => props.theme.colors.surface.highlight};
+ color: ${(props) => props.theme.colors.text.default};As per coding guidelines, “Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/bruno-app/src/components/CollectionSettings/Auth/EdgeGridAuth/StyledWrapper.js`
around lines 54 - 83, The styled-component uses hardcoded colors in selectors
.advanced-settings-hint, .field-info svg, and .field-tooltip (background-color
and color); replace those literal color values with theme tokens accessed via
the styled-component props (e.g. props.theme.colors.* or
props.theme.font.color.*) so the styles read from the app theme instead of fixed
hex/rgb values—update the color for .advanced-settings-hint, the svg color
inside .field-info, and both background-color and color inside .field-tooltip to
use the appropriate theme keys.
Source: Coding guidelines
Comment on lines
+54
to
+83
| .advanced-settings-hint { | ||
| margin: -0.25rem 0 0.75rem; | ||
| font-size: ${(props) => props.theme.font.size.sm}; | ||
| color: rgb(107 114 128); | ||
| } | ||
|
|
||
| .field-info { | ||
| position: relative; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| margin-left: 6px; | ||
| cursor: pointer; | ||
|
|
||
| svg { | ||
| color: rgb(107 114 128); | ||
| } | ||
|
|
||
| .field-tooltip { | ||
| position: absolute; | ||
| left: 0; | ||
| bottom: 100%; | ||
| z-index: 10; | ||
| width: max-content; | ||
| max-width: 15rem; | ||
| margin-bottom: 0.25rem; | ||
| padding: 0.5rem; | ||
| border-radius: 0.375rem; | ||
| background-color: #374151; | ||
| color: #fff; | ||
| font-size: 0.75rem; |
Contributor
There was a problem hiding this comment.
Use theme color tokens instead of literal color values
Line 57, Line 68, Line 81, and Line 82 hardcode colors in styled-components. Please route these through props.theme for consistent theming and dark/light compatibility.
As per coding guidelines, “Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/bruno-app/src/components/RequestPane/Auth/EdgeGridAuth/StyledWrapper.js`
around lines 54 - 83, The CSS in StyledWrapper.js hardcodes several colors
instead of using the styled-components theme: replace the literal color values
in .advanced-settings-hint (rgb(107 114 128)), .field-info svg (rgb(107 114
128)), and .field-tooltip (background-color: `#374151` and color: `#fff`) with the
appropriate props.theme tokens used across the app (e.g., theme color tokens for
gray/neutral text and tooltip bg/foreground) so the styles read from props.theme
(like the existing font size usage) and thus support theming/dark mode; update
the selectors .advanced-settings-hint, .field-info svg, and .field-tooltip
accordingly.
Source: Coding guidelines
Comment on lines
+1
to
+2
| const crypto = require('crypto'); | ||
| const { URL } = require('node:url'); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Convert to TypeScript for type safety and consistency.
This file uses JavaScript while other auth helpers (digestauth-helper.ts, oauth1-request-authorization.ts, oauth2-helper.ts) are TypeScript. It also mixes CommonJS imports (require) with ES6 exports (export), which works but is inconsistent. More importantly, without TypeScript or .d.ts definitions, consumers importing from ./auth get any types for addEdgeGridInterceptor and signEdgeGridRequest, losing type safety.
Recommended approach
- Rename to
edgegrid-helper.ts - Replace CommonJS imports with ES6:
-const crypto = require('crypto');
-const { URL } = require('node:url');
+import crypto from 'crypto';
+import { URL } from 'node:url';- Add TypeScript interfaces for config and request parameters
- Update test file to
edgegrid-helper.spec.tsif needed
This aligns with project conventions and provides type safety for the ~10+ callsites across CLI/Electron/UI layers.
Also applies to: 131-131, 193-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-requests/src/auth/edgegrid-helper.js` around lines 1 - 2, The
file uses CommonJS and plain JavaScript causing loss of types for
addEdgeGridInterceptor and signEdgeGridRequest; convert it to TypeScript by
renaming to edgegrid-helper.ts, replace require(...) with ES module imports
(e.g., import crypto from 'crypto' and import { URL } from 'node:url'), add
exported TypeScript interfaces/types for the EdgeGrid config and request
parameter shapes used by addEdgeGridInterceptor and signEdgeGridRequest, and
update any tests to .ts/.spec.ts accordingly so callers get strong types instead
of any.
Comment on lines
+97
to
+99
| const maxBodySize = req.headers['x-eg-max-body-size'] | ||
| ? parseInt(req.headers['x-eg-max-body-size'], 10) | ||
| : MAX_BODY_SIZE_DEFAULT; |
Contributor
There was a problem hiding this comment.
Validate x-eg-max-body-size before using it in hashing logic.
parseInt() is used directly, so NaN/non-positive values can silently alter truncation behavior and make signature checks inconsistent.
Suggested patch
- const maxBodySize = req.headers['x-eg-max-body-size']
- ? parseInt(req.headers['x-eg-max-body-size'], 10)
- : MAX_BODY_SIZE_DEFAULT;
+ const parsedMaxBodySize = req.headers['x-eg-max-body-size']
+ ? Number.parseInt(req.headers['x-eg-max-body-size'], 10)
+ : NaN;
+ const maxBodySize = Number.isInteger(parsedMaxBodySize) && parsedMaxBodySize > 0
+ ? parsedMaxBodySize
+ : MAX_BODY_SIZE_DEFAULT;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-tests/src/auth/edgegrid.js` around lines 97 - 99, The parsed
x-eg-max-body-size header is used directly via parseInt and can yield NaN or
non-positive values that break truncation/hashing; validate the header value
after parseInt in the code that sets maxBodySize (and before any
hashing/truncation) by checking Number.isInteger(parsed) && parsed > 0
(optionally clamp to a sensible MAX_LIMIT) and fallback to MAX_BODY_SIZE_DEFAULT
when invalid, ensuring all uses of maxBodySize (truncation/hashing routines)
rely on this validated value.
Comment on lines
+53
to
+79
| test.afterAll(async ({ pageWithUserData: page }) => { | ||
| await closeAllCollections(page); | ||
| }); | ||
|
|
||
| test.describe('[bru]', () => { | ||
| test('Send individual requests', async ({ pageWithUserData: page }) => { | ||
| test.setTimeout(3 * 60 * 1000); | ||
| await sendAllRequests(page, BRU_COLLECTION); | ||
| }); | ||
|
|
||
| test('Run collection and verify all assertions pass', async ({ pageWithUserData: page }) => { | ||
| test.setTimeout(3 * 60 * 1000); | ||
| await runAndValidate(page, BRU_COLLECTION); | ||
| }); | ||
| }); | ||
|
|
||
| test.describe('[yml]', () => { | ||
| test('Send individual requests', async ({ pageWithUserData: page }) => { | ||
| test.setTimeout(3 * 60 * 1000); | ||
| await sendAllRequests(page, YML_COLLECTION); | ||
| }); | ||
|
|
||
| test('Run collection and verify all assertions pass', async ({ pageWithUserData: page }) => { | ||
| test.setTimeout(3 * 60 * 1000); | ||
| await runAndValidate(page, YML_COLLECTION); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Tests use shared pageWithUserData fixture, violating parallel-safety.
The pageWithUserData fixture suggests a shared user data directory across all tests in this suite. E2E tests must be parallel-safe with isolated temp paths per test. Replace with the standard page fixture and use createTmpDir per test.
♻️ Proposed fix for parallel-safe isolation
test.describe('Akamai EdgeGrid Runner', () => {
- test.afterAll(async ({ pageWithUserData: page }) => {
+ test.afterEach(async ({ page }) => {
await closeAllCollections(page);
});
test.describe('[bru]', () => {
- test('Send individual requests', async ({ pageWithUserData: page }) => {
+ test('Send individual requests', async ({ page }) => {
test.setTimeout(3 * 60 * 1000);
await sendAllRequests(page, BRU_COLLECTION);
});
- test('Run collection and verify all assertions pass', async ({ pageWithUserData: page }) => {
+ test('Run collection and verify all assertions pass', async ({ page }) => {
test.setTimeout(3 * 60 * 1000);
await runAndValidate(page, BRU_COLLECTION);
});
});
test.describe('[yml]', () => {
- test('Send individual requests', async ({ pageWithUserData: page }) => {
+ test('Send individual requests', async ({ page }) => {
test.setTimeout(3 * 60 * 1000);
await sendAllRequests(page, YML_COLLECTION);
});
- test('Run collection and verify all assertions pass', async ({ pageWithUserData: page }) => {
+ test('Run collection and verify all assertions pass', async ({ page }) => {
test.setTimeout(3 * 60 * 1000);
await runAndValidate(page, YML_COLLECTION);
});Based on learnings: E2E tests must be parallel-safe. No shared user data directories, ports, files, DBs, caches, clipboard assumptions, or global app state. Each test gets isolated temp paths and unique workspace/project names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/auth/edgegrid/edgegrid-runner.spec.ts` around lines 53 - 79, Tests in
edgegrid-runner.spec.ts are using the shared fixture pageWithUserData (violates
parallel-safety); change each test to use the standard page fixture and create
an isolated temp user-data dir per test (use createTmpDir or equivalent) before
launching/visiting the app, then pass that page into the existing helpers
(sendAllRequests and runAndValidate) unchanged; update the two test.describe
blocks so the test signatures use async ({ page }) instead of ({
pageWithUserData: page }) and ensure any setup/teardown that referenced the
shared pageWithUserData (e.g., closeAllCollections) uses the per-test temp dir
or per-test cleanup tied to createTmpDir.
Source: Learnings
Comment on lines
+88
to
+103
| let originalShowOpenDialog; | ||
|
|
||
| test.beforeAll(async ({ electronApp }) => { | ||
| await electronApp.evaluate(({ dialog }) => { | ||
| originalShowOpenDialog = dialog.showOpenDialog; | ||
| }); | ||
| // Pin the fixture to the exact key/value pairs we assert in the UI (exact, not a count). | ||
| expect(collectionExpected).toEqual(EXPECTED_COLLECTION); | ||
| expect(requestExpected).toEqual(EXPECTED_REQUEST); | ||
| }); | ||
|
|
||
| test.afterAll(async ({ electronApp, page }) => { | ||
| await closeAllCollections(page); | ||
| await electronApp.evaluate(({ dialog }) => { | ||
| dialog.showOpenDialog = originalShowOpenDialog; | ||
| }); |
Contributor
There was a problem hiding this comment.
Make showOpenDialog stub restoration explicit and context-safe.
The current electronApp.evaluate usage relies on implicit cross-context state for originalShowOpenDialog; if restoration misses, this can leak dialog stubs and destabilize subsequent tests.
Proposed fix
-let originalShowOpenDialog;
-
test.beforeAll(async ({ electronApp }) => {
await electronApp.evaluate(({ dialog }) => {
- originalShowOpenDialog = dialog.showOpenDialog;
+ globalThis.__originalShowOpenDialog = dialog.showOpenDialog;
});
// Pin the fixture to the exact key/value pairs we assert in the UI (exact, not a count).
expect(collectionExpected).toEqual(EXPECTED_COLLECTION);
expect(requestExpected).toEqual(EXPECTED_REQUEST);
});
test.afterAll(async ({ electronApp, page }) => {
await closeAllCollections(page);
await electronApp.evaluate(({ dialog }) => {
- dialog.showOpenDialog = originalShowOpenDialog;
+ if (globalThis.__originalShowOpenDialog) {
+ dialog.showOpenDialog = globalThis.__originalShowOpenDialog;
+ delete globalThis.__originalShowOpenDialog;
+ }
});
});As per coding guidelines, E2E tests must be parallel-safe and include deterministic cleanup with isolated, non-leaky state handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/import/postman/import-edgegrid-collection.spec.ts` around lines 88 -
103, Capture the original dialog.showOpenDialog value by having test.beforeAll
call await electronApp.evaluate(() => dialog.showOpenDialog) and assign that
return value to the outer-scope originalShowOpenDialog variable, then in
test.afterAll restore it by calling await electronApp.evaluate((original) => {
dialog.showOpenDialog = original }, originalShowOpenDialog) (with a
null/undefined guard before restoring); keep the existing
closeAllCollections(page) call so cleanup runs, ensuring the stub is restored in
the renderer/main context explicitly and reliably.
Source: Coding guidelines
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
1 participant
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.
Description
NOT READY FOR REVIEW
JIRA
Add Akamai EdgeGrid (EG1-HMAC-SHA256) authentication
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
New Features
Tests