Skip to content

Comments

Fix CodeQL security alerts#2927

Merged
JacobCoffee merged 3 commits intomainfrom
fix/codeql-security-alerts
Feb 20, 2026
Merged

Fix CodeQL security alerts#2927
JacobCoffee merged 3 commits intomainfrom
fix/codeql-security-alerts

Conversation

@JacobCoffee
Copy link
Member

@JacobCoffee JacobCoffee commented Feb 20, 2026

Summary

  • Add permissions: contents: read to CI and static workflows (least-privilege)
  • Use sandboxed Django Engine for mailing email templates to mitigate SSTI
  • Sanitize file paths in fix_success_story_images management command (path traversal)
  • Validate redirect URL in MediaMigrationView to prevent open redirect
  • Replace innerHTML with textContent in font demo script (DOM XSS)
  • Add isSafeImageSrc() validation for sponsor application form image sources (DOM XSS)
  • Validate event detail date range select value is a relative URL (DOM XSS)
  • Dismiss SHA1 CodeQL alert as won't-fix (required by PyCon API signature scheme)

Resolves 15 of 16 open CodeQL alerts; 1 dismissed as external API requirement.

Test plan

  • Verify CI and static workflows still run successfully
  • Test mailing email template rendering in admin
  • Test sponsor application form benefit selection toggle
  • Test event detail date range selector navigation
  • Verify media migration redirects still work

🤖 Generated with Claude Code

- Add permissions: contents: read to CI and static workflows
- Use sandboxed Django template Engine for mailing templates (SSTI)
- Sanitize file paths in fix_success_story_images command (path injection)
- Validate redirect URL path in MediaMigrationView (open redirect)
- Use textContent instead of innerHTML in font demo (DOM XSS)
- Validate image src URLs in sponsor application form (DOM XSS)
- Validate select value is relative URL in event detail (DOM XSS)
- Dismiss SHA1 alert as won't-fix (PyCon API requirement)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 20, 2026 15:23
Use Django storage API (ContentFile + ImageField.save) instead of raw
file I/O to eliminate path injection taint chain. Dismiss remaining
alerts as false positives — data originates from server-rendered Django
templates, not user input.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request implements security fixes to address 15 of 16 CodeQL security alerts, covering SSTI, XSS, path traversal, and open redirect vulnerabilities. The changes include GitHub Actions permission hardening, sandboxed template rendering for email templates, input sanitization for file paths and URLs, and DOM XSS mitigations in JavaScript code.

Changes:

  • Added least-privilege permissions: contents: read to CI and static GitHub workflows
  • Implemented sandboxed Django Engine for email template rendering to prevent SSTI attacks
  • Added path sanitization in management commands and redirect views to prevent path traversal
  • Implemented client-side URL validation to prevent DOM-based XSS and open redirect vulnerabilities
  • Updated noqa comment for SHA1 usage to document it as an external API requirement

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
.github/workflows/ci.yml Added read-only permissions for least-privilege security
.github/workflows/static.yml Added read-only permissions for least-privilege security
static/js/sponsors/applicationForm.js Added isSafeImageSrc() validation for image sources
static/fonts/demo/demo.js Replaced innerHTML with textContent to prevent XSS
apps/events/templates/events/event_detail.html Added URL validation for date range selector
apps/pages/management/commands/fix_success_story_images.py Added path traversal protections for file operations
pydotorg/views.py Added path sanitization in MediaMigrationView redirect
apps/mailing/models.py Implemented sandboxed Engine for template rendering
apps/mailing/forms.py Updated to use sandboxed engine for validation
apps/sponsors/management/commands/create_pycon_vouchers_for_sponsors.py Updated noqa comment for SHA1 usage
Comments suppressed due to low confidence (1)

apps/pages/management/commands/fix_success_story_images.py:43

  • The path traversal protection has a subtle flaw. Line 43 uses Path(parsed_name).name after already extracting .name from PurePosixPath on line 42. Since .name already returns just the filename component without directory parts, calling Path().name again is redundant but harmless. However, if parsed_name contains path separators for some reason (which shouldn't happen after .name), the second .name call could potentially allow them through on Windows due to different path separator handling between PurePosixPath and Path.

Consider simplifying to just use filename = PurePosixPath(urlparse(url).path).name and remove the redundant second Path wrapper, or document why both are necessary for defense-in-depth.

    def find_image_paths(self, page):
        content = page.content.raw
        paths = set(re.findall(r"(/files/success.*)\b", content))

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

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b61b5e4186

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

- Use permissions: {} per Hugo's suggestion (cherry-picker pattern)
- Fix protocol-relative URL bypass (//evil.com) in event detail and
  isSafeImageSrc
- Use PurePosixPath + part filtering for robust path traversal
  prevention in MediaMigrationView
- var -> let in JS

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@JacobCoffee JacobCoffee enabled auto-merge (squash) February 20, 2026 15:48
@JacobCoffee JacobCoffee disabled auto-merge February 20, 2026 15:49
@JacobCoffee JacobCoffee merged commit ee7481b into main Feb 20, 2026
14 checks passed
@JacobCoffee JacobCoffee deleted the fix/codeql-security-alerts branch February 20, 2026 15:57
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