Skip to content

fix(api): stop returning empty device credential properties in responses - #1293

Merged
rsdmike merged 2 commits into
mainfrom
fix/device-credentials-in-responses
Oct 3, 2026
Merged

rsdmike merged 2 commits into
mainfrom
fix/device-credentials-in-responses

Conversation

@rsdmike

@rsdmike rsdmike commented Oct 1, 2026

Copy link
Copy Markdown
Member

Device responses carried password, mpspassword and mebxpassword as empty properties. When a client edits a device it fetched and sends the whole object back with PATCH, as the Sample Web UI does when editing tags, those empty properties were treated as provided values and replaced the stored passwords.

Device JSON output now leaves the three password properties out, so a round-tripped object no longer touches them. They are still read from request bodies, and an explicit empty mpspassword on PATCH still clears the stored value.

Device responses carried password, mpspassword and mebxpassword as
empty properties. When a client edits a device it fetched and sends the
whole object back with PATCH, as the Sample Web UI does when editing
tags, those empty properties were treated as provided values and
replaced the stored passwords.

Device JSON output now leaves the three password properties out, so a
round-tripped object no longer touches them. They are still read from
request bodies, and an explicit empty mpspassword on PATCH still clears
the stored value.
@rsdmike
rsdmike requested a review from a team as a code owner October 1, 2026 23:07
Copilot AI balanced review requested due to automatic review settings October 1, 2026 23:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation consistently prevents credential output while preserving request handling and includes appropriate regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Prevents device API responses from exposing empty credential fields, preserving stored passwords during object round trips.

Changes:

  • Omits device credentials during JSON serialization while retaining request decoding.
  • Updates OpenAPI examples and Postman assertions.
  • Adds DTO and HTTP regression tests.
File Description
internal/​entity/​dto/​v1/​device.go Suppresses credentials in JSON output.
internal/​entity/​dto/​v1/​device_test.go Tests serialization and decoding.
internal/​controller/​openapi/​devices.go Removes credentials from response examples.
internal/​controller/​httpapi/​v1/​devices_test.go Tests safe response round trips.
integration-test/​collections/​console_mps_apis.postman_collection.json Verifies credential omission.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.99%. Comparing base (2a8e16b) to head (ff10d44).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1293      +/-   ##
==========================================
+ Coverage   61.97%   61.99%   +0.01%     
==========================================
  Files         158      158              
  Lines       13336    13337       +1     
==========================================
+ Hits         8265     8268       +3     
+ Misses       5071     5069       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rsdmike
rsdmike enabled auto-merge (rebase) October 2, 2026 18:17

@sudhir-intc sudhir-intc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working the fix, reviewed and tried it out as below:

## Steps to reproduce
Requires a provisioned device under console management with a known AMT password.
(`$BASE` = console URL, `$GUID` = device GUID, `$TOKEN` = JWT from `POST /api/v1/authorize`.)

1. **`GET /api/v1/devices/$GUID`** — the response body contains the credential
   properties as empty strings:
   `"password":"", "mpspassword":"", "mebxpassword":""`
2. **Confirm the device is manageable:** `GET /api/v1/amt/power/state/$GUID` → `200`.
3. **Round-trip the object back**, changing only an unrelated field — the same thing the
   Sample Web UI does when editing tags:
   `PATCH /api/v1/devices` with the step-1 body, `tags` set to `["test"]`.
   Because PATCH is a merge patch keyed on the JSON properties present in the body, the
   empty `password` is treated as a submitted value and overwrites the stored credential.
4. **`GET /api/v1/amt/power/state/$GUID`** → now fails with an authorization error. The
   device is unmanageable; the tag edit silently destroyed its AMT credentials.
5. Restore with `PATCH /api/v1/devices` → `{"guid":"$GUID","password":"<real password>"}`.

## Expected after this change

- Step 1: the three credential properties are absent from the response entirely
  (single GET, list, and insert responses).
- Step 4: still returns `200` — the round-trip PATCH leaves credentials untouched.
- The tag change in step 3 still persists, and an explicit `{"mpspassword":""}` still
  clears the stored value, so deliberate blanking is unaffected.

@rsdmike
rsdmike merged commit a20b7c7 into main Oct 3, 2026
22 checks passed
@rsdmike
rsdmike deleted the fix/device-credentials-in-responses branch October 3, 2026 02:00
@RosieAMT

RosieAMT commented Oct 3, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 1.45.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants