Skip to content

fix(private-api): device routes only act on the signed-in account - #102

Merged
feruzm merged 1 commit into
mainfrom
fix/register-device-owner
Sep 17, 2026
Merged

feruzm merged 1 commit into
mainfrom
fix/register-device-owner

Conversation

@feruzm

@feruzm feruzm commented Sep 17, 2026

Copy link
Copy Markdown
Member

/private-api/register-device and /private-api/detail-device now require the body username to be the account the code belongs to. The comparison is case-insensitive, as in ResolveNotificationsTarget. Anything else gets 403 without an upstream call.

The web app and the mobile app already send the signed-in account's own username with its own code, so nothing legitimate changes.

To test the handlers without chain RPC or network, code validation and the upstream call get test seams (DeviceValidateCode, DeviceUpstream), following the curation desk routes.

Test plan

  • DeviceAuthorizationTests: the rule itself (own account in any case accepted; other, near-miss, missing and unvalidated refused).
  • DeviceHandlerTests: both handlers forward the own account (endpoint and payload checked), and answer 403 for another or missing username and 401 for an invalid code, with no upstream call.
  • Mutation check: removing either handler's check, making the comparison case-sensitive, or dropping the validated-name guard each fails the tests.
  • dotnet test: 523 passed.

Summary by CodeRabbit

  • Bug Fixes
    • Device registration and device details requests now verify that the requested username matches the account validated by the authorization code.
    • Username matching is case-insensitive.
    • Requests with a missing or mismatched username are rejected with a 403 response.
    • Requests using an invalid authorization code return a 401 response.
    • Rejected requests are not forwarded to the upstream service.

register-device and detail-device now require the body username to be the
account the code belongs to (case-insensitive, like the notifications feed),
and answer 403 otherwise without calling upstream. Code validation and the
upstream call get test seams, as the curation desk routes have.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Restrict device routes to the signed-in account

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Require device registration and detail requests to target the validated signed-in account.
• Reject mismatched or missing usernames with 403 before contacting the upstream service.
• Add isolated authorization and handler tests for allowed, forbidden, and unauthorized requests.
Diagram

