16348 http url query contains semicolon which is no longer a supported separator 1 #967
Conversation
WalkthroughThe pull request introduces a new function Changes
Sequence DiagramsequenceDiagram
participant Client
participant CustomHandler
participant FixSemicolonQuery
participant DefaultServeMux
Client->>CustomHandler: Send HTTP Request
CustomHandler->>FixSemicolonQuery: Preprocess Query
FixSemicolonQuery-->>CustomHandler: Sanitized Query
CustomHandler->>DefaultServeMux: Forward Request
DefaultServeMux-->>Client: Response
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(5 hunks)server/main.go(2 hunks)
🔇 Additional comments (2)
server/main.go (1)
83-89: 🛠️ Refactor suggestionConsider edge cases in query parameter values.
The current implementation might incorrectly replace semicolons that are part of legitimate query parameter values (e.g.,
?data=value;more). Consider URL-decoding the query string first, only replacing semicolons used as separators, and then URL-encoding the result.func fixSemicolonQuery(r *http.Request) { if strings.Contains(r.URL.RawQuery, ";") { - r.URL.RawQuery = strings.ReplaceAll(r.URL.RawQuery, ";", "&") + // URL-decode the query string + decoded, err := url.QueryUnescape(r.URL.RawQuery) + if err != nil { + log.Printf("Warning: Failed to decode query string: %v", err) + return + } + // Replace semicolons between key-value pairs + parts := strings.Split(decoded, "&") + for i, part := range parts { + if !strings.Contains(part, "=") { + continue + } + parts[i] = strings.ReplaceAll(part, ";", "&") + } + // URL-encode and reassemble + r.URL.RawQuery = url.QueryEscape(strings.Join(parts, "&")) } }Additionally, consider adding debug logging to track when this function modifies queries:
func fixSemicolonQuery(r *http.Request) { if strings.Contains(r.URL.RawQuery, ";") { + original := r.URL.RawQuery r.URL.RawQuery = strings.ReplaceAll(r.URL.RawQuery, ";", "&") + log.Printf("Modified query string: %s -> %s", original, r.URL.RawQuery) } }package.json (1)
Line range hint
81-205: Verify compatibility with updated dependencies.While the dependency updates appear routine, please ensure that the following major version bumps have been tested thoroughly:
@sentry/reactfrom version 7.x to 8.x@testing-library/reactfrom version 15.x to 16.x
dqunbp
force-pushed
the
16348-http-url-query-contains-semicolon-which-is-no-longer-a-supported-separator-1
branch
from
4302f1c to
5731e63
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
server/main.go(2 hunks)
🔇 Additional comments (3)
server/main.go (3)
83-89: Add metrics for query modifications.As suggested in the previous review, it would be valuable to track how often queries are modified using Prometheus metrics.
115-119: Verify performance impact of query string modification.The query string modification is applied to all requests, which could impact performance. Consider adding benchmarks to measure the impact.
83-89: Consider validating the query string before modification.The function modifies the query string without validating its format. Consider adding basic validation to ensure the query string is well-formed before modification.
dqunbp
force-pushed
the
16348-http-url-query-contains-semicolon-which-is-no-longer-a-supported-separator-1
branch
from
5731e63 to
7aaca2a
Compare
|
Bundle size diff
|
|
Preview environments for this PR:
These previews are automatically updated with each commit. Note: After a new deployment, it may take a few minutes for the changes to propagate and for caches to update. During this time, you might experience temporary loading issues or see an older version of the app. If the app fails to load, please wait a few minutes and try again. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/main.go (2)
83-89: 🧹 Nitpick (assertive)Add request validation and improve error handling.
While the function serves its purpose, it could be more robust:
- Add request parameter validation
- Consider error handling for edge cases
func fixSemicolonQuery(r *http.Request) { + if r == nil || r.URL == nil { + return + } if strings.Contains(r.URL.RawQuery, ";") { - r.URL.RawQuery = strings.ReplaceAll(r.URL.RawQuery, ";", "&") + oldQuery := r.URL.RawQuery + r.URL.RawQuery = strings.ReplaceAll(r.URL.RawQuery, ";", "&") + log.Printf("Modified query string from '%s' to '%s'", oldQuery, r.URL.RawQuery) } }Note: The logging suggestion and indentation fix are duplicates of previous comments.
115-119: 🛠️ Refactor suggestionAvoid using DefaultServeMux directly.
Using
http.DefaultServeMuxdirectly is not recommended for production as it's a global variable that could be modified by any package. Consider using a localServeMuxinstance instead.+func newServer() http.Handler { + mux := http.NewServeMux() + + // Register routes on mux instead of http.Handle + mux.Handle("/live", http.StripPrefix("/live", http.HandlerFunc(redirectWithDn1Params))) + mux.Handle("/live/", http.StripPrefix("/live/", http.HandlerFunc(redirectWithDn1Params))) + // ... register other routes + + // Wrap mux with the query fix middleware + return withQueryFix(mux) +} + +func withQueryFix(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fixSemicolonQuery(r) + next.ServeHTTP(w, r) + }) +}Note: The middleware pattern suggestion is a duplicate of a previous comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
server/main.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
server/main.go (1)
121-126: Improve server startup logging and error handling.The current error handling could be more informative and follow better logging practices.
dqunbp
force-pushed
the
16348-http-url-query-contains-semicolon-which-is-no-longer-a-supported-separator-1
branch
from
1de8774 to
7aaca2a
Compare
https://kontur.fibery.io/Tasks/Task/http-URL-query-contains-semicolon,-which-is-no-longer-a-supported-separator-16348
Summary by CodeRabbit