Skip to content

Commit 9337d65

Browse files
JacobCoffeeclaude
andcommitted
Address PR review 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>
1 parent b59161e commit 9337d65

5 files changed

Lines changed: 13 additions & 13 deletions

File tree

.github/workflows/ci.yml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@ name: CI
22

33
on: [push, pull_request, workflow_dispatch]
44

5-
permissions:
6-
contents: read
5+
permissions: {}
76

87
jobs:
98
migrations:

.github/workflows/static.yml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@ name: Check collectstatic
22

33
on: [push, pull_request, workflow_dispatch]
44

5-
permissions:
6-
contents: read
5+
permissions: {}
76

87
jobs:
98
collectstatic:

apps/events/templates/events/event_detail.html

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,8 @@ <h3 class="widget-title">More events in <!-- The current category--><a href="{{
184184
<script>
185185
$(function(){
186186
$('#dataRangeSelect').change(function(e) {
187-
var url = $(this).val();
188-
if (url && url.charAt(0) === '/') {
187+
let url = $(this).val();
188+
if (url && url.charAt(0) === '/' && url.charAt(1) !== '/') {
189189
window.location = url;
190190
}
191191
});

pydotorg/views.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import json
55
import re
66
from collections import defaultdict
7-
from pathlib import Path
7+
from pathlib import Path, PurePosixPath
88

99
from django.conf import settings
1010
from django.http import HttpResponse, JsonResponse
@@ -84,9 +84,11 @@ class MediaMigrationView(RedirectView):
8484

8585
def get_redirect_url(self, *args, **kwargs):
8686
"""Build the S3 redirect URL from the media path."""
87-
image_path = kwargs["url"]
88-
# Sanitize path to prevent open redirect via path traversal
89-
image_path = image_path.lstrip("/").replace("../", "")
87+
# Normalize path to prevent traversal (../, ....//, %2e%2e/, etc.)
88+
image_path = PurePosixPath(kwargs["url"]).as_posix().lstrip("/")
89+
# Collapse any remaining parent references after normalization
90+
parts = [p for p in image_path.split("/") if p not in (".", "..")]
91+
image_path = "/".join(parts)
9092
if self.prefix:
9193
image_path = f"{self.prefix}/{image_path}"
9294
return f"{settings.AWS_S3_ENDPOINT_URL}/{settings.AWS_STORAGE_BUCKET_NAME}/{image_path}"

static/js/sponsors/applicationForm.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@ const DESKTOP_WIDTH_LIMIT = 1200;
22

33
function isSafeImageSrc(url) {
44
if (!url) return false;
5-
// Allow relative URLs and data URIs for static assets
6-
if (url.charAt(0) === '/' || url.indexOf('/static/') === 0) return true;
5+
// Allow relative URLs starting with / (but not protocol-relative //)
6+
if (url.charAt(0) === '/' && url.charAt(1) !== '/') return true;
77
try {
8-
var parsed = new URL(url, window.location.origin);
8+
let parsed = new URL(url, window.location.origin);
99
return parsed.origin === window.location.origin;
1010
} catch (e) {
1111
return false;

0 commit comments

Comments
 (0)