[NOCSL] Add url validation

[stanlp1]

• Adds url validation for canonical url

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Good improvement to handle relative canonical URLs by using the URL constructor with a base URL parameter.

🚨 Critical Issues

None

⚠️ Important Issues

[File: spec/src/utils/helpers.js Line: 238]
There is a leftover console.log(result) statement that should be removed. Debug logging should not be committed to the codebase.

[File: src/utils/helpers.js Lines: 102-120]
The error handling is incomplete. The empty catch block silently swallows all errors, including important ones like invalid URLs. Consider:

1. At minimum, add a comment explaining why errors are suppressed
2. Better yet, log errors in development mode or distinguish between expected errors (non-DOM context) and unexpected errors (malformed URLs)

[File: spec/src/utils/helpers.js Lines: 230-243]
The test for relative URLs lacks specificity. The test should verify that:

1. The URL is properly constructed with the correct base (checking it just includes the relative path is weak)
2. The expected output format matches what would be produced by combining document.location.href with the relative URL

💡 Suggestions

[File: src/utils/helpers.js Lines: 102-120]
Consider adding test coverage for edge cases such as when canonical link element exists but has no href attribute, when href is an invalid URL format, when href is empty string, or when the document location itself is not a valid base URL.

[File: src/utils/helpers.js Line: 111]
The new URL() constructor can throw a TypeError for invalid URLs. While this is caught by the try-catch, the function could be more robust by validating that href is a non-empty string before attempting URL construction.

Overall Assessment: ⚠️ Needs Work

The implementation correctly solves the problem of handling relative canonical URLs, but needs minor fixes (remove console.log) and improved error handling documentation before merging.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

The PR adds proper URL validation for canonical URLs by converting relative URLs to absolute ones using the URL constructor, which is a good improvement for handling various canonical URL formats.

🚨 Critical Issues

None identified.

⚠️ Important Issues

1. [File: src/utils/helpers.js Lines: L115-L116] Silent error handling with empty catch block
The try-catch block catches all exceptions but silently ignores them with just a comment // do nothing. This makes debugging difficult if URL parsing fails.

Suggestion: Add minimal error logging or at least document what errors are expected:

} catch (e) {
  // Silently fail for invalid URLs or missing document context
  // Expected errors: TypeError for invalid URL format
}

2. [File: spec/src/utils/helpers.js Lines: L217-L242] Missing test coverage for edge cases
The test suite covers happy path and relative URLs but is missing several important edge cases:

• What happens when the canonical link element doesn't exist?
• What happens when href attribute is empty string?
• What happens when href contains an invalid URL that throws during URL construction?
• What happens when not in a DOM context (should return null)?

Suggestion: Add these test cases:

it('Should return null when canonical link element does not exist', () => {
  const cleanup = jsdom();
  const canonicalEle = document.querySelector('[rel=canonical]');
  canonicalEle.remove();
  
  expect(getCanonicalUrl()).to.be.null;
  cleanup();
});

it('Should return null when href attribute is empty', () => {
  const cleanup = jsdom();
  const canonicalEle = document.querySelector('[rel=canonical]');
  canonicalEle.setAttribute('href', '');
  
  expect(getCanonicalUrl()).to.be.null;
  cleanup();
});

it('Should return null when href contains invalid URL', () => {
  const cleanup = jsdom();
  const canonicalEle = document.querySelector('[rel=canonical]');
  canonicalEle.setAttribute('href', 'ht!tp://invalid url with spaces');
  
  expect(getCanonicalUrl()).to.be.null;
  cleanup();
});

it('Should return null when not in a DOM context', () => {
  expect(getCanonicalUrl()).to.be.null;
});

💡 Suggestions

1. [File: src/utils/helpers.js Lines: L102-L120] Consider extracting URL validation logic
The try-catch block and URL parsing logic could be extracted into a separate helper function for better testability and reusability:

validateAndNormalizeUrl: (href, baseUrl) => {
  try {
    if (!href) return null;
    const url = new URL(href, baseUrl);
    return url.toString();
  } catch (e) {
    return null;
  }
},

getCanonicalUrl: () => {
  if (!utils.canUseDOM()) return null;
  
  const linkEle = document?.querySelector('link[rel="canonical"]');
  const href = linkEle?.getAttribute('href');
  
  return utils.validateAndNormalizeUrl(href, document.location.href);
},

2. [File: src/utils/helpers.js Line: L102] Variable initialization could be simplified
Since the function returns canonicalURL at the end and it's only set inside the try block, you could simplify by returning directly from within the try block and null at the end.

Overall Assessment: ⚠️ Needs Work

The implementation improves URL handling, but the missing edge case tests and silent error handling should be addressed before merging. Adding comprehensive test coverage for error scenarios is important to ensure reliability.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Good test coverage with edge cases (relative URLs, empty href, missing element, non-DOM context) and proper URL validation implementation.

