fix: gate TypeNameHandling.Auto with a resolver-scoped SerializationBinder [AIS-255] - #399
Conversation
…tType in ContentJsonConverter [AIS-255] TypeNameHandling.All causes Newtonsoft.Json to honor any $type discriminator present in deserialized JSON, enabling RCE via gadget chains in untrusted input. Switched to TypeNameHandling.None and removed the ContentJsonConverter path that called Type.GetType on a $type value from the JSON object — all IContent node types are handled by the existing safe switch statement instead. The ResolveContentTypes/$type round-trip in ContentfulClient is unaffected: it only writes developer-configured CLR types, not attacker-controlled values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #0a94d9Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
|
| Source | Requirement / Code Area | Status | Notes |
|---|---|---|---|
| AIS-255 | Fix insecure deserialization vulnerability in ContentfulClient by replacing TypeNameHandling.All with safe type resolution that prevents arbitrary type loading from attacker-influenced $type discriminators | ✅ Met | The insecure deserialization vulnerability in ContentfulClient has been fixed. The PR replaced `TypeNameHandling.All` (line 269) with `TypeNameHandling.Auto` combined with a custom ContentTypeResolverBinder (line 277). The ResolveLinks method now uses `_typeResolverBinder.TryResolveAllowed()` instead of direct `Type.GetType()` call on untrusted `$type` fields (lines 301-304). Types are explicitly whitelisted via `_typeResolverBinder.Allow(type)` in ResolveContentTypes (line 287) before `$type` is written, ensuring only developer-configured resolver types can be bound. This prevents arbitrary type loading from attacker-influenced `$type` discriminators (CWE-502). |
| AIS-255 | Fix insecure deserialization in ContentJsonConverter by implementing safe type resolution for the $type discriminator instead of direct Type.GetType call | ✅ Met | The insecure deserialization vulnerability in ContentJsonConverter has been fixed. In ReadJson (lines 131-134), the code now checks `typeof(IContent).IsAssignableFrom(typeinfo)` before calling `jObject.ToObject(typeinfo, serializer)`. This ensures that only types implementing the `IContent` interface can be loaded via the `$type` discriminator, preventing arbitrary CLR type loading from attacker-influenced JSON. Combined with the SerializationBinder in ContentfulClient, this provides defense-in-depth against CWE-502 gadget-chain deserialization attacks. |
Impact Analysis by BitoInteraction DiagramsequenceDiagram
participant Dev as Developer
participant Client as ContentfulClient<br/>🔄 Updated | ●●● High
participant Binder as ContentTypeResolverBinder<br/>🟩 Added | ●●● High
participant Conv as ContentJsonConverter<br/>🔄 Updated | ●●○ Medium
participant API as Contentful API
participant Test as Test Suite
participant Gadget as MaliciousGadget<br/>🟩 Added | ●○○ Low
Note over Client, Binder: Security fix for CWE-502 deserialization vulnerability
Dev->>Client: Configure IContentTypeResolver
Client->>Binder: Initialize SerializationBinder
Binder-->>Client: Binder configured
alt Positive Path: Valid Type Resolution
Client->>API: GetEntries<T>() request
API-->>Client: Return JSON with entries
Client->>Client: ResolveContentTypes()
Client->>Binder: Allow(developer-registered type)
Binder->>Binder: Add type to allowlist
Client->>Conv: Deserialize with $type
Conv->>Conv: Verify type implements IContent
Conv-->>Client: Return deserialized object
end
alt Negative Path: Attack Prevention
Test->>Client: Inject malicious $type in response
Client->>Binder: TryResolveAllowed(maliciousType)
Binder->>Binder: Check allowlist (not found)
Binder-->>Client: Return false
Client->>Client: Fall back to caller-provided type
Conv->>Conv: Deserialize without $type
Conv-->>Test: JsonSerializationException thrown
Test->>Gadget: Verify WasConstructed = false
end
This MR implements a security fix for CWE-502 (deserialization of untrusted data) by introducing ContentTypeResolverBinder, an ISerializationBinder that restricts $type resolution to types explicitly allowed by the developer-configured IContentTypeResolver. The ContentfulClient now uses TypeNameHandling.Auto with the custom binder instead of TypeNameHandling.All, preventing arbitrary gadget-chain attacks while maintaining legitimate type resolution functionality. ContentJsonConverter adds an IContent interface check for additional safety. No cross-repo dependencies detected. Cross-Repository Impact Analysis
Code Paths AnalyzedImpact: Flow: Direct Changes (Diff Files): Repository Impact: Cross-Repository Dependencies: Database/Caching Impact: API Contract Violations: Infrastructure Dependencies: Additional Insights: Testing RecommendationsFrontend Impact: Service Integration: Data Serialization: Privacy Compliance: Backward Compatibility: OAuth Functionality: Cross-Service Communication: Reliability Testing: Additional Insights: Analysis based on known dependency patterns and edges. Actual impact may vary. |
✅ Review Settings OverriddenStatus: Guidelines:
Note: Extra guidelines beyond 3 general purpose guidelines and 1 language specific guideline per language are not processed. Guidelines are fetched from the source branch. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
…ut enabling gadget chains [AIS-255] TypeNameHandling.None broke the ContentTypeResolver mechanism: when GetEntries<IMarker>() is called, Newtonsoft needs to read the $type values that ResolveContentTypes() writes (from developer-registered IContentTypeResolver mappings) in order to instantiate concrete CLR types for abstract/interface-typed generics. TypeNameHandling.Auto only activates $type resolution when the declared target type is abstract or an interface — it never kicks in for concrete types coming from the Contentful API. The actual gadget-chain attack surface (Type.GetType on attacker-controlled $type from raw API JSON in ContentJsonConverter) was already removed in the prior commit, so Auto does not reopen AIS-255. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review Agent Run #35e4ba
Actionable Suggestions - 1
-
Contentful.Core/ContentfulClient.cs - 1
- CWE-502: Unpatched deserialization vector · Line 53-58
Review Details
-
Files reviewed - 1 · Commit Range:
1c94a18..56ad3c4- Contentful.Core/ContentfulClient.cs
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Csharp (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at jared.jolton@contentful.com.
Documentation & Help
AI Code Review powered by
…mplementors [AIS-255] TypeNameHandling.Auto restored IContentTypeResolver support but ContentJsonConverter intercepts IContent deserialization before TypeNameHandling logic runs. Restore $type reading in the converter, but constrain it to types that implement IContent — the gadget-chain types exploited by the original vulnerability (ObjectDataProvider, etc.) do not implement IContent, so the attack surface remains closed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #b0332bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…inder [AIS-255] TypeNameHandling.Auto is required for IContentTypeResolver to instantiate concrete CLR types, but on its own it lets any $type in an API response load an arbitrary type on object/dynamic members that never pass through ContentJsonConverter (e.g. ContentfulCollection.IncludedEntries) — the CWE-502 gadget-chain vector the earlier commits left open. Add ContentTypeResolverBinder, an ISerializationBinder that only binds $type values to types explicitly produced by a developer-configured IContentTypeResolver. ResolveContentTypes() whitelists each resolved type before writing its $type; every other $type is rejected with a JsonSerializationException. Tests: negative test proves an injected, genuinely-loadable gadget type is rejected and never constructed (verified to fail with the binder disabled); positive test confirms resolver-produced types still deserialize under Auto. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #8bc1f5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…IS-255] ResolveLinks called Type.GetType directly on the entry's $type value to pick the CLR type used for nested-property reflection. The result is not instantiated there, so it is not a gadget sink, but it still triggers arbitrary type/assembly resolution from an attacker-influenceable string. Route it through ContentTypeResolverBinder.TryResolveAllowed, a non-throwing lookup that only returns types previously whitelisted from a developer IContentTypeResolver mapping. An injected $type is now ignored (Type.GetType is never called on it) and link resolution falls back to the caller-provided type — mirroring the SerializationBinder gate used during deserialization. Behavior for legitimate resolver-produced $type values is unchanged (they are always whitelisted before being written). Full Core suite green (677). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #aac027Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Tyler Pina (tylerpina)
changed the title
2 participants
Summary
Hardens the delivery/preview client against Newtonsoft.Json gadget-chain deserialization (CWE-502) while preserving
IContentTypeResolversupport.IContentTypeResolverrequiresTypeNameHandling.Autoso the serializer can instantiate developer-registered concrete CLR types from a$typemarker. The problem:Autoon the globalSerializerSettingshonors$typeon everyobject/dynamic/interface-typed member — including ones that never pass throughContentJsonConverter(e.g.ContentfulCollection.IncludedEntries, which isEntry<dynamic>). Without a binder, any$typein a raw API response could load an arbitrary CLR type.This PR closes that vector across four layers:
ContentTypeResolverBinder(ISerializationBinder) — the actual guard. It binds a$typevalue only to a type the client explicitly allowed.ResolveContentTypes()whitelists each resolved type immediately before writing its$type, so only developer-configured resolver mappings are ever bindable. Every other$type(including attacker-injected ones) is rejected with aJsonSerializationException.ContentfulClientconstructor — setsTypeNameHandling.Auto(needed for the resolver) and installs the binder onSerializerSettings.ResolveLinks— previously calledType.GetTypedirectly on the entry's$type. Not a gadget sink (the result is only used for reflection, not instantiation), but it still resolved an arbitrary type from an attacker-influenceable string. Now routed through the same whitelist (TryResolveAllowed, a non-throwing lookup); an injected$typeis ignored and resolution falls back to the caller-provided type.ContentJsonConverter.ReadJson— the$typebranch is additionally constrained toIContentimplementors as defense-in-depth on the rich-text node path.Why
Auto+ binder rather thanNoneTypeNameHandling.Nonefully closes the vector but breaksIContentTypeResolver(it can no longer instantiate concrete types from the injected$type).Auto+ a whitelist binder keeps the resolver working while constraining$typeresolution to exactly the set of types the developer registered — no attacker-controlled type is loadable.Tickets
Closes AIS-255
Test plan
Contentful.Coresuite passes (677 tests) — no regressionsContentful.AspNetCoresuite passes (17 tests)InjectedTypeInApiResponseShouldBeRejectedBySerializationBinder— an injected, genuinely-loadable gadget type is rejected and never constructed. Verified this test fails with the binder disabled (proves it exercises the guard, not an assembly-not-found accident). The injected$typesits onitems[0], so it also exercises the hardenedResolveLinkspath.ResolverProducedTypesShouldStillDeserializeUnderSerializationBinder— resolver-produced types still deserialize correctly underAuto+ binder.UsingAContentTypeResolver*(incl. with-includes, which relies onResolveLinkshonoring$type) and rich-text rendering tests unaffected.🤖 Generated with Claude Code
Summary by Bito
This PR hardens the Contentful delivery/preview client against Newtonsoft.Json gadget-chain deserialization (CWE-502) by introducing a whitelist-based serialization binder while preserving IContentTypeResolver functionality. The binder constrains TypeNameHandling.Auto so only developer-registered types can be deserialized, preventing arbitrary CLR type loading from injected $type values in API responses.
Detailed Changes