scep/transport: remove no-op url.PathUnescape call

[Copilot]

url.PathUnescape in the message() function's PKIOperation branch was a no-op: msg is obtained via r.URL.Query().Get("message"), which internally calls url.QueryUnescape — so %XX sequences are already decoded before url.PathUnescape ever sees them.

Changes

• server/mdm/scep/server/transport.go
  • Removed "net/url" import (no longer referenced)
  • Dropped url.PathUnescape + msg2 intermediary; strings.ReplaceAll and base64.StdEncoding.DecodeString now operate directly on msg

Before:

msg2, err := url.PathUnescape(msg)
if err != nil {
    return nil, &BadRequestError{Message: fmt.Sprintf("invalid PKIOperation message: %s", msg)}
}
msg2 = strings.ReplaceAll(msg2, " ", "+")
decoded, err := base64.StdEncoding.DecodeString(msg2)

After:

msg = strings.ReplaceAll(msg, " ", "+")
decoded, err := base64.StdEncoding.DecodeString(msg)

Checklist for submitter

If some of the following don't apply, delete the relevant line.

• Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.
• Timeouts are implemented and retries are limited to avoid infinite loops
• If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes

Testing

• Added/updated automated tests

• Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)

• QA'd all new/changed functionality manually

For unreleased bug fixes in a release candidate, one of:

• Confirmed that the fix is not expected to adversely impact load test results
• Alerted the release DRI if additional load testing is needed

Database migrations

• Checked schema for all modified table for columns that will auto-update timestamps during migration.
• Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
• Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).

New Fleet configuration settings

• Setting(s) is/are explicitly excluded from GitOps

If you didn't check the box above, follow this checklist for GitOps-enabled settings:

• Verified that the setting is exported via fleetctl generate-gitops
• Verified the setting is documented in a separate PR to the GitOps documentation
• Verified that the setting is cleared on the server if it is not supplied in a YAML file (or that it is documented as being optional)
• Verified that any relevant UI is disabled when GitOps mode is enabled

fleetd/orbit/Fleet Desktop

• Verified compatibility with the latest released version of Fleet (see Must rule)
• If the change applies to only one platform, confirmed that runtime.GOOS is used as needed to isolate changes
• Verified that fleetd runs on macOS, Linux and Windows
• Verified auto-update works from the released version of component to the new version (see tools/tuf/test)

Original prompt

In server/mdm/scep/server/transport.go, make the following minimal changes — do NOT modify any other lines, and do NOT add or remove trailing newlines:

1. Remove "net/url" from the import block.
2. In the message function, replace the url.PathUnescape block with a direct strings.ReplaceAll. Specifically, replace these lines:

msg2, err := url.PathUnescape(msg)
if err != nil {
    return nil, &BadRequestError{Message: fmt.Sprintf("invalid PKIOperation message: %s", msg)}
}

msg2 = strings.ReplaceAll(msg2, " ", "+")
decoded, err := base64.StdEncoding.DecodeString(msg2)
if err != nil {
    return nil, &BadRequestError{Message: fmt.Sprintf("failed to base64 decode message: %s: %s", err.Error(), msg2)}
}

With:

msg = strings.ReplaceAll(msg, " ", "+")
decoded, err := base64.StdEncoding.DecodeString(msg)
if err != nil {
    return nil, &BadRequestError{Message: fmt.Sprintf("failed to base64 decode message: %s: %s", err.Error(), msg)}
}

No other changes. Do not modify any other functions, types, constants, comments, or whitespace elsewhere in the file. Do not add or remove a trailing newline — preserve the file's ending exactly as it is in the current branch.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: see fleetdm#43319 (comment)

input is always assumed base64 so never contains actual spaces. however I see that PathUnescape is a no-op. does it make sense to remove it?

Assistant: Yes, you're right — url.PathUnescape is effectively a no-op here and can be removed.

Here's why:

msg is already decoded by the time it reaches url.PathUnescape

msg is obtained via r.URL.Query().Get("message"), which internally calls url.QueryUnescape. That means:

• %XX sequences → already decoded
• + → already decoded (to space)

So by the time msg hits url.PathUnescape(msg), there are no %XX sequences left to decode. url.PathUnescape (which handles %XX but not +) is indeed a no-op at that point.

The real fix is to not use r.URL.Query() at all — parse r.URL.RawQuery directly so + is never misinterpreted as a space. The pattern would be:

// Instead of:
//   msg = r.URL.Query().Get("message")  // QueryUnescape converts + → space
// Parse RawQuery so + is preserved, then apply PathUnescape for %XX only:
rawMsg := getRawQueryParam(r.URL.RawQuery, "message") // extract raw value
msg2, err := url.PathUnescape(rawMsg)                 // decode %XX, leave + alone
if err != nil { ... }
decoded, err := base64.StdEncoding.DecodeString(msg2)

With that approach, url.PathUnescape is not a no-op — it does the right job of decoding any %XX-encoded characters (e.g. / sent as %2F) while preserving + as +. The strings.ReplaceAll workaround can then be removed too.

Summary:

• In the current code (reading from r.URL.Query()): url.PathUnescape is a no-op → safe to remove.
• In the correct fix (reading from r.URL.RawQuery): url.PathUnescape is necessary and should stay.

So yes, removing it from the current approach is fine, but the better fix is to switch to RawQuery + PathUnescape, as the Copilot review comment already suggested.

User: I'd like not to complicate things because current fix does not introduce a regression. Let's remove PathUnescape

Assistant: We're waiting for your response to the tool call confirmation dialog to continue.

User: @copilot Accepted Confirmation: Are you sure?

Assistant: We're waiting for your response to the tool call confirmation dialog to continue.

Done! Once you confirm, the commit will remove the no-op url.PathUnescape call and its error-handling block, and drop the now-unused "net/url" import. The message function in the PKIOperation branch becomes simply:

msg = strings.ReplaceAll(msg, " ", "+")
decoded, err := base64.StdEncoding.DecodeString(msg)

User: Do not change other methods and parts of the file!

Assistant: We're waiting for your response to the tool call confirmation dialog to continue.

User: @copilot Accepted Confirmation: Are you sure?

Assistant: You're right, sorry about that. Here is the exact updated file — only the message function body and the import block are changed; everything else is untouched:

package scepserver

import (
	"bytes"
	"context"
	"encoding/base64"
	"errors"
	"fmt"
	"io"
	"log/slog"
	"net/http"
	"strings"

	"github.com/fleetdm/fleet/v4/server/mdm/scep/kitlogadapter"
	"github.com/go-kit/kit/transport"
	kithttp "github.com/go-kit/kit/transport/http"
	"github.com/gorilla/mux"
	"gi...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

[Copilot]

Pull request overview

Removes a redundant URL unescaping step in SCEP PKIOperation message handling, since the query parameter is already unescaped when retrieved via r.URL.Query().Get("message").

Changes:

• Dropped the no-op url.PathUnescape call and intermediary msg2 variable in message() for PKIOperation.
• Removed the now-unused "net/url" import.
• Kept the existing space-to-plus normalization and base64 decoding, now operating directly on msg.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.