fix(http-hooks): apply url encoding for ERROR_* env vars #538
Conversation
|
""" WalkthroughThe changes introduce URL encoding for error-related environment variables when used in HTTP hook URLs, ensuring proper query escaping for fields such as error messages, command lines, and stderr output. Documentation is updated to clarify this behaviour, and new tests are added to verify correct URL encoding in hook requests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Sender
participant HTTPServer
Caller->>Sender: Send(ctx)
Sender->>Sender: resolveURL(cfg.URL, ctx)
Sender->>Sender: resolveBody(cfg.Body, ctx)
Sender->>HTTPServer: HTTP request with URL-encoded error fields
HTTPServer-->>Sender: Response
Sender-->>Caller: Result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monitor/hook/sender_test.go (1)
277-277: Address the TODO comment.The TODO suggests checking if environment variables have changed. Consider implementing this check or creating a separate issue to track it.
Would you like me to help implement the environment variable change detection or create an issue to track this improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/content/configuration/http_hooks.md(1 hunks)monitor/hook/sender.go(5 hunks)monitor/hook/sender_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
monitor/hook/sender_test.go (1)
Learnt from: creativeprojects
PR: #425
File: schedule/handler_windows.go:97-118
Timestamp: 2025-02-04T14:38:07.701Z
Learning: The shell.SplitArguments function in the resticprofile project returns only []string and does not return any error.
🧬 Code Graph Analysis (2)
monitor/hook/sender.go (2)
monitor/hook/context.go (1)
Context(5-11)constants/env.go (6)
EnvProfileName(4-4)EnvProfileCommand(5-5)EnvError(6-6)EnvErrorCommandLine(8-8)EnvErrorExitCode(9-9)EnvErrorStderr(10-10)
monitor/hook/sender_test.go (5)
monitor/hook/context.go (2)
Context(5-11)ErrorContext(13-18)context.go (1)
Request(11-17)constants/env.go (4)
EnvError(6-6)EnvErrorCommandLine(8-8)EnvErrorExitCode(9-9)EnvErrorStderr(10-10)config/send.go (1)
SendMonitoringSection(31-38)config/confidential.go (1)
NewConfidentialValue(81-83)
🔇 Additional comments (8)
docs/content/configuration/http_hooks.md (1)
298-301: Documentation accurately reflects URL encoding implementation.The updates correctly specify which environment variables are URL encoded. The distinction between text fields (ERROR, ERROR_COMMANDLINE, ERROR_STDERR) being encoded and numeric fields (ERROR_EXIT_CODE) remaining unencoded is appropriate and matches the implementation.
monitor/hook/sender_test.go (2)
21-21: Import addition looks good.The constants package import is necessary for accessing environment variable names used in the test.
278-319: Excellent test coverage for URL encoding.The test effectively verifies the URL encoding behaviour:
- Uses realistic error data with special characters and multiline content
- Properly constructs URLs with environment variable placeholders
- Verifies server receives correctly decoded values (not encoded values)
- Tests the complete flow through the Send method
The test data includes various edge cases like shell operators (
<,||,&) and multiline strings with special characters, which thoroughly exercises the URL encoding logic.monitor/hook/sender.go (5)
12-12: Appropriate import alias for URL package.Using
urlpkgalias avoids naming conflicts with theurlvariable used in the code.
78-79: Correct usage of resolveURL for URL fields.Both the private URL value and public URL string are properly resolved using the new URL-specific function that applies appropriate encoding.
97-97: Correct usage of resolveBody for body content.Body content uses the renamed resolveBody function which doesn't apply URL encoding, appropriate for request body data.
206-234: Well-implemented resolveBody function.The function correctly handles all environment variables without URL encoding, which is appropriate for body content. The simplification from the original resolve function (removing intermediate variable) improves readability.
236-264: Excellent implementation of URL-specific resolution.The resolveURL function correctly applies URL encoding to error-related fields that may contain special characters:
- ERROR, ERROR_COMMANDLINE, and ERROR_STDERR are URL encoded
- ERROR_EXIT_CODE remains unencoded (appropriate for numeric values)
- PROFILE_NAME and PROFILE_COMMAND remain unencoded (aligns with documentation and PR objectives)
This addresses the core issue where error messages with special characters cause invalid URLs when used as query parameters.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #538 +/- ##
==========================================
+ Coverage 79.33% 79.36% +0.03%
==========================================
Files 136 136
Lines 13305 13323 +18
==========================================
+ Hits 10555 10573 +18
Misses 2332 2332
Partials 418 418
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Actually i messed up docs a bit. It needs clarification about url encoding applying only if env vars used in url. |
|
Should i care about
|
|
It's fine, don't worry about it. The issue here is that secret variables in the pipeline configuration are not available to a pull request pipeline (other than created by the owner or maintainer of the repository). |
|
That should fix it 🤞🏻 #542 |
|
Seems like it fixed! Anything else i can do here? |
|
Looks good to me! thanks for this PR 👍🏻 |
2 participants
Resolves #534
ERROR,ERROR_COMMANDLINEandERROR_STDERRare encoded (url.QueryEscape).ERROR_EXIT_CODEandPROFILE_COMMANDdon't need to be encoded.I'm not sure about
PROFILE_NAMEtho.