Skip to content

fix: five high-severity bugs from the adversarial code audit#1539

Merged
obiot merged 2 commits into
masterfrom
fix/bug-hunt-high-severity
Jul 5, 2026
Merged

fix: five high-severity bugs from the adversarial code audit#1539
obiot merged 2 commits into
masterfrom
fix/bug-hunt-high-severity

Conversation

@obiot

@obiot obiot commented Jul 5, 2026

Copy link
Copy Markdown
Member

The high-severity tier from the parallel bug hunt (loader / audio / math beats), every finding re-verified by hand against the code before fixing. All specs written failing-first: 17 of 27 new tests red on the old code, 27/27 green after.

1. new Color("darkgray") threw + wrong/missing CSS keywords — math/color.ts

  • darkgray / darkgrey table keys were pasted from a spec table with their footnote marker ("darkgray[*]") → lookup missed → parseCSS fell through to parseHexthrow invalid parameter.
  • cyan, magenta, rebeccapurple missing entirely (same throw).
  • silver [192,192,129], aliceblue [240,248,245], burlywood [222,184,35] — single-digit channel typos.

New tests/css-colors.spec.ts validates the full 148-keyword table against the CSS spec (expected values derived via parseHex, no hand-transcribed channels), so a paste error can never hide again.

2. Matrix2d 6-arg canvas form dropped translation + transposed rotation — math/matrix2d.ts

The layout is column-major (tx/ty getters read val[6]/val[7]; apply() agrees), but the 6-arg branch stored (a,b,c,d,e,f) row-major: e/f landed in the unused projective row — new Matrix2d(1,0,0,1,x,y) produced the identity with no translation — and rotation came out inverted. 9-arg form unaffected. New spec includes a 6-arg ≡ 9-arg equivalence check.

3. moveTowards() never arrived and returned the target — all 4 vector classes

Two bugs, copy-pasted across Vector2d/Vector3d/ObservableVector2d/ObservableVector3d:

  • arrival test was distance <= step * step — wrong units (snapped early for step > 1, dithered around the target forever for fractional steps);
  • on "arrival" it returned the caller's target vector without moving this, violating its own @returns Reference to this object JSDoc — chained calls silently mutated the target.

Now lands exactly on the target, always returns this; documented negative-step flee semantics preserved (covered by a test). Observables land via set() so observers fire. New spec runs the same 4 cases across all four classes via describe.each.

4. Every video preloaded with forced anonymous CORS — loader/parsers/video.js

setAttribute("crossorigin", settings.crossOrigin) ran unconditionally; the default (undefined) stamps the string "undefined", and per the HTML spec an invalid enumerated value maps to Anonymous (a missing attribute means no CORS). Cross-origin videos served without CORS headers failed to load when they would have played fine untainted. Now guarded on typeof === "string" — the empty string (a valid value meaning anonymous) is still honored.

5. Video preload hung forever on autoplay-restricted browsers — loader/parsers/video.js

The completion event was canplay unless autoplay was explicitly false — but the common manifest shape simply omits autoplay, and iOS Safari & friends never fire canplay for a video that isn't allowed to play → the whole preloader stalled and the game never started (the code's own comment described the Safari behavior; the guard just didn't match it). Now loadedmetadata unless autoplay: true explicitly asks to wait for canplay; streaming videos keep loadedmetadata.

Test plan

  • 4 new spec files (27 tests), failing-first: 17 red on old code, all green after
  • Full suite: 4597 passed / 0 failed / 15 skipped
  • eslint + biome + build clean

🤖 Generated with Claude Code

https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A

Color (math/color.ts):
- the CSS named-color table's darkgray/darkgrey keys were pasted from a
  spec table WITH their footnote marker ("darkgray[*]"), so parseCSS
  missed the table and threw "invalid parameter" for both names
- cyan, magenta and rebeccapurple were missing entirely (same throw)
- silver / aliceblue / burlywood carried single-digit channel typos
- new tests/css-colors.spec.ts validates the full 148-keyword table
  against the CSS spec so a paste error can never hide again

Matrix2d (math/matrix2d.ts): the 6-argument canvas-convention form
stored components row-major into the column-major layout — e/f landed
in the unused projective row (translation silently dropped: identity +
translate produced plain identity) and rotation came out transposed.
The 9-argument form was unaffected.

moveTowards (all four vector classes): the arrival test used
`distance <= step * step` (wrong units — snapped early for step > 1,
dithered forever for fractional steps) and returned the CALLER'S TARGET
vector without moving `this`, violating its own "@returns Reference to
this object" JSDoc and making chained calls mutate the target. Now
lands exactly on the target and always returns `this`; negative-step
flee semantics preserved.

Video parser (loader/parsers/video.js):
- crossorigin was stamped unconditionally; the default (undefined)
  becomes the string "undefined", an INVALID enumerated value that the
  HTML spec maps to Anonymous — forced CORS broke cross-origin videos
  served without CORS headers. Now only set when configured ("" stays
  valid).
- preload completion waited on canplay unless autoplay was EXPLICITLY
  false; with autoplay simply omitted, autoplay-restricted browsers
  (iOS Safari) never fire canplay and preload hung forever. Now
  loadedmetadata unless autoplay: true asks for canplay; streaming
  videos keep loadedmetadata.

All specs written failing-first: 17 of 27 new tests red on the old
code, 27/27 green after. Full suite 4597 passed / 0 failed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Copilot AI review requested due to automatic review settings July 5, 2026 01:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a set of high-severity correctness issues across melonJS math utilities and the video loader, and adds targeted regression specs to prevent reintroduction.

