CWE-94 - Unsanitized input is run as code

[h4ck32n4u75]

Security Fix Request: querySelector Code Injection Vulnerability

Issue

• Severity: Critical
• Type: CWE-94 - Code Injection
• Location: cacheDomElements_() method in src/pages/mission-control/tabs/satellite-dashboard-tab.ts
• Line: 188

The Problem

The current code uses querySelector with string interpolation that could potentially be exploited:

private cacheDomElements_(): void {
  const ids = [
    'sat-azimuth', 'sat-elevation', 'sat-rotation',
    'sat-health-badge', 'sat-active-transponders',
  ];

  ids.forEach(id => {
    const el = qs(`#${id}`, this.dom_);  // ⚠️ Vulnerable line
    if (el) {
      this.domCache_.set(id, el);
    }
  });
}

Why this needs to be fixed: While the current IDs are hardcoded and safe, using string interpolation in querySelector calls is flagged as a potential code injection vector. If these IDs ever come from user input or external sources in the future, it could allow CSS selector injection attacks.

Requested Fix

The fix involves two options - choose the one that best fits your codebase:

Option 1: Use Direct querySelector (Recommended - Minimal Change)

Simply keep the current approach but add a comment acknowledging the pattern is safe since IDs are hardcoded:

private cacheDomElements_(): void {
  const ids = [
    'sat-azimuth', 'sat-elevation', 'sat-rotation',
    'sat-health-badge', 'sat-active-transponders',
  ];

  ids.forEach(id => {
    // Safe: IDs are hardcoded constants, not user input
    const el = this.dom_.querySelector(`#${id}`) as HTMLElement | null;
    if (el) {
      this.domCache_.set(id, el);
    }
  });
}

Option 2: Use getElementById (Most Secure)

If the qs helper supports it, or if you can modify the code to use native DOM methods:

private cacheDomElements_(): void {
  const ids = [
    'sat-azimuth', 'sat-elevation', 'sat-rotation',
    'sat-health-badge', 'sat-active-transponders',
  ];

  ids.forEach(id => {
    // getElementById is immune to CSS selector injection
    const el = this.dom_.ownerDocument?.getElementById(id) ?? 
              this.dom_.querySelector(`#${id}`) as HTMLElement | null;
    if (el) {
      this.domCache_.set(id, el);
    }
  });
}

Option 3: Add CSS Selector Escaping (Defense in Depth)

Add a helper method to escape CSS selector special characters:

/**
 * Helper function to safely escape CSS selector special characters
 * Prevents CSS selector injection attacks
 */
private escapeCssSelector_(id: string): string {
  // Escape special CSS selector characters
  return id.replace(/[!"#$%&'()*+,.\/:;<=>?@\[\\\]^`{|}~]/g, '\\$&');
}

private cacheDomElements_(): void {
  const ids = [
    'sat-azimuth', 'sat-elevation', 'sat-rotation',
    'sat-health-badge', 'sat-active-transponders',
  ];

  ids.forEach(id => {
    const escapedId = this.escapeCssSelector_(id);
    const el = qs(`#${escapedId}`, this.dom_);
    if (el) {
      this.domCache_.set(id, el);
    }
  });
}

Why This Fix is Better

• ✅ Security: Eliminates potential CSS selector injection vulnerability
• ✅ Future-proof: Protects against future changes where IDs might come from external sources
• ✅ Best Practice: Follows secure coding standards
• ✅ Functionality: Maintains exact same behavior with current hardcoded IDs

Current Risk Assessment

Low immediate risk - The current IDs are hardcoded string literals, so there's no actual exploit path with the current code. However, this pattern is flagged because:

1. It could become vulnerable if the code is modified in the future
2. It violates security scanning rules that prevent potentially dangerous patterns
3. It's considered a code smell in security-conscious codebases

Testing Recommendations

After implementing the fix:

1. Verify all cached DOM elements are still accessible
2. Test satellite dashboard display and updates
3. Verify traffic control UI functionality
4. Ensure no console errors appear during element caching

Action Required

Please review and implement one of the fixes shown above (Option 1 is recommended for minimal changes). A complete fixed version of the file with Option 3 is attached for reference. Let me know if you have any questions or concerns about this security fix.

[thkruz]

False positive from the static analysis tool.

This is client-side code for selecting a dom element. The ids are hardcoded and the user already has complete control - this creates no risk and the argument that "It could become vulnerable if the code is modified in the future" makes every line of code a risk.

[h4ck32n4u75]

Valid pushback and with no user input the finding is clearly false positive. I treated both the same and should have caught the lack of user controlled input on this one. Good news is I have a better understanding of this part of the code base.