Fix GltfLoader JSON size limits and ShaderCache key validation centralization#359
Fix GltfLoader JSON size limits and ShaderCache key validation centralization#359
Conversation
- Split maxJsonBufferBytes into separate raw-byte and decoded-string limits (new maxJsonStringBytes option, defaults to maxJsonBufferBytes * 2) to avoid unexpectedly halving the ASCII payload limit - Apply maxJsonBufferBytes and maxJsonStringBytes bounds inside parseGlb for the GLB JSON chunk, closing the resource-exhaustion gap on the GLB path - Centralize explicit key length validation in ShaderCache.assertKeyLength() private helper; replace three duplicated inline checks in getShader, getProgram, and getProgramKey - Update tests: rename string-guard test to use maxJsonStringBytes, add independence test, and add two GLB JSON chunk size limit tests Co-authored-by: ormidales <46538211+ormidales@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens glTF/GLB container parsing limits in GltfLoader by separating raw-byte vs decoded-string heap guards (and applying them to GLB JSON chunks), and removes duplicated explicit-key length checks in ShaderCache by centralizing validation.
Changes:
- Introduces
maxJsonStringBytes(defaulting tomaxJsonBufferBytes * 2) to decouple raw UTF-8 size limits from decoded UTF-16 heap footprint checks. - Applies both size bounds to the GLB JSON chunk path in
parseGlb. - Extracts
ShaderCache.assertKeyLength(key)and routesgetShader,getProgram, andgetProgramKeythrough it.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/core/GltfLoader.ts |
Adds independent raw/decoded JSON limits and enforces them for both .gltf and GLB JSON chunks. |
src/core/ShaderCache.ts |
Centralizes explicit cache-key length validation into a single helper. |
tests/gltf.test.ts |
Updates/extends tests to cover maxJsonStringBytes and GLB JSON chunk bounds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
…tle test - Validate maxJsonBufferBytes and maxJsonStringBytes in parseContainer: NaN, Infinity, or negative values now throw a RangeError immediately, preventing the size guards from being silently disabled - Add chunk length overflow bounds check in parseGlb: verify offset+8+chunkLength <= buffer.byteLength before slicing; malformed or truncated GLBs now throw a clear 'extends beyond end of file' error - Fix brittle GLB chunk size test: use getUint32(12, true) to read the actual on-disk JSON chunk length from the GLB header instead of re-stringifying the parsed result (which can differ due to padding) - Add 6 new tests: NaN/Infinity/negative for both limits, and chunk-extends-beyond-file Co-authored-by: ormidales <46538211+ormidales@users.noreply.github.com>
All three review comments addressed in commit 209675a:
6 new tests added covering: NaN, Infinity, negative for both limits, and the chunk-extends-beyond-file error. |
maxJsonBufferBytes/maxJsonStringBytesare finite and non-negative (NaN/Infinity/negative →RangeError)parseGlb(extends beyond file → clear error)getUint32(12, true)to read actual GLB chunk size from header instead of re-stringify📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.