You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fory.apache.org by "miantalha45 (via GitHub)" <gi...@apache.org> on 2026/02/20 10:48:27 UTC
[PR] feat(javascript): limit the depth of deserialize & serialize [fory]
miantalha45 opened a new pull request, #3382:
URL: https://github.com/apache/fory/pull/3382
## What does this PR do?
Adds depth limiting for deserialization to prevent stack overflow and denial-of-service attacks from maliciously crafted deeply nested data structures.
## Why is this needed?
Without depth limits, an attacker could send deeply nested serialized data that causes stack overflow during deserialization, crashing the application or causing resource exhaustion.
## Implementation
- Added `maxDepth` config option (default: 50, minimum: 2)
- Depth tracked only during deserialization (security-focused)
- Integrated into code generator with try/finally for proper cleanup
- Comprehensive test coverage (29 tests)
## Usage
```typescript
const fory = new Fory({ maxDepth: 100 });
## Consistency
Follows the same pattern as Java and Python implementations for cross-language alignment.
Fixes #3335
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #3382:
URL: https://github.com/apache/fory/pull/3382#discussion_r2835639607
##########
javascript/packages/fory/lib/gen/serializer.ts:
##########
@@ -200,12 +200,8 @@ export abstract class BaseSerializerGenerator implements SerializerGenerator {
readNoRef(assignStmt: (v: string) => string, refState: string): string {
return `
fory.incReadDepth();
- try {
- ${this.readTypeInfo()}
- ${this.read(assignStmt, refState)};
- } finally {
- fory.decReadDepth();
- }
+ ${this.readTypeInfo()}
+ ${this.read(assignStmt, refState)};
Review Comment:
I am also curious why the tests don't fail? The tests for depth is also wrong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang merged PR #3382:
URL: https://github.com/apache/fory/pull/3382
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "miantalha45 (via GitHub)" <gi...@apache.org>.
miantalha45 commented on PR #3382:
URL: https://github.com/apache/fory/pull/3382#issuecomment-4037914324
Thank you so much @chaokunyang for your help in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on PR #3382:
URL: https://github.com/apache/fory/pull/3382#issuecomment-3976253184
@chaokunyang Should depth tracking be limited to complex types like structs and maps? If primitive fields don't require it, we should place the incReadDepth and decReadDepth logic inside struct.ts and map.ts instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "miantalha45 (via GitHub)" <gi...@apache.org>.
miantalha45 commented on code in PR #3382:
URL: https://github.com/apache/fory/pull/3382#discussion_r2832819761
##########
javascript/packages/fory/lib/gen/serializer.ts:
##########
@@ -199,8 +199,13 @@ export abstract class BaseSerializerGenerator implements SerializerGenerator {
readNoRef(assignStmt: (v: string) => string, refState: string): string {
return `
- ${this.readTypeInfo()}
- ${this.read(assignStmt, refState)};
+ fory.incReadDepth();
+ try {
Review Comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #3382:
URL: https://github.com/apache/fory/pull/3382#discussion_r2835638351
##########
javascript/packages/fory/lib/gen/serializer.ts:
##########
@@ -200,12 +200,8 @@ export abstract class BaseSerializerGenerator implements SerializerGenerator {
readNoRef(assignStmt: (v: string) => string, refState: string): string {
return `
fory.incReadDepth();
- try {
- ${this.readTypeInfo()}
- ${this.read(assignStmt, refState)};
- } finally {
- fory.decReadDepth();
- }
+ ${this.readTypeInfo()}
+ ${this.read(assignStmt, refState)};
Review Comment:
Where you `decReadDepth`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #3382:
URL: https://github.com/apache/fory/pull/3382#issuecomment-3937846698
Please do not write filename such as `javascript/test/depth-limit.test.ts`, this is not right. Please see how other javascript files are named.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #3382:
URL: https://github.com/apache/fory/pull/3382#discussion_r2832802159
##########
javascript/packages/fory/lib/gen/serializer.ts:
##########
@@ -199,8 +199,13 @@ export abstract class BaseSerializerGenerator implements SerializerGenerator {
readNoRef(assignStmt: (v: string) => string, refState: string): string {
return `
- ${this.readTypeInfo()}
- ${this.read(assignStmt, refState)};
+ fory.incReadDepth();
+ try {
Review Comment:
the ray finally will introduce extra cost. Please remove try finally, and refactor serialize/deserializae entry point, create resetWrite/resetRead methods, move code like:
```
this.referenceResolver.resetRead();
this.binaryReader.reset(bytes);
this.typeMetaResolver.resetRead();
this.metaStringResolver.resetRead();
```
into `resetRead()`
and move smililiar methods into `resetWrite`.
Then set depth to 0 in `resetRead()` and `resetWrite`.
Current referenceResolver/typeMetaResolver/metaStringResolver may lack some such methods, please take java as referencev and add to javascript
##########
javascript/packages/fory/lib/gen/serializer.ts:
##########
@@ -199,8 +199,13 @@ export abstract class BaseSerializerGenerator implements SerializerGenerator {
readNoRef(assignStmt: (v: string) => string, refState: string): string {
return `
- ${this.readTypeInfo()}
- ${this.read(assignStmt, refState)};
+ fory.incReadDepth();
+ try {
Review Comment:
the try finally will introduce extra cost. Please remove try finally, and refactor serialize/deserializae entry point, create resetWrite/resetRead methods, move code like:
```
this.referenceResolver.resetRead();
this.binaryReader.reset(bytes);
this.typeMetaResolver.resetRead();
this.metaStringResolver.resetRead();
```
into `resetRead()`
and move smililiar methods into `resetWrite`.
Then set depth to 0 in `resetRead()` and `resetWrite`.
Current referenceResolver/typeMetaResolver/metaStringResolver may lack some such methods, please take java as referencev and add to javascript
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "miantalha45 (via GitHub)" <gi...@apache.org>.
miantalha45 commented on PR #3382:
URL: https://github.com/apache/fory/pull/3382#issuecomment-3940430655
@chaokunyang I have fixed all issues listed by you. Please have a look on it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(javascript): limit the depth of deserialize & serialize [fory]
Posted by "miantalha45 (via GitHub)" <gi...@apache.org>.
miantalha45 commented on PR #3382:
URL: https://github.com/apache/fory/pull/3382#issuecomment-3978860733
@theweipeng Should i place incReadDepth and decReadDepth logic inside struct.ts and map.ts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org