feat: add HAR capture and runtime help - #338
Merged
Merged
Conversation
Centralize HAR capture configuration in the shared har package so HTTP clients can use consistent property-driven behavior. Support metadata-only and full capture levels, optional replayable credentials, and safer permissions for sensitive archives. Reuse the shared middleware and writer across property-based and client-managed capture.
Add reusable operator help for logging, HTTP tracing, HAR capture, properties, and output formats. Derive the HTTP trace ladder from runtime configuration and cover topic selection and rendering with tests.
WalkthroughChangesHAR capture
Runtime help
Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant Registry
participant HARMiddleware
participant HARCollector
participant WriteFile
HTTPClient->>Registry: request capture transport
Registry->>HARMiddleware: select metadata or full capture
HTTPClient->>HARMiddleware: execute HTTP request
HARMiddleware->>HARCollector: record HAR entry
Registry->>WriteFile: flush collector
WriteFile-->>Registry: write HAR archive
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Gavel summary
Totals: 496 passed · 0 failed · 3 skipped · 15.4s |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
http/client.go (1)
834-839: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInstall the metadata HAR middleware through
harMiddlewares.
HARCollectorappends toharMiddlewares, butHARMetadatacallsClient.Use, which appends totransportMiddlewares. The round-trip appliesharMiddlewaresinternally beforetransportMiddlewares, so metadata capture runs outer to auth headers. Callc.HARCollector(collector)forHARMetadata, or mirrorHARCollector’sharMiddlewaresappend in this branch.🤖 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 `@http/client.go` around lines 834 - 839, Update the HARMetadata branch in the level switch to install the metadata middleware through harMiddlewares, matching HARCollector’s ordering; replace the Client.Use call with c.HARCollector(collector) or the equivalent harMiddlewares append while preserving the existing HARFull behavior.Source: Coding guidelines
🧹 Nitpick comments (2)
har/level.go (1)
36-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueList every accepted value in the error text.
ParseLevelacceptsoff,none,debug,traceandbodies, but the error names onlymetadata,fullanddisabled. An operator who mistypestracesgets an error that hides the synonym they wanted.♻️ Proposed message update
- return def, fmt.Errorf("invalid HAR level %q: expected metadata, full or disabled", value) + return def, fmt.Errorf("invalid HAR level %q: expected one of disabled (off, none), metadata (debug) or full (trace, bodies)", value)🤖 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 `@har/level.go` around lines 36 - 49, Update the invalid-value error returned by ParseLevel to list every accepted non-empty value, including disabled, off, none, metadata, debug, full, trace, and bodies, while preserving the existing parsing behavior.har/registry_test.go (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the previous property value, not the empty string.
setPropertyclears the key on cleanup. Properties are process-global, and this file nests property scopes (TestRegistry_Levelsetshttp.harin the parent test andhttp.har.levelin each subtest). No current test sets the same key at two levels, so the tests pass today. If a future subtest overrides a key its parent set, the subtest cleanup will clear the parent value for the remaining subtests, and the failure will look unrelated.♻️ Proposed change
func setProperty(t *testing.T, key, value string) { t.Helper() + previous := properties.String("", key) properties.Set(key, value) - t.Cleanup(func() { properties.Set(key, "") }) + t.Cleanup(func() { properties.Set(key, previous) }) }🤖 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 `@har/registry_test.go` around lines 28 - 34, Update setProperty to capture the property’s existing value before calling properties.Set, then restore that captured value in t.Cleanup instead of always setting the key to an empty string. Preserve the current test-scoped setup and cleanup behavior for properties without a prior value.
🤖 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 `@har/write.go`:
- Around line 29-37: Update Registry.Flush and the fileMode-related write flow
so that when collector.Config.CaptureSensitive is true, the HAR file mode is
explicitly set to 0600 after os.WriteFile succeeds, including for existing
files; preserve the current 0644 behavior for non-sensitive output.
---
Outside diff comments:
In `@http/client.go`:
- Around line 834-839: Update the HARMetadata branch in the level switch to
install the metadata middleware through harMiddlewares, matching HARCollector’s
ordering; replace the Client.Use call with c.HARCollector(collector) or the
equivalent harMiddlewares append while preserving the existing HARFull behavior.
---
Nitpick comments:
In `@har/level.go`:
- Around line 36-49: Update the invalid-value error returned by ParseLevel to
list every accepted non-empty value, including disabled, off, none, metadata,
debug, full, trace, and bodies, while preserving the existing parsing behavior.
In `@har/registry_test.go`:
- Around line 28-34: Update setProperty to capture the property’s existing value
before calling properties.Set, then restore that captured value in t.Cleanup
instead of always setting the key to an empty string. Preserve the current
test-scoped setup and cleanup behavior for properties without a prior value.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62748455-f844-4c43-b03b-4ea0db2d82af
📒 Files selected for processing (16)
har/har.gohar/level.gohar/metadata.gohar/middleware.gohar/middleware_test.gohar/registry.gohar/registry_test.gohar/write.gohelp/format.gohelp/har.gohelp/help.gohelp/help_suite_test.gohelp/help_test.gohelp/http.gohelp/logging.gohttp/client.go
Comment on lines
+29
to
+37
| return os.WriteFile(path, append(data, '\n'), fileMode(collector)) | ||
| } | ||
|
|
||
| func fileMode(collector *Collector) fs.FileMode { | ||
| if collector != nil && collector.Config.CaptureSensitive { | ||
| return 0o600 | ||
| } | ||
| return 0o644 | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no other code chmods the HAR file after WriteFile, and check
# whether any caller reuses an existing HAR path across runs.
rg -nP -C4 '\bWriteFile\s*\(|\bos\.Chmod\s*\(' --type=go
rg -nP -C6 'func \(r \*Registry\) Flush' --type=goRepository: flanksource/commons
Length of output: 157
🏁 Script executed:
#!/bin/bash
set +e
printf 'Files of interest:\n'
git ls-files | rg '(^|/)har/(write\.go|registry\.go|collector\.go|har\.go|registry_test\.go|.*\.go)$|(^|/)http/client\.go$' || true
printf '\nWriteFile and Chmod grep:\n'
rg -n -C4 '\bWriteFile\s*\(|\bos\.Chmod\s*\(' -t go || true
printf '\nRegistry flush:\n'
rg -n -C6 'func \(r \*Registry\) Flush' -t go || true
printf '\nRelevant source snippets:\n'
for f in har/write.go har/registry.go http/client.go har/registry_test.go; do
if [ -f "$f" ]; then
echo "---- $f ----"
wc -l "$f"
sed -n '1,140p' "$f"
fi
doneRepository: flanksource/commons
Length of output: 437
🏁 Script executed:
#!/bin/bash
set +e
printf 'WriteFile and Chmod grep:\n'
rg -n -C4 'WriteFile\s*\(|\bos\.Chmod\s*\(' -t go || true
printf '\nRegistry flush:\n'
rg -n -C6 'func \(r \*Registry\) Flush' -t go || true
printf '\nWriteHARFile callers:\n'
rg -n -C4 'WriteHARFile\(' -t go || true
printf '\nhar/write.go:\n'
sed -n '1,80p' har/write.go
printf '\nhar/registry.go flush section:\n'
sed -n '80,140p' har/registry.go
printf '\nhttp/client.go around WriteHARFile:\n'
sed -n '820,870p' http/client.go
printf '\nregistry tests relevant section:\n'
sed -n '300,365p' har/registry_test.goRepository: flanksource/commons
Length of output: 183
Sensitive Data Exposure (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: Internal
Reachability path
● Entry
har/middleware_test.go
│
▼
● Hop
http/client.go:352
Transport
│
▼
● Hop
har/registry.go:104
Flush
│
▼
● Sink
har/write.go
Enforce 0600 on an existing sensitive HAR file.
os.WriteFile only applies the mode when it creates the file. If path already exists, Registry.Flush can leave a sensitive archive group- and world-readable, including when it overwrites a prior non-sensitive run. Set the mode explicitly after the write when collector.Config.CaptureSensitive is true.
🔒️ Proposed fix
data, err := json.MarshalIndent(file, "", " ")
if err != nil {
return fmt.Errorf("marshal HAR: %w", err)
}
- return os.WriteFile(path, append(data, '\n'), fileMode(collector))
+ mode := fileMode(collector)
+ if err := os.WriteFile(path, append(data, '\n'), mode); err != nil {
+ return err
+ }
+ // os.WriteFile honours mode only on creation; an archive holding live
+ // credentials must not inherit a looser mode from a previous run.
+ return os.Chmod(path, mode)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return os.WriteFile(path, append(data, '\n'), fileMode(collector)) | |
| } | |
| func fileMode(collector *Collector) fs.FileMode { | |
| if collector != nil && collector.Config.CaptureSensitive { | |
| return 0o600 | |
| } | |
| return 0o644 | |
| } | |
| mode := fileMode(collector) | |
| if err := os.WriteFile(path, append(data, '\n'), mode); err != nil { | |
| return err | |
| } | |
| // os.WriteFile honours mode only on creation; an archive holding live | |
| // credentials must not inherit a looser mode from a previous run. | |
| return os.Chmod(path, mode) | |
| } | |
| func fileMode(collector *Collector) fs.FileMode { | |
| if collector != nil && collector.Config.CaptureSensitive { | |
| return 0o600 | |
| } | |
| return 0o644 | |
| } |
🤖 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 `@har/write.go` around lines 29 - 37, Update Registry.Flush and the
fileMode-related write flow so that when collector.Config.CaptureSensitive is
true, the HAR file mode is explicitly set to 0600 after os.WriteFile succeeds,
including for existing files; preserve the current 0644 behavior for
non-sensitive output.
Contributor
|
🎉 This PR is included in version 1.55.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
What
Notes
Summary by CodeRabbit
New Features
Documentation
Improvements