graph TD
    A["Client"] --> B["Device routes"] --> C["Code validator"] --> D{"Code valid?"}
    D -- "Yes" --> F{"Own account?"} -- "Yes" --> H["Device upstream"]
    D -- "No" --> E["401 Unauthorized"]
    F -- "No" --> G["403 Forbidden"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use the validated username directly
  • ➕ Eliminates reliance on the request username for upstream resource selection.
  • ➕ Removes the need for a separate equality check.
  • ➖ Silently accepts malformed or cross-account requests instead of rejecting them.
  • ➖ Changes the existing request contract and may alter username casing in forwarded payloads.
  • ➖ Makes client-side identity mistakes less visible.

Recommendation: Keep the PR's fail-closed comparison before the upstream boundary. Explicitly returning 403 for missing or mismatched usernames preserves the established payload contract, prevents unintended cross-account operations, and exposes invalid client behavior rather than silently rewriting it.

Files changed (2) +181 / -5

Bug fix (1) +33 / -5
PrivateApi.UserData1.csEnforce signed-in account ownership on device operations +33/-5

Enforce signed-in account ownership on device operations

• Adds a case-insensitive ownership helper and applies it to register-device and detail-device after signed-code validation. Missing or mismatched usernames now return 403 without an upstream call, while replaceable validation and upstream delegates allow network-free handler testing.

dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs

Tests (1) +148 / -0
DeviceAuthorizationTests.csAdd device-route authorization and forwarding coverage +148/-0

Add device-route authorization and forwarding coverage

• Adds unit tests for exact, case-insensitive account ownership matching and rejection of missing or unvalidated identities. Handler tests use isolated validation and upstream seams to verify status codes, endpoints, payloads, and that rejected requests never leave the service; a nonparallel collection protects the mutable static seams.

dotnet/EcencyApi.Tests/DeviceAuthorizationTests.cs

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 17, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Device changes lack parity records 📘 Rule violation ▣ Testability
Description
The new 403 branches in RegisterDevice and DetailDevice alter their HTTP contracts, but no
matching entry was added to dotnet/parity/driver.py's KNOWN_DIVERGENCES. A valid code paired
with another or missing username now takes these branches, while the new tests cover the status and
blocked upstream call without documenting either endpoint's departure from the Node reference.
Code

dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs[R219-222]

+        if (!IsOwnDeviceRequest(authedUsername, body.Str("username")))
+        {
+            await ctx.SendText(403, "Forbidden");
+            return;
Evidence
Compliance rule 2667942 requires every observable endpoint behavior change to include both automated
tests and a parity-divergence entry. The handlers add new 403 outcomes at lines 219-222 and 246-249,
and tests were added, but the parity harness identifies driver.py's KNOWN_DIVERGENCES as the
documentation location and its current map contains no entry for either device endpoint.

Rule 2667942: Require tests and parity divergence docs for observable endpoint behavior changes
dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs[219-223]
dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs[246-250]
dotnet/parity/README.md[18-21]
dotnet/parity/driver.py[268-339]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The register-device and detail-device handlers now return 403 for usernames that do not match the signed-in account, but the parity harness does not document these intentional differences from the Node reference.
## Fix Focus Areas
- dotnet/parity/driver.py[268-339]
## Recommended Fix
Add `KNOWN_DIVERGENCES` entries for the affected register-device and detail-device parity cases, explicitly describing the new 403 response and absence of an upstream call for another or missing username.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs
@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 160159e4-05d7-4f25-996c-fd0b89901723

📥 Commits

Reviewing files that changed from the base of the PR and between dfbe219 and 861a172.

📒 Files selected for processing (2)
  • dotnet/EcencyApi.Tests/DeviceAuthorizationTests.cs
  • dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Device routes now verify that the requested username matches the account validated by the device code. Accepted requests use replaceable validation and upstream delegates. Tests cover registration, detail retrieval, invalid codes, mismatches, missing usernames, and case-insensitive matches.

Changes

Device authorization

Layer / File(s) Summary
Authorize device routes
dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs
Adds username matching, replaceable device delegates, and 403 responses for mismatches. Invalid codes return 401 without upstream calls.
Cover authorization outcomes
dotnet/EcencyApi.Tests/DeviceAuthorizationTests.cs
Tests accepted and rejected registration and detail requests, invalid codes, missing usernames, and upstream-call suppression.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant RegisterDevice
  participant DeviceValidateCode
  participant IsOwnDeviceRequest
  participant DeviceUpstream
  RegisterDevice->>DeviceValidateCode: Validate device code
  DeviceValidateCode-->>RegisterDevice: Return account or null
  RegisterDevice->>IsOwnDeviceRequest: Compare requested username
  alt Account matches
    IsOwnDeviceRequest-->>RegisterDevice: Return true
    RegisterDevice->>DeviceUpstream: Forward device request
    DeviceUpstream-->>RegisterDevice: Return response
  else Invalid code or mismatch
    IsOwnDeviceRequest-->>RegisterDevice: Return false
  end
Loading

Merge Risk: ⚪ Minimal · up to 861a1

The device routes now reject missing or mismatched usernames before forwarding requests, with tests covering accepted and rejected authorization outcomes. No unresolved merge-readiness risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: device routes now act only for the signed-in account.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

A rabbit checks the code at dawn
The matching name is carried on
A stranger's path is turned away
Safe hooks guide the route today
Tests hop through each guarded way

Comment @coderabbitai help to get the list of available commands.

@feruzm
feruzm merged commit a5b753f into main Sep 17, 2026
4 checks passed
@feruzm
feruzm deleted the fix/register-device-owner branch September 17, 2026 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant