Skip to content

Validate edge device ID headers#4825

Open
fallintoplace wants to merge 3 commits into
NVIDIA:mainfrom
fallintoplace:fix-edge-device-id-header
Open

Validate edge device ID headers#4825
fallintoplace wants to merge 3 commits into
NVIDIA:mainfrom
fallintoplace:fix-edge-device-id-header

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • Keep X-Flare-Device-ID as the canonical device identity for FEG requests.
  • Reject requests when X-Flare-Device-Info contains a conflicting device_id.
  • Add focused request-context tests for absent, matching, and mismatched metadata device IDs.

Why

The FEG parser creates DeviceInfo from X-Flare-Device-ID, but then blindly applies fields parsed from X-Flare-Device-Info. A device_id embedded in the metadata header could override the canonical ID used for routing, selection, device tracking, and result attribution. The simulation client already removes device_id from the metadata header, so treating the standalone ID header as canonical matches the existing client behavior.

Testing

  • .venv/bin/python -m pytest tests/unit_test/edge/web/views/feg_views_test.py -q
  • .venv/bin/python -m black --check nvflare/edge/web/views/feg_views.py tests/unit_test/edge/web/views/feg_views_test.py
  • .venv/bin/python -m isort --check-only nvflare/edge/web/views/feg_views.py tests/unit_test/edge/web/views/feg_views_test.py
  • .venv/bin/python -m flake8 nvflare/edge/web/views/feg_views.py tests/unit_test/edge/web/views/feg_views_test.py
  • source .venv/bin/activate && ./runtest.sh -s

@greptile-apps

greptile-apps Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR hardens the FEG header-parsing logic by treating X-Flare-Device-ID as the authoritative device identity. Previously, from_query_string() could silently overwrite the canonical ID with a device_id embedded in X-Flare-Device-Info. Now the parser actively rejects conflicting IDs with a 400 and always reassigns the canonical value after parsing.

  • feg_views.py: adds a mismatch guard inside _process_headers() and moves the canonical device_id re-assignment outside the if device_info_header: block so the invariant holds on every code path.
  • feg_views_test.py: new test file covering all four relevant branches — absent DEVICE_INFO, DEVICE_INFO without device_id, matching device_id, and mismatched device_id.

Confidence Score: 5/5

Safe to merge — the change is a narrow defensive guard with no effect on the happy path and full branch coverage in the new tests.

The two modified lines (mismatch check + re-assignment) are logically correct: DeviceInfo is a dict subclass, so device_info.get('device_id') after from_query_string reliably returns the overwritten value only when the query string actually contained that key. The canonical re-assignment on line 72 is now unconditional, closing the previously addressed gap. All four branches are exercised by the new test suite.

No files require special attention.

Important Files Changed

Filename Overview
nvflare/edge/web/views/feg_views.py Adds a device_id mismatch check after parsing X-Flare-Device-Info and moves the canonical re-assignment outside the conditional block; logic is correct across all branches.
tests/unit_test/edge/web/views/feg_views_test.py New test file; covers all four branches of the updated _process_headers() including absent header, header without device_id, matching device_id, and mismatched device_id.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Request arrives] --> B{X-Flare-Device-ID\npresent?}
    B -- No --> C[400 INVALID_REQUEST\nDevice ID missing]
    B -- Yes --> D[DeviceInfo device_id\n= canonical ID]
    D --> E{X-Flare-Device-Info\npresent?}
    E -- No --> H
    E -- Yes --> F[from_query_string\nparses metadata]
    F --> G{device_id in\nDEVICE_INFO AND\n!= canonical?}
    G -- Yes --> J[400 INVALID_REQUEST\nDevice ID mismatch]
    G -- No --> H[device_info.device_id\n= canonical ID]
    H --> I[Continue to\nroute handler]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Request arrives] --> B{X-Flare-Device-ID\npresent?}
    B -- No --> C[400 INVALID_REQUEST\nDevice ID missing]
    B -- Yes --> D[DeviceInfo device_id\n= canonical ID]
    D --> E{X-Flare-Device-Info\npresent?}
    E -- No --> H
    E -- Yes --> F[from_query_string\nparses metadata]
    F --> G{device_id in\nDEVICE_INFO AND\n!= canonical?}
    G -- Yes --> J[400 INVALID_REQUEST\nDevice ID mismatch]
    G -- No --> H[device_info.device_id\n= canonical ID]
    H --> I[Continue to\nroute handler]
Loading

Reviews (4): Last reviewed commit: "Merge branch 'main' into fix-edge-device..." | Re-trigger Greptile

Comment thread nvflare/edge/web/views/feg_views.py
Comment thread tests/unit_test/edge/web/views/feg_views_test.py
@fallintoplace fallintoplace force-pushed the fix-edge-device-id-header branch from f275c5d to 8b5f5cc Compare June 21, 2026 20:18
@YuanTingHsieh

Copy link
Copy Markdown
Collaborator

@codecov-commenter

codecov-commenter commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.60%. Comparing base (2005421) to head (5cdadb1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4825      +/-   ##
==========================================
+ Coverage   56.39%   56.60%   +0.21%     
==========================================
  Files         968      968              
  Lines       92166    92170       +4     
==========================================
+ Hits        51976    52172     +196     
+ Misses      40190    39998     -192     
Flag Coverage Δ
unit-tests 56.60% <100.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fallintoplace

Copy link
Copy Markdown
Contributor Author

@YuanTingHsieh I have updated both branch. Thank you for your attention.

@pcnudde pcnudde enabled auto-merge (squash) June 30, 2026 18:43
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.

4 participants