feat: inline asn1 library - #50
Open
Julusian wants to merge 4 commits into
Open
Conversation
WalkthroughThe PR adds local ASN.1 BER reader and writer modules, rewires BER wrappers, removes the external ChangesLocal ASN.1 BER implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EmberWriter
participant LocalWriter
participant LocalReader
participant EmberReader
EmberWriter->>LocalWriter: Encode BER values
LocalWriter-->>EmberWriter: Return BER buffer
EmberReader->>LocalReader: Decode BER buffer
LocalReader-->>EmberReader: Return values or null
EmberReader->>EmberReader: Preserve existing values for nullish reads
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/ASN1/ber/__tests__/writer.spec.ts (1)
291-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOID test has no assertions, so regressions won’t be caught.
Replace logging with explicit byte assertions against expected BER output.
Suggested fix
test('Write OID', () => { const oid = '1.2.840.113549.1.1.1' const writer = new Writer() writer.writeOID(oid) const ber = writer.buffer expect(ber).toBeInstanceOf(Buffer) - console.log(util.inspect(ber)) - console.log(util.inspect(Buffer.from([0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01]))) + expect(ber).toEqual(Buffer.from([0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01])) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ASN1/ber/__tests__/writer.spec.ts` around lines 291 - 300, The Write OID test in writer.spec.ts only logs the actual and expected buffers, so it never verifies behavior. In the test named Write OID, replace the console.log calls with explicit assertions on the Writer.buffer output from writeOID(oid), and compare it against the expected BER bytes using the existing oid and Writer symbols so regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ASN1/ber/reader.ts`:
- Around line 71-103: `readBlock()` in the ASN.1 BER reader is validating
nested-length bytes against the wrong cursor state, so truncated
indefinite-length inputs can overrun the buffer or loop forever. Update the
bounds checks in `readBlock()` to use `currOffset` consistently for the
length-byte reads and payload advance, and ensure the nested-length branch
returns null or throws before `currOffset` can move past `this._size`.
- Around line 178-180: readBoolean() in the BER reader is treating a null result
from _readTag(Types.Boolean) as true, which fabricates a value on truncated
input. Update readBoolean() to explicitly handle a null return from _readTag()
as an error or null-equivalent path before converting the tag value, and ensure
it does not leave the cursor unchanged on malformed BER.
- Around line 281-300: The INTEGER decoding logic in the reader’s long-form
integer path is using 32-bit bitwise operations, so values longer than 4 bytes
can overflow or decode incorrectly; update this branch in the ASN.1 BER reader’s
integer parsing routine to reject lengths greater than 4 bytes (or otherwise
switch to a wider numeric representation). Keep the existing length checks near
readLength/readInteger handling, and adjust the overflow guard so it aligns with
the 32-bit arithmetic used by the value assembly and final shift.
In `@src/ASN1/ber/writer.ts`:
- Around line 109-117: writeBuffer currently rejects empty Buffers because it
always calls _ensure(buf.length), which breaks zero-length TLV encoding. Update
writeBuffer to handle Buffer.alloc(0) as a valid case by skipping the buffer
growth check when buf.length is 0, while still writing the tag and length; use
writeBuffer and _ensure as the key locations, and keep behavior consistent with
writeString’s empty-content handling.
- Around line 29-32: Validate the writer resize settings in the ASN.1 BER writer
constructor so invalid buffer growth values cannot reach _ensure. In the Writer
constructor, where _options is assigned, add validation for growthFactor (and
any related size constraints) to reject zero, negative, or non-finite values
before they are used by _ensure and the reallocation logic. Keep the checks
close to the _options initialization so bad options are caught early and the
writer state cannot become corrupted during buffer growth.
- Around line 132-138: The OID validation in the writer logic is too permissive
and allows trailing-dot strings like a dangling arc, which later gets parsed
into invalid output; tighten the check in the OID parsing path so empty segments
are rejected before splitting/parsing, and make sure the validation in the same
writer function that processes the input string fails fast for malformed OIDs
instead of producing bytes from a NaN value.
- Around line 50-62: The writeInt method is relying on bitwise coercion before
validating the input, so invalid or non-integer numbers can be silently
truncated. Update writeInt in the ASN1 BER writer to explicitly validate that i
is a safe integer within the supported 32-bit unsigned range before any bit
operations, and keep the existing Types.Integer/tag handling unchanged. Use the
writeInt guard path and the newInvalidAsn1Error check to reject out-of-range
values instead of letting the loop encode them incorrectly.
In `@src/encodings/ber/decoder/Command.ts`:
- Around line 40-43: The `Command` decoder is treating a valid zero mask as
missing because the current falsy check on `dirFieldMask`/the returned mask from
`intToMask` conflates `FieldFlags.Default` with `undefined`. Update the logic in
the `decoder/Command` flow to check explicitly for `undefined` (or use a nullish
check) before rejecting the value, and keep the rest of the `reader.readInt()`
to `intToMask` mapping unchanged.
In `@src/encodings/ber/decoder/Matrix.ts`:
- Line 218: The BER matrix decoding in Matrix.ts is still forcing nullable
results from reader.readInt() into number[] by asserting non-null. Update the
array population logic at the affected push sites in the Matrix decoder so that
readInt() failures are handled explicitly instead of cast away, and only valid
numeric values are stored in targets. Use the Matrix decoder methods that build
the numeric arrays to locate and remove the unsafe assertions.
In `@src/encodings/ber/decoder/Parameter.ts`:
- Line 108: In Parameter.ts, the templateReference decode in the BER parameter
decoder is using the wrong reader type: it is parsed as a STRING even though
templateReference is a RelativeOID and should match the other BER OID decoders.
Update the decode logic in the relevant parameter-handling path to read
templateReference with the RELATIVE_OID BER data type, using the same reader
pattern used for other OID-like fields so valid BER OID payloads are decoded
correctly.
---
Nitpick comments:
In `@src/ASN1/ber/__tests__/writer.spec.ts`:
- Around line 291-300: The Write OID test in writer.spec.ts only logs the actual
and expected buffers, so it never verifies behavior. In the test named Write
OID, replace the console.log calls with explicit assertions on the Writer.buffer
output from writeOID(oid), and compare it against the expected BER bytes using
the existing oid and Writer symbols so regressions are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76a11de5-e032-4f2e-8a65-faa656bc6c1f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
package.jsonsrc/ASN1/LICENSEsrc/ASN1/README.mdsrc/ASN1/ber/__tests__/reader.spec.tssrc/ASN1/ber/__tests__/writer.spec.tssrc/ASN1/ber/errors.tssrc/ASN1/ber/reader.tssrc/ASN1/ber/types.tssrc/ASN1/ber/writer.tssrc/Ber/Reader.tssrc/Ber/Writer.tssrc/encodings/ber/decoder/Command.tssrc/encodings/ber/decoder/Connection.tssrc/encodings/ber/decoder/DecodeResult.tssrc/encodings/ber/decoder/EmberFunction.tssrc/encodings/ber/decoder/EmberNode.tssrc/encodings/ber/decoder/FunctionArgument.tssrc/encodings/ber/decoder/Invocation.tssrc/encodings/ber/decoder/Matrix.tssrc/encodings/ber/decoder/Parameter.tssrc/encodings/ber/decoder/StreamDescription.tssrc/encodings/ber/decoder/Template.ts
💤 Files with no reviewable changes (1)
- package.json
Comment on lines
+281
to
+300
| const o = this.readLength(this._offset + 1) // stored in `length` | ||
| if (o === null) return null | ||
|
|
||
| if (this.length > 8) throw newInvalidAsn1Error('Integer too long: ' + this.length) | ||
|
|
||
| if (this.length > this._size - o) return null | ||
| this._offset = o | ||
|
|
||
| const fb = this._buf[this._offset] | ||
| let value = 0 | ||
|
|
||
| let i: number | ||
| for (i = 0; i < this.length; i++) { | ||
| value <<= 8 | ||
| value |= this._buf[this._offset++] & 0xff | ||
| } | ||
|
|
||
| if ((fb & 0x80) === 0x80 && i !== 4) value -= 1 << (i * 8) | ||
|
|
||
| return value >> 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
let value = 0
for (const byte of [0x01, 0x00, 0x00, 0x00, 0x00]) {
value <<= 8
value |= byte
}
console.log('current logic returns:', value >> 0)
console.log('expected integer is :', 0x0100000000)
NODERepository: Sofie-Automation/sofie-emberplus-connection
Length of output: 241
Limit INTEGER decoding to 32 bits
This path uses 32-bit bitwise ops (<<, |, >> 0), so any INTEGER longer than 4 bytes can overflow or decode incorrectly (e.g. 0x0100000000 becomes 0). Cap this at 4 bytes unless the reader is rewritten to use a wider representation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ASN1/ber/reader.ts` around lines 281 - 300, The INTEGER decoding logic in
the reader’s long-form integer path is using 32-bit bitwise operations, so
values longer than 4 bytes can overflow or decode incorrectly; update this
branch in the ASN.1 BER reader’s integer parsing routine to reject lengths
greater than 4 bytes (or otherwise switch to a wider numeric representation).
Keep the existing length checks near readLength/readInteger handling, and adjust
the overflow guard so it aligns with the 32-bit arithmetic used by the value
assembly and final shift.
Comment on lines
+29
to
+32
| this._options = { | ||
| size: options?.size ?? 1024, | ||
| growthFactor: options?.growthFactor ?? 8, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate resize options to prevent invalid buffer growth behavior.
growthFactor is unvalidated; values like 0 can produce undersized reallocation and corrupted writer state during _ensure (Line 283+). Add constructor validation.
Suggested fix
constructor(options?: Partial<WriterOptions>) {
this._options = {
size: options?.size ?? 1024,
growthFactor: options?.growthFactor ?? 8,
}
+ if (!Number.isInteger(this._options.size) || this._options.size <= 0) {
+ throw new TypeError('size must be a positive integer')
+ }
+ if (!Number.isFinite(this._options.growthFactor) || this._options.growthFactor <= 1) {
+ throw new TypeError('growthFactor must be > 1')
+ }Also applies to: 283-291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ASN1/ber/writer.ts` around lines 29 - 32, Validate the writer resize
settings in the ASN.1 BER writer constructor so invalid buffer growth values
cannot reach _ensure. In the Writer constructor, where _options is assigned, add
validation for growthFactor (and any related size constraints) to reject zero,
negative, or non-finite values before they are used by _ensure and the
reallocation logic. Keep the checks close to the _options initialization so bad
options are caught early and the writer state cannot become corrupted during
buffer growth.
Comment on lines
+50
to
+62
| writeInt(i: number, tag?: number): void { | ||
| if (typeof i !== 'number') throw new TypeError('argument must be a Number') | ||
| if (typeof tag !== 'number') tag = Types.Integer | ||
|
|
||
| let sz = 4 | ||
|
|
||
| while (((i & 0xff800000) === 0 || (i & 0xff800000) === 0xff800000 >> 0) && sz > 1) { | ||
| sz-- | ||
| i <<= 8 | ||
| } | ||
|
|
||
| if (sz > 4) throw newInvalidAsn1Error('BER ints cannot be > 0xffffffff') | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
writeInt silently truncates invalid numeric inputs.
Line 61’s guard is unreachable (sz starts at 4 and only decreases), so out-of-range/non-integer values can be encoded incorrectly via 32-bit coercion. Validate integer-ness and range before bit operations.
Suggested fix
writeInt(i: number, tag?: number): void {
if (typeof i !== 'number') throw new TypeError('argument must be a Number')
+ if (!Number.isInteger(i)) throw new TypeError('argument must be an integer')
+ if (i < -0x80000000 || i > 0x7fffffff) {
+ throw newInvalidAsn1Error('BER ints must be within signed 32-bit range')
+ }
if (typeof tag !== 'number') tag = Types.Integer
@@
- if (sz > 4) throw newInvalidAsn1Error('BER ints cannot be > 0xffffffff')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| writeInt(i: number, tag?: number): void { | |
| if (typeof i !== 'number') throw new TypeError('argument must be a Number') | |
| if (typeof tag !== 'number') tag = Types.Integer | |
| let sz = 4 | |
| while (((i & 0xff800000) === 0 || (i & 0xff800000) === 0xff800000 >> 0) && sz > 1) { | |
| sz-- | |
| i <<= 8 | |
| } | |
| if (sz > 4) throw newInvalidAsn1Error('BER ints cannot be > 0xffffffff') | |
| writeInt(i: number, tag?: number): void { | |
| if (typeof i !== 'number') throw new TypeError('argument must be a Number') | |
| if (!Number.isInteger(i)) throw new TypeError('argument must be an integer') | |
| if (i < -0x80000000 || i > 0x7fffffff) { | |
| throw newInvalidAsn1Error('BER ints must be within signed 32-bit range') | |
| } | |
| if (typeof tag !== 'number') tag = Types.Integer | |
| let sz = 4 | |
| while (((i & 0xff800000) === 0 || (i & 0xff800000) === 0xff800000 >> 0) && sz > 1) { | |
| sz-- | |
| i <<= 8 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ASN1/ber/writer.ts` around lines 50 - 62, The writeInt method is relying
on bitwise coercion before validating the input, so invalid or non-integer
numbers can be silently truncated. Update writeInt in the ASN1 BER writer to
explicitly validate that i is a safe integer within the supported 32-bit
unsigned range before any bit operations, and keep the existing
Types.Integer/tag handling unchanged. Use the writeInt guard path and the
newInvalidAsn1Error check to reject out-of-range values instead of letting the
loop encode them incorrectly.
Comment on lines
+40
to
+43
| const int = reader.readInt() | ||
| if (typeof int !== 'number') return undefined | ||
|
|
||
| return intToMask[int] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the FieldFlags definition and the downstream falsy check.
rg -n -C2 '\b(enum|type)\s+FieldFlags\b|FieldFlags\.(Default|All|Sparse)' src --type=ts
rg -n -C2 'if \(!dirFieldMask\)' src/encodings/ber/decoder/Command.tsRepository: Sofie-Automation/sofie-emberplus-connection
Length of output: 2535
Check dirFieldMask explicitly for undefined FieldFlags.Default is 0, so if (!dirFieldMask) treats a valid default mask as unknown. Use a nullish/explicit undefined check here instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/encodings/ber/decoder/Command.ts` around lines 40 - 43, The `Command`
decoder is treating a valid zero mask as missing because the current falsy check
on `dirFieldMask`/the returned mask from `intToMask` conflates
`FieldFlags.Default` with `undefined`. Update the logic in the `decoder/Command`
flow to check explicitly for `undefined` (or use a nullish check) before
rejecting the value, and keep the rest of the `reader.readInt()` to `intToMask`
mapping unchanged.
| break | ||
| case Ber.CONTEXT(18): | ||
| templateReference = reader.readString(Ber.BERDataTypes.STRING) | ||
| templateReference = reader.readString(Ber.BERDataTypes.STRING) ?? templateReference |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Decode templateReference as RELATIVE_OID, not STRING.
At Line 108, templateReference is typed as RelativeOID but read via readString(...). This diverges from the other decoders and can misdecode valid BER OID data.
Suggested fix
- case Ber.CONTEXT(18):
- templateReference = reader.readString(Ber.BERDataTypes.STRING) ?? templateReference
+ case Ber.CONTEXT(18):
+ templateReference =
+ reader.readRelativeOID(Ber.BERDataTypes.RELATIVE_OID) ?? templateReference
break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| templateReference = reader.readString(Ber.BERDataTypes.STRING) ?? templateReference | |
| case Ber.CONTEXT(18): | |
| templateReference = | |
| reader.readRelativeOID(Ber.BERDataTypes.RELATIVE_OID) ?? templateReference | |
| break |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/encodings/ber/decoder/Parameter.ts` at line 108, In Parameter.ts, the
templateReference decode in the BER parameter decoder is using the wrong reader
type: it is parsed as a STRING even though templateReference is a RelativeOID
and should match the other BER OID decoders. Update the decode logic in the
relevant parameter-handling path to read templateReference with the RELATIVE_OID
BER data type, using the same reader pattern used for other OID-like fields so
valid BER OID payloads are decoded correctly.
nytamin
approved these changes
Aug 10, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ASN1/ber/reader.ts (1)
273-275: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDecode the first OID subidentifier with BER first-arc rules.
For a first subidentifier of
80or greater, BER requires first arc2and second arcvalue - 80. The current division and modulo logic decodes2.100.xas4.20.x.Use the ranges
0..39,40..79, and80+to produce the first two arcs. Add coverage for an OID such as2.100.3.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ASN1/ber/reader.ts` around lines 273 - 275, Update the first-subidentifier decoding near values.shift() to apply BER’s three ranges: 0–39 maps to arcs 0 and value, 40–79 maps to arcs 1 and value minus 40, and 80+ maps to arcs 2 and value minus 80. Preserve subsequent subidentifier decoding and add coverage for an OID such as 2.100.3.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/ASN1/ber/reader.ts`:
- Around line 273-275: Update the first-subidentifier decoding near
values.shift() to apply BER’s three ranges: 0–39 maps to arcs 0 and value, 40–79
maps to arcs 1 and value minus 40, and 80+ maps to arcs 2 and value minus 80.
Preserve subsequent subidentifier decoding and add coverage for an OID such as
2.100.3.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5129511e-85e6-454b-8b7f-0f7a08bc5936
📒 Files selected for processing (11)
src/ASN1/ber/__tests__/reader.spec.tssrc/ASN1/ber/__tests__/writer.spec.tssrc/ASN1/ber/reader.tssrc/ASN1/ber/writer.tssrc/Ber/Reader.tssrc/Ber/Writer.tssrc/Ber/__tests__/index.spec.tssrc/encodings/ber/decoder/EmberNode.tssrc/encodings/ber/decoder/InvocationResult.tssrc/encodings/ber/decoder/Matrix.tssrc/encodings/ber/decoder/Parameter.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Ber/Reader.ts
- src/encodings/ber/decoder/EmberNode.ts
- src/Ber/Writer.ts
- src/encodings/ber/decoder/Parameter.ts
- src/ASN1/ber/writer.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of myself
Type of Contribution
This is a: Code improvement
Current Behavior
This library has a dependency on an asn1 fork in a github repository.
Some security/dependency tooling gets upset about this and flags this as dangerous/risky as while package managers do pin versions, they don't do it in a truely immutable way.
New Behavior
This inlines the library and does a quick port to typescript. The used library has had no changes in 7 years, so we won't be missing any maintenance by doing this. In the future we could reduce the complexity by fully inlining the reader/writer classes, as there are extensions of those within this library which could now be fully merged instead.
I am not happy with the cleanliness of this change. The previous types neglected many places where null could be returned, which were not being handled correctly so this PR patches over crudely. Hopefully this wont break anything.
Testing Instructions
Other Information
Status