shekhar/windows-iso-downloader

Fork 0

refactor(backend): validate numeric IDs and use url.Values for upstream URLs #2

Merged

starkSV merged 1 commit from improve-backend-url-encoding into main

2026-05-12 19:09:32 +05:30

starkSV commented

2026-05-12 19:01:01 +05:30

(Migrated from github.com)

Summary

Reject non-numeric product_id and sku_id with HTTP 400 before any upstream request is made, preventing parameter injection into Microsoft API query strings
Replace all fmt.Sprintf URL construction with url.Values.Set + .Encode() across handleSkuInfo, handleProxy, and setupSession

Test plan

product_id=abc → 400 product_id must be a numeric value
product_id=2618%26profile=injected → 400 (injection blocked)
Missing product_id → 400 product_id query parameter is required
sku_id=notanumber → 400 sku_id must be a numeric value
product_id=2618 (valid) → 200 OK from Microsoft

Summary by CodeRabbit

Bug Fixes
- Implemented parameter validation to ensure product identifiers and SKU values are properly formatted as numeric values before processing, preventing invalid requests from reaching downstream services.
Refactor
- Standardized request parameter encoding across endpoints to ensure consistent URL formatting, proper escaping of special characters, and improved overall system reliability.

## Summary - Reject non-numeric `product_id` and `sku_id` with HTTP 400 before any upstream request is made, preventing parameter injection into Microsoft API query strings - Replace all `fmt.Sprintf` URL construction with `url.Values.Set` + `.Encode()` across `handleSkuInfo`, `handleProxy`, and `setupSession` ## Test plan - [x] `product_id=abc` → 400 `product_id must be a numeric value` - [x] `product_id=2618%26profile=injected` → 400 (injection blocked) - [x] Missing `product_id` → 400 `product_id query parameter is required` - [x] `sku_id=notanumber` → 400 `sku_id must be a numeric value` - [x] `product_id=2618` (valid) → 200 OK from Microsoft  ## Summary by CodeRabbit * **Bug Fixes** * Implemented parameter validation to ensure product identifiers and SKU values are properly formatted as numeric values before processing, preventing invalid requests from reaching downstream services. * **Refactor** * Standardized request parameter encoding across endpoints to ensure consistent URL formatting, proper escaping of special characters, and improved overall system reliability. [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/starkSV/windows-iso-downloader/pull/2)

coderabbitai[bot] commented

2026-05-12 19:01:12 +05:30

(Migrated from github.com)

📝 Walkthrough

Walkthrough

Single file update to backend/main.go that refactors URL query construction across three handlers to use net/url encoding instead of manual fmt.Sprintf strings, adds numeric validation for product ID and SKU ID parameters, and updates a related comment.

Changes

Query Construction and Input Validation

Layer / File(s)	Summary
setupSession handler refactoring `backend/main.go`	Imports `net/url` and refactors three outbound Microsoft requests to build query strings via `url.Values` instead of manual formatting: session/org queries, mdt.js parameters, and fingerprint response parameters.
handleSkuInfo validation and query refactoring `backend/main.go`	Validates that `product_id` is numeric before processing, then replaces the `getskuinformationbyproductedition` query assembly with `url.Values`-based encoding.
handleProxy validation and multi-query refactoring `backend/main.go`	Validates both `product_id` and `sku_id` as numeric values, returning JSON 400 errors on invalid input. Refactors warmup request and `GetProductDownloadLinksBySku` request URLs to use `url.Values`. Updates comment on `SetEscapeHTML(false)`.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Query strings encode with grace,
No Sprintf left to trace—
url.Values leads the way,
Validation guards each day,
Cleaner code in every place! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor(backend): validate numeric IDs and use url.Values for upstream URLs' accurately and specifically describes both main changes in the pull request: input validation for numeric IDs and replacement of sprintf-based URL construction with url.Values encoding.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch improve-backend-url-encoding

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 help to get the list of available commands and usage tips.}