🚨 Critical Issues

None

⚠️ Important Issues

[File: src/utils/helpers.js Line: 115-116]
The error handling silently swallows all exceptions without any logging or context. While the try-catch prevents crashes from invalid URLs, it makes debugging difficult. Consider logging the error for debugging purposes:

} catch (e) {
  // Log error for debugging invalid canonical URLs
  if (console && console.debug) {
    console.debug('Invalid canonical URL:', e);
  }
}

[File: spec/src/utils/helpers.js Line: 237-239]
The test for relative URLs uses imprecise assertions. Instead of using .to.include(relativeUrl) and a regex match, verify the exact expected output:

const expected = `${document.location.origin}${relativeUrl}`;
expect(result).to.equal(expected);

💡 Suggestions

[File: spec/src/utils/helpers.js Line: 230-242]
Consider adding a test case for malformed URLs that would trigger the catch block (e.g., href="ht!tp://invalid") to verify the error handling path works correctly and returns null as expected.

[File: src/utils/helpers.js Line: 111]
The URL constructor with the base parameter elegantly handles both absolute and relative URLs. This is a good implementation pattern that aligns with web standards.

Overall Assessment: ✅ Pass

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

The PR adds proper URL validation for canonical URLs with comprehensive test coverage and improved handling of relative URLs.

🚨 Critical Issues

None

⚠️ Important Issues

1. [File: src/utils/helpers.js Line: 111] - Potential security issue: Invalid URL construction
The current implementation passes an untrusted href value directly to the URL constructor without validation. This could be exploited with malicious input. The URL constructor will throw an error for invalid URLs (which is caught), but the second parameter document.location.href could also be manipulated in certain contexts.

Recommendation: Add explicit validation before URL construction:

if (href) {
  // Basic validation before URL construction
  if (typeof href \!== 'string' || href.trim() === '') {
    return null;
  }
  const url = new URL(href, document.location.href);
  canonicalURL = url.toString();
}

2. [File: src/utils/helpers.js Line: 115-116] - Silent error handling reduces debuggability
The empty catch block silently swallows all errors without logging. This makes debugging production issues difficult when URL parsing fails for legitimate reasons (e.g., malformed canonical URLs in the DOM).

Recommendation: Add minimal error logging or at least a comment explaining why errors are intentionally ignored:

} catch (e) {
  // Intentionally silenced - invalid canonical URLs should not break the application
  // URL parsing errors are expected when malformed href attributes exist
}

3. [File: spec/src/utils/helpers.js Line: 237-239] - Test assertion is too weak
The test "Should return a complete URL when given a relative canonical URL" uses loose assertions that don't verify the URL is actually valid or correctly constructed. The test only checks if the relative URL substring exists and has a protocol, but doesn't validate the complete URL structure.

Recommendation: Use more specific assertions:

const result = getCanonicalUrl();
expect(result).to.be.a('string');
expect(result).to.match(/^https?:\/\/[^/]+\/products\/item$/);
// Or check against the expected full URL if jsdom provides a predictable base

💡 Suggestions

1. [File: src/utils/helpers.js Line: 108] - Redundant optional chaining
The linkEle?.getAttribute('href') uses optional chaining after already checking if (utils.canUseDOM()). If querySelector returns null, optional chaining handles it correctly, but the variable name linkEle suggests it's always an element when it could be null.

Consider renaming for clarity:

const linkEle = document?.querySelector('link[rel="canonical"]');
const href = linkEle?.getAttribute('href');

is fine, but consider:

const canonicalLink = document?.querySelector('link[rel="canonical"]');
if (\!canonicalLink) return null;
const href = canonicalLink.getAttribute('href');

2. [File: spec/src/modules/tracker.js Line: 30] - Test constant location
The canonicalUrl constant is defined at the top level with other test fixtures, which is good. However, consider whether it should match the pattern of other URLs (like url and referrer) or if the trailing slash inconsistency is intentional.

The other URLs don't have trailing slashes in their paths, but canonicalUrl = 'https://localhost/' does. Verify this is the intended test data.

3. [General] - Consider adding edge case tests
The test suite covers the main scenarios well, but consider adding tests for:

• Canonical URL with query parameters
• Canonical URL with hash fragments
• Malformed URLs that would throw errors (e.g., href="javascript:alert(1)")
• Protocol-relative URLs (e.g., //example.com/page)

These edge cases would ensure the URL validation handles all real-world scenarios correctly.

Overall Assessment: ⚠️ Needs Work

The implementation is solid with good test coverage, but the important issues around error handling and security validation should be addressed before merging. The silent error handling and weak test assertions reduce the reliability and maintainability of the code.

[Mudaafi]

lgtm