Skip to content

Fix g-flag regex in getValidId and CSS selector injection in getIdFromUrl#1558

Merged
kfarr merged 1 commit into
mainfrom
fix-valid-id
Apr 17, 2026
Merged

Fix g-flag regex in getValidId and CSS selector injection in getIdFromUrl#1558
kfarr merged 1 commit into
mainfrom
fix-valid-id

Conversation

@vincentfretin

Copy link
Copy Markdown
Collaborator

Backport of aframevr/aframe-inspector#853

This addresses summary point 3 #1540 (comment)

…n getIdFromUrl (aframevr/aframe-inspector#853)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vincentfretin
vincentfretin requested a review from kfarr as a code owner April 16, 2026 09:51
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review for PR 1558 - see detailed review below

@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review - PR 1558

Note: replacing placeholder comment above

Overview: This PR backports bug fixes from aframe-inspector#853 into 3DStreet, addressing: (1) a CSS selector injection vulnerability in getIdFromUrl, (2) missing g-flag on two regexes in getValidId, and (3) expanding isValidId to allow underscore as the first character.


Security Fix - getIdFromUrl

Correct and important fix. The old code concatenated url directly into a CSS attribute selector -- a URL containing a single quote could break out of the selector quotes or cause unexpected CSS injection behavior. The fix escapes single quotes before interpolation, which is correct and sufficient for this context.


Regex Bug Fixes - getValidId

Both missing g flags were real bugs causing only the first match to be replaced:

  • /\s/ to /\s+/g: now replaces all whitespace runs, not just the first space
  • /[\W]/ to /[^\w-]+/g: now removes all invalid chars, not just the first one
  • /^\d+\s*/ to /^[-\d]+/: strips leading hyphens too (needed since spaces become hyphens in the prior step)

Examples that were silently broken before:

  • 'my great file.png' Old: 'mygreat file' (space left in middle). New: 'my-great-file' (correct)
  • ' hello.png' Old: ' hello' (leading spaces remain). New: 'hello' (correct)

isValidId - Consistent Expansion

Good fix. \w already matches underscore, so getValidId could produce IDs starting with underscore (e.g. from '_asset.png'), but the old isValidId would reject them. This change aligns the validator with the generator.


Suggestions

  1. Add unit tests - assetsUtils.js has no test coverage. These subtle regex edge cases are exactly the kind that benefit from regression tests. Suggested cases:

    • getValidId('my great file.png') should return 'my-great-file' (was 'mygreat file')
    • getValidId('hello world foo.png') should return 'hello-world-foo' (was broken)
    • getValidId(' hello.png') should return 'hello' (was ' hello')
    • getValidId('123-abc.png') should return 'abc'
    • getIdFromUrl with a URL containing a single quote should not throw
  2. Minor behavioral change to note - '123_file.png' now produces '_file' (old code produced 'file' since the old strip only targeted leading digits). The new result is valid per the updated isValidId, but it is a subtle change worth a test case.


Summary

Area Assessment
Correctness All fixes address real bugs
Security CSS injection fix is correct and sufficient
Style/conventions Matches project conventions; inline comments are helpful
Backport provenance Clear upstream reference to aframe-inspector#853
Test coverage No tests exist or were added for assetsUtils.js

The changes are correct and low-risk. Adding unit tests would lock in the expected behavior and prevent regressions, but the core fixes are solid and ready to merge.

@kfarr
kfarr merged commit 44a51b9 into main Apr 17, 2026
2 checks passed
@kfarr
kfarr deleted the fix-valid-id branch April 17, 2026 01:25
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