Conversation
Greptile SummaryThis PR adds
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[fobs.unpack / deserialize] --> B{type_name in whitelist?}
B -- No --> C[Reject: security guard blocks deserialization]
B -- Yes --> D{xgboost installed?}
D -- No --> E[ImportError at load_class]
D -- Yes --> F{Is type an IntEnum?}
F -- Yes --> G[EnumTypeDecomposer handles DataSplitMode]
G --> H[Object reconstructed successfully]
Last reviewed commit: 409e6eb |
| "nvflare.apis.analytix.LogWriterName", | ||
| # Found in integration test | ||
| "nvflare.apis.job_def.JobMetaKey", | ||
| "xgboost.core.DataSplitMode", |
There was a problem hiding this comment.
Missing explanatory comment for optional dependency
The file consistently documents why each type is included in this security-sensitive whitelist (e.g., tenseal.CKKSVector has a multi-line note about it being an optional dependency, and JobMetaKey says "Found in integration test"). Since xgboost is also an optional dependency, adding a comment here would:
- Help future maintainers understand why this type was added
- Clarify that if xgboost is not installed,
fobs.unpack()will still fail with anImportError(consistent with thetensealbehavior)
| "xgboost.core.DataSplitMode", | |
| # XGBoost type used in federated XGBoost training (optional dependency). | |
| # If xgboost is not installed, fobs.unpack() will fail at load_class() with ImportError. | |
| "xgboost.core.DataSplitMode", |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Added more types to the FOBS types white-list
Types of changes
./runtest.sh.