<details> <summary>📝 Walkthrough</summary> ## Walkthrough Single file update to `backend/main.go` that refactors URL query construction across three handlers to use `net/url` encoding instead of manual `fmt.Sprintf` strings, adds numeric validation for product ID and SKU ID parameters, and updates a related comment. ## Changes **Query Construction and Input Validation** | Layer / File(s) | Summary | |---|---| | **setupSession handler refactoring** `backend/main.go` | Imports `net/url` and refactors three outbound Microsoft requests to build query strings via `url.Values` instead of manual formatting: session/org queries, mdt.js parameters, and fingerprint response parameters. | | **handleSkuInfo validation and query refactoring** `backend/main.go` | Validates that `product_id` is numeric before processing, then replaces the `getskuinformationbyproductedition` query assembly with `url.Values`-based encoding. | | **handleProxy validation and multi-query refactoring** `backend/main.go` | Validates both `product_id` and `sku_id` as numeric values, returning JSON 400 errors on invalid input. Refactors warmup request and `GetProductDownloadLinksBySku` request URLs to use `url.Values`. Updates comment on `SetEscapeHTML(false)`. | ## Estimated Code Review Effort 🎯 2 (Simple) | ⏱️ ~10 minutes ## Poem > 🐰 Query strings encode with grace, > No `Sprintf` left to trace— > `url.Values` leads the way, > Validation guards each day, > Cleaner code in every place! ✨ </details>   <details> <summary>🚥 Pre-merge checks | ✅ 5</summary> <details> <summary>✅ Passed checks (5 passed)</summary> | Check name | Status | Explanation | | :------------------------: | :------- | :--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. | | Title check | ✅ Passed | The title 'refactor(backend): validate numeric IDs and use url.Values for upstream URLs' accurately and specifically describes both main changes in the pull request: input validation for numeric IDs and replacement of sprintf-based URL construction with url.Values encoding. | | Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. | | Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. | </details> ✏️ Tip: You can configure your own custom pre-merge checks in the settings. </details>   <details> <summary>✨ Finishing Touches</summary> <details> <summary>📝 Generate docstrings</summary> - [ ]  Create stacked PR - [ ]  Commit on current branch </details> <details> <summary>🧪 Generate unit tests (beta)</summary> - [ ]  Create PR with unit tests - [ ]  Commit unit tests in branch `improve-backend-url-encoding` </details> </details>   > [!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. > > <details> > <summary>🔧 golangci-lint (2.12.2)</summary> > > level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" > > > > > </details>   --- Comment `@coderabbitai help` to get the list of available commands and usage tips.

cloudflare-workers-and-pages[bot] commented

2026-05-12 19:01:23 +05:30

(Migrated from github.com)

Deploying msdl-frontend with Cloudflare Pages

Latest commit:	`37f947c`
Status:	✅ Deploy successful!
Preview URL:	https://98dd8258.msdl-frontend.pages.dev
Branch Preview URL:	https://improve-backend-url-encoding.msdl-frontend.pages.dev

View logs

## Deploying msdl-frontend with  <a href="https://pages.dev"><img alt="Cloudflare Pages" src="https://user-images.githubusercontent.com/23264/106598434-9e719e00-654f-11eb-9e59-6167043cfa01.png" width="16"></a>  Cloudflare Pages <table><tr><td>Latest commit: </td><td> <code>37f947c</code> </td></tr> <tr><td>Status:</td><td> ✅  Deploy successful!</td></tr> <tr><td>Preview URL:</td><td> <a href='https://98dd8258.msdl-frontend.pages.dev'>https://98dd8258.msdl-frontend.pages.dev</a> </td></tr> <tr><td>Branch Preview URL:</td><td> <a href='https://improve-backend-url-encoding.msdl-frontend.pages.dev'>https://improve-backend-url-encoding.msdl-frontend.pages.dev</a> </td></tr> </table> [View logs](https://dash.cloudflare.com/?to=/630cc520524402c8c1de35025c15b122/pages/view/msdl-frontend/98dd8258-c846-4d70-b2f5-2ebbb02cc324)

coderabbitai[bot] (Migrated from github.com) reviewed 2026-05-12 19:03:36 +05:30

coderabbitai[bot] (Migrated from github.com) left a comment

🧹 Nitpick comments (1)

backend/main.go (1)

175-178: ⚡ Quick win

Consider extracting repeated numeric-param validation into one helper.

Both handlers duplicate the same parse/error pattern; centralizing it will keep behavior/messages consistent and reduce drift.

♻️ Suggested refactor

+func requireNumericQueryParam(w http.ResponseWriter, name, value string) bool {
+	if value == "" {
+		respondJSONError(w, http.StatusBadRequest, fmt.Sprintf("%s query parameter is required", name))
+		return false
+	}
+	if _, err := strconv.Atoi(value); err != nil {
+		respondJSONError(w, http.StatusBadRequest, fmt.Sprintf("%s must be a numeric value", name))
+		return false
+	}
+	return true
+}