Changes:

  • Correct CSS named-color keyword mapping (including missing keywords and incorrect RGB channels) and add a full keyword-table verification spec.
  • Fix Matrix2d 6-argument canvas convention mapping (translation + rotation layout) and add equivalence/behavior specs.
  • Fix moveTowards() behavior across vector implementations (arrival condition + return/mutation contract) and adjust video preloader wiring (crossorigin handling + completion event selection), with new regression tests and changelog entries.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/melonjs/src/math/color.ts Fix CSS color keyword table entries (missing/incorrect keys and channels).
packages/melonjs/tests/css-colors.spec.ts Add spec validating full CSS named-color keyword table against hex values.
packages/melonjs/src/math/matrix2d.ts Fix 6-arg setTransform(a,b,c,d,e,f) mapping for column-major storage.
packages/melonjs/tests/matrix2d-settransform.spec.ts Add regression tests for 6-arg canvas form translation/rotation + 6-arg≡9-arg equivalence.
packages/melonjs/src/math/vector2d.ts Fix moveTowards() arrival threshold and return/mutation semantics.
packages/melonjs/src/math/vector3d.ts Fix moveTowards() arrival threshold and return/mutation semantics (x/y interpolation).
packages/melonjs/src/math/observableVector2d.ts Fix moveTowards() arrival threshold and ensure observer notifications via set().
packages/melonjs/src/math/observableVector3d.ts Fix moveTowards() arrival threshold and ensure observer notifications via set().
packages/melonjs/tests/vector-movetowards.spec.ts Add shared contract tests covering all 4 vector classes (step, arrival, fractional convergence, negative-step flee).
packages/melonjs/src/loader/parsers/video.js Fix crossorigin stamping and reliable preload completion event selection.
packages/melonjs/tests/video-parser.spec.js Add regression tests for CORS attribute handling and autoplay-related completion event selection.
packages/melonjs/CHANGELOG.md Document the fixed regressions/bugs for release notes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/melonjs/tests/video-parser.spec.js
Comment thread packages/melonjs/tests/video-parser.spec.js
Comment thread packages/melonjs/tests/video-parser.spec.js
Comment thread packages/melonjs/tests/video-parser.spec.js
Comment thread packages/melonjs/tests/video-parser.spec.js
Comment thread packages/melonjs/tests/video-parser.spec.js
Copilot flagged that media/clip.webm does not exist under tests/public —
every test fired a pointless 404 fetch on videoElement.load(). The spec
now uses an inline data:video/webm URL (which also exercises the
parser's MIME-sniff + canPlayType gate; isDataUrl requires a non-empty
base64 payload, so it carries the WebM/EBML magic bytes) and passes a
no-op onerror everywhere — these tests assert wiring, not playback.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
@obiot obiot merged commit b7b23c0 into master Jul 5, 2026
6 checks passed
@obiot obiot deleted the fix/bug-hunt-high-severity branch July 5, 2026 07:24
obiot added a commit to asadullahbro/melonJS that referenced this pull request Jul 7, 2026
…ine-wide

Rework of the originPoint proposal into the hybrid agreed in review:
Sprite3d anchors through the SAME settings.anchorPoint key, convention
and centered default as the 2D Sprite — no second anchor API — while
keeping the (verified-correct) vertex-bake mechanism from the original
commit.

- new shared resolver (renderable/anchorPoint.ts): the named presets
  ("center", "top", "bottom", ..., "bottom-right") now work everywhere
  settings.anchorPoint does — Sprite, Entity, Collectable, ImageLayer,
  Text, BitmapText (and subclasses), plus Sprite3d. On the 2D classes
  invalid values keep their historical silent (0, 0) outcome for full
  backward compatibility, but log a console warning; Sprite3d (a new
  surface) throws. Values are never clamped; Vector2d instances and
  bare {x, y} objects pass through unchanged.
- Sprite resolves the anchor once, up front, and forwards the RESOLVED
  pair into the texture-cache descriptor (spritesheet atlases store it
  as the cached per-frame pivot), so presets and {x, y} behave
  identically on both paths and a raw string or aliased Vector2d never
  lands in the shared cache.
- ImageLayer normalizes its bare-number shorthand pre-super (its Sprite
  base consumes the same setting and the strict resolver rejects bare
  numbers).
- Sprite3d wires the inherited ObservablePoint as the LIVE anchor:
  setMuted + setCallback post-super; runtime anchorPoint.set() re-bakes
  the quad (region path via _applyFrame, plain-quad path via
  bakeQuadVertices) and re-derives the cull bounds (direct point
  mutation — the Rect width setter's recalc() walks Mesh's repurposed
  normals array and would throw).
- Mesh gains the _anchorBaked opt-out consulted by preDraw's per-frame
  applyAnchorTransform recompute, so the anchor never leaks into the
  renderer transform on either camera path (plain Mesh behavior
  unchanged, webgl_mesh_anchor.spec.js untouched and green).
- cull bounds fixed: side = max(max(w,h), sqrt2*hypot(hw+|ox|, hh+|oy|))
  — the sphere exactly encloses the anchored quad in any billboard
  orientation (a bottom-anchored 130x225 character was ~47% under-sized
  and popped out on screen); centered default byte-identical.
- billboard example: anchorPoint: "bottom", camera-target framing
  restored (GY * 0.6 became dead code aiming at the floor), stale
  comment fixed.
- tests: new tests/anchorpoint-presets.spec.js (cross-renderable
  contract, 72 tests, failing-first) + 9 geometry/runtime/cull tests in
  sprite3d.spec.js (failing-first). Full suite 4698 passed / 0 failed.

Master merged in (CHANGELOG + the melonjs#1539-melonjs#1541 fixes).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants