fix: handle url encoded file name in url params for single file serving endpoint - #4495
Conversation
## Summary Fixes a bug where OTEL plugin headers were being overwritten with redacted placeholder values when saving a plugin configuration. After the multi-profile change, header values stored as plain strings inside the `profiles` array were not being restored from the database before saving, causing real credentials to be replaced with masked values like `****`. ## Changes - Extracted `restoreRedactedValue` as a standalone recursive helper, replacing the inline logic in `restoreRedactedFromExisting`. This allows the restoration logic to descend into both nested maps and slices. - Added slice traversal support (index-aligned) so that elements within arrays like the OTEL `profiles` array are individually checked and restored. - Added plain-string redaction detection so that header values stored as raw strings (rather than `EnvVar` objects) are also restored from the existing DB config when they carry a redaction artifact. Empty strings are intentionally left as-is to allow clearing a value. - Added `TestRestoreRedacted_OTELProfilesHeaders` to cover both failure modes: slice traversal and plain-string secret restoration. Also asserts that genuinely new (non-redacted) values pass through unchanged. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./transports/bifrost-http/handlers/... ``` Verify that saving an OTEL plugin configuration with multiple profiles, after a GET that returns redacted header values, does not overwrite the stored credentials in the database. Confirm that providing a genuinely new header value still persists correctly. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations This fix ensures that redacted credential placeholders returned to the client are never written back over real secrets stored in the database. The restoration logic only replaces values that are confirmed redaction artifacts; empty strings and non-redacted values are always passed through as-is, preserving the ability to clear a credential intentionally. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo independent handler improvements: (1) ChangesPlugin Redaction Restore
Skills Path Parameter Decoding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/plugins_test.go (1)
76-84: 💤 Low valueOrphaned doc comment for
TestUpdatePlugin_ConfigMerge.Lines 76-79 describe
TestUpdatePlugin_ConfigMerge, but the newTestRestoreRedacted_OTELProfilesHeaderscomment block (lines 80-84) was inserted between the doc comment and the test it now documents. The doc comment forTestUpdatePlugin_ConfigMergeshould be moved to directly precede that function (line 143).📝 Suggested fix
Move lines 76-79 to directly before
TestUpdatePlugin_ConfigMergeat line 143:-// TestUpdatePlugin_ConfigMerge verifies that updatePlugin merges the incoming -// config over the existing DB config, preserving fields the caller did not send. -// This is critical for the plugin_span_filter field: the OTEL config form in the -// UI does not send plugin_span_filter, so it must survive a save without being wiped. // TestRestoreRedacted_OTELProfilesHeaders covers the two gaps that broke OTEL header // round-trips after the multi-profile change: (1) headers live inside the `profiles` // array (slice traversal), and (2) header values are plain redacted strings, not EnvVar // objects. Saving a config whose headers came back redacted must not overwrite the // stored credentials. func TestRestoreRedacted_OTELProfilesHeaders(t *testing.T) {And before line 143:
+// TestUpdatePlugin_ConfigMerge verifies that updatePlugin merges the incoming +// config over the existing DB config, preserving fields the caller did not send. +// This is critical for the plugin_span_filter field: the OTEL config form in the +// UI does not send plugin_span_filter, so it must survive a save without being wiped. func TestUpdatePlugin_ConfigMerge(t *testing.T) {🤖 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 `@transports/bifrost-http/handlers/plugins_test.go` around lines 76 - 84, The doc comment for the TestUpdatePlugin_ConfigMerge function (lines 76-79) is currently orphaned because the TestRestoreRedacted_OTELProfilesHeaders comment block was inserted between it and the actual TestUpdatePlugin_ConfigMerge function definition. Move the TestUpdatePlugin_ConfigMerge comment block to directly precede the TestUpdatePlugin_ConfigMerge function at line 143, ensuring each test function has its doc comment immediately above it without any other comments in between.
🤖 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.
Nitpick comments:
In `@transports/bifrost-http/handlers/plugins_test.go`:
- Around line 76-84: The doc comment for the TestUpdatePlugin_ConfigMerge
function (lines 76-79) is currently orphaned because the
TestRestoreRedacted_OTELProfilesHeaders comment block was inserted between it
and the actual TestUpdatePlugin_ConfigMerge function definition. Move the
TestUpdatePlugin_ConfigMerge comment block to directly precede the
TestUpdatePlugin_ConfigMerge function at line 143, ensuring each test function
has its doc comment immediately above it without any other comments in between.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd0d3f7c-3a7e-4cf1-9fd7-6013d7f78dda
📒 Files selected for processing (4)
transports/bifrost-http/handlers/plugins.gotransports/bifrost-http/handlers/plugins_test.gotransports/bifrost-http/handlers/skills_serving.gotransports/bifrost-http/handlers/skills_serving_test.go
Confidence Score: 4/5The file-serving fix is correct and the integration test confirms end-to-end behaviour; the bundled plugins.go refactor is well-tested. The one gap is Both changed code paths are correct and covered by tests. The transports/bifrost-http/handlers/skills_serving.go — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["GET /api/skills/serve/{skill-name}/files/{filepath:*}"] --> B["lookupSkillByPathParam"]
B --> C["decodeStringPathParam(ctx, 'skill-name', ...)"]
C --> D{val nil?}
D -- yes --> E["400 Bad Request"]
D -- no --> F["url.PathUnescape(raw)"]
F --> G{error or empty?}
G -- yes --> E
G -- no --> H["store.GetSkillByName(decoded)"]
H --> I["doServeFileContent"]
I --> J["decodeStringPathParam(ctx, 'filepath', ...)"]
J --> K["url.PathUnescape(raw filepath)"]
K --> L["Match f.NormalizedPath() == decoded"]
L -- found --> M["serveSkillFile → HTTP 200"]
L -- not found --> N["404 Not Found"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["GET /api/skills/serve/{skill-name}/files/{filepath:*}"] --> B["lookupSkillByPathParam"]
B --> C["decodeStringPathParam(ctx, 'skill-name', ...)"]
C --> D{val nil?}
D -- yes --> E["400 Bad Request"]
D -- no --> F["url.PathUnescape(raw)"]
F --> G{error or empty?}
G -- yes --> E
G -- no --> H["store.GetSkillByName(decoded)"]
H --> I["doServeFileContent"]
I --> J["decodeStringPathParam(ctx, 'filepath', ...)"]
J --> K["url.PathUnescape(raw filepath)"]
K --> L["Match f.NormalizedPath() == decoded"]
L -- found --> M["serveSkillFile → HTTP 200"]
L -- not found --> N["404 Not Found"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Merge activity
|
4 participants
Summary
URL-encoded path parameters (e.g., file paths containing spaces like
nested dir/file with spaces.txt) were not being decoded before use, causingfile serving requests with percent-encoded characters to fail. This PR
introduces a shared helper that centralises path parameter extraction and adds
proper
url.PathUnescapedecoding.Changes
decodeStringPathParamhelper that extracts a named path parameterfrom a
fasthttp.RequestCtx, validates it, and URL-decodes it viaurl.PathUnescapebefore returning it to the caller.doServeFileContentand
lookupSkillByPathParamwith calls to the new helper.(
TestSkillsServingGenericFileDownloadDecodesEncodedPathParams) that createsa skill with a file at a path containing spaces, issues a request with a
percent-encoded URI, and asserts the file content is returned with HTTP 200.
Type of change
Affected areas
How to test
go test ./transports/bifrost-http/handlers/...The new test
TestSkillsServingGenericFileDownloadDecodesEncodedPathParamsexercises the fix end-to-end by serving a file whose path contains spaces via a
percent-encoded URL and asserting the correct body and status code are returned.
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
url.PathUnescapeis used rather thanurl.QueryUnescapeto avoidmisinterpreting
+as a space in file paths. Decoded paths are validated to benon-empty before use, preventing empty-string path traversal edge cases.
Checklist
docs/contributing/README.mdand followed the guidelines