-	if productID == "" {
-		respondJSONError(w, http.StatusBadRequest, "product_id query parameter is required")
-		return
-	}
-	if _, err := strconv.Atoi(productID); err != nil {
-		respondJSONError(w, http.StatusBadRequest, "product_id must be a numeric value")
+	if !requireNumericQueryParam(w, "product_id", productID) {
 		return
 	}

-	if productID == "" || skuID == "" {
-		respondJSONError(w, http.StatusBadRequest, "product_id and sku_id query parameters are required")
-		return
-	}
-	if _, err := strconv.Atoi(productID); err != nil {
-		respondJSONError(w, http.StatusBadRequest, "product_id must be a numeric value")
-		return
-	}
-	if _, err := strconv.Atoi(skuID); err != nil {
-		respondJSONError(w, http.StatusBadRequest, "sku_id must be a numeric value")
+	if !requireNumericQueryParam(w, "product_id", productID) || !requireNumericQueryParam(w, "sku_id", skuID) {
 		return
 	}

Also applies to: 277-284

🤖 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 `@backend/main.go` around lines 175 - 178, Handlers duplicate numeric parameter
parsing using strconv.Atoi and respondJSONError; extract this into a single
helper (e.g., parseNumericParam or parseIDFromRequest) that accepts the string
value and parameter name, returns the int (or error), and centralizes the "must
be a numeric value" response using respondJSONError and the same HTTP status.
Replace direct strconv.Atoi calls for productID (and the other handler's param)
with calls to this helper to keep behavior and messages consistent across
handlers.

🤖 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 `@backend/main.go`:
- Around line 175-178: Handlers duplicate numeric parameter parsing using
strconv.Atoi and respondJSONError; extract this into a single helper (e.g.,
parseNumericParam or parseIDFromRequest) that accepts the string value and
parameter name, returns the int (or error), and centralizes the "must be a
numeric value" response using respondJSONError and the same HTTP status. Replace
direct strconv.Atoi calls for productID (and the other handler's param) with
calls to this helper to keep behavior and messages consistent across handlers.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ab568493-41d9-447b-bc3b-b592061e7dd8

📥 Commits

Reviewing files that changed from the base of the PR and between 10ad222dde and 37f947cb32.

📒 Files selected for processing (1)

backend/main.go

<details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>backend/main.go (1)</summary><blockquote> `175-178`: _⚡ Quick win_ **Consider extracting repeated numeric-param validation into one helper.** Both handlers duplicate the same parse/error pattern; centralizing it will keep behavior/messages consistent and reduce drift. <details> <summary>♻️ Suggested refactor</summary> ```diff +func requireNumericQueryParam(w http.ResponseWriter, name, value string) bool { + if value == "" { + respondJSONError(w, http.StatusBadRequest, fmt.Sprintf("%s query parameter is required", name)) + return false + } + if _, err := strconv.Atoi(value); err != nil { + respondJSONError(w, http.StatusBadRequest, fmt.Sprintf("%s must be a numeric value", name)) + return false + } + return true +} ``` ```diff - if productID == "" { - respondJSONError(w, http.StatusBadRequest, "product_id query parameter is required") - return - } - if _, err := strconv.Atoi(productID); err != nil { - respondJSONError(w, http.StatusBadRequest, "product_id must be a numeric value") + if !requireNumericQueryParam(w, "product_id", productID) { return } ``` ```diff - if productID == "" || skuID == "" { - respondJSONError(w, http.StatusBadRequest, "product_id and sku_id query parameters are required") - return - } - if _, err := strconv.Atoi(productID); err != nil { - respondJSONError(w, http.StatusBadRequest, "product_id must be a numeric value") - return - } - if _, err := strconv.Atoi(skuID); err != nil { - respondJSONError(w, http.StatusBadRequest, "sku_id must be a numeric value") + if !requireNumericQueryParam(w, "product_id", productID) || !requireNumericQueryParam(w, "sku_id", skuID) { return } ``` </details> Also applies to: 277-284 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/main.go` around lines 175 - 178, Handlers duplicate numeric parameter parsing using strconv.Atoi and respondJSONError; extract this into a single helper (e.g., parseNumericParam or parseIDFromRequest) that accepts the string value and parameter name, returns the int (or error), and centralizes the "must be a numeric value" response using respondJSONError and the same HTTP status. Replace direct strconv.Atoi calls for productID (and the other handler's param) with calls to this helper to keep behavior and messages consistent across handlers. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary> ``` 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 `@backend/main.go`: - Around line 175-178: Handlers duplicate numeric parameter parsing using strconv.Atoi and respondJSONError; extract this into a single helper (e.g., parseNumericParam or parseIDFromRequest) that accepts the string value and parameter name, returns the int (or error), and centralizes the "must be a numeric value" response using respondJSONError and the same HTTP status. Replace direct strconv.Atoi calls for productID (and the other handler's param) with calls to this helper to keep behavior and messages consistent across handlers. ``` </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `ab568493-41d9-447b-bc3b-b592061e7dd8` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 10ad222dde866874e1ccf2ccf9d0e63cc6b199e7 and 37f947cb32bb5cb36e8cf193a8d15f694d6a570c. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `backend/main.go` </details> </details>