Skip to content

security: fix CQL injection and path traversal vulnerabilities#87

Open
yaowei1980 wants to merge 1 commit intopchuri:mainfrom
yaowei1980:security/fix-cql-injection-and-path-traversal
Open

security: fix CQL injection and path traversal vulnerabilities#87
yaowei1980 wants to merge 1 commit intopchuri:mainfrom
yaowei1980:security/fix-cql-injection-and-path-traversal

Conversation

@yaowei1980
Copy link
Copy Markdown

  • Add escapeCql() function to sanitize CQL query strings

    • Escapes backslashes, double quotes, wildcards (*, ?), and fuzzy match (~)
    • Applied to search() and findPageByTitle() methods
  • Strengthen sanitizeTitle() to prevent path traversal

    • Remove '..' sequences
    • Remove leading dots (hidden files)
    • Replace filesystem reserved characters with underscore
  • Add sanitizeFilename() function for safe filename handling

    • Used in uniquePathFor() for attachment downloads and exports

Fixes: CQL injection via search queries, path traversal via page titles and attachment filenames

All tests passing (159/159)

Pull Request Template

Description

Brief description of what this PR does.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Performance improvement
  • Code refactoring

Testing

  • Tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Screenshots (if applicable)

Add screenshots to help explain your changes.

Additional Context

Add any other context about the pull request here.

- Add escapeCql() function to sanitize CQL query strings
  - Escapes backslashes, double quotes, wildcards (*, ?), and fuzzy match (~)
  - Applied to search() and findPageByTitle() methods

- Strengthen sanitizeTitle() to prevent path traversal
  - Remove '..' sequences
  - Remove leading dots (hidden files)
  - Replace filesystem reserved characters with underscore

- Add sanitizeFilename() function for safe filename handling
  - Used in uniquePathFor() for attachment downloads and exports

Fixes: CQL injection via search queries, path traversal via page titles
and attachment filenames

All tests passing (159/159)
Copy link
Copy Markdown
Owner

@pchuri pchuri left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Security hardening for CQL injection and path traversal is a valuable contribution.

A few comments on the changes:

1. .. removal can be bypassed (Critical)

sanitizeFilename() and sanitizeTitle() both use:

.replace(/\.\./g, '')

This is a single-pass replacement, so .... becomes .. after one pass — path traversal is still possible.

A safer approach would be using path.basename() to strip directory components entirely:

function sanitizeFilename(filename) {
  if (!filename || typeof filename !== 'string') {
    return 'unnamed';
  }
  return path.basename(filename)
    .replace(/[\\/:*?"<>|]/g, '_')
    .replace(/^\.+/, '')
    .trim() || 'unnamed';
}

2. sanitizeTitle behavior change — ' ''_'

The existing code replaces reserved characters with a space, but this PR changes it to underscore. This could break compatibility with previously exported directories (e.g., re-exporting a page won't match the existing directory name).

Was this intentional? If so, it might be worth noting as a breaking change.

3. escapeCql — is ~ escape necessary?

~ is a CQL operator (fuzzy match), but inside a quoted string literal it may not need escaping depending on the Confluence version. Could you verify this doesn't cause unexpected search behavior?

4. package-lock.json changes

The lock file includes unrelated dev dependency version bumps (ajv, brace-expansion, flatted, minimatch, picomatch). Consider splitting these into a separate commit or PR to keep the security fix focused.

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.

3